1562033 - (CVE-2019-11744) title.innerHTML = "</title><foo>" parses <foo> as a tag

Status: RESOLVED FIXED

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

Description

[Rakesh]

When creating any element (except "template" and "script" tags) using "document.createElement()" and "document.createElementNS()" function the provided HTML code gets rendered directly. Ideally the HTML code should not get rendered until it is manually inserted into DOM using "appendChild()" or similar function. I tested this issue on Chrome, Opera and Safari, they do not render the HTML code directly, also the HTML specs does not explicitly states to render the HTML code directly when creating the Elements. (Reference : https://dom.spec.whatwg.org/#dom-document-createelement)

For Example :
document.createElement("AnyElementBlahBlah").innerHTML="<svg/onload=alert()>"

This code would execute "alert()" function directly. We did not even need to insert the element in DOM.

Here's another interesting example :
document.createElement("title").innerHTML="</title><svg/onload=alert()>"

Here the content of "title" element should not be rendered at all but we are able to escape out of "title" tag context by prepending a closing "title" tag as shown in above example. Using this trick we can render our HTML code inside "title" element as well and execute our Javascript code. Damn.

The same flaw applies to "document.createElementNS()" as well.

For example :
document.createElementNS("http://www.w3.org/1999/xhtml", "title").innerHTML="</title><svg onload=alert()>"

Impact:

It can introduce XSS vulnerability to many web applications because HTML specs does not states to render HTML such a way and also the other browsers does not follow this practice so technically it would be safe to allow untrusted input like "document.createElement("title").innerHTML='UntrustedUserInput' " as long as the input is verified before inserting it to DOM, however for Firefox that's not the case because Firefox will render the Untrusted Input directly which would lead to execution of malicious Javascript as well.

Tested On :

• Firefox 68.0b13 (64-bit)
• Firefox 67.0.4 (64-bit)

Comment 1

[:Gijs (he/him)]

Henri, is this something you can look at? It looks vaguely related to bug 395597, but script elements don't seem to work. I don't know if there are other mechanisms besides <svg onload> that would accomplish script execution here.

Comment 2

[Rakesh]

Hi,
It’s not just about SVG element. Any element that can fire event handler without user interaction would work here.
For example : <img/src onerror=alert()>

Comment 3

[Henri Sivonen (:hsivonen)]

(In reply to Rakesh from comment #2)

It’s not just about SVG element. Any element that can fire event handler without user interaction would work here.
For example : <img/src onerror=alert()>

This is cross-browser behavior. The Web depends on disconnected img elements starting fetches. (Not sure about the level of dependency of onerror working for those fetches.)

The idea that innerHTML on disconnected elements is inert without a template is mistaken.

document.createElement("title").innerHTML="</title><svg/onload=alert()>"

Two different bugs here: breakout from title and the onload.

For the SVG bug, investigation of the XML side is also needed.

Comment 4

[Henri Sivonen (:hsivonen)]

(Given the img onerror behavior, I'm unconvinced that svg onload is a security bug, but the title breakout is.)

Comment 5

[Henri Sivonen (:hsivonen)]

The SVG2 spec suggests that the parser no longer needs to dispatch a load event for every svg element. Am I reading the spec correctly and is the change Web-compatible?

https://svgwg.org/svg2-draft/single-page.html#interact-SVGEvents

Comment 6

[Rakesh]

Allowing to break out of "title" tag context definitely has more security impact.
However I am not able to understand why this SVG thing is being investigated here. I believe there's no bug in SVG because any HTML code would work.

For example :

document.createElement("x").innerHTML="<audio src=x onerror=alert()>" // It would alert
document.createElement("x").innerHTML="<input type=image src=x onerror=alert()>" // It would alert
document.createElement("x").innerHTML="<img src=x onerror=alert()>" // It would alert

If you are thinking it's an "onerror" event handler issue then let me tell you it's not. Because:

document.createElement("x").innerHTML="<img src='https://secure.gravatar.com/avatar/0' onload=alert()>" // It would alert

There is really nothing special about "SVG". From my understanding what actually happening is the provided code (Example : <audio src=x onerror=alert()>) is temporarily getting rendered and then removed instantly.

Comment 7

[:Gijs (he/him)]

(In reply to Rakesh from comment #6)

Allowing to break out of "title" tag context definitely has more security impact.
However I am not able to understand why this SVG thing is being investigated here. I believe there's no bug in SVG because any HTML code would work.

For example :

document.createElement("x").innerHTML="<audio src=x onerror=alert()>" // It would alert
document.createElement("x").innerHTML="<input type=image src=x onerror=alert()>" // It would alert
document.createElement("x").innerHTML="<img src=x onerror=alert()>" // It would alert

If you are thinking it's an "onerror" event handler issue then let me tell you it's not. Because:

document.createElement("x").innerHTML="<img src='https://secure.gravatar.com/avatar/0' onload=alert()>" // It would alert

All of these alert in Chrome as well (except the input type=image, which seems like it's probably for some other reason like whether or not they fire the error event at all in that case).

Henri already addressed this:

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

This is cross-browser behavior. The Web depends on disconnected img elements starting fetches. (Not sure about the level of dependency of onerror working for those fetches.)

The idea that innerHTML on disconnected elements is inert without a template is mistaken.

In other words, the behaviour for img and audio is per spec. Websites should not depend on innerHTML being inert when run on disconnected elements. It isn't. I would be very surprised if any widespread HTML sanitizer library did have such a dependency - this type of thing is why <template> exists.

Comment 8

[Henri Sivonen (:hsivonen)]

Let's handle svg onload as a different bug and keep this one for the title break-out, which clearly is a security bug.

Comment 9

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1562033.

Comment 10

[Henri Sivonen (:hsivonen)]

(I tested the patch manually. I don't have time to write a proper test today, but the test needs to go in a different changeset anyway.)

Comment 11

[Rakesh]

Thanks for the further clarification. I agree that HTML sanitizers would most likely use "<template>" for sanitization purpose. Anyways I believe that allowing to break out of the elements definitely poses security risk. Also please note that just like "title" element, all of other elements like textarea, noscript, title, iframe, etc also breaks (except "template" element).

For example :

document.createElement("textarea").innerHTML="</textarea><svg/onload=alert()>"
document.createElement("noscript").innerHTML="</noscript><svg/onload=alert()>"
document.createElement("title").innerHTML="</title><svg/onload=alert()>"
document.createElement("iframe").innerHTML="</iframe><svg/onload=alert()>"

All of above lines of code will executed alert(). I'm still investigating this behaviour to find any good example to demonstrate the proper impact though. :)

Comment 12

[Robert Longson [:longsonr]]

In SVG 2 various SVG events with html equivalents use the html definition. bug 500261 implements error and abort as html does. We've dropped SVGZoom and SVGScroll. I'm not sure we've ever sent SVGResize or maybe we did and dropped support. We've not changed SVGLoad to load yet but we should for simplicity.

We need to dispatch load events from documents as lots of code checks that when SVG is externally loaded e.g. from object/iframe. Currently we do that from the root svg element but we could probably do something more like html (and the spec) where it's to the window isn't it when the document has loaded? We should dispatch load events from <image> and <feImage> elements but I'm not sure we do.

Comment 13

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1562033 test - Test context node end tag with special tokenizer modes in innerHTML setter.

Comment 14

[Henri Sivonen (:hsivonen)]

Marking sec-high due to XSS risk.

Comment 15

[Henri Sivonen (:hsivonen)]

Comment on attachment 9074846 [details]
Bug 1562033.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Extremely easily from the patch. The harder part is locating a site that sets innerHTML on one of the affected elements with user-supplied unescaped input.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: Bug 487949
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: The same patch applies.
• How likely is this patch to cause regressions; how much testing does it need?: Should be very unlikely. The other changeset (not to be landed at the same time) tests the various cases.

Comment 16

[Henri Sivonen (:hsivonen)]

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

Let's handle svg onload as a different bug and keep this one for the title break-out, which clearly is a security bug.

Filed bug 1562581 for the svg onload bug.

Comment 17

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

We're out of betas and making release candidates for 68 now. Is there a specific reason that this can't wait until Firefox 69 or a 68 point release at this point?

Comment 18

[Henri Sivonen (:hsivonen)]

(In reply to Al Billings [:abillings] from comment #17)

We're out of betas and making release candidates for 68 now. Is there a specific reason that this can't wait until Firefox 69 or a 68 point release at this point?

I think there isn't such a reason.

Comment 19

[Henri Sivonen (:hsivonen)]

If this gets sec-approval+ while I'm away, please ask a sheriff to land the patch.

Comment 20

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

I'd like to land this once you're back so it is late in the cycle, given the obviousness of it from your sec-approval comments. So, sec-approval for August 5 (or immediately therafter). We'll want it on Beta and ESR60 after it lands there.

Comment 21

[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 22

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

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:hsivonen, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 23

[Rakesh]

Is it possible to reward the bounty sooner?
Thanks.

Comment 24

[Henri Sivonen (:hsivonen)]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #22)

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:hsivonen, could you have a look please?

Landed.

Comment 25

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

https://hg.mozilla.org/integration/autoland/rev/08dac69a37275acc3f15b3d3a31c4f3b19aebbc8
https://hg.mozilla.org/mozilla-central/rev/08dac69a3727

Comment 26

[Brindusa Tot, Desktop QA]

If there is qa need to verify this, could you please provide a standalone testcase?

Comment 27

[Henri Sivonen (:hsivonen)]

Attachment: Demo

(In reply to Brindusa Tot[:brindusat] from comment #26)

If there is qa need to verify this, could you please provide a standalone testcase?

This attachment should alert PASS and should not alert FAIL.

Comment 28

[Henri Sivonen (:hsivonen)]

Comment on attachment 9074846 [details]
Bug 1562033.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
• User impact if declined: XSS if there is a site that relies on being able to assign unsanitized user-supplied content to e.g. title or textarea via innerHTML.
• Fix Landed on Version: 70
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively small change that aligns us with other browsers.
• String or UUID changes made by this patch: None

Comment 29

[Brindusa Tot, Desktop QA]

Henri, I tried with the attachment from comment 27 and I get PASS on latest Nightly 70.0a1 (build ID 20190806214332) but also on an older build, without the fix: Nightly 70.0a1 from 08.05.2019 and on Beta 69.0b11. Shouldn't we get FAIL on builds where the fix was not landed?
I tested on Windows 10 x64.

Comment 30

[Henri Sivonen (:hsivonen)]

Attachment: Demo v2

(In reply to Brindusa Tot[:brindusat] from comment #29)

Henri, I tried with the attachment from comment 27 and I get PASS on latest Nightly 70.0a1 (build ID 20190806214332) but also on an older build, without the fix: Nightly 70.0a1 from 08.05.2019 and on Beta 69.0b11. Shouldn't we get FAIL on builds where the fix was not landed?
I tested on Windows 10 x64.

Indeed. Sorry about a faulty test. Uploading a corrected test.

This one alerts only BASELINE when the bug is fixed and alerts both BASELINE and FAIL when the bug has not been fixed.

Comment 31

[Henri Sivonen (:hsivonen)]

Comment on attachment 9074846 [details]
Bug 1562033.

Beta/Release Uplift Approval Request

• User impact if declined: XSS if there is a site that relies on being able to assign unsanitized user-supplied content to e.g. title or textarea via innerHTML.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• 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): Relatively small change that aligns us with other browsers.
• String changes made/needed: None

Comment 32

[Pascal Chevrel:pascalc]

Comment on attachment 9074846 [details]
Bug 1562033.

Uplift approved for 69 beta

Comment 33

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

https://hg.mozilla.org/releases/mozilla-beta/rev/04b175b78928

Comment 34

[Julien Cristau [:jcristau]]

Looks like this is missing an uplift request for esr60?

Comment 35

[Henri Sivonen (:hsivonen)]

Comment on attachment 9074846 [details]
Bug 1562033.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
• User impact if declined: XSS if there is a site that relies on being able to assign unsanitized user-supplied content to e.g. title or textarea via innerHTML.
• Fix Landed on Version: 70
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively small change that aligns us with other browsers.
• String or UUID changes made by this patch: None

Comment 36

[Julien Cristau [:jcristau]]

Comment on attachment 9074846 [details]
Bug 1562033.

sec-high fix for 60.9 and 68.1 ESR

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/747a334cde46

Comment 38

[Ryan VanderMeulen [:RyanVM]]

This bounced on ESR60 due to build bustage:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=261163820&repo=mozilla-esr60&lineNumber=15185

Please provide a rebased patch when you get a chance.

Comment 39

[Brindusa Tot, Desktop QA]

Reproduced this issue on Windows 10 on an older Nightly 70.0a1 without the fix.
Confirm the fix using the attachment from comment 30 on latest Nightly 70.0a1, build ID 20190814094216 and Beta 69.0b13. Verified on Window 10 x64, Mac OS 10.14 and Ubuntu 18.04.

But, on latest ESR build 68.0.2esr (build id 20190813155538) with the same attachment from comment 30 I get both alerts: FAIL and BASELINE.
Henri, could you please take a look?

Comment 40

[Julien Cristau [:jcristau]]

This is in 68.1esr not 68.0.2esr.

Comment 41

[Brindusa Tot, Desktop QA]

Thank you Julien for clarification. Confirm the fix on esr build 68.1.0esr, build id 20190814122844.

Comment 42

[Henri Sivonen (:hsivonen)]

Attachment: ESR60 rebase of Bug 1466449

(In reply to Ryan VanderMeulen [:RyanVM] from comment #38)

This bounced on ESR60 due to build bustage:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=261163820&repo=mozilla-esr60&lineNumber=15185

Please provide a rebased patch when you get a chance.

The patch itself doesn't need rebasing, but bug 1466449 needs uplifting into ESR60 as a prerequisite for the patch for this bug. Here's the rebased version of that patch. Where do we go from here in terms of the ESR uplift process?

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9086014 [details] [diff] [review]
ESR60 rebase of Bug 1466449

Approved for 60.9esr, thanks.

Comment 44

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

https://hg.mozilla.org/releases/mozilla-esr60/rev/3e5da344d87de189fb192032d3fecc23a6d58933
https://hg.mozilla.org/releases/mozilla-esr60/rev/ca88b9fad7d5ee879e81dc2c8347b913718faa8e

Comment 45

[Henri Sivonen (:hsivonen)]

https://hg.mozilla.org/projects/htmlparser/rev/929a60b32949661eac953c908262e8ac420a9467

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4f306b4af1b
test - Test context node end tag with special tokenizer modes in innerHTML setter. r=alchen

Comment 47

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/24117 for changes under testing/web-platform/tests

Comment 48

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 49

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/a4f306b4af1b

Comment 50

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot