GNOME Bugzilla – Bug 639203
Buffer overflows in gimp
Last modified: 2017-01-19 12:07:50 UTC
Apparently, nobody forwarded http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=608497 upstream. The bug talks about four buffer overflows. CVEs have been assigned: CVE-2010-4540 gimp LIGHTING EFFECTS > LIGHT plugin stack buffer overflow CVE-2010-4541 gimp SPHERE DESIGNER plugin stack buffer overflow CVE-2010-4542 gimp GFIG plugin stack buffer overflow CVE-2010-4543 gimp heap overflow read_channel_data() in file-psp.c The first three ones all share a common pattern: gchar buffer[G_ASCII_DTOSTR_BUF_SIZE]; gdouble value; sscanf (ptr, "%s", buffer); value = g_ascii_strtod (buffer, &endptr); I'm not sure why the code doesn't simply do: gdouble value; sscanf (ptr, "%lf", &value); Here are the functions affected: CVE-2010-4540: load_preset_response() in plugins/lighting/lighting-ui.c CVE-2010-4541: loadit() in plugins/common/sphere-designer.c CVE-2010-4542: gfig_read_parameter_gimp_rgb() in plugins/gfig/gfig-style.c CVE-2010-4543 is described as follows: The function "read_channel_data()" in plug-ins/common/file-psp.c has an overflow when handling PSP_COMP_RLE type files. A malicious file that starts a long runcount at the end of an image will write outside of allocated memory.
Created attachment 178015 [details] Proof of concept for CVE-2010-4540 Open with: FILTERS > LIGHT AND SHADOW > LIGHTING EFFECTS > LIGHT > OPEN
Created attachment 178016 [details] Proof of concept for CVE-2010-4541 Open with: FILTERS > RENDER > SPHERE DESIGNER > OPEN
Created attachment 178017 [details] Proof of concept for CVE-2010-4542 Open with: FILTERS > RENDER > GFIG > FILE > OPEN
The reason for the g_ascii_strtod() / g_ascii_dtostr() usage is that it is (unlike sscanf) locale independant and hence makes the saved files consistent for all users.
Ah, right. I guess a fix could be to just do this: old_locale = setlocale(LC_NUMERIC, "C"); [... read file ...] setlocale(LC_NUMERIC, old_locale);
Actually this is discouraged, because it is not thread-safe (which would not matter for the plugins, but anyway). However, the IMHO right fix is, to specify the buffer length in the scanf format string (as in "%39s") and add one Byte to the size of the buffers for the zero-byte. This should be trivial and do the right thing. I will have a look at it this evening.
Created attachment 178090 [details] [review] Proposed patch for the format-string based buffer overflows. This is a patch that attempts to fix the format-string based buffer overflows. Relatively straightforward, including a guard against a potentially changing G_ASCII_DTOSTR_BUF_SIZE.
Created attachment 178097 [details] [review] revised patch, avoids the ugly #define orgy. Here is a revised patch, that avoids the #define orgy.
If you're confident that the patch is ok, just push it please.
The patch looks good in general but it can be simplified by using '*' in the original format strings to specify the field width and/or precision. For example: printf("%*.*s", width, precision, string); Use "*", ".*", or "*.*" as required. You still need to have some care in buffer sizes as I have seen cases where more characters are output than was specified by width (usually with numbers too large to be shown in the number of places specified). For strings, use "%.*s" to limit the number of characters.
Note that printf() and scanf() are two different functions. For scanf() the asterisk is the assignment suppression character.
Created attachment 179375 [details] [review] Fix (?) for CVE-2010-4543 This is a completely untested patch for CVE-2010-4543, and it might be completely wrong -- I just took a quick glance at the code.
*** Bug 641105 has been marked as a duplicate of this bug. ***
Both aspects of this bug are fixed in git now: commit 48ec15890e1751dede061f6d1f469b6508c13439 Author: Simon Budig <simon@budig.de> Date: Mon Feb 14 21:46:31 2011 +0100 file-psp: fix for bogus input data. Fixes bug #639203 commit 7fb0300e1cfdb98a3bde54dbc73a0f3eda375162 Author: Simon Budig <simon@budig.de> Date: Tue Jan 11 23:28:16 2011 +0100 fixes for some buffer overflow problems (see bug #639203)