1224100 - "Conditional jump or move depends on uninitialised value(s)" at imgFrame::Optimize

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bob Clary [:bc] (inactive)]

1. http://www.ref4bux.com/index.php?view=ads

2. Spin the wheel to determine your Crash signature.

bp-29c4ae76-8643-4453-a758-3b4da2151112
[@ js::jit::EnterBaselineMethod ]
exploitability low

bp-3e41d6cd-49e4-4ab2-86ef-1357c2151112
[@ nsDisplayList::SortByZOrder ]

bp-001dcdfd-2e77-454e-af93-96fb92151112
[@ AddRule ]

Bughunter shows crashes across the board on all branches/platforms for opt and debug with a variety of stacks/signatures including some rated low on Windows -> S-S

Asan reports 

==18315==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x619000d308f0 at pc 0x7f8c08342d5e bp 0x7f8bf0954e20 sp 0x7f8bf0954e18
==18315==AddressSanitizer: while reporting a bug found another one.Ignoring.

Comment 1

[Julian Seward [:jseward]]

I suspect this is the same as bug 1223465
(AddressSanitizer: heap-buffer-overflow in mozilla::image::nsGIFDecoder2::DoLzw)
or at least is somehow related.  I see invalid writes in 
mozilla::image::nsGIFDecoder2::DoLzw.   These also are present in 1223465.

Comment 2

[Julian Seward [:jseward]]

Attachment: Valgrind complaints and eventual crash

Comment 3

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

Tracking since this may be a recent regression and it also seems to be a high volume crash (for  js::jit::EnterBaselineMethod)

Tomcat mentioned it may be a regression from bug 1207378, but that landed in 43 and these signatures are showing up on 42 as well.

Comment 4

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

We're going to operate under the assumption that this is the same as bug 1223465 for now.

Comment 5

[Julian Seward [:jseward]]

(In reply to Milan Sreckovic [:milan] from comment #4)
> We're going to operate under the assumption that this is the same as bug
> 1223465 for now.

See bug 1223465 comment 9 for more details.

Comment 6

[Julian Seward [:jseward]]

With the patch at bug 1223465 comment 8 applied, this no longer crashes.

However, we are still leaking uninitialised memory into the decoded image.
Should I file that as a separate bug, or add the details here?

Comment 7

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

We can modify this bug to track the uninitialized memory used in the decoded image, changing the summary if necessary.

Comment 8

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

Attachment: 1224100.patch

Comment 9

[Julian Seward [:jseward]]

Comment on attachment 8688921 [details] [diff] [review]
1224100.patch

Works for me. f+

Comment 10

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

Comment on attachment 8688921 [details] [diff] [review]
1224100.patch

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

Looks good, but I'd prefer a change before landing; see below.

::: image/Downscaler.cpp
@@ +111,5 @@
>      return NS_ERROR_OUT_OF_MEMORY;
>    }
>  
> +  // Zero padding to keep valgrind happy.
> +  memset(mRowBuffer.get() + mOriginalSize.width * sizeof(uint32_t), 0, 15);

I think I'd be more comfortable just zero'ing the entire row buffer, just so we're not reliant on correct behavior by the decoders to avoid leaking uninitialized memory.

Comment 11

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

Comment on attachment 8688921 [details] [diff] [review]
1224100.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Extremely unlikely. There are only 15 bytes of uninitialised memory here, the results of which would be cropped from the output anyway.

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.

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?
No; uplift trivial.

How likely is this patch to cause regressions; how much testing does it need?
Patch is trivial and doesn't make any functional changes.

Comment 12

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

Comment on attachment 8688921 [details] [diff] [review]
1224100.patch

sec-approval+ for trunk. We'll want patches made and nominated for Aurora, Beta, and ESR38.

Comment 13

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

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

Comment 14

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

Er, that should be:

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

Comment 15

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

Attachment: Rebased to 43

Approval Request Comment
[Feature/regressing bug #]: Image downscaler.
[User impact if declined]: Small possibility of data leakage.
[Describe test coverage new/current, TreeHerder]: None.
[Risks and why]: None.
[String/UUID change made/needed]: None.

Comment 16

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

Comment on attachment 8688921 [details] [diff] [review]
1224100.patch

Approval Request Comment
[Feature/regressing bug #]: Image downscaler.
[User impact if declined]: Small possibility of data leakage.
[Describe test coverage new/current, TreeHerder]: None.
[Risks and why]: None.
[String/UUID change made/needed]: None.

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

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

Comment on attachment 8691944 [details] [diff] [review]
Rebased to 43

Crash fix, please uplift this patch (only) to beta.

Comment 19

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

Comment on attachment 8688921 [details] [diff] [review]
1224100.patch

Crash fix, ok to uplift to aurora.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd29d7c987b5
 https://hg.mozilla.org/releases/mozilla-beta/rev/e6edea42ef9b

Comment 21

[Carsten Book [:Tomcat]]

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

Comment 22

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

Hi Bob, can you verify this fix?

Comment 23

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

Comment 24

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

Is there a reason we didn't take this on ESR38 since it is affected and this is a sec-critical?

Comment 25

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

Adding Sylvestre as I've been told he's doing approvals for ESR38.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8691944 [details] [diff] [review]
Rebased to 43

Taking it in ESR (it had no uplift request). Thanks for the ping.

Comment 27

[Carsten Book [:Tomcat]]

does not apply cleanly to esr38

adding 1224100esr.patch to series file
applying 1224100esr.patch
unable to find 'image/Downscaler.cpp' for patching
1 out of 1 hunks FAILED -- saving rejects to file image/Downscaler.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 1224100esr.patch

Comment 28

[Daniel Veditz [:dveditz]]

The critical bits were fixed in other bugs, memory disclosure via images is sec-high.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/f6c1116a4295