GNOME Bugzilla – Bug 777374
Fix potential overflows in io-xbm and tests
Last modified: 2017-02-07 11:28:34 UTC
Two completely unrelated patches, which are only being submitted together because the Coverity issues were next to each other.
Created attachment 343626 [details] [review] tests: Fix signed/unsigned handling in pixdata_equal() The return values from the GdkPixbuf getters are signed, so assign them to signed variables and then check that the returned values are non-negative. Coverity IDs: 1388535, 1388536, 1388537 Signed-off-by: Philip Withnall <withnall@endlessm.com>
Created attachment 343627 [details] [review] io-xbm: Fix potential overflows in read_bitmap_file_data() ww and hh are both potentially tainted data (as they come from a potentially attacker controlled file), so we need to ensure that all arithmetic is bounds checked. Coverity ID: 1388540 Signed-off-by: Philip Withnall <withnall@endlessm.com>
Whoops, seems this doesn’t quite work. Update coming.
Created attachment 343628 [details] [review] io-xbm: Fix potential overflows in read_bitmap_file_data() ww and hh are both potentially tainted data (as they come from a potentially attacker controlled file), so we need to ensure that all arithmetic is bounds checked. Coverity ID: 1388540 Signed-off-by: Philip Withnall <withnall@endlessm.com>
Created attachment 343708 [details] [review] gdk-pixbuf-io: Add precondition checks for image dimensions Refuse to save an invalid GdkPixbuf: one which has negative width, height or number of channels. This allows us to put assertions for all those properties in the I/O code, which helps static analysis not go off on one because it thinks the image width could be negative. Coverity ID: 1388534 Signed-off-by: Philip Withnall <withnall@endlessm.com>
Created attachment 343889 [details] [review] io-ico: Add an assertion to clarify potential NULL pointer dereference At a first read through, it looks like the call to OneLine() could end up dereferencing context->pixbuf when it’s NULL. However, due to a combination of other checks in the caller, OneLine() will only be called after DecodeHeader() has set context->pixbuf to a valid object. Otherwise, if DecodeHeader() bails with an error, the pixbuf will never be dereferenced. Add a comment trying to explain this, and an assertion which backs it up more rigorously. Coverity ID: 1388531 Signed-off-by: Philip Withnall <withnall@endlessm.com>
Review of attachment 343626 [details] [review]: Sure. Remove the SoB though.
Review of attachment 343628 [details] [review]: Remove the SoB. Looks good otherwise. ::: gdk-pixbuf/io-xbm.c @@ +235,3 @@ padding = 0; + if (ww > G_MAXUINT - 7) Maybe move this up to before you do the padding calculations? Also please add a comment. @@ +252,3 @@ + + /* @bytes is guaranteed not to overflow (which could + * happen if @size is the odd-valued %G_MAXUINT) Can you please expand on this in-parens comment?
Review of attachment 343708 [details] [review]: Remove the SoB.
Review of attachment 343889 [details] [review]: Remove the SoB again. ::: gdk-pixbuf/io-ico.c @@ +947,3 @@ if ((context->LineDone >= context->LineWidth) && + (context->LineWidth > 0)) { + /* By this point, DecodeHeader() will have been Please use longer lines, we have wide screens, and we're not limited to 80 chars like in the kernel.
Review of attachment 343628 [details] [review]: ::: gdk-pixbuf/io-xbm.c @@ +235,3 @@ padding = 0; + if (ww > G_MAXUINT - 7) I’ve added a comment, but I don’t think moving it up before the padding calculation makes sense; it’s not needed for the padding calculation, and this means the overflow check is further from where the overflow could happen. I think that makes the code less clear.
Thanks for the reviews. SoBs dropped (I type -s on autopilot these days, sorry), and other comments fixed apart from the one I replied to above. Attachment 343626 [details] pushed as ebb88fe - tests: Fix signed/unsigned handling in pixdata_equal() Attachment 343628 [details] pushed as 3dacbb4 - io-xbm: Fix potential overflows in read_bitmap_file_data() Attachment 343708 [details] pushed as 71e3b4a - gdk-pixbuf-io: Add precondition checks for image dimensions Attachment 343889 [details] pushed as d5fc7e3 - io-ico: Add an assertion to clarify potential NULL pointer dereference
(In reply to Philip Withnall from comment #11) > Review of attachment 343628 [details] [review] [review]: > > ::: gdk-pixbuf/io-xbm.c > @@ +235,3 @@ > padding = 0; > > + if (ww > G_MAXUINT - 7) > > I’ve added a comment, but I don’t think moving it up before the padding > calculation makes sense; it’s not needed for the padding calculation, and > this means the overflow check is further from where the overflow could > happen. I think that makes the code less clear. Fair.