Bug 89577

Summary: Implement the script-nonce Content Security Policy directive.
Product: WebKit Reporter: Mike West <mkwst>
Component: WebCore Misc.Assignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ojan, webkit.review.bot
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 90564    
Bug Blocks: 85558    
Attachments:
Description Flags
WIP
none
WIP: Moved test out to ScriptElement
none
Patch
none
Tiny fixes.
none
Patch none

Description Mike West 2012-06-20 09:40:24 PDT
CSP 1.1 defines the (experimental) script-nonce directive[1] as a mechanism for allowing only specific inline scripts. We should experiment with it behind the newly-landed ENABLE_CSP_NEXT flag. I'll upload a WIP patch to give us something concrete to talk about: it doesn't yet hide the functionality behind the flag, and I'm not convinced that the interaction with `script-src` is correct.

I'll upload the patch and add specific questions inline.

[1]: https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#script-nonce--experimental
Comment 1 Mike West 2012-06-20 09:56:19 PDT
Created attachment 148588 [details]
WIP
Comment 2 Mike West 2012-06-20 10:04:38 PDT
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 3 Adam Barth 2012-06-20 10:38:06 PDT
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.
Comment 4 Mike West 2012-06-21 08:29:16 PDT
(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!
Comment 5 Adam Barth 2012-06-21 12:58:54 PDT
> > >> 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 6 Mike West 2012-07-03 12:32:58 PDT
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?
Comment 7 Adam Barth 2012-07-03 13:39:05 PDT
Yes, do this work in ScriptElement sounds exactly right.
Comment 8 Mike West 2012-07-03 19:40:24 PDT
Created attachment 150710 [details]
WIP: Moved test out to ScriptElement
Comment 9 Mike West 2012-07-04 09:20:42 PDT
Created attachment 150812 [details]
Patch
Comment 10 Mike West 2012-07-04 09:24:15 PDT
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 11 Adam Barth 2012-07-04 09:27:53 PDT
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
Comment 12 Adam Barth 2012-07-04 09:28:09 PDT
Sorry, I accidentally sent this to the bots.
Comment 13 Mike West 2012-07-04 09:43:07 PDT
(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!
Comment 14 Mike West 2012-07-04 20:59:02 PDT
Created attachment 150869 [details]
Tiny fixes.
Comment 15 Adam Barth 2012-07-04 21:07:25 PDT
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
Comment 16 Mike West 2012-07-04 21:38:00 PDT
Created attachment 150871 [details]
Patch
Comment 17 Mike West 2012-07-04 21:40:36 PDT
Comment on attachment 150871 [details]
Patch

You're right. The `isEmpty` check wasn't necessary. I've dropped it. Thanks!
Comment 18 WebKit Review Bot 2012-07-04 23:45:53 PDT
Comment on attachment 150871 [details]
Patch

Clearing flags on attachment: 150871

Committed r121883: <http://trac.webkit.org/changeset/121883>
Comment 19 WebKit Review Bot 2012-07-04 23:45:59 PDT
All reviewed patches have been landed.  Closing bug.