Skip to content

Conversation

@andypaicu
Copy link
Collaborator

@andypaicu andypaicu commented Oct 18, 2018

Fixes #258

Also @arturjanc PTAL.


Preview | Diff

@andypaicu andypaicu requested a review from mikewest October 18, 2018 14:34
Copy link
Member

@mikewest mikewest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is enough. Maybe we can chat about it this week? Basically: we need to either change Chrome's behavior, or accept that this is a "feature" of the web.

index.src.html Outdated
attack by walking through <{script}> or <{style}> element attributes, looking for the
string "<code>&lt;script</code>" or "<code>&lt;style</code>" in their names or values.

User-agents should pay particular attention when implemeting this algorithm to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chrome doesn't do this today. It would be a pain in the ass to make Chrome do this. I'm not sure we should say "should" in places where we really mean "MUST (BUT WE KNOW YOU WON'T)" in the RFC6919 sense.

Or are we planning on piping through the value of the discarded attributes at parse time? In which case, we ought to poke at the HTML spec to make it clear what the behavior ought to be.

Copy link
Collaborator Author

@andypaicu andypaicu Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I did not mean it as a loophole for Chrome, I did not think in terms of RFC "should/must" keywords.

We should chat about where Chrome is but I don't think that that should matter for the wording in the spec.

Copy link
Collaborator Author

@andypaicu andypaicu Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced "should" with "must".

@andypaicu andypaicu force-pushed the nonce-hijacking-notes branch from 684ea64 to 24523a8 Compare October 23, 2018 11:27
@arturjanc
Copy link

I like this change, but I defer to Mike on what's reasonable here spec-wise.

(Changing Chrome's behavior as described in the diff doesn't seem bad. IIRC one of the things that broke as a result of related changes was Google Docs which used duplicate nonce attributes in some cases. If this is the main blocker, we can work with application owners to fix this; maybe rejecting all nonced scripts with a duplicate attribute is easier?)

@andypaicu
Copy link
Collaborator Author

@mikewest PTAL

Copy link
Member

@mikewest mikewest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the ISSUE added.

index.src.html Outdated
not ignore duplicate attributes. If an element has a duplicate attribute any
instance of the attribute after the first one is ignored but in the
[[#is-element-nonceable]] algorithm, all attributes including the
duplicate ones need to be checked. For the following example page:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add this text to the spec, but it's unimplementable given HTML's parsing algorithm, which removes this information well before we get to any consideration of CSP.

Perhaps add an ISSUE line linking to whatwg/html#3257? And then follow up on that bug? :)

@andypaicu andypaicu force-pushed the nonce-hijacking-notes branch from 0223486 to 27af107 Compare November 9, 2018 12:50
@andypaicu andypaicu merged commit 48b736b into w3c:master Nov 9, 2018
april added a commit to april/webappsec-csp that referenced this pull request Nov 12, 2018
* 'master' of https://github.com/w3c/webappsec-csp: (38 commits)
  Added more notes about nonce attacks (w3c#356)
  Inherit source browsing context's CSP instead of parent/opener (w3c#358)
  Added a note about fetch redirects being covered (w3c#359)
  Changed names of some SPV event members (w3c#353)
  Added extra note in 'strict-dynamic' (w3c#357)
  Added note for 'strict-dynamic' to alert of avenues of attack(w3c#357)
  Fixed various bikeshed linking warnings and removed embedded from makefile (w3c#355)
  Added repo-type
  Move CSPEE to its own repository: w3c/webappsec-cspee.
  Updated published WD.
  Fixed navigation checks running on URL instead of current URL (w3c#347)
  Directive names should be lowercased (basically case-insensitive) (w3c#346)
  Fixed text for form-action pre-navigate description (w3c#345)
  Added note explaining difference between SRI and CSP hashes (w3c#344)
  Fixing whitespace issues and 2 comments in the area (w3c#340)
  Convert string to UTF-8 before applying hash algorithms (w3c#342)
  Using the correct directive name when reporting violations (w3c#337)
  Admin stuff (w3c#341)
  Code of Conduct
  Added [Exposed] to the SecurityPolicyViolationEvent interface (w3c#338)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants