Bug 116508 – CSP: Implement support for script and style nonces

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Ryosuke Niwa]

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

[Mike West]

Created attachment 234932 [details]
Patch

[Mike West]

Apologies. This patch should have gone on the bug I just created as blocking this bug. I'll move it now...

[Build Bot]

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

[Build Bot]

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

[Mike West]

Created attachment 239319 [details]
Patch

[WebKit Commit Bot]

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.

[Mike West]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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)

[Daniel Bates]

<rdar://problem/24963980>

[Daniel Bates]

Created attachment 273378 [details]
Patch

This patch will fail to apply because it depends on the patch for bug #155007.

[Daniel Bates]

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.

[Brent Fulgham]

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?

[Daniel Bates]

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

[Daniel Bates]

Committed r197944: <http://trac.webkit.org/changeset/197944>