Bug 116508 - CSP: Implement support for script and style nonces
Summary: CSP: Implement support for script and style nonces
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: BlinkMergeCandidate, InRadar, WebExposed
Depends on: 134926 155007
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-20 19:38 PDT by Ryosuke Niwa
Modified: 2016-04-12 13:10 PDT (History)
14 users (show)

See Also:


Attachments
Patch (30.25 KB, patch)
2014-07-15 10:07 PDT, Mike West
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (752.33 KB, application/zip)
2014-07-15 11:45 PDT, Build Bot
no flags Details
Patch (72.29 KB, patch)
2014-10-06 01:31 PDT, Mike West
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (573.56 KB, application/zip)
2014-10-06 02:10 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (534.78 KB, application/zip)
2014-10-06 03:15 PDT, Build Bot
no flags Details
Patch (39.21 KB, patch)
2016-03-08 20:10 PST, Daniel Bates
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-05-20 19:38:48 PDT
Consider merging https://chromium.googlesource.com/chromium/blink/+/adbf3bb0338931076b7c7bd002b043def760cc61

Implementation of W3C compliant CSP script-src nonce. Removes the old 'script-nonce' implementation.

See section 3.2.2 of https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html
Comment 1 Mike West 2014-07-15 10:07:02 PDT
Created attachment 234932 [details]
Patch
Comment 2 Mike West 2014-07-15 10:08:09 PDT
Apologies. This patch should have gone on the bug I just created as blocking this bug. I'll move it now...
Comment 3 Build Bot 2014-07-15 11:45:28 PDT
Comment on attachment 234932 [details]
Patch

Attachment 234932 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5750538901651456

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html
Comment 4 Build Bot 2014-07-15 11:45:32 PDT
Created attachment 234941 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 5 Mike West 2014-10-06 01:31:07 PDT
Created attachment 239319 [details]
Patch
Comment 6 WebKit Commit Bot 2014-10-06 01:32:48 PDT
Attachment 239319 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/cache/CachedResourceLoader.cpp:306:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/page/ContentSecurityPolicy.cpp:1190:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 2 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Mike West 2014-10-06 01:34:03 PDT
ap@, rniwa@: Would either of you mind taking a look at this patch, and helping me find an appropriate reviewer. It's not clear to me who the right person is; there hasn't been much non-mechanical work on WebKit's CSP implementation recently.
Comment 8 Build Bot 2014-10-06 02:10:05 PDT
Comment on attachment 239319 [details]
Patch

Attachment 239319 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6560930326380544

New failing tests:
http/tests/security/contentSecurityPolicy/object-src-none-allowed.html
Comment 9 Build Bot 2014-10-06 02:10:10 PDT
Created attachment 239321 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 10 Build Bot 2014-10-06 03:15:08 PDT
Comment on attachment 239319 [details]
Patch

Attachment 239319 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6621008060481536

New failing tests:
http/tests/security/contentSecurityPolicy/object-src-none-allowed.html
Comment 11 Build Bot 2014-10-06 03:15:13 PDT
Created attachment 239323 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 12 Darin Adler 2014-10-07 22:22:11 PDT
Comment on attachment 239319 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239319&action=review

Doesn't build and needs a bit of work.

> Source/WebCore/page/ContentSecurityPolicy.cpp:107
> +bool isBase64EncodedCharacter(UChar c)

Please mark this function static to give it internal linkage.

> Source/WebCore/page/ContentSecurityPolicy.cpp:623
> +    if (!equalIgnoringCase(noncePrefix.characters8(), begin, noncePrefix.length()))
> +        return true;

This code is incorrect. We can’t call characters8() on a string without first checking if it’s 8 bit or not. Instead this should call the String::startsWith function. Unfortunately I see some mistakes in the implementation of that function that probably need to be fixed.

I think there’s a second mistake here. I don't see any code checking that end - begin is >= nonceprefix.length(). So there could be a buffer overrun.

> Source/WebCore/page/ContentSecurityPolicy.cpp:632
> +    if ((position + 1) != end || *position != '\'' || !(position - nonceBegin))
> +        return false;

