Closed Bug 1223465 Opened 5 years ago Closed 5 years ago

AddressSanitizer: heap-buffer-overflow in mozilla::image::nsGIFDecoder2::DoLzw


(Core :: ImageLib, defect)

43 Branch
Not set



Tracking Status
firefox42 --- unaffected
firefox43 + fixed
firefox44 + fixed
firefox45 + fixed
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.5 --- fixed
b2g-v2.2r --- unaffected
b2g-master --- fixed


(Reporter: bc, Assigned: eflores)


(Blocks 1 open bug, )


(4 keywords, Whiteboard: [post-critsmash-triage][b2g-adv-main2.5-])


(3 files)

Attached file asan messages
2. ==24745==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x611000897aeb at pc 0x7f66bb754f59 bp 0x7f66a66e0eb0 sp 0x7f66a66e0ea8
WRITE of size 1 at 0x611000897aeb thread T22 (ImgDecoder #1)

Beta 43, Aurora 44, Nightly 45 Linux on a RHEL6 ESXi VM.

Also crashes at this same url with a variety of signatures on Linux, OSX, Windows 7
Bug 1213744 is similar, though that was fixed on 10/25.
Group: core-security → gfx-core-security
Yes, same complaints w/ valgrind, ending in terminal heap corruption + segv.
Seems that rowp is assigned to beyond what was allocated...
Assignee: nobody → edwin
See Also: → 1224185
This has two similar looking crashes in random JS engine stuff, so it sounds like whatever is happening is pretty bad, so I'm going to mark this sec-critical.
See Also: → 1224100
[Tracking Requested - why for this release]: sec-critical regression
Tracking for 43+  since this is a recent regression.
I can't reproduce this, but looking at the code and the ASan output, it looks like this is what's happening:

- The downscaler buffer is only the length of |screen_width| (plus a little extra room).
- Bug 1207378 changed Downscaler so that the buffer it returns is now offset by |frameRect.x|.
- nsGIFDecoder2 makes sure that |screen_width >= frameRect.width|, but not that |screen_width >= frameRect.width + frameRect.x|.

This means that we can run off the end of the downscaler buffer when |frameRect.x + frameRect.width| is greater than |screen_width|.
Attached patch 1223465.patchSplinter Review
Are you able to try with this patch? I can't reproduce the failure myself, but this *should* fix it.
Flags: needinfo?(bob)
(In reply to Edwin Flores [:eflores] [:edwin] from comment #8)
> Are you able to try with this patch? I can't reproduce the failure myself,
> but this *should* fix it.

Without this patch, I can no longer reproduce the V errors with the
URL shown in Comment 0, so I assume the page has changed.  However, I
can still reproduce the V errors using the URL shown at bug 1224100
comment 0, which we theorise as being a dup of this bug.  Hence I
infer that the bug is still un-fixed in m-c.

With the patch applied, I can no longer reproduce the V errors with
either URL.  So from my point of view, the patch looks good.

I should add that even with the patch applied, the URL in bug 1224100
still causes leaking of uninitialised memory via the PNG decoder into
the decoded image.  It seems very similar, but not identical, to bug
1100325, which is marked "csectype-uninitialized, sec-moderate" and is
still open.
Flags: needinfo?(bob)
(In reply to Julian Seward [:jseward] from comment #9)
> I should add that even with the patch applied, the URL in bug 1224100
> still causes leaking of uninitialised memory via the PNG decoder into
> the decoded image.  It seems very similar, but not identical, to bug
> 1100325, which is marked "csectype-uninitialized, sec-moderate" and is
> still open.

I think that's a separate issue, but will look into it to make sure.
Sorry for the delay. I've only see a signature beginning like this on two urls:

and neither reproduces at this time with today's builds.
Comment on attachment 8688037 [details] [diff] [review]

Review of attachment 8688037 [details] [diff] [review]:

Thank you very much for taking this on, Edwin. I don't love this approach (as we discussed on IRC) since it seems quite error-prone; we have to be very careful to use clamped_width/height in all the appropriate places. I'm working on a cleaner fix, but it'll likely be too large a patch for beta uplift, so let's go ahead and land this and get it uplifted everywhere it needs to go, and then we can worry about refactoring to ensure similar problems don't occur again in the future.

::: image/decoders/GIF2.h
@@ +74,5 @@
>      // Parameters for image frame currently being decoded
>      unsigned x_offset, y_offset; // With respect to "screen" origin
>      unsigned height, width;
> +    unsigned clamped_height, clamped_width;

Please add a comment. This might be easier to align if you declare them on separate lines. I generally prefer one declaration per line, and I believe our coding standards do as well, but I realize that from a poetic perspective you are preserving parallelism with the |height| and |width| declarations on the previous line, and I salute you for paying attention to aesthetics in a security bug.

::: image/decoders/nsGIFDecoder2.cpp
@@ +1116,5 @@
> +      IntRect clampedRect =
> +        ClampToImageRect(IntRect(mGIFStruct.x_offset, mGIFStruct.y_offset,
> +                                 mGIFStruct.width, mGIFStruct.height));
> +      if (clampedRect.IsEmpty()) {
> +        mGIFStruct.state = gif_error;

Ugh, someone is going to file a bug about this. It'd be preferable if we didn't treat this case as an error and just produced an empty, fully transparent image. If the code currently can't handle clamped_width / clamped_height being 0, we don't need to fix it here, but you should file a followup.

@@ +1120,5 @@
> +        mGIFStruct.state = gif_error;
> +        break;
> +      }
> +      mGIFStruct.clamped_width = clampedRect.width;
> +      mGIFStruct.clamped_height = clampedRect.height;

MOZ_ASSERT(mGIFStruct.clamped_width <= mGIFStruct.width), and same for height, since we depend on that for correctness.
Attachment #8688037 - Flags: review?(seth) → review+
Comment on attachment 8688037 [details] [diff] [review]

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It's pretty clear that we're playing with bounds checks here. It wouldn't be hard to construct a GIF to poke at the problem and crash the browser, but a "real" exploit arbitrarily writing to memory to do something horrible without crashing would be a lot more difficult.

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

Which older supported branches are affected by this flaw?
43 and up.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No, but uplift should be relatively straightforward.

How likely is this patch to cause regressions; how much testing does it need?
It's extremely unlikely to make things any worse than the regression it fixes.
Attachment #8688037 - Flags: sec-approval?
Comment on attachment 8688037 [details] [diff] [review]

sec-approval+ for trunk. Please nominate for Aurora and Beta approval so we can avoid shipping the flaw.
Attachment #8688037 - Flags: sec-approval? → sec-approval+
Comment on attachment 8688037 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: bug 1207378
[User impact if declined]: Crashes on some malformed GIFs
[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: Minimal -- can't make it much worse...
[String/UUID change made/needed]: None.
Attachment #8688037 - Flags: approval-mozilla-beta?
Attachment #8688037 - Flags: approval-mozilla-aurora?
Comment on attachment 8688037 [details] [diff] [review]

Regression in 43, bad crash, let's uplift this to aurora and beta.
Attachment #8688037 - Flags: approval-mozilla-beta?
Attachment #8688037 - Flags: approval-mozilla-beta+
Attachment #8688037 - Flags: approval-mozilla-aurora?
Attachment #8688037 - Flags: approval-mozilla-aurora+
Hi Bob, if you have time, could you check out this fix and see if it works for you?
Flags: needinfo?(bob)
Whiteboard: [post-critsmash-triage]
bughunter did not find any crashes on Beta/43, Aurora/44, Nightly/45 on Linux, OS X, Windows testing this url.
Group: gfx-core-security → core-security-release
Duplicate of this bug: 1224185
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][b2g-adv-main2.5-]
Depends on: 1234077
Depends on: 1234804
Duplicate of this bug: 1231071
Group: core-security-release
Depends on: 1267865
You need to log in before you can comment on or make changes to this bug.