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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: asan messages

1. http://www.lesiteimmo.com/immobilier/immo/la-fleche-72200/2
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

Comment 1

[Andrew McCreight [:mccr8]]

Bug 1213744 is similar, though that was fixed on 10/25.

Comment 2

[Julian Seward [:jseward]]

Attachment: Valgrind complaints and eventual crash

Yes, same complaints w/ valgrind, ending in terminal heap corruption + segv.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Seems that rowp is assigned to beyond what was allocated...

Comment 4

[Andrew McCreight [:mccr8]]

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.

Comment 5

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: sec-critical regression

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 43+  since this is a recent regression.

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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|.

Comment 8

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1223465.patch

Are you able to try with this patch? I can't reproduce the failure myself, but this *should* fix it.

Comment 9

[Julian Seward [:jseward]]

(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.

Comment 10

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(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.

Comment 11

[Bob Clary [:bc] (inactive)]

Sorry for the delay. I've only see a signature beginning like this on two urls:

http://www.01net.com/outils/telecharger/windows/Securite/anti-spyware/fiches/tele26157.html
http://www.lesiteimmo.com/immobilier/immo/la-fleche-72200/2

and neither reproduces at this time with today's builds.

Comment 12

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8688037 [details] [diff] [review]
1223465.patch

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.

Comment 13

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8688037 [details] [diff] [review]
1223465.patch

[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?
No.

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.

Comment 14

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

Comment on attachment 8688037 [details] [diff] [review]
1223465.patch

sec-approval+ for trunk. Please nominate for Aurora and Beta approval so we can avoid shipping the flaw.

Comment 15

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8688037 [details] [diff] [review]
1223465.patch

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.

Comment 16

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a69d71324577

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a69d71324577

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8688037 [details] [diff] [review]
1223465.patch

Regression in 43, bad crash, let's uplift this to aurora and beta.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b1361284d64d
https://hg.mozilla.org/releases/mozilla-beta/rev/8d145fbf233f

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/b1361284d64d

Comment 21

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Bob, if you have time, could you check out this fix and see if it works for you?

Comment 22

[Bob Clary [:bc] (inactive)]

bughunter did not find any crashes on Beta/43, Aurora/44, Nightly/45 on Linux, OS X, Windows testing this url.