Just before 0.45.1 was going to release I happened to do an audit of Inkscape's use of printf-style format strings (and the "..." argument). Unfortunately, there were multiple vulnerabilities[1]. Aaron gracefully delayed the final release so I could get CVEs issued, and notify other distributions.
The basic problem the use of a *printf call directly on a variable without a format string. For example:
printf(variable);
vs
printf("%s",variable);
In the former, printf will parse any format chars in "variable", including the dangerous "%n", which allows for arbitrary memory overwriting.
When functions use "..." to take in format strings for later processing by a *printf function, you can end up in the same situation. (Sorry to pick on ishmal's code, but it's a good example.) From the recent security fix commit:
- status(event.getData().c_str()); + status("%s", event.getData().c_str());
"status" was defined as:
virtual void status(const char *fmt, ...);
Now, since the code is *filled* with format string "..." usage, it can be a real pain to find them all. Luckily, gcc has an attribute to flag a function as acting like a printf-family call, and Glib has a macro to create this attribute if gcc is being used as the compiler. So, now "status" is defined as:
virtual void status(const char *fmt, ...) G_GNUC_PRINTF(2,3);
The two args in G_GNUC_PRINTF point[2] to the format string variable, and the "..." argument. In the case of a class (as above) there is an implicit "this" argument as argument #1, meaning the format string "fmt" is really arg 2, and the "..." happens at position 3, as reflected in the G_GNUC_PRINTF usage.
These markings, combined with the gcc compiler flag "-Wformat-security" will issue warnings about missing format strings, and do other strict type-checking. I've added this to the default CFLAGS as well. (And -D_FORTIFY_SOURCE=2, which adds even more protections via glibc.)
Overall, many format issues were corrected[3], only a few of which were security-sensitive (37 files changed, 120 insertions(+), 111 deletions(-)). When making new "..."-using function, please review the glib macros for appropriate attributes, and take care to use them safely. I'd especially appreciate it if someone could look at:
./display/sp-canvas.h:SPCanvasItem *sp_canvas_item_new(SPCanvasGroup *parent, GtkType type, const gchar *first_arg_name, ...);
This seems to be a NULL-terminated list (and not a format string, obviously), but it should get marked as such[4] if that's true.
The dom/js/ tree is stuffed with "..." and I did not audit it, since it is not hooked up yet. However, we need to be very careful with it, and I'd MUCH prefer this code stayed outside of the inkscape code base so we can just link to an external library in the event of security updates to those libraries.
Let me know if anyone has any questions; I'd like to keep inkscape as safe as possible. :)
Thanks,
-Kees
[1] Try it yourself: prior to current trunk and 0.45.1, run this: inkscape /tmp/no-such-file-%x.svg And you'll see the "%x" turned into a number off the edge of the stack. The Jabber protocol handler was also vulnerable.
[2] http://developer.gnome.org/doc/API/2.0/glib/glib-Miscellaneous-Macros.html#G...
[3] http://inkscape.svn.sourceforge.net/viewvc/inkscape?view=rev&revision=14...
[4] http://developer.gnome.org/doc/API/2.0/glib/glib-Miscellaneous-Macros.html#G...