WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 89577
Implement the script-nonce Content Security Policy directive.
https://bugs.webkit.org/show_bug.cgi?id=89577
Summary
Implement the script-nonce Content Security Policy directive.
Mike West
Reported
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
Attachments
WIP
(19.39 KB, patch)
2012-06-20 09:56 PDT
,
Mike West
no flags
Details
Formatted Diff
Diff
WIP: Moved test out to ScriptElement
(27.05 KB, patch)
2012-07-03 19:40 PDT
,
Mike West
no flags
Details
Formatted Diff
Diff
Patch
(28.99 KB, patch)
2012-07-04 09:20 PDT
,
Mike West
no flags
Details
Formatted Diff
Diff
Tiny fixes.
(29.05 KB, patch)
2012-07-04 20:59 PDT
,
Mike West
no flags
Details
Formatted Diff
Diff
Patch
(29.96 KB, patch)
2012-07-04 21:38 PDT
,
Mike West
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Mike West
Comment 1
2012-06-20 09:56:19 PDT
Created
attachment 148588
[details]
WIP
Mike West
Comment 2
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.
Adam Barth
Comment 3
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.
Mike West
Comment 4
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!
Adam Barth
Comment 5
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.
Mike West
Comment 6
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?
Adam Barth
Comment 7
2012-07-03 13:39:05 PDT
Yes, do this work in ScriptElement sounds exactly right.
Mike West
Comment 8
2012-07-03 19:40:24 PDT
Created
attachment 150710
[details]
WIP: Moved test out to ScriptElement
Mike West
Comment 9
2012-07-04 09:20:42 PDT
Created
attachment 150812
[details]
Patch
Mike West
Comment 10
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.
Adam Barth
Comment 11
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
Adam Barth
Comment 12
2012-07-04 09:28:09 PDT
Sorry, I accidentally sent this to the bots.
Mike West
Comment 13
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!
Mike West
Comment 14
2012-07-04 20:59:02 PDT
Created
attachment 150869
[details]
Tiny fixes.
Adam Barth
Comment 15
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
Mike West
Comment 16
2012-07-04 21:38:00 PDT
Created
attachment 150871
[details]
Patch
Mike West
Comment 17
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!
WebKit Review Bot
Comment 18
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
>
WebKit Review Bot
Comment 19
2012-07-04 23:45:59 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug