Bug 89577 – Implement the script-nonce Content Security Policy directive.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

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

[Mike West]

Created attachment 148588 [details]
WIP

[Mike West]

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.

[Adam Barth]

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.

[Mike West]

(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!

[Adam Barth]

> > >> 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.

[Mike West]

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?

[Adam Barth]

Yes, do this work in ScriptElement sounds exactly right.

[Mike West]

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

[Mike West]

Created attachment 150812 [details]
Patch

[Mike West]

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.

[Adam Barth]

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

[Adam Barth]

Sorry, I accidentally sent this to the bots.

[Mike West]

(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!

[Mike West]

Created attachment 150869 [details]
Tiny fixes.

[Adam Barth]

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

[Mike West]

Created attachment 150871 [details]
Patch

[Mike West]

Comment on attachment 150871 [details]
Patch

You're right. The `isEmpty` check wasn't necessary. I've dropped it. Thanks!

[WebKit Review Bot]

Comment on attachment 150871 [details]
Patch

Clearing flags on attachment: 150871

Committed r121883: <http://trac.webkit.org/changeset/121883>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.