865919 - BMP and ICO decoders have issues if the height in the bitmap header is INT32_MIN

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Jeff Walden [:Waldo]]

I recently got inadvertently pointed at bug 775794, which suggests that it's a bad thing if an ICO file's height is interpreted as a negative value.  That bug was fixed by applying some abs() magic, roughly.

But as I noticed when working on bug 847480, abs() has the flaw that it's kind of not guaranteed to work on the most-negative value.  Behavior is technically undefined, but what often happens is the bits are inverted and one is added -- which on twos-complement gives you back the original value.

The BMP decoder has this issue for BMP files -- see bug 847140.  Per my analysis there, I don't *think* this poses any issue beyond an assertion in debug builds.  Subsequent validation checks in the main image processing code cause the image to be treated as malformed, in opt builds.

That the ICO decoder has the same sort of issue, I wasn't aware of until now.  Looking at the code, it's unclear to me whether the overlarge-multiplication issue that happened there, will still happen for INT32_MIN or not.  The consequences might not be as bad as it's going to be a multiply by a large value that's *constant*, but I really don't know for sure.

Could someone take a look at both of these abs() uses and make them handle INT32_MIN correctly, and particularly make sure there's no security concern with the ICO decoder's mishandling of it?

Comment 1

[Christoph Diehl [:posidron]]

We respect the file format specification for fuzzing ICO files - we should normally have seen issues there already eg. crashes in ASan enabled builds.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: use new MFBT Abs() in BMP and ICO decoders

Comment 3

[Joe Drew (not getting mail)]

Comment on attachment 742663 [details] [diff] [review]
use new MFBT Abs() in BMP and ICO decoders

Review of attachment 742663 [details] [diff] [review]:
-----------------------------------------------------------------

Presuming MFBT Abs() is smart, looks good to me.

Comment 4

[Jeff Walden [:Waldo]]

Doesn't that let the negative-height propagate, still?  The propagation was the big concern here, not so much the abs() use exactly (although that certainly needed fixing).  Abs is going to return uint32_t(1u << 31) there, which will wrap around back to int32_t(INT32_MIN) by the compiler, generally.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: 0xINT32_MIN.bmp (causes abort, don't load)

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: add test with INT32_MIN.bmp

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: The long way to shore everything up

Here's the more complete solution. We should look at a minimal fix with special casing for uplift purposes, though.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 763050 [details] [diff] [review]
patch: The long way to shore everything up

Review of attachment 763050 [details] [diff] [review]:
-----------------------------------------------------------------

I'd have expected the long way to be something like making the dimensions all uint32_t, and storing a bool to indicate invertedness or something.  Checked math seems like the sort of thing that's pretty nice internally as you process, but at interface boundaries like GetWidth/GetHeight I'd expect you'd only want to expose guaranteed-valid values, and that errors would have been handled earlier.  It's all your image code, of course, to do what you want with, but I'd approach this a bit differently.  (Totally agree about something spot-fixy for backporting, tho.)

Comment 9

[Joe Drew (not getting mail)]

Comment on attachment 763049 [details] [diff] [review]
patch: add test with INT32_MIN.bmp

Review of attachment 763049 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/test/mochitest/test_bug865919.html
@@ +23,5 @@
> +var req = new XMLHttpRequest();
> +req.onload  = function() { CallbackAssert(true,  'Request for file succeeded.'); };
> +req.onerror = function() { CallbackAssert(false, 'Request for file failed! Failed to test non-existent file.'); };
> +req.open('GET', 'INT32_MIN.bmp');
> +req.send(null);

why are you using an XHR here? just to ensure that it exists? If so, can you document as such?

@@ +29,5 @@
> +var outstandingCallbacks = 2;
> +
> +function CallbackAssert(assertVal, failText) {
> +  ok(assertVal, failText);
> +  

whitespace

Comment 10

[Joe Drew (not getting mail)]

Comment on attachment 763050 [details] [diff] [review]
patch: The long way to shore everything up

Review of attachment 763050 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/decoders/nsBMPDecoder.cpp
@@ +44,4 @@
>  // Convert from row (1..height) to absolute line (0..height-1)
> +  CheckedUint32 line = IsInverted() ? (GetHeight().value() - checkedRow)
> +                                    : (checkedRow - 1);
> +                                       

omggg whitespace

@@ +389,5 @@
>                PostDataError();
>                return;
>              }
>          }
> +        

whitespace

::: image/decoders/nsBMPDecoder.h
@@ +101,5 @@
> +    
> +    CheckedUint32 PixelOffset(uint32_t row, uint32_t col);
> +    
> +    CheckedUint32 mWidth;
> +    CheckedUint32 mHeight;

If, at any point, we underflow uint32_t, we should error out immediately. At future points, we should use CheckedInt appropriately to ensure we don't under/overflow. Otherwise, we should assume it's OK.

I don't want the BMP decoder to have a second copy of the size.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: The simple fix

This will fix the immediate problem.

I think it would still be good to do something similar to what was attempted in the 'long way' bug, where we probe the extents of the image to make sure the math works for all four corners.

Comment 12

[Joe Drew (not getting mail)]

