Improper handling of duplicate `<html>` and `<body>` tags enables CSP nonce leakage
Categories
(Core :: DOM: HTML Parser, defect)
Tracking
()
People
(Reporter: felber151, Assigned: tschuster)
References
Details
(Keywords: reporter-external, sec-moderate, Whiteboard: [adv-main124+][adv-esr115.9+])
Attachments
(3 files, 1 obsolete file)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:121.0) Gecko/20100101 Firefox/121.0
Steps to reproduce:
-
Visit https://poc.minimal.blue/nonce?name=%3Cstyle%3Ebody[nonce*=secret]{background:red}%3C/style%3E%3Cbody The page is served with the header
Content-Security-Policy: script-src 'nonce-secret' -
Notice that the CSS selector
body[nonce*=secret]can access the value of the CSP nonce, applying the style to the page (red background). This enables an attacker to bypass the CSP by leaking the nonce value via CSS. -
An equivalent attack can be performed by injecting an unterminated
<htmltag: https://poc.minimal.blue/nonce?name=%3Cstyle%3Ehtml[nonce*=secret]{background:red}%3C/style%3E%3Chtml
Actual results:
We discovered that a markup injection before a <script> element, which includes the nonce attribute, can result in the nonce value being accessible. This behavior can be replicated by injecting a dangling <html or <body tag, as shown in the code below. Assume that a valid CSP policy is set via an HTTP header, e.g., Content-Security-Policy: script-src 'nonce-secret':
<!DOCTYPE html>
<head>
<title>PoC</title>
</head>
<body>
<h1>CSP Nonce Bypass</h1>
Hello <style>body[nonce*=sec]{background:green}</style><body
<script nonce="secret" src="https://example.com/good.js"></script>
</body>
</html>
Please refer to https://poc.minimal.blue/nonce?name=%3Cstyle%3Ebody[nonce*=secret]{background:red}%3C/style%3E%3Cbody for a working example.
The issue affects major browsers including Firefox, Chromium and Safari. We are jointly reporting the vulnerability to Chromium (see bug id 1513216) and Safari
Expected results:
According to the HTML spec, whenever the CSP policy is delivered to the browser over an HTTP header, CSP nonce values must by hidden to CSS selectors to prevent exfiltration via side-channels, see https://html.spec.whatwg.org/multipage/urls-and-fetching.html#nonce-attributes and https://github.com/whatwg/html/issues/2369.
Detailed information and root cause
After some investigation, we believe that the issue is caused by how the HTML parser handles duplicate <html> and <body> elements. These elements are merged into their original (i.e., parent) elements, causing all the attributes of the duplicate tags to be added to the parent element. To exemplify this, the following code
<!DOCTYPE html>
<html lang="en">
<head></head>
<body>
<html a="w">
<html b="0"></html>
</html>
<html c="y"></html>
</body>
</html>
gets parsed as
<!DOCTYPE html>
<html lang="en" a="w" b="0" c="y">
<head></head>
<body></body>
</html>
We noticed that the merging operation bypasses standard security checks, including hiding the nonce attribute. Given that these checks are omitted, the nonce attribute is merged into the parent <html> tag as a normal attribute resulting accessible through CSS selectors.
Please CC Marco Squarcina (squarcina@gmail.com) to this bug
Cheers,
Georg Felber (TU Wien)
Marco Squarcina (TU Wien)
|
||
Updated•1 year ago
|
|
|
||
Updated•1 year ago
|
|
|
||
Comment 1•1 year ago
|
||
Disclaimer: I'm not at all familiar with this code, and I've had to stop before I finished digging here, but my impression is that because we're in the "in body" mode, for HTML and body nodes we non-overwrite-copy any attributes, per https://html.spec.whatwg.org/multipage/parsing.html#parsing-main-inbody .
The nonce hiding code added in https://phabricator.services.mozilla.com/D62811 seems to expect that the attribute is there when the element is bound to tree. But here, that isn't the case because the initial html/body elements are bound before we encounter the second one and copy over the attrs.
|
||
Updated•1 year ago
|
|
|
||
Comment 2•1 year ago
|
||
The amount of context / discussion on https://github.com/whatwg/html/pull/2373 for this behaviour is somewhat eye-watering, but it does seem like this is somewhat a consequence of the spec and the discussions there. In particular:
Whenever an element including HTMLOrSVGElement becomes browsing-context connected, the user agent must execute the following steps on the element [snip explanation of how to hide nonce]
hides the nonce when the element is connected, but the attribute change steps explicitly do not do this (they set the internal slot but don't hide the attribute).
Olli or Simon, can you check that I'm reading this right and confirm that we'd need a spec change to address this?
Assuming I've got that right, the somewhat "obvious" fix in terms of code would be to change things such that if the attribute is set on an element that is already browsing-context connected, we should also clear the attribute. But the original spec change didn't do this and this seems to have been deliberate (from e.g. this comment).
Can we change the attribute steps to somehow check if we're still parsing a document or fragment (and clear the attribute after setting the internal slot in that state, too)? I'm not quite sure how that is represented in HTML/DOM spec language and couldn't easily find any other case where we do that - which may mean that I did not search well enough, or that this is a bad idea. :-)
|
||
Comment 3•1 year ago
|
||
Nice attack, and it seems like the hoisting of attributes of body and html was not considered in the nonce-hiding change.
See "html" and "body" in https://html.spec.whatwg.org/#parsing-main-inbody
I think checking if there's an active parser isn't the right fix, since setAttribute shouldn't hide even if the document is still parsing. Maybe the HTML parser's hoisting of attributes needs a special hook that hides nonces? NI :hsivonen for his opinion.
|
||
Comment 4•1 year ago
|
||
Maybe the HTML parser's hoisting of attributes needs a special hook that hides nonces?
It would be possible to check for nonce attributes at https://searchfox.org/mozilla-central/rev/05178ae3d8ed27d47b340094de52bd3f572a5e1d/parser/html/nsHtml5TreeOperation.cpp#404 and skip them.
Now that I tried to witness the value of the nonce attribute getting changed to something other than what the parser provided, I observe that neither Firefox nor Chrome seems to hide the value from the DOM on the Live DOM Viewer (i.e. on a document.open()ed document). Is that expected?
(I'm generally not a fan of special cases to how markup maps to the DOM, but it seems withholding the HTML source attribute value from the DOM attribute value at least in some cases here has already happened long ago even though I learned about it just today.)
|
||
Comment 5•1 year ago
|
||
So this sounds like a problem in the spec per comment 2.
Has a spec issue been filed? Though, I'm not sure if there is a way to file security sensitive spec issues - I mean in a way which doesn't make them public immediately.
|
||
Comment 6•1 year ago
|
||
The severity field is not set for this bug.
:freddy, could you have a look please?
For more information, please visit BugBot documentation.
|
|
||
Comment 7•1 year ago
|
||
Note that Gecko is also missing the basic dangling-markup nonce protection that is already in the spec: see bug 1397308.
|
||
Updated•1 year ago
|
|
||
Comment 8•1 year ago
|
||
Heya!
- Chromium landed https://github.com/web-platform-tests/wpt/pull/43803 in public (without .tentative as they should have, since it's not per spec). Beware.
- Chromium's fix is to hide the new nonce, but this allows the original to be overridden.
- WebKit's tentative fix is to only forward the attribute when there is no attribute already and the private nonce field is the empty string. We'd be open to not forwarding the nonce attribute at all.
Hopefully we can make progress on https://github.com/whatwg/meta/issues/281 so this will go better in the future.
|
|
||
Comment 9•1 year ago
|
||
My preference would be to never hoist nonce attributes to html or body. :hsivonen WDYT?
|
|
||
Comment 10•1 year ago
|
||
(In reply to Simon Pieters [:zcorpan] from comment #9)
My preference would be to never hoist
nonceattributes tohtmlorbody. :hsivonen WDYT?
That's doable and probably makes more sense than a more elaborate special case.
|
|
||
Comment 11•1 year ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #10)
(In reply to Simon Pieters [:zcorpan] from comment #9)
My preference would be to never hoist
nonceattributes tohtmlorbody. :hsivonen WDYT?That's doable and probably makes more sense than a more elaborate special case.
Fewer special cases is good, yes. Anne, was this what you meant with Webkit being open to "not forwarding the nonce attribute at all"? And if so, where is the best place to file a [confidential] spec issue and/or otherwise tell/ask Chromium folks?
|
|
||
Comment 12•1 year ago
|
||
Yes. Though I think that will mean everyone will initially have a different fix. I created https://github.com/whatwg/html/security/advisories/GHSA-wwmc-fjr4-mcg5 to coordinate. Gijs, I couldn't find your GitHub ID so a colleague will have to add you.
|
|
||
Updated•1 year ago
|
|
|
||
Updated•1 year ago
|
|
|
||
Comment 13•1 year ago
|
||
tschuster, are you planning to work on this considering your work on bug 1397308?
|
Assignee | |
Comment 14•1 year ago
|
||
I wasn't actually working on this, but I just had a bit of time and took a look at it. I think I might have a fix, because the testcase in comment 0 passes.
So we care about the "in body" case for the html/body tag, especially when hoisting attributes. I assume the responsible code for that is nsHtml5TreeBuilder::addAttributesToBody and nsHtml5TreeBuilder::addAttributesToHtml. They both end up calling nsHtml5TreeOperation::AddAttributes (also more indirectly via opAddAttributes). In fact both of these cases seem to be the only caller of AddAttributes, so just ignoring the nonce in that function would fix it. That does seem a bit brittle, so maybe we should add an assert for html/body.
--- a/parser/html/nsHtml5TreeOperation.cpp
+++ b/parser/html/nsHtml5TreeOperation.cpp
@@ -401,7 +401,8 @@ nsresult nsHtml5TreeOperation::AddAttributes(nsIContent* aNode,
--i;
nsAtom* localName = aAttributes->getLocalNameNoBoundsCheck(i);
int32_t nsuri = aAttributes->getURINoBoundsCheck(i);
- if (!node->HasAttr(nsuri, localName)) {
+ if (!node->HasAttr(nsuri, localName) &&
+ !(nsuri == kNameSpaceID_None && localName == nsGkAtoms::nonce)) {
nsString value; // Not Auto, because using it to hold nsStringBuffer*
aAttributes->getValueNoBoundsCheck(i).ToString(value);
node->SetAttr(nsuri, localName, aAttributes->getPrefixNoBoundsCheck(i),
|
|
Assignee | |
Comment 15•1 year ago
|
||
|
||
Updated•1 year ago
|
|
|
Assignee | |
Comment 16•1 year ago
|
||
Because we already have a test for this (content-security-policy/nonce-hiding/dangling-html-or-body.tentative.html), that we now pass, there isn't really a way of trying to hide this fix.
|
||
Comment 17•1 year ago
|
||
|
||
Comment 18•1 year ago
|
||
|
||
Updated•1 year ago
|
|
|
Assignee | |
Comment 19•1 year ago
|
||
|
|
Assignee | |
Comment 20•1 year ago
|
||
This almost applies cleanly to ESR115, but I will leave the uplift decision to someone else.
|
|
Assignee | |
Comment 21•1 year ago
|
||
Comment on attachment 9381177 [details]
Bug 1871112 - ESR 115: Specialize AddAttributes for <html>/<body>. r?hsivonen
ESR Uplift Approval Request
- If this is not a sec:{high,crit} bug, please state case for ESR consideration: Using a markup injection it's possible to steal nonce values. This can be used to bypass strict content security policies.
- User impact if declined:
- Fix Landed on Version: 124
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Verified on Nightly and this code is really uncommon to even hit.
|
||
Comment 22•1 year ago
|
||
Comment on attachment 9381177 [details]
Bug 1871112 - ESR 115: Specialize AddAttributes for <html>/<body>. r?hsivonen
Approved for 115.9esr
|
|
||
Comment 23•1 year ago
|
||
| uplift | ||
|
|
||
Updated•1 year ago
|
|
||
Updated•1 year ago
|
|
|
||
Updated•1 year ago
|
|
|
||
Comment 24•1 year ago
|
||
|
|
||
Comment 25•1 year ago
|
||
|
|
||
Updated•1 year ago
|
|
|
||
Comment 26•9 months ago
|
||
Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons
|
|
||
Updated•5 months ago
|
Description
•