1413815 - ASAN Arbitrary-Address-Read Crash and Heap Overflow Crash

Status: VERIFIED FIXED

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

Description

[zhanghanming]

Attachment: 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

Comment 1

[zhanghanming]

Attachment: crash-AAR.txt

Firefox ASAN Arbitrary-Address-Read Crash log

Comment 2

[zhanghanming]

Attachment: crash-heap-overflow.html

Firefox ASAN heap overflow crash html

Comment 3

[zhanghanming]

Attachment: crash-heap-overflow.txt

Firefox ASAN Heap Overflow Crash log

Comment 4

[Daniel Veditz [:dveditz]]

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?

Comment 5

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

Note, the crash itself if sec-critical, but the code runs only if a pref is enabled.

Comment 6

[Tyson Smith [:tsmith]]

Attachment: crash-AAR.txt

Symbolized ASan stack.

Comment 7

[Edgar Chen [:edgar]]

Attachment: Patch, v1

Comment 9

[Boris Zbarsky [:bzbarsky]]

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...

Comment 10

[Edgar Chen [:edgar]]

Attachment: Patch, v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fb8b4e6b34b5801749537a93413faf5a28bb3c4&filter-tier=1&group_state=expanded

Comment 11

[Edgar Chen [:edgar]]

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.

Comment 12

[Edgar Chen [:edgar]]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8924893 [details] [diff] [review]
Patch, v2

r=me. Thank you!

Comment 14

[Edgar Chen [:edgar]]

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.

Comment 15

[zhanghanming]

so can I get a sec-bounty or a CVE-number for report this issue?

Comment 16

[Andrew McCreight [:mccr8]]

(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 [e-mail]

Comment 17

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

Comment on attachment 8924893 [details] [diff] [review]
Patch, v2

sec-approval+

Comment 18

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ebf48d1f544

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ebf48d1f544

Comment 20

[Edgar Chen [:edgar]]

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.

Comment 21

[Julien Cristau [:jcristau]]

Comment on attachment 8924893 [details] [diff] [review]
Patch, v2

very late and behind a pref for 57, a-

Comment 22

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

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.