Closed Bug 1413815 Opened 2 years ago Closed 2 years ago

ASAN Arbitrary-Address-Read Crash and Heap Overflow Crash

Categories

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

57 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- disabled
firefox58 --- verified

People

(Reporter: zhanghanming, Assigned: edgar)

References

Details

(Keywords: crash, sec-critical, testcase, Whiteboard: [fuzzblocker][post-critsmash-triage])

Crash Data

Attachments

(5 files, 2 obsolete files)

Attached file crash-AAR.html
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

1.	launch a ASAN firefox or nightly firefox;
2.	make sure “dom.webcomponents.customelements.enabled” is true in “about:config”
3.	open attachment html file.
4.	Firefox should be crash now.



Actual results:

Firefox ASAN Crash with AAR or Heap Overflow


Expected results:

Not Crash
Attached file crash-AAR.txt (obsolete) —
Firefox ASAN Arbitrary-Address-Read Crash log
Firefox ASAN heap overflow crash html
Firefox ASAN Heap Overflow Crash log
The testcases also crash a nightly opt build, both with the signature [@ bool AssignJSString<T> ] but in slightly different spots (they are essentially the same testcase, except one has a string variable and one a numeric).
attachment 8924440 [details]: bp-c9c4b8bc-abd5-48e7-816e-b50e80171102
attachment 8924442 [details]: bp-6fe5a2f2-d91e-410c-81b2-42b7d0171102

I don't crash in an ESR-52 or 56.0.2 release builds. I do get the same crash in 57 Beta so this could be a regression between then. Might also just be masked and an ASAN build would reveal the problem is still there.

The opt crash is in bindings code so I don't know if the problem is CustomElement misuse of bindings or in the bindings code itself.

Olli: you've reviewed most of the patches in CustomElements recently. Ideas on who should look into this?
Group: firefox-core-security → dom-core-security
Crash Signature: [@ bool AssignJSString<T> ]
Component: Untriaged → DOM
Flags: sec-bounty?
Flags: needinfo?(bugs)
Product: Firefox → Core
Flags: needinfo?(bugs) → needinfo?(echen)
Note, the crash itself if sec-critical, but the code runs only if a pref is enabled.
Attached file crash-AAR.txt
Symbolized ASan stack.
Attachment #8924441 - Attachment is obsolete: true
Attached patch Patch, v1 (obsolete) — Splinter Review
Flags: needinfo?(echen)
Assignee: nobody → echen
Attachment #8924812 - Flags: review?(bugs)
Duplicate of this bug: 1413785
Comment on attachment 8924812 [details] [diff] [review]
Patch, v1

>+            if (attribute.isString()) {

This is highly unlikely to be what the spec says.  In fact, it's not what the spec says to do.  It says to convert observedAttributesIterable to a sequence<DOMString>.  If you look at how that actually works, it will do things like invoke ToString(), etc.

You should probably more or less copy the code we have for this in some binding (e.g. CSPDictionariesBinding.cpp has this sort of thing in CSP::Init) for now and file a followup bug to create a helper method for it.

Incidentally, there are other issues with this code too, like crashing on OOM from AppendElement instead of throwing an exception, that the generated code gets right...
Attachment #8924812 - Flags: review?(bugs) → review-
Whiteboard: [fuzzblocker]
Comment on attachment 8924893 [details] [diff] [review]
Patch, v2

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

bz, could you help to review again? Thank you.
Attachment #8924893 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> file a followup bug to create a helper method for it.

Filed bug 1414289.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 8924893 [details] [diff] [review]
Patch, v2

r=me. Thank you!
Attachment #8924893 - Flags: review?(bzbarsky) → review+
Comment on attachment 8924893 [details] [diff] [review]
Patch, v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Easy, calling customElements.define with custom elements definition that contains non-string entry in observedAttributes array.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The patch includes crash test, but the code only run if pref is enabled.
I could remove crash test if needed.

Which older supported branches are affected by this flaw?
This flaw is introduced after 57.

If not all supported branches, which bug introduced the flaw?
Bug 1334051.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I could prepare uplift patch for 57 beta if needed.

How likely is this patch to cause regressions; how much testing does it need?
Low since the code only run if pref is enabled.
Attachment #8924893 - Flags: sec-approval?
so can I get a sec-bounty or a CVE-number for report this issue?
(In reply to zhanghanming from comment #15)
> so can I get a sec-bounty or a CVE-number for report this issue?

The sec-bounty? flag is set on this bug, so it will be reviewed by the bounty committee that meets once a week. That may not happen until the fix lands. (I'm not exactly sure how it works.) If you have questions about the process, you should email security@mozilla.org
Priority: -- → P1
Comment on attachment 8924893 [details] [diff] [review]
Patch, v2

sec-approval+
Attachment #8924893 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/0ebf48d1f544
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment on attachment 8924893 [details] [diff] [review]
Patch, v2

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1334051.
[User impact if declined]: No. The code runs only if pref is enabled.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No. 
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Not risky.
[Why is the change risky/not risky?]: The code runs only if pref is enabled.
[String changes made/needed]: None.
Attachment #8924893 - Flags: approval-mozilla-beta?
Comment on attachment 8924893 [details] [diff] [review]
Patch, v2

very late and behind a pref for 57, a-
Attachment #8924893 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Group: dom-core-security → core-security-release
Flags: sec-bounty? → sec-bounty+
Flags: qe-verify+
Whiteboard: [fuzzblocker] → [fuzzblocker][post-critsmash-triage]
I have reproduced this issue using nightly Firefox 58.0a1 (2017.11.01) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 58.0 on Win 8.1 x64, Ubuntu 14.04 x64 and Mac OS X 10.12.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.