GNOME Bugzilla – Bug 778584
gif: Prevent access to negative array indexes
Last modified: 2017-12-04 16:32:37 UTC
It seems that a pathological gif file can cause a negative array index to be read. UBSAN reported this: io-gif.c:509:44: runtime error: index -2 out of bounds for type 'guchar [280]' io-gif.c:510:44: runtime error: index -1 out of bounds for type 'guchar [280]'
Created attachment 345696 [details] file exposing the issue
Created attachment 345697 [details] [review] patch
Created attachment 345698 [details] [review] left shift patch
Created attachment 363390 [details] [review] patch changing the initialisation to 2 The Gif patch breaks existing tests: ./tests/pixbuf-short-gif-write /animation/short_gif_write: ** ERROR:pixbuf-short-gif-write.c:32:loader_write_from_channel: assertion failed (error == NULL): Expected last byte to be >= 2, but is 0 (gdk-pixbuf-error-quark, 5) fish: “./tests/pixbuf-short-gif-write” terminated by signal SIGABRT (Abort) But the unpatched behaviour might as well be broken, given that UBSan complains with the messages I put in comment 0. I cooked up a patch that seems to avoid the unintended array access.
The GIMP initialises that value to "2" also. Thanks to Federico for finding that. https://git.gnome.org/browse/gimp/tree/plug-ins/common/file-gif-load.c?id=fe3339e2156c3edf540bf548e8b149abe0f0a369#n693
Yes, please initialize it to 2; I think it's OK to also have the check as in your comment #2. Specifically, the GIMP uses a buffer of the same size as gdk-pixbuf's. To avoid a special case during the first read, it initializes the offset to 2; the buffer is zeroed out in the beginning, so the code in question just copies two zeros to the beginning of the buffer.
Review of attachment 363390 [details] [review]: ::: gdk-pixbuf/io-gif.c @@ +1167,3 @@ context->code_curbit = 0; context->code_lastbit = 0; + /* We later substract 2 from this value to peek into a buffer. > later Please be more precise as to where we do that. Please also add a note pointing to this comment where the "-2" is done. @@ +1168,3 @@ context->code_lastbit = 0; + /* We later substract 2 from this value to peek into a buffer. + In order to not get negative array index later, we set the value Missing "*" at the start of the line. @@ +1170,3 @@ + In order to not get negative array index later, we set the value + to that magic 2 now. + */ You can finish the comment on the same line as the last bits of text.
Are those 2 early patches also to review? Could you please attach the test file as a patch as well?
Created attachment 364884 [details] [review] gif: Initialise code_last_byte to not cause undefined behaviour Currently, code_last_byte is set only after it has been used, i.e. context->block_buf[0] = context->block_buf[context->code_last_byte - 2]; comes before anything has touched context->code_last_byte yet. Except for the initialisation. context->code_last_byte is set a few lines later, though. And nowhere else, except for the initialisation which sets it to 0. That will inevitably lead to context->block_buf[-2] which is undefined behaviour. We hence set the code_last_byte to 2 in order to not make that array index invalid.
(In reply to Bastien Nocera from comment #8) > Are those 2 early patches also to review? yes. > Could you please attach the test file as a patch as well? It's in https://bugzilla.gnome.org/attachment.cgi?id=34569
(In reply to Tobias Mueller from comment #10) > (In reply to Bastien Nocera from comment #8) > > Are those 2 early patches also to review? > yes. > > Could you please attach the test file as a patch as well? > It's in https://bugzilla.gnome.org/attachment.cgi?id=34569 I'm guessing you ate the last digit and meant 345697. Thanks.
Review of attachment 345697 [details] [review]: I've split up the test image in a separate patch as well. ::: gdk-pixbuf/io-gif.c @@ +510,3 @@ + if (context->code_last_byte < 2) + { The coding style is different in this file. @@ +514,3 @@ + GDK_PIXBUF_ERROR, + GDK_PIXBUF_ERROR_FAILED, + _("Expected last byte to be >= 2, but is %d"), This is far too technical, this error message would appear in user interfaces. I'll try to find a more generic error message that doesn't add new translatable strings.
Created attachment 364925 [details] [review] gif: Prevent access to negative array indexes It seems that a pathological gif file can cause a negative array index to be read. UBSAN reported this: io-gif.c:509:44: runtime error: index -2 out of bounds for type 'guchar [280]' io-gif.c:510:44: runtime error: index -1 out of bounds for type 'guchar [280]'
Created attachment 364926 [details] [review] tests: Add test for bug 778584
Created attachment 364927 [details] [review] gif: Preventing undefined behaviour by making left shift unsigned io-gif.c:509:44: runtime error: index -2 out of bounds for type 'guchar [280]' io-gif.c:510:44: runtime error: index -1 out of bounds for type 'guchar [280]' io-gif-animation.c:422:68: runtime error: left shift of 249 by 24 places cannot be represented in type 'int'
Created attachment 364928 [details] [review] gif: Initialise code_last_byte to not cause undefined behaviour Currently, code_last_byte is set only after it has been used, i.e. context->block_buf[0] = context->block_buf[context->code_last_byte - 2]; comes before anything has touched context->code_last_byte yet. Except for the initialisation. context->code_last_byte is set a few lines later, though. And nowhere else, except for the initialisation which sets it to 0. That will inevitably lead to context->block_buf[-2] which is undefined behaviour. We hence set the code_last_byte to 2 in order to not make that array index invalid.
Attachment 364925 [details] pushed as 23e2a7c - gif: Prevent access to negative array indexes Attachment 364926 [details] pushed as af94eab - tests: Add test for bug 778584 Attachment 364927 [details] pushed as ced897e - gif: Preventing undefined behaviour by making left shift unsigned Attachment 364928 [details] pushed as c1fd9f5 - gif: Initialise code_last_byte to not cause undefined behaviour