Closed Bug 1224100 Opened 9 years ago Closed 8 years ago

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

Categories

(Core :: Graphics: ImageLib, defect)

41 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla45
Tracking Status
firefox42 --- wontfix
firefox43 + fixed
firefox44 + fixed
firefox45 + fixed
firefox-esr38 43+ fixed
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- wontfix
b2g-v2.1S --- wontfix
b2g-v2.2 --- affected
b2g-v2.5 --- fixed
b2g-v2.2r --- affected
b2g-master --- fixed

People

(Reporter: bc, Assigned: eflores)

References

Details

(Keywords: crash, csectype-bounds, sec-high, Whiteboard: [asan][post-critsmash-triage][adv-main43+][adv-esr38.5+])

Crash Data

Attachments

(3 files)

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.
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.
See Also: → 1224185
Group: core-security → gfx-core-security
Component: General → ImageLib
See Also: → 1223465
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.
We're going to operate under the assumption that this is the same as bug 1223465 for now.
(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.
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?
We can modify this bug to track the uninitialized memory used in the decoded image, changing the summary if necessary.
Assignee: nobody → edwin
Summary: AddressSanitizer: heap-buffer-overflow - Wheel of crashes [@ js::jit::EnterBaselineMethod ] | [@ nsDisplayList::SortByZOrder ] | [@ AddRule ] → "Conditional jump or move depends on uninitialised value(s)" at imgFrame::Optimize
Comment on attachment 8688921 [details] [diff] [review]
1224100.patch

Works for me. f+
Attachment #8688921 - Flags: feedback+
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.
Attachment #8688921 - Flags: review?(seth) → review+
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.
Attachment #8688921 - Flags: sec-approval?
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.
Attachment #8688921 - Flags: sec-approval? → sec-approval+
Attached patch Rebased to 43Splinter Review
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.
Attachment #8691944 - Flags: review+
Attachment #8691944 - Flags: approval-mozilla-beta?
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.
Attachment #8688921 - Flags: approval-mozilla-aurora?
Comment on attachment 8691944 [details] [diff] [review]
Rebased to 43

Crash fix, please uplift this patch (only) to beta.
Attachment #8691944 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8688921 [details] [diff] [review]
1224100.patch

Crash fix, ok to uplift to aurora.
Attachment #8688921 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Bob, can you verify this fix?
Flags: needinfo?(bob)
Whiteboard: [asan] → [asan][post-critsmash-triage]
bughunter did not find any crashes on Beta/43, Aurora/44, Nightly/45 on Linux, OS X, Windows testing this url.
Status: RESOLVED → VERIFIED
Flags: needinfo?(bob)
Group: gfx-core-security → core-security-release
Is there a reason we didn't take this on ESR38 since it is affected and this is a sec-critical?
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Adding Sylvestre as I've been told he's doing approvals for ESR38.
Flags: needinfo?(sledru)
Comment on attachment 8691944 [details] [diff] [review]
Rebased to 43

Taking it in ESR (it had no uplift request). Thanks for the ping.
Flags: needinfo?(sledru)
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Attachment #8691944 - Flags: approval-mozilla-esr38+
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
Flags: needinfo?(edwin)
The critical bits were fixed in other bugs, memory disclosure via images is sec-high.
Keywords: sec-criticalsec-high
Whiteboard: [asan][post-critsmash-triage] → [asan][post-critsmash-triage][adv-main43+][adv-esr38.5+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.