No need for parentheses in the (position + 1) != end expression.

!(position - nonceBegin) seems like a strange way to write position != nonceBegin.

> Source/WebCore/page/ContentSecurityPolicy.cpp:807
> +    bool allowNonce(const String& nonce) const { return m_sourceList.allowNonce(nonce.stripWhiteSpace()); }

Seems a little strange that the stripWhitespace is done here, rather than in the CSPSourceList::allowNonce function or at the call site. Seems almost random to do it at this level rather than the lowest level or the highest. Maybe there's some rationale for this being the right level.

I think we probably want stripLeadingAndTrailingHTMLSpaces here, rather than stripWhiteSpace. I don’t think we want to strip arbitrary Unicode whitespace, rather just the usual HTML parser idiom whitespace characters.

> Source/WebCore/page/ContentSecurityPolicy.cpp:1011
> +    String suffix = directive->allowInline() && directive->isNoncePresent()

Can we use const char* here to avoid creating a String multiple times? Should require only a small amount of rearranging.

> Source/WebCore/page/ContentSecurityPolicy.cpp:1016
> -        suffix = makeString(" Note that '", (isScript ? "script" : "style"), "-src' was not explicitly set, so 'default-src' is used as a fallback.");
> +        suffix = suffix + " Note also that '" + String(isScript ? "script" : "style") + "-src' was not explicitly set, so 'default-src' is used as a fallback.";

Why switch away from makeString here? That just forces us to allocate an extra string for the "script" and "style" for no good reason.

> Source/WebCore/page/ContentSecurityPolicy.cpp:1516
> +    for (size_t i = 0; i < policies.size(); ++i) {
> +        if (!(policies[i].get()->*allowed)(nonce))
> +            return false;
> +    }

Should use a modern for loop:

    for (auto& policy : policies)
Comment 13 Daniel Bates 2016-03-08 19:42:43 PST
<rdar://problem/24963980>
Comment 14 Daniel Bates 2016-03-08 20:10:47 PST
Created attachment 273378 [details]
Patch

This patch will fail to apply because it depends on the patch for bug #155007.
Comment 15 Daniel Bates 2016-03-08 20:31:54 PST
Comment on attachment 273378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273378&action=review

> Source/WebCore/ChangeLog:3
> +        Implementation of W3C compliant CSP script-src nonce

Will fix bug title before landing.
Comment 16 Brent Fulgham 2016-03-10 09:29:50 PST
Comment on attachment 273378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273378&action=review

r=me. Please make sure tests pass, since EWS is not happy.

> Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:422
> +    if (!StringView(begin, end - begin).startsWithIgnoringASCIICase(String(ASCIILiteral(noncePrefix))))

Is there any reason not to make this String(ASCIILiteral(noncePrefix))) its own Static variable, rather than computing it each time we parse a nonce?
Comment 17 Daniel Bates 2016-03-10 09:53:56 PST
(In reply to comment #16)
> Comment on attachment 273378 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273378&action=review
> 
> r=me. Please make sure tests pass, since EWS is not happy.
> 
> > Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp:422
> > +    if (!StringView(begin, end - begin).startsWithIgnoringASCIICase(String(ASCIILiteral(noncePrefix))))
> 
> Is there any reason not to make this String(ASCIILiteral(noncePrefix))) its
> own Static variable, rather than computing it each time we parse a nonce?

No, there is no reason not to create a String object for this with static storage duration. Will change the first line in this function from:

static const char noncePrefix[] = "'nonce-";

to

static NeverDestroyed<String> noncePrefix("'nonce-", String::ConstructFromLiteral);

Then this line can be written as:

if (!StringView(begin, end - begin).startsWithIgnoringASCIICase(noncePrefix.get()))

And will substitute "noncePrefix.get().length()" for "sizeof(noncePrefix) - 1" on line 424.
Comment 18 Daniel Bates 2016-03-10 10:30:33 PST
Committed r197944: <http://trac.webkit.org/changeset/197944>