CSP: Fix handling of nonces in report-only mode.

CSP: Fix handling of nonces in report-only mode.

Our current implementation will only allow a nonce to bypass whitelists if
it is present in all active policies, not just those delivered in
enforce-mode.

This patch adjusts the logic in ContentSecurityPolicy to ensure that nonces
bypass whitelists if and only if they are present in each enforced policy.
In particular, note that policies which do not contain the relevant directive
will no longer be considered as passing a nonce check (though they will, of
course, continue to pass the whitelist check which happens during fetching).

BUG=611652
Committed: https://crrev.com/a309dc2c752b103e6e858a92a0b68ab824212cb0
Cr-Commit-Position: refs/heads/master@{#395189}

[Mike West, 2016-05-13 15:02:01]

Description was changed from

==========
CSP: Fix handling of nonces in report-only mode.

BUG=611652
==========

to

==========
CSP: Fix handling of nonces in report-only mode.

Our current implementation will only allow a nonce to bypass whitelists if
it is present in all active policies, not just those delivered in
enforce-mode.

This patch adjusts the logic in ContentSecurityPolicy to ensure that nonces
bypass whitelists if and only if they are present in each enforced policy.
In particular, note that policies which do not contain the relevant directive
will no longer be considered as passing a nonce check (though they will, of
course, continue to pass the whitelist check which happens during fetching).

BUG=611652
==========

[Mike West, 2016-05-13 15:02:01]

mkwst@chromium.org changed reviewers:
+ estark@chromium.org

[Mike West, 2016-05-13 15:02:23]

Emily, mind taking a look at this?

aaj@: FYI.

[estark, 2016-05-13 15:25:50]

https://codereview.chromium.org/1980533002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/1980533002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:364: int
isExplicitlyAllowed = false;
why an int instead of a bool?

https://codereview.chromium.org/1980533002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:366: if
(policy.get()->headerType() == ContentSecurityPolicyHeaderTypeEnforce) {
Wait... are we skipping report-only policies all together here? Is that
intentional?

[Mike West, 2016-05-15 14:10:08]

On 2016/05/13 at 15:25:50, estark wrote:
>
https://codereview.chromium.org/1980533002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp
(right):
> 
>
https://codereview.chromium.org/1980533002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:364: int
isExplicitlyAllowed = false;
> why an int instead of a bool?

Jetlag? I have no idea why I typed `int`.

>
https://codereview.chromium.org/1980533002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:366: if
(policy.get()->headerType() == ContentSecurityPolicyHeaderTypeEnforce) {
> Wait... are we skipping report-only policies all together here? Is that
intentional?

Yes. Because the nonce check is more or less a bypass. That is, the flow in
ScriptLoader checks whether the nonce attribute matches the active policy, and
if so sets a flag on the request to ignore whitelist violations (see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
That bypass logic only makes sense for enforce-mode.

That said, I think you're right that this doesn't actually make sense, as it
means that we're going to be doing a bad job of reporting the effect of nonces
in a report-only policy. Unfortunately, I think that's going to require some
substantial refactoring of all of this code (and it doesn't work correctly in
the status quo either).

[estark, 2016-05-17 05:57:07]

Ah, ok, thanks for the explanation. LGTM with two caveats:

1. Looks like you might have forgotten to upload a patchset with the int -> bool
change (or maybe you just didn't do it yet, not sure).

2. Could you add a comment with the explanation for why we skip report-only
policies in isAllowedByAllWithNonce()?

[Mike West, 2016-05-20 20:35:52]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-05-20 20:35:53]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org
Link to the patchset: https://codereview.chromium.org/1980533002/#ps20001
(title: "estark@")

[commit-bot: I haz the power, 2016-05-20 20:36:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1980533002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1980533002/20001

[commit-bot: I haz the power, 2016-05-20 22:30:22]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-20 22:31:57]

Description was changed from

==========
CSP: Fix handling of nonces in report-only mode.

Our current implementation will only allow a nonce to bypass whitelists if
it is present in all active policies, not just those delivered in
enforce-mode.

This patch adjusts the logic in ContentSecurityPolicy to ensure that nonces
bypass whitelists if and only if they are present in each enforced policy.
In particular, note that policies which do not contain the relevant directive
will no longer be considered as passing a nonce check (though they will, of
course, continue to pass the whitelist check which happens during fetching).

BUG=611652
==========

to

==========
CSP: Fix handling of nonces in report-only mode.

Our current implementation will only allow a nonce to bypass whitelists if
it is present in all active policies, not just those delivered in
enforce-mode.

This patch adjusts the logic in ContentSecurityPolicy to ensure that nonces
bypass whitelists if and only if they are present in each enforced policy.
In particular, note that policies which do not contain the relevant directive
will no longer be considered as passing a nonce check (though they will, of
course, continue to pass the whitelist check which happens during fetching).

BUG=611652

Committed: https://crrev.com/a309dc2c752b103e6e858a92a0b68ab824212cb0
Cr-Commit-Position: refs/heads/master@{#395189}
==========

[commit-bot: I haz the power, 2016-05-20 22:31:58]

Patchset 2 (id:??) landed as
https://crrev.com/a309dc2c752b103e6e858a92a0b68ab824212cb0
Cr-Commit-Position: refs/heads/master@{#395189}