1555523 - (CVE-2019-11715) Incorrect parsing of style tag leads to XSS if HTML+CSS is allowed but no JS

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, task)

Description

[Linus Särud]

I was pentesting a web-site with a Javascript-filter. It wanted to allow HTML+CSS but no Javascript. During the assessment I found a CSS-parser bug in Firefox, where the parsing is different from other tested web browsers (Chrome/Safari) in how you terminate CSS.

This trick allowed me to bypass their protection and I think it is likely it will work in other places as well.

This was tested on the latest stable/regular version on OSX.

PoC: https://playground.zulln.se/poc/firefoxcss.html

Source code:
<style></style</style><img src="</style><svg/onload=alert(document.domain)>">

Comment 1

[:Gijs (he/him)]

Henri or Anne, can you take a look?

Looks like Chrome closes the style tag before the <img whereas we close it after the <img src=" bit.

Although comment #0 suggests this is to do with the CSS parser, I assume based on the https://html.spec.whatwg.org/multipage/parsing.html#parsing-main-inhead entry for "noframes" and "style" that this is actually about the HTML parser.

Comment 2

[Henri Sivonen (:hsivonen)]

This indeed looks like a bug. The relevant spec text last changed in https://searchfox.org/whatwg-html/diff/d82b76dd6c1f87c3100126e87da8d66a6708b888/source#101288

Comment 3

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1555523.

Comment 4

[Henri Sivonen (:hsivonen)]

(I'll open a PR for html5lib-tests once we've disclosed the test here by landing it.)

Comment 5

[Henri Sivonen (:hsivonen)]

Comment on attachment 9069654 [details]
Bug 1555523.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: So easy that I figured I might as well include the test cases and make the commit message say what the patch is about.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: Bug 509009
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: I expect the same patch to apply.
• How likely is this patch to cause regressions; how much testing does it need?: The patch has tests, and I have manually tested the U+0000 case, which doesn't fit nicely in the test harness, so the patch is unlikely to cause regressions. If this patch broke something on the Web, it would already be broken in other browsers.

Comment 6

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

This is a "sec-other" bug so doesn't normally need sec-approval to land. Is it mis-rated?

Comment 7

[Henri Sivonen (:hsivonen)]

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

This is a "sec-other" bug so doesn't normally need sec-approval to land. Is it mis-rated?

I think this doesn't fit the description of sec-other. If this was actually practical to exploit, this would be sec-high on the XSS criterion. While an exploit with the style element is very implausible, with textarea an exploit would be more plausible but would still require the site to live dangerously (to escape only full textarea close tags in textarea content rather than escaping all less-than signs), so we should probably rate this lower than sec-high due to an exploit require a failure on the site's part as well.

Comment 8

[Henri Sivonen (:hsivonen)]

(That is, this is a sec-high assuming a site that does the minimum it has an obligation to do if it assumes browser compliance, but for sites that take a reasonable approach, this is probably indeed sec-other.)

Comment 9

[Linus Särud]

The original vulnerable application accepted this HTML in a bad way of accepting svg-images. This was done wrong in several ways, but would not have been exploitable if it was not for this Firefox bug.

Thought I could provide some more context if you are looking into potential vulnerable libraries or so, I unfortunately have no idea how common this is.

Comment 10

[Henri Sivonen (:hsivonen)]

Taking comment 9 as evidence of an actual path the XSS and changing to sec-high.

Comment 11

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

sec-approval+ for trunk (but without the tests). We'll want beta and ESR60 patches made and nominated as well, to land after it is on mozilla-central.

As far as the tests goes, these need to land in another patch after we ship a release with this fix. I don't think we really need to 0day ourselves with tests for a sec-high issue. :-)

Comment 12

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1555523 test.

Comment 13

[Henri Sivonen (:hsivonen)]

Comment on attachment 9069654 [details]
Bug 1555523.

Beta/Release Uplift Approval Request

• User impact if declined: XSS on sites that accept untrusted style content or sites that do the minimum to sanitize untrusted textarea content (as opposed to escaping all less-than signs in textarea).
• 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):
• String changes made/needed: None

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 on sites that accept untrusted style content or sites that do the minimum to sanitize untrusted textarea content (as opposed to escaping all less-than signs in textarea).
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String or UUID changes made by this patch: None

Comment 14

[Henri Sivonen (:hsivonen)]

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

sec-approval+ for trunk (but without the tests).

Thanks.

We'll want beta and ESR60 patches made and nominated as well, to land after it is on mozilla-central.

The same patch applies. Nominated.

As far as the tests goes, these need to land in another patch after we ship a release with this fix. I don't think we really need to 0day ourselves with tests for a sec-high issue. :-)

I split out the tests.

Comment 15

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

https://hg.mozilla.org/integration/autoland/rev/345ababc2c6c16153e0f220364ea27ffe95228bf
https://hg.mozilla.org/mozilla-central/rev/345ababc2c6c

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 9069654 [details]
Bug 1555523.

sec-high fix for 68.0b11

Comment 17

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b9755d1531ecfe4219b40c5faf025fbbb7f105d4

Comment 18

[Cristian Fogel [:cfogel]]

As far as verification goes, noticed that on builds pre-fix there was the pop up message on the provided link.
Considering that: confirmed the issue with 68.0b7 and 69.0a1 (2019-05-27); "fix" confirmed with 69.0a1 (2019-06-19) and 68.0b11 on Windows 10, Ubuntu16.03, macOS 10.11.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9069654 [details]
Bug 1555523.

Fixes a sec-high. Verified on Nightly and Beta. Approved for 60.8esr.

Comment 20

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

https://hg.mozilla.org/releases/mozilla-esr60/rev/911475b61be3

Comment 21

[Cristian Fogel [:cfogel]]

Verified with 60.7.3 build from task cluster.

Comment 22

[Daniel Veditz [:dveditz]]

This isn't an attack on Firefox itself so sec-high is too high, but it is a bug that, when combined with a site issue, could be exploited.

Comment 23

[Henri Sivonen (:hsivonen)]

https://hg.mozilla.org/projects/htmlparser/rev/8c38facec8e157094cf0dd954a494c98b3986fef

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/201dee8cca62
test. r=alchen

Comment 25

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/201dee8cca62