Closed Bug 865919 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 + fixed
firefox25 + fixed
firefox-esr17 - wontfix
b2g18 - wontfix
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- affected
b2g-v1.1hd --- wontfix

People

(Reporter: Waldo, Assigned: jgilbert)

Details

(Keywords: sec-audit, Whiteboard: [adv-main24-])

Attachments

(4 files, 4 obsolete files)

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?
We respect the file format specification for fuzzing ICO files - we should normally have seen issues there already eg. crashes in ASan enabled builds.
Keywords: sec-audit
Assignee: nobody → jgilbert
Attachment #742663 - Flags: review?(joe)
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.
Attachment #742663 - Flags: review?(joe) → review+
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.
Attachment #742663 - Attachment is obsolete: true
Attachment #763049 - Flags: review?(joe)
Here's the more complete solution. We should look at a minimal fix with special casing for uplift purposes, though.
Attachment #763050 - Flags: review?(joe)
Attachment #763050 - Flags: feedback?(jwalden+bmo)
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.)
Attachment #763050 - Flags: feedback?(jwalden+bmo)
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
Attachment #763049 - Flags: review?(joe) → review+
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.
Attachment #763050 - Flags: review?(joe) → review-
Attached patch patch: The simple fix (obsolete) — Splinter Review
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.
Attachment #768703 - Flags: review?(joe)
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.
Attachment #768703 - Flags: review?(joe) → review+
Great. I'll fix up the CheckedInt version as time permits. I think we should land this more immediately, though.
Attached patch patch: simple fix with comment (obsolete) — Splinter Review
With a comment.
Attachment #768703 - Attachment is obsolete: true
Attachment #769248 - Flags: review+
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.
Attachment #769248 - Flags: sec-approval?
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.
Attachment #769248 - Flags: sec-approval? → sec-approval+
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
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 )
Flags: needinfo?(jwalden+bmo)
Discussed over IRL, sounds like INT_MIN is going to be special-cased here.
Flags: needinfo?(jwalden+bmo)
Attachment #769248 - Attachment is obsolete: true
Attachment #782061 - Flags: review?(joe)
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?
Attachment #782061 - Flags: review?(joe) → review+
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))
Attachment #782061 - Attachment is obsolete: true
Attachment #782904 - Flags: review+
Attachment #782904 - Flags: checkin?
Attachment #763049 - Flags: checkin?
Everything's affected, though I don't think this is that dangerous by itself.
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 :-)
Attachment #763049 - Flags: checkin?
Attachment #782904 - Flags: checkin?
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)
Attachment #782904 - Flags: sec-approval?
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.
Attachment #782904 - Flags: approval-mozilla-aurora+
Attachment #782904 - Flags: sec-approval? → sec-approval+
(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.
https://hg.mozilla.org/mozilla-central/rev/f4a9f6e12d4b
https://hg.mozilla.org/mozilla-central/rev/7a0cfc63400f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
This is a sec-audit so we're not going to take this on ESR17.
Whiteboard: [adv-main24-]
(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?
Group: core-security
You need to log in before you can comment on or make changes to this bug.