Comment on attachment 768703 [details] [diff] [review]
patch: The simple fix

Review of attachment 768703 [details] [diff] [review]:
-----------------------------------------------------------------

I should mention that I wasn't against the previous version with CheckedInt that probed all sorts of stuff; I was against the second copy of the size being stored.

::: image/decoders/nsBMPDecoder.cpp
@@ +246,5 @@
>              PostDataError();
>              return;
>          }
>  
> +        if (GetHeight() < 0) {

Add "// overflow" or something like that.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Great. I'll fix up the CheckedInt version as time permits. I think we should land this more immediately, though.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: simple fix with comment

With a comment.

Comment 15

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 769248 [details] [diff] [review]
patch: simple fix with comment

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Constructing the bad image would be easy, though I don't know if it can be exploited further than DOS crashing.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes.

Which older supported branches are affected by this flaw?
All of them.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This same patch should work for backports.

How likely is this patch to cause regressions; how much testing does it need?
We have a test we'd like to add for it.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 769248 [details] [diff] [review]
patch: simple fix with comment

sec-approval+.

Technically, as a sec-audit, this doesn't need approval but looks pretty sane.

Comment 17

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/066a0110bd0b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dc02c10629c

Comment 18

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/5cf40d79e5a5 - at least on Linux (so far), debug mochitest-4 hits "ABORT: Height can't be negative!: 'aHeight >= 0', file ../../../image/src/Decoder.cpp, line 289" in test_bug865919.html

Comment 19

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=d87181896094

This appears to be a bug in the compiler we use on these machines. I've looked this over a number of times, and can't see any reason why this fix wouldn't work (it works locally, and on other platforms) on this platform on try.

I think GCC(?) is automatically dismissing tests for negative numbers for signed integers after an `abs()` call, unless they're separated by function boundaries.

Maybe Waldo can find something wrong with my previous approaches? (csets 'foldme' and 'foldme2') 'foldme3' actually finally gets it to assert, whereas without it, it only asserts in `PostSize()`. (Here's the Try run without 'foldme3': https://tbpl.mozilla.org/?tree=Try&rev=b060fa906195 )

Comment 20

[Jeff Walden [:Waldo]]

Discussed over IRL, sounds like INT_MIN is going to be special-cased here.

Comment 21

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=f8b32eed8b85

Comment 22

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Check for INT_MIN explicitly

Comment 23

[Joe Drew (not getting mail)]

Comment on attachment 782061 [details] [diff] [review]
patch: Check for INT_MIN explicitly

Review of attachment 782061 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/decoders/nsBMPDecoder.cpp
@@ +250,5 @@
> +        if (mBIH.height == INT_MIN) {
> +            PostDataError();
> +            return;
> +        }
> +        

whitespace. Should we also check width?

Comment 24

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Check for INT_MIN explicitly

Fixed the whitespace.
We don't need to check the width, since that's handled by erroring if width is <0. The trick with height is that height<0 is valid, just y-flipped. (though clearly we can't take abs(INT_MIN))

Comment 25

[Kelsey Gilbert [:jgilbert]]

Everything's affected, though I don't think this is that dangerous by itself.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 763049 [details] [diff] [review]
patch: add test with INT32_MIN.bmp

My understanding is that s-s bugs need sec-approval if they affect all branches and don't have a rating of less than sec-high. So either get approval or get a sec-low or sec-moderate rating before requesting checkin :-)

Comment 27

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 782904 [details] [diff] [review]
patch: Check for INT_MIN explicitly

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Easy construct an exploit to crash.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
This is a very specific patch.

Which older supported branches are affected by this flaw?
All of them.

If not all supported branches, which bug introduced the flaw?
Something ancient in the BMP decoder.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
It should just apply to old branches, given how little this code changes. Simple and pretty risk-free to make backports, though.

How likely is this patch to cause regressions; how much testing does it need?
There is a patch attached to this bug which includes the testcase. (Though it is also a POC exploit, when served to the browser)

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 782904 [details] [diff] [review]
patch: Check for INT_MIN explicitly


I'm not sure it needs approval since it isn't a sec-critical or high but sec-approval+.

I'll approve it for Aurora as well. We're too late for Beta since the last beta has been built and we're shipping and branching next week.

Comment 29

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3eae1636fa57
https://hg.mozilla.org/releases/mozilla-aurora/rev/5f25b50252f2

Still waiting on Inbound to reopen.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

(In reply to Al Billings [:abillings] from comment #28)
> I'm not sure it needs approval since it isn't a sec-critical or high but
> sec-approval+.

Sorry, I didn't see any sec rating at all, so I wasn't sure and I figured it was better to assume the worst in such a case.

Comment 31

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a0cfc63400f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4a9f6e12d4b

Yeah, according to comment 16, sec-audit appears to be a really low sec rating.

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f4a9f6e12d4b
https://hg.mozilla.org/mozilla-central/rev/7a0cfc63400f

Comment 33

[Al Billings [:abillings - ex-MoCo]]

This is a sec-audit so we're not going to take this on ESR17.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

(In reply to Al Billings [:abillings] from comment #33)
> This is a sec-audit so we're not going to take this on ESR17.

Same for b2g18?