987140 - (CVE-2014-1531) ASAN heap-use-after-free in nsGenericHTMLElement::GetWidthHeightForImage

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nils]

Attachment: testcase.html

The attached test case crashes the latest ASAN build of Firefox. ASAN output attached in crash.txt.

The imgLoader object is freed while setting a new src in the onresize event handler.

Comment 1

[Nils]

Attachment: testcase.html

Comment 2

[Nils]

Attachment: crash.txt

This should have been the ASAN output

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ugh.  The layout flush GetWidthHeightForImage does can nuke the image request member, presumably.  We should protect agains that, but why are we firing resize sync on layout flush?  It seems weird to me to do that.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

That sync resize firing is a pre-refreshdriver thing.
IIRC the idea was to fire the event before we do any painting.
https://bugzilla.mozilla.org/show_bug.cgi?id=472730#c6
We should make resize event to fire at refreshdriver tick.

But flushing may run scripts anyway, so resize event isn't the issue.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> But flushing may run scripts anyway

Sure.  The question is what this method should do if those scripts change the image size by loading a new image.  That is, is the right fix to use the post-flush image or to use the pre-flush image but keep it alive?  In the case when resize fires sync, the difference is web-detectable pretty easily.  If not, then I'm not sure it is, so we can just do whatever is simplest.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I still don't see how resize is interesting, since flush may run other random scripts.
I think we should flush, and then take request.
Could GetWidthHeightForImage take a nsCOMPtr<imgIRequest>& aRequest, so that when we access it, 
we access the current value?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: GetWidthHeightForImage_crash.diff

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8396489 [details] [diff] [review]
GetWidthHeightForImage_crash.diff

Add a comment about how the argument is a reference because the function might change the value and we want to use the updated value if it does that?

r=me with that.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +comment

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It is a kungfuDeathGrip style fix, but an usual variant of it, so perhaps not so obvious.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Check-in comment should be something like "Return width/height from the most recent image request"

Which older supported branches are affected by this flaw?
Looks like a regression from bug 683855, so all the branches

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?
The patch seems to apply at least to beta/aurora, and with some fuzz to esr24

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

Comment 10

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

Comment on attachment 8396521 [details] [diff] [review]
+comment

sec-approval+ for trunk. Please get it in and nominate Aurora, Beta, and ERS24 patches.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ede230e0938

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4ede230e0938

Please request aurora/beta/esr24 approval on this when you get a chance.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8396521 [details] [diff] [review]
+comment

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 683855
User impact if declined: sec-crit crash
Testing completed (on m-c, etc.): Landed to m-c
Risk to taking this patch (and alternatives if risky): Should be low risk 
String or IDL/UUID changes made by this patch: NA

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8fac4b0aed5e
https://hg.mozilla.org/releases/mozilla-beta/rev/dfb99f6e4843
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/ab4a97b39d9d
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/ab6660d2d3eb
https://hg.mozilla.org/releases/mozilla-esr24/rev/c54ad9af7c5c

Comment 15

[Ioana (away)]

I tried to reproduce this issues on Ubuntu 12.10 x86_64 using the attached testcase on the following ASAN builds:
* 03/29 Firefox 28
* 03/23 Firefox 29
* 03/24 Firefox 30

Unfortunately I didn't get one crash on any of the builds.

Nils, could you please verify that this is fixed for you on the versions where it was marked as such?

Comment 16

[Nils]

Ioana, I was able to reproduce the issue reliably before. I have now tried on the latest ASAN build from http://download.cdn.mozilla.net/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1396346523/ and can confirm that the issue doesn't reproduce anymore.

Comment 17

[Ioana (away)]

Thanks Nils!

If you happen to have some free time soon, please also verify on:
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-04-01-mozilla-beta-debug
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-04-01-mozilla-aurora-debug
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-04-01-mozilla-esr24-debug

These will get released earlier than central and we'd like to know the bug is out for all of them.