Bug 89577

Created attachment 148588 [details] WIP

Comment on attachment 148588 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=148588&action=review A few questions inline. Thanks! > Source/WebCore/page/ContentSecurityPolicy.cpp:763 > + return checkNonceAndReportViolation(nonce, consoleMessage, contextURL, contextLine); I'm not sure this is correct: we only check `script-src` iff script-nonce isn't set. This means that `script-src 'self'; script-nonce myawesomenonce` allows script to run on the page. I _think_ this is correct behavior, but I'm not sure if the spec's language precludes it. In particular, I'm not sure if "Whenever the user agent would execute a script from a script element" means "If CSP would otherwise allow a script element to execute" or "If there's a script element on the page that the browser understands". Adam? :) > Source/WebCore/page/ContentSecurityPolicy.cpp:970 > + DEFINE_STATIC_LOCAL(String, scriptNonce, ("script-nonce")); if ENABLE(CSP_NEXT) > Source/WebCore/page/ContentSecurityPolicy.cpp:1005 > + else if (equalIgnoringCase(name, scriptNonce)) if ENABLE(CSP_NEXT) > LayoutTests/ChangeLog:13 > + * http/tests/security/contentSecurityPolicy/scriptnonce-blocked.html: Added. Once the `script-src` and `script-nonce` interaction is clear to me, I'll add some tests for that combination.

Comment on attachment 148588 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=148588&action=review > Source/WebCore/html/HTMLAttributeNames.in:161 > +nonce We should add this to HTMLScriptElement.idl (behind the ENABLE flag) >> Source/WebCore/page/ContentSecurityPolicy.cpp:763 >> + return checkNonceAndReportViolation(nonce, consoleMessage, contextURL, contextLine); > > I'm not sure this is correct: we only check `script-src` iff script-nonce isn't set. This means that `script-src 'self'; script-nonce myawesomenonce` allows script to run on the page. I _think_ this is correct behavior, but I'm not sure if the spec's language precludes it. In particular, I'm not sure if "Whenever the user agent would execute a script from a script element" means "If CSP would otherwise allow a script element to execute" or "If there's a script element on the page that the browser understands". > > Adam? :) I had in mind that you'd need to satisfy both script-src and script-nonce independently. So, script-src 'self'; script-nonce awesome would only allow a script of it's URL both from a URL that's same-origin with 'self' and had an appropriate nonce. Maybe that's too restrictive? > Source/WebCore/page/ContentSecurityPolicy.cpp:923 > + if (!m_scriptNonce.isEmpty()) { I wonder if we should distinguish between isNull and isEmpty. We could use isNull to represent "no script-nonce directive" and isEmpty to represent "a script-nonce directive with an empty value". That's basically what we do with HTML attributes. > Source/WebCore/page/ContentSecurityPolicy.h:71 > - bool allowInlineScript(const String& contextURL, const WTF::OrdinalNumber& contextLine) const; > + bool allowInlineScript(const String& nonce, const String& contextURL, const WTF::OrdinalNumber& contextLine) const; The script-nonce directive is supposed to apply to out-of-line script too.

