Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider recording the "duplicate-attribute" error state. #3257

Open
mikewest opened this issue Nov 27, 2017 · 7 comments
Open

Consider recording the "duplicate-attribute" error state. #3257

mikewest opened this issue Nov 27, 2017 · 7 comments
Labels
security/privacy There are security or privacy implications topic: parser

Comments

@mikewest
Copy link
Member

CSP attempts to defend against some kinds of dangling-markup attacks by preventing the execution of scripts that include "<script" in an attribute name or value. Discarding attributes during parsing makes it possible to trivially work around this defense, as described in https://crbug.com/740615. As dropping repeated attributes is also the root cause of the dangling-markup risk in the first place, it might be reasonable to dig into it a little more deeply to see if something more fundamental can be done.

One idea we (briefly) discussed at TPAC is to prevent nonced script execution for <script> elements which had duplicate-attribute parse errors. An approach to this that I'm exploring in https://chromium-review.googlesource.com/c/chromium/src/+/566822 does more or less the following:

  1. Records duplicate-attribute parse errors on the tag during tokenization.
  2. Persists that error bit onto the Node generated during tree-building.
  3. Reads that error bit during "Is element nonceable?".

WDYT?

/cc @whatwg/security (@ckerschb, @dbates-wk, @patrickkettner in particular as folks working on CSP)

mikewest added a commit to w3c/webappsec-csp that referenced this issue Nov 27, 2017
@annevk annevk added security/privacy There are security or privacy implications topic: parser labels Nov 27, 2017
@annevk
Copy link
Member

annevk commented Nov 27, 2017

cc @whatwg/html-parser

@bzbarsky
Copy link
Contributor

@hsivonen

@andypaicu
Copy link
Contributor

Unfortunately this is still a pretty big hole in nonce based CSP policies because it opens very simple avenues for injection points to become dangling-markup nonce-stealing attacks.

Does the above approach detailed by Mike seem reasonable. What are peoples thoughts?

@gsnedders
Copy link

To me this seems like a good idea given the potential threats, though it definitely adds some complexity (given the need to propagate the error). That said, my opinion probably shouldn't for a huge amount here. 🙂

@hsivonen
Copy link
Member

hsivonen commented Nov 12, 2018

Propagating a "this script is questionable enough that it should not run" bit to the DOM is doable from the parser perspective. Gecko already does this for scripts that were truncated by early EOF.

Let's not overdo it, though. Let's make it just one bit that says the there was at least one duplicate attribute without trying to retain knowledge of which attribute was duplicated and how all the way through the stack. Or even just one bit saying the script isn't eligible to run (in case we want to add other reasons later or combine it right away with the truncation-by-early-EOF case).

aarongable pushed a commit to chromium/chromium that referenced this issue Dec 3, 2018
Docs have fixed their duplicate nonces b/70010257
Initial CR: https://chromium-review.googlesource.com/c/chromium/src/+/566822/
Spec: whatwg/html#3257

Bug: 908207
Change-Id: I95be2e84d216147483bfdf816878fe4058451065
Reviewed-on: https://chromium-review.googlesource.com/c/1356984
Reviewed-by: Mike West <[email protected]>
Commit-Queue: Andy Paicu <[email protected]>
Cr-Commit-Position: refs/heads/master@{#613099}
@andypaicu
Copy link
Contributor

I've raised the 2 referenced PR's, please have a look and let me know what you think.

@evilpie
Copy link

evilpie commented Jan 26, 2024

This is still a problem, that I discovered while implementing the nonceable check in Firefox.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security/privacy There are security or privacy implications topic: parser
Development

No branches or pull requests

7 participants