Closed Bug 987140 (CVE-2014-1531) Opened 10 years ago Closed 10 years ago

ASAN heap-use-after-free in nsGenericHTMLElement::GetWidthHeightForImage

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
firefox28 --- wontfix
firefox29 + fixed
firefox30 + fixed
firefox31 + verified
firefox-esr24 + fixed
b2g18 --- wontfix
b2g-v1.1hd --- wontfix
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: nils, Assigned: smaug)

Details

(Keywords: csectype-uaf, sec-critical, Whiteboard: [asan][adv-main29+][adv-esr24.5+])

Attachments

(4 files, 1 obsolete file)

Attached file 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.
Attached file testcase.html (obsolete) —
Attached file crash.txt
This should have been the ASAN output
Attachment #8395672 - Attachment is obsolete: true
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.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Whiteboard: [asan]
> 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.
Flags: needinfo?(bugs)
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?
Flags: needinfo?(bugs)
Assignee: nobody → bugs
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.
Attachment #8396489 - Flags: review?(bzbarsky) → review+
Attached patch +commentSplinter Review
[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
Attachment #8396521 - Flags: sec-approval?
Comment on attachment 8396521 [details] [diff] [review]
+comment

sec-approval+ for trunk. Please get it in and nominate Aurora, Beta, and ERS24 patches.
Attachment #8396521 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/4ede230e0938

Please request aurora/beta/esr24 approval on this when you get a chance.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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
Attachment #8396521 - Flags: approval-mozilla-esr24?
Attachment #8396521 - Flags: approval-mozilla-beta?
Attachment #8396521 - Flags: approval-mozilla-b2g28?
Attachment #8396521 - Flags: approval-mozilla-b2g26?
Attachment #8396521 - Flags: approval-mozilla-aurora?
Attachment #8396521 - Flags: approval-mozilla-esr24?
Attachment #8396521 - Flags: approval-mozilla-esr24+
Attachment #8396521 - Flags: approval-mozilla-beta?
Attachment #8396521 - Flags: approval-mozilla-beta+
Attachment #8396521 - Flags: approval-mozilla-aurora?
Attachment #8396521 - Flags: approval-mozilla-aurora+
Attachment #8396521 - Flags: approval-mozilla-b2g28?
Attachment #8396521 - Flags: approval-mozilla-b2g26?
Keywords: verifyme
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?
Flags: needinfo?(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.
Flags: needinfo?(nils)
Keywords: verifyme
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.
Status: RESOLVED → VERIFIED
Whiteboard: [asan] → [asan][adv-main29+][adv-esr24.5+]
Alias: CVE-2014-1531
Group: core-security
You need to log in before you can comment on or make changes to this bug.