Possible integer overflow in allocation size in nsBMPEncoder::AddImageFrame?

RESOLVED FIXED in Firefox -esr45

Status

()

Core
ImageLib
RESOLVED FIXED
8 months ago
22 days ago

People

(Reporter: mats, Assigned: tnikkel)

Tracking

({csectype-intoverflow, sec-high})

unspecified
mozilla55
csectype-intoverflow, sec-high
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr4553+ fixed, firefox52 wontfix, firefox-esr5253+ fixed, firefox53+ fixed, firefox54+ fixed, firefox55+ fixed)

Details

(Whiteboard: [adv-main53+][adv-esr45.9+][adv-esr52.1+])

Attachments

(1 attachment)

(Reporter)

Description

8 months ago
Are the values multiplied here controlled by content?
http://searchfox.org/mozilla-central/rev/557f236c19730116d3bf53c0deef36362cafafcd/image/encoders/bmp/nsBMPEncoder.cpp#190
If so, I think this can lead to integer overflow and might be exploitable.

Updated

8 months ago
Group: core-security → gfx-core-security
Nick, could you maybe take a look at this?
Flags: needinfo?(n.nethercote)
Keywords: csectype-intoverflow
This is in the BMP *encoder*, not decoder. (And the BMP encoder is also used by the ICO encoder.)

I don't really understand how the BMP encoder is used. It's certainly less directly exposed to web content than the BMP decoder. image/build/nsImageModule.cpp, where there is NS_BMPENCODER_CID which can be summoned via "@mozilla.org/image/encoder;2?type=image/bmp". I don't really know how that gets used, though.

tnikkel, do you know about this stuff?
Flags: needinfo?(n.nethercote) → needinfo?(tnikkel)
(Assignee)

Comment 3

8 months ago
Not really, no. But I'm pretty sure the bmp encoder is exposed to the web via canvas.toDataURL(...,"image/bmp",...) ie

https://dxr.mozilla.org/mozilla-central/source/dom/canvas/test/test_toDataURL_alpha.html#192

Other starting points in the tree imgIEncoder class

https://dxr.mozilla.org/mozilla-central/source/image/imgIEncoder.idl

And some of the users of that interface

+callers:"nsBMPEncoder::InitFromData(const uint8_t *, uint32_t, uint32_t, uint32_t, uint32_t, uint32_t, const nsAString &)"

which shows users in dom/base/ImageEncoder.cpp, gfx/thebes/gfxUtils.cpp, image/imgTools.cpp.

It might just be easier to fix this then slog through the source code.
Flags: needinfo?(tnikkel)
This stuff's all a bit dodgy:

>  auto row = MakeUniqueFallible<uint8_t[]>(mBMPInfoHeader.width *
>                                           BytesPerPixel(mBMPInfoHeader.bpp));

Here |mBMPInfoHeader.width| is an |int32_t| and |mBMPInfoHeader.bpp| is a |uint16_t|. BytesPerPixel promotes bpp to a uint32_t, that doesn't really matter but it's a point of inconsistency for later.

The only place |mBMPInfoHeader.width| is set is in |nsBMPEncoder::InitInfoHeader| [1] which takes a |uint32_t aWidth| and shoves it in a |int32_t width|. It also takes a |uint32_t aBpp| and shoves it in a |uint16_t bpp|. So we've got unsigned -> signed conversion which is a bit sketchy. If you dig a bit further out it looks like width from a canvas comes in as an int32_t, gets converted to a uint32_t at some point, then gets converted back to an int32_t when we store it in the header and then here we're doing multiplication of |int32_t| * |uint32_t| and passing that to a function that takes a |size_t|.

So yes it appears it can overflow and the values are somewhat user controlled. Arguably we shouldn't be storing width as a signed int in the header struct at all but maybe that's spec driven? Given that the only function with access to it takes an unsigned int we should just change it.

|bpp| can really only be 24 or 32, it comes from |nsBMPEncoder::ParseOptions| [2], so width is the main concern here.

[1] http://searchfox.org/mozilla-central/rev/c48398abd9f0f074c69f2223260939e30e8f99a8/image/encoders/bmp/nsBMPEncoder.cpp#509
[2] http://searchfox.org/mozilla-central/rev/c48398abd9f0f074c69f2223260939e30e8f99a8/image/encoders/bmp/nsBMPEncoder.cpp#258,262,300,302
I agree with tnikkel that it looks like fixing it is worthwhile.

BMP doesn't really have a spec, but e.g. https://en.wikipedia.org/wiki/BMP_file_format says that the height and width are signed 32-bit integers. I know that the height has to be signed because it can be negative, which means that the lines are encoded from top to bottom instead of bottom to top (or vice versa, I don't remember which way it goes).

I don't think there's any particular reason why the width has to be signed; I think it's just like that for consistency with the height. The BMP decoder rejects images with negative widths:

>  // BMPs with negative width are invalid. Also, reject extremely wide images
>  // to keep the math sane. And reject INT_MIN as a height because you can't
>  // get its absolute value (because -INT_MIN is one more than INT_MAX).
>  const int32_t k64KWidth = 0x0000FFFF;
>  bool sizeOk = 0 <= mH.mWidth && mH.mWidth <= k64KWidth &&
>                mH.mHeight != INT_MIN;
(Assignee)

Comment 6

