1602944 - (CVE-2020-6798) Possible Mutation XSS or DOM XSS in innerHTML method due to incorrect parsing <template>

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P2)

Description

[terjanq]

Description

When doing some fuzzing, I discovered that it is possible to execute scripts inside <select> element when using innerHTML method. The bug is that the parsing gets broken after injecting <template> element.

Generally, no scripts should be executed inside <select> element, and the only allowed tags seem to be <option>, <optgroup>, <script>, <template>. None of these can execute javascript when called through innerHTML, and the bug seems to be only working in the Firefox within browsers that I tested this on.

Steps

Reproduction steps (Firefox Browser 71.0 (64-bit), Windows 10):
var s = document.createElement('select');
document.body.appendChild(s);
s.innerHTML = '<svg/onload=alert(/wont_trigger/)></svg><template></template><svg/onload=alert(/will_trigger/)></svg>'

I also discovered that the issue is very innerHTML specific and will not work when the element is inserted directly through insertAdjacentHTML, document.write() or
<select><template></template><svg onload=alert()></svg></select> as a HTML code.

Impact

I don't know what's the biggest impact yet, but I fell that there must a serious bug in the parser that allows inserting any HTML after <template></template> when using innerHTML, even if the element could not be put there otherwise.

Comment 1

[Frederik Braun [:freddy]]

Thank you very much for this report, terjanq! This is really interesting.

My reading of the spec is that <script> ought to be allowed in <select> but that's not true. So there's one thing.

Then from looking at the current implementation, it seems that HTML parsing when inside a <select> is picky about what to allow and what isn't but starts allowing everything once you have inserted <template></template>.

I gather the ending template is resetting parser state but doing it incorrectly.

CCing hsivonen for the parser stuff and annevk for the spec bit (mostly out of curiosity)

Comment 2

[terjanq]

(In reply to Frederik Braun [:freddyb] from comment #1)

My reading of the spec is that <script> ought to be allowed in <select> but that's not true. So there's one thing.

Executing document.write('<select><script>alert()</script></select>') will trigger an alert, so will when used as a plain HTML <select><script>alert()</script></select>. innerHTML will also insert a <script> tag inside <select> element but scripts are not executed when inserted via this method.

Another observation by me is that when calling document.body.innerHTML = '<select><option>1</option><template></template><svg/onload=alert()></select>' script will not be executed, so it's strictly related to the case scenario that I discovered.

Comment 3

[Frederik Braun [:freddy]]

It seems the ending template element resets the state incorrectly and the parser doesn't remember it was inside a <select> element.

Comment 4

[Daniel Veditz [:dveditz]]

Is the problem <template> or is this really the same svg-related thing in bug 1598466?

Comment 5

[terjanq]

The problem is <select> and <template> related.

var s = document.createElement('select');
document.body.appendChild(s);
s.innerHTML = '<template></template><img src=a onerror="alert(/loaded/)"/>'

The above also triggers the alert.

Comment 6

[Henri Sivonen (:hsivonen)]

The first and most obvious thing about template end tag processing is that the spec has gained an action "generate implied end tags thoroughly" but we only do the old "generate implied end tags" when that's required. That's unlikely to be the problem here, though.

The more likely cause is that the "reset the insertion mode appropriately" algorithm has gained a variable last that we don't have.

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #6)

The first and most obvious thing about template end tag processing is that the spec has gained an action "generate implied end tags thoroughly" but we only do the old "generate implied end tags" when that's required. That's unlikely to be the problem here, though.

The more likely cause is that the "reset the insertion mode appropriately" algorithm has gained a variable last that we don't have.

We should also fix these, but since these aren't the root cause, not right before holiday without test cases.

Comment 8

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1602944 - Move setting context to null.

Comment 9

[Henri Sivonen (:hsivonen)]

Alphan, if you r+ while I'm away, please also request sec-approval, etc.

Comment 10

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #7)

(In reply to Henri Sivonen (:hsivonen) from comment #6)

The first and most obvious thing about template end tag processing is that the spec has gained an action "generate implied end tags thoroughly" but we only do the old "generate implied end tags" when that's required. That's unlikely to be the problem here, though.

The more likely cause is that the "reset the insertion mode appropriately" algorithm has gained a variable last that we don't have.

We should also fix these, but since these aren't the root cause, not right before holiday without test cases.

Filed as bug 1605364.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:hsinyi, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 12

[Andrew McCreight [:mccr8]]

Olli, any idea what sort of security rating this should get?

Comment 13

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

I would say sec-moderate and not sec-high, since in general many event handlers may run when using innerHTML.

Alphan, could you request approval, per comment 9.

Comment 14

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

And Hsin-Yi reminded me that sec-moderate doesn't need approval :)

Comment 15

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

And bug 1562581 is about load event handlers on svg, "I'm pretty sure that this isn't actually a security bug".

RyanVM, I do think this should land to all the branches around the same time. Do you know what is the right procedure for that?
Ask sec-approval and other approvals at the same time? (even though sec-approval isn't strictly needed)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

If you do the branch approvals up front, you could wait to autoland until they're given (with a comment noting that as the intended plan). From there, uplifts should happen pretty shortly after the patches reach m-c per our usual practices.

That said, we're in RC week at the moment and a new cycle starts on Monday, so you might want to wait a couple weeks if you're worried about an exposure window here.

Comment 17

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

Ah, right. Perhaps hsivonen can then deal with this once he is back in a week or so.

Comment 18

[Henri Sivonen (:hsivonen)]

Comment on attachment 9117223 [details]
Bug 1602944 - Move setting context to null.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: A template in select can confuse the parser, so a site that relies on the browser behaving correctly could in theory suffer XSS. However, in practice, it's unlikely that a site with proper XSS-avoidance mechanisms would be affected.
• User impact if declined: Small possibility of XSS.
• Fix Landed on Version: Not yet landed; see comment 15
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very low risk. Nulling out a couple of fields is deferred.
• String or UUID changes made by this patch: None.

Beta/Release Uplift Approval Request

• User impact if declined: A template in select can confuse the parser, so a site that relies on the browser behaving correctly could in theory suffer XSS. However, in practice, it's unlikely that a site with proper XSS-avoidance mechanisms would be affected.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very low risk. Nulling out a couple of fields is deferred.
• String changes made/needed: None.

Comment 19

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #14)

And Hsin-Yi reminded me that sec-moderate doesn't need approval :)

Not requesting sec-approval due to this.

(In reply to Olli Pettay [:smaug] from comment #15)

I do think this should land to all the branches around the same time.

Requesting branch approval in advance due to this.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9117223 [details]
Bug 1602944 - Move setting context to null.

Fixes a sec issue, approved for 73.0b5 and 68.5esr. Let's hold off on uplifting until this merges to m-c, though.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/c7545f9cfe8f64a53400054f703b05989f90ada7
https://hg.mozilla.org/mozilla-central/rev/c7545f9cfe8f

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8c63ea54dd6b

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/3b3f5444c547

Comment 24

[Henri Sivonen (:hsivonen)]

https://hg.mozilla.org/projects/htmlparser/rev/bf1dd019d127e946af4a2baeab20352eae860897

Comment 25

[Tom Ritter [:tjr]]

Attachment: advisory.txt