(In reply to comment #3) > (From update of attachment 148588 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148588&action=review > > > Source/WebCore/html/HTMLAttributeNames.in:161 > > +nonce > > We should add this to HTMLScriptElement.idl (behind the ENABLE flag) Will do. > >> Source/WebCore/page/ContentSecurityPolicy.cpp:763 > >> + return checkNonceAndReportViolation(nonce, consoleMessage, contextURL, contextLine); > > > > I had in mind that you'd need to satisfy both script-src and script-nonce independently. So, script-src 'self'; script-nonce awesome would only allow a script of it's URL both from a URL that's same-origin with 'self' and had an appropriate nonce. Maybe that's too restrictive? It makes sense. So, if I had `script-src 'unsafe-inline'; script-nonce mynonce`, then: <script nonce='mynonce'>// script goes here</script> would execute, <script>//script</script> would not execute, and <a onclick="javascript"> would not trigger? > > Source/WebCore/page/ContentSecurityPolicy.cpp:923 > > + if (!m_scriptNonce.isEmpty()) { > > I wonder if we should distinguish between isNull and isEmpty. We could use isNull to represent "no script-nonce directive" and isEmpty to represent "a script-nonce directive with an empty value". That's basically what we do with HTML attributes. Do we want to support empty nonces? I'm not sure I see the value. > > > Source/WebCore/page/ContentSecurityPolicy.h:71 > > - bool allowInlineScript(const String& contextURL, const WTF::OrdinalNumber& contextLine) const; > > + bool allowInlineScript(const String& nonce, const String& contextURL, const WTF::OrdinalNumber& contextLine) const; > > The script-nonce directive is supposed to apply to out-of-line script too. Will do. Thanks!

> > >> Source/WebCore/page/ContentSecurityPolicy.cpp:763 > > >> + return checkNonceAndReportViolation(nonce, consoleMessage, contextURL, contextLine); > > > > > > > I had in mind that you'd need to satisfy both script-src and script-nonce independently. So, script-src 'self'; script-nonce awesome would only allow a script of it's URL both from a URL that's same-origin with 'self' and had an appropriate nonce. Maybe that's too restrictive? > > It makes sense. > > So, if I had `script-src 'unsafe-inline'; script-nonce mynonce`, then: > > <script nonce='mynonce'>// script goes here</script> > > would execute, > > <script>//script</script> > > would not execute, and > > <a onclick="javascript"> > > would not trigger? Correct. > > > Source/WebCore/page/ContentSecurityPolicy.cpp:923 > > > + if (!m_scriptNonce.isEmpty()) { > > > > I wonder if we should distinguish between isNull and isEmpty. We could use isNull to represent "no script-nonce directive" and isEmpty to represent "a script-nonce directive with an empty value". That's basically what we do with HTML attributes. > > Do we want to support empty nonces? I'm not sure I see the value. We need to do something if the site uses it. Maybe we should treat it as if the nonce never matches anything? Whatever we do, please send an email to public-webappsec so I remember to put it into the spec.

Comment on attachment 148588 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=148588&action=review >>> Source/WebCore/page/ContentSecurityPolicy.h:71 >>> + bool allowInlineScript(const String& nonce, const String& contextURL, const WTF::OrdinalNumber& contextLine) const; >> >> The script-nonce directive is supposed to apply to out-of-line script too. > > Will do. > > Thanks! I've been looking at this a bit this afternoon, and it's more work than I expected. The check for `allowScriptFromSource` is done inside of CachedResourceLoader::canRequest, which doesn't have access to the element. It looks like more work than it's probably worth to pipe the nonce down to that point for the comparison. How would you feel about performing the nonce check inside ScriptElement::requestScript instead? Assuming that nonces aren't going to expand to other resource types, this seems like the right place to do it. If more resource types would get on board, then piping things into the resource loader might be worth the effort. If we pull the nonce check for external scripts into ScriptElement, perhaps we also should pull inline script nonce checks in into ScriptElement::executeScript rather than ContentSecurityPolicy::allowInlineScript? WDYT?

Yes, do this work in ScriptElement sounds exactly right.

Created attachment 150710 [details] WIP: Moved test out to ScriptElement

Created attachment 150812 [details] Patch

I think the current patch is ready for you to take a look at, Adam, but I'm not going to throw it at the bots until I know it won't explode. :) Blocking this on skipping tests for ports where the feature isn't yet supported.

Comment on attachment 150812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150812&action=review This looks pretty good. I'm off to the airport, so I might not be able to finish the review for a bit. > Source/WebCore/page/ContentSecurityPolicy.cpp:752 > + && checkNonceAndReportViolation("", consoleMessage, contextURL, contextLine)); "" -> String() ? > Source/WebCore/page/ContentSecurityPolicy.cpp:986 > +#endif // ENABLE(CSP_NEXT) No need for the comment on such a short ifdef

Sorry, I accidentally sent this to the bots.

(In reply to comment #11) > (From update of attachment 150812 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150812&action=review > > This looks pretty good. I'm off to the airport, so I might not be able to finish the review for a bit. No worries. I didn't expect you to look at anything today, actually. :) Happy holidays!

Created attachment 150869 [details] Tiny fixes.

Comment on attachment 150869 [details] Tiny fixes. View in context: https://bugs.webkit.org/attachment.cgi?id=150869&action=review This came out very well. > Source/WebCore/page/ContentSecurityPolicy.cpp:726 > + if (m_scriptNonce.isEmpty() || (!nonce.isEmpty() && nonce.stripWhiteSpace() == m_scriptNonce)) > + return true; Is the nonce.isEmpty() check needed here? Given that m_scriptNonce is not empty, nonce wouldn't be empty if it matches m_scriptNonce

Created attachment 150871 [details] Patch

Comment on attachment 150871 [details] Patch You're right. The `isEmpty` check wasn't necessary. I've dropped it. Thanks!

Comment on attachment 150871 [details] Patch Clearing flags on attachment: 150871 Committed r121883: <http://trac.webkit.org/changeset/121883>

All reviewed patches have been landed. Closing bug.