GNOME Bugzilla – Bug 600741
"read_channel_data()" Integer Overflow Vulnerability
Last modified: 2015-11-29 12:50:31 UTC
Secunia Research has discovered a vulnerability in Gimp, which potentially can be exploited by malicious people to compromise a user's system. The vulnerability is caused due to an integer overflow within the "read_channel_data()" function in plug-ins/file-psd/psd-load.c (confirmed in line 1764. Note that there are further calculations in e.g. line 1803, 1808, and 1813). This can be exploited to cause a heap-based buffer overflow by e.g. tricking a user into opening a specially crafted PSD file. Secunia Research has created a PoC, which is available upon request. The vulnerability is confirmed in version 2.6.7. Other versions may also be affected. We have assigned this vulnerability Secunia advisory SA37232 and CVE identifier CVE-2009-1570, which will be shared with the BMP integer overflow reported to Sven Neumann on Monday. If you prefer a separate CVE identifier, we can assign a new one. A preliminary disclosure date of 25-11-2009 10am CET has been set, where the details will be publicly disclosed. However, we are naturally prepared to push the disclosure date if you need more time to address the vulnerability. Please acknowledge receiving this e-mail and let us know when you expect to fix the vulnerability. Additionally, we would like to be able to follow the bug report regarding the BMP integer overflow. Credits should go to: Stefan Cornelius, Secunia Research
Created attachment 147344 [details] [review] start of a patch against int overflow issues in the PSD plugin Ok, this is the start of a patch against int overflow issues in the PSD loader. This is quite nasty code and I have trouble tracing back the variables. There are two #warnings in the patch that show potential problems not yet addressed.
+#warning what can happen here? pixels = g_malloc (layer_size); Well, the plug-in can exit with an out-of-memory error. Or the OS may decide to grant the plug-in the memory it asks for, which may put quite a bit of load on the system. But I don't think we have a chance to catch this in a reasonable way.
Note that the layer_size is a guint32, and gets computed by layer_size = lm_w * lm_h. So there is another potential overflow issue there, later the memory gets written by two nested loops. The warning really is just a reminder for me to check this issue, I need to trace back where the numbers lm_w and lm_h come from and if they are checked against int overflow already.
Review of attachment 147344 [details] [review]: Thanks a lot for looking into this Simon. Setting to 'needs-work' to reflect your comments on the patch.
Created attachment 147676 [details] [review] My patch for this issue I'm on the security list too, and saw the email. This is a patch that I had written for the bug and was showing to others on IRC. Didn't know this bug existed for it :)
Created attachment 147749 [details] [review] Patch against int overflow issues in the PSD plugin Ok, here is an updated version of the patch and it is ready for commit from my side of things. I don't have lots of PSD to check if they still load properly, it would be useful if someone could do some tests on that.
I haven't reviewed the whole PSD code, but the last hunk of the patch could potentially overflow the expression type (gint64) if channel->rows, channel->columns and bps aren't guaranteed to be less than certain max values: + + /* int overflow check */ + if (((gint64) channel->rows) * channel->columns * MAX (bps >> 3, 1) > G_MAXINT32) + { + g_set_error (error, G_FILE_ERROR, G_FILE_ERROR_FAILED, + _("Unsupported or invalid channel size")); + return -1; + } + E.g. if both rows and columns were G_MAXUINT32 (0xFFFFFFFF) and bps were 16, the expression would evaluate to 0x1fffffffc00000002 which is still an order of magnitude greater than G_MAXUINT64, i.e. it would overflow. While the other comparisons in the patch look safe to me (even though they use multiplication), I'd still suggest using divisions as a defensive measure because they cannot overflow (and get by without casts to larger types -- useful if you have to check numbers of the largest type available), e.g. replace the above comparison with: if (channel->rows > G_MAXINT32 / channel->columns / MAX (bps >> 3, 1)) ...
Created attachment 147866 [details] [review] Use more defensive code for type boundary checks. This uses divisions (which cannot overflow) for type boundary checks. Apply on top of Simon's patch (attachment #147749 [details]).
Review of attachment 147866 [details] [review]: Doesn't completely avoid dividing by zero.
Created attachment 147917 [details] [review] Use more defensive code for type boundary checks. Avoid division by zero. This uses divisions (which cannot overflow) for type boundary checks. It now even avoids dividing by zero and documents where this is already done by existing sanity checks. Apply on top of Simon's patch (attachment #147749 [details]).
Created attachment 147944 [details] [review] Patch against the int overflow issue in the psd plugin This patch is a new version for the int overflow problem. This incorporates the "division" approach for the checks by Nils and also adds a few more sanity checks (it right now barfs on channel data with ->rows or ->columns = 0. if there are PSDs in the wild that have this rather weird property the code is unlikely to do the right thing there). Note that this patch might refuse to load images when not also using commit 0e440cb6d4d6ee029667363d244aff61b154c33c (already in master and gimp-2-6), which corrects the signedness of the top/bottom/left/right bbox coordinates in the PSD structs.
Review of attachment 147944 [details] [review]: The CVE identifier is CVE-2009-3909 (-1570 is a separate one for the BMP issue). Otherwise the patch looks good to me.
Fixed in both master and gimp-2-6: commit 9cc8d78ff33b7a36852b74e64b427489cad44d0e Author: Simon Budig <simon@gimp.org> Date: Tue Nov 17 00:41:39 2009 +0100 Harden the PSD plugin against integer overflows. Issues discovered by Stefan Cornelius, Secunia Research, advisory SA37232 and CVE identifier CVE-2009-3909. Fixes bug #600741.