8 months ago
Created attachment 8850905 [details] [diff] [review]
patch

It's the weekend for Nick now, so I'll pick on you Eric.

I went very far down this rabbit hole. I think every multiplication should be guarded by a checkedint check. And I think I removed all the cases of bad type conversion.

This is gross. I sure hope there is a better way.
Assignee: nobody → tnikkel
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8850905 - Flags: review?(erahm)
Comment on attachment 8850905 [details] [diff] [review]
patch

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

It's not my weekend any more...

I think this is exactly as gross as it needs to be :)  Thank you for fixing.
Attachment #8850905 - Flags: review?(erahm) → review+
(Assignee)

Comment 8

8 months ago
Comment on attachment 8850905 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

usually the values that might overflow come from the size of an existing surface, so we'd have to already have an extremely large surface to get this far.

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

the problem is pretty obvious from the patch, no way to really hide that integer overflow is possible here

Which older supported branches are affected by this flaw?

probably all of them

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

see above

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

should be easy

How likely is this patch to cause regressions; how much testing does it need?

should be safe
Attachment #8850905 - Flags: sec-approval?
status-firefox52: --- → wontfix
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr45: --- → affected
status-firefox-esr52: --- → affected
tracking-firefox53: --- → ?
tracking-firefox54: --- → ?
tracking-firefox55: --- → ?
tracking-firefox-esr45: --- → ?
tracking-firefox-esr52: --- → ?
I'm calling this a sec-high and giving it sec-approval. We'll want this on affected branches as well.
tracking-firefox53: ? → +
tracking-firefox54: ? → +
tracking-firefox55: ? → +
tracking-firefox-esr45: ? → 53+
tracking-firefox-esr52: ? → 53+
Keywords: sec-high
Attachment #8850905 - Flags: sec-approval? → sec-approval+
Duplicate of this bug: 1352122
Can you request uplift? Thanks.
Flags: needinfo?(tnikkel)
(Assignee)

Comment 12

8 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ada34cba00522abfd3ddc3784f1adaa796a10d64
(Assignee)

Comment 13

8 months ago
backout because I need to use try server
https://hg.mozilla.org/integration/mozilla-inbound/rev/8778b9d432f1cbef670e460d52cd255e631dad04
(Assignee)

Comment 14

8 months ago
I forgot the ! in a few isValid checks and failed at using parenthesis correctly in one place.

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=37d0f0947ff8117018996abeb578b8a7a2ae4320

relanding:
https://hg.mozilla.org/integration/mozilla-inbound/rev/708c303cb8383311a413180007e74c114cdc8ec2
https://hg.mozilla.org/mozilla-central/rev/708c303cb838
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 16

8 months ago
Comment on attachment 8850905 [details] [diff] [review]
patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: possible sec issue
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): not risky
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: I guess bug 670466 which added the bmp encoder back in 2011 and Firefox 9
[User impact if declined]: possible sec issue
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not the kind of thing you can verify
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: just makes sure some multiplications that represent the size of an in memory bitmap don't overflow
[String changes made/needed]: none
Flags: needinfo?(tnikkel)
Attachment #8850905 - Flags: approval-mozilla-esr52?
Attachment #8850905 - Flags: approval-mozilla-esr45?
Attachment #8850905 - Flags: approval-mozilla-beta?
Attachment #8850905 - Flags: approval-mozilla-aurora?
Comment on attachment 8850905 [details] [diff] [review]
patch

sec-high, let's take this up to beta. Should land for the beta 10 build later this week.
Attachment #8850905 - Flags: approval-mozilla-beta?
Attachment #8850905 - Flags: approval-mozilla-beta+
Attachment #8850905 - Flags: approval-mozilla-aurora?
Attachment #8850905 - Flags: approval-mozilla-aurora+
Comment on attachment 8850905 [details] [diff] [review]
patch

integer overflow fix, sec-high, esr45+/esr52+
Attachment #8850905 - Flags: approval-mozilla-esr52?
Attachment #8850905 - Flags: approval-mozilla-esr52+
Attachment #8850905 - Flags: approval-mozilla-esr45?
Attachment #8850905 - Flags: approval-mozilla-esr45+

Comment 19

8 months ago
uplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/e1e5ac908ce3
https://hg.mozilla.org/releases/mozilla-beta/rev/47919d9e58bf
https://hg.mozilla.org/releases/mozilla-esr52/rev/39da731d80ed

This doesn't graft cleanly to ESR45. Can you please attach a rebased patch?
status-firefox53: affected → fixed
status-firefox54: affected → fixed
status-firefox-esr52: affected → fixed
Flags: needinfo?(tnikkel)
(Assignee)

Comment 20

8 months ago
The conflicts come from bug 1278157. I think we should just uplift that too: the risk of any mistakes in that patch is much much lower than the risk that I make a mistake in rewriting the patch for this bug. I requested uplift in that bug.
Flags: needinfo?(tnikkel)
Group: gfx-core-security → core-security-release
Setting qe-verify- based on Timothy's assessment on manual testing needs (see Comment 16) and the fact that this fix has automated coverage.
Flags: qe-verify-

Comment 22

7 months ago
uplift
https://hg.mozilla.org/releases/mozilla-esr45/rev/4fe9b979b84d
status-firefox-esr45: affected → fixed
Whiteboard: [adv-main53+][adv-esr45.9+][adv-esr52.1+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.