Bug 987140 (CVE-2014-1531)

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

VERIFIED FIXED in Firefox 29

Status

()

defect
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: nils, Assigned: smaug)

Tracking

({csectype-uaf, sec-critical})

Trunk
mozilla31
x86_64
Linux
Points:
---

Firefox Tracking Flags

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

Details

(Whiteboard: [asan][adv-main29+][adv-esr24.5+])

Attachments

(4 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
Posted 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.
Reporter

Comment 1

5 years ago
Posted file testcase.html (obsolete) —
Reporter

Comment 2

5 years ago
Posted 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)
Assignee

Comment 4

5 years ago
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)
Assignee

Comment 6

5 years ago
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

Updated

5 years ago
Assignee: nobody → bugs
Assignee

Comment 7

5 years ago
Attachment #8396489 - Flags: review?(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.
Attachment #8396489 - Flags: review?(bzbarsky) → review+
Assignee

Comment 9

5 years ago
Posted 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+
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4ede230e0938

Please request aurora/beta/esr24 approval on this when you get a chance.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee

Comment 13

5 years ago
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

Comment 15

5 years ago
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)
Reporter

Comment 16

5 years ago
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)

Updated

5 years ago
Keywords: verifyme

Comment 17

5 years ago
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.