Part 3.4: Is policy list subsumed under subsuming policy?

Part 3.4: Is policy list subsumed under subsuming policy?

In this patch we consider special keywords such as `unsafe-eval`,
`unsafe-hashed-attributes`, `strict-dynamic`.

BUG=647588
Committed: https://crrev.com/522a12cdff7503f3ea42773e09976d84d230a9d9
Cr-Commit-Position: refs/heads/master@{#435957}

[amalika, 2016-11-29 10:46:02]

amalika@google.com changed reviewers:
+ jochen@chromium.org, mkwst@chromium.org

[amalika, 2016-11-29 10:46:03]

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

https://codereview.chromium.org/2538623003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:852: true},
Just wanted to check with you:
This is a desired behavior: if Embedding-CSP allows `unsafe-inline` but returned
csp specifies `strict-dynamic`, the subsumption should be true? i.e.
`unsafe-inline` allows not parser-inserted scripts.

[Mike West, 2016-11-29 12:05:48]

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

https://codereview.chromium.org/2538623003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:630: if (type
== ContentSecurityPolicy::DirectiveType::ScriptSrc &&
Default? Or is |type| the effective directive here?

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

https://codereview.chromium.org/2538623003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:852: true},
On 2016/11/29 at 10:46:03, amalika wrote:
> Just wanted to check with you:
> This is a desired behavior: if Embedding-CSP allows `unsafe-inline` but
returned csp specifies `strict-dynamic`, the subsumption should be true? i.e.
`unsafe-inline` allows not parser-inserted scripts.

Hrm. I don't think that's correct, as `strict-dynamic` might allow more than the
rest of the requested CSP. That is, `'unsafe-inline' 'nonce-a'` doesn't seem to
subsume `'nonce-a' 'strict-dynamic'`, as the latter allows the nonced script to
load anything, while the former doesn't.

I think it's true that `'unsafe-inline'` would subsume `'unsafe-inline'
'strict-dynamic'`, but only because the latter doesn't allow anything. :)

[amalika, 2016-11-29 13:37:45]

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

https://codereview.chromium.org/2538623003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:630: if (type
== ContentSecurityPolicy::DirectiveType::ScriptSrc &&
On 2016/11/29 at 12:05:48, Mike West (slow) wrote:
> Default? Or is |type| the effective directive here?

Yes! It is only the operative directive. On CSPDirctiveList level we never check
DefaultSrc.

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

https://codereview.chromium.org/2538623003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:852: true},
On 2016/11/29 at 12:05:48, Mike West (slow) wrote:
> On 2016/11/29 at 10:46:03, amalika wrote:
> > Just wanted to check with you:
> > This is a desired behavior: if Embedding-CSP allows `unsafe-inline` but
returned csp specifies `strict-dynamic`, the subsumption should be true? i.e.
`unsafe-inline` allows not parser-inserted scripts.
> 
> Hrm. I don't think that's correct, as `strict-dynamic` might allow more than
the rest of the requested CSP. That is, `'unsafe-inline' 'nonce-a'` doesn't seem
to subsume `'nonce-a' 'strict-dynamic'`, as the latter allows the nonced script
to load anything, while the former doesn't.
> 
> I think it's true that `'unsafe-inline'` would subsume `'unsafe-inline'
'strict-dynamic'`, but only because the latter doesn't allow anything. :)

From my understanding, then this example would be `true`? Then I think the logic
right now is correct.
The check now is `!m_allowDynamic && !allowAllInline() && allowDynamicOther`, so
the embedding csp does not subsume the returned csp when the former does *not*
allow all inline behavior (i.e. there are hashes/nonces) and does *not* specify
`strict-dynamic`, while the latter does:
- "'unsafe-inline'" subsumes "'strict-dynamic'"
- "'unsafe-inline' 'nonce-123'" does not subsume "'strict-dynamic'"
- "'unsafe-inline' 'nonce-123' 'strict-dynamic'" subsumes "'strict-dynamic'".

I added these tests right after this one.

[Mike West, 2016-11-30 10:12:53]

https://codereview.chromium.org/2538623003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp
(right):

https://codereview.chromium.org/2538623003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:848: true},
Why is this true?

The group of sources looks like it boils down to something like
`https://example1.com/foo/bar 'strict-dynamic'`. That allows a different set of
sources than `http://example1.com/foo/ 'self' 'unsafe-inline'`, as
`'strict-dynamic'` means that  a script loaded from
`https://example1.com/foo/bar/script.js` could load
`https://evil.com/script.js`.

[amalika, 2016-11-30 10:54:33]

Patchset #3 (id:40001) has been deleted

[amalika, 2016-11-30 10:55:35]

On 2016/11/30 at 10:12:53, mkwst wrote:
>
https://codereview.chromium.org/2538623003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp
(right):
> 
>
https://codereview.chromium.org/2538623003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:848:
true},
> Why is this true?
> 
> The group of sources looks like it boils down to something like
`https://example1.com/foo/bar 'strict-dynamic'`. That allows a different set of
sources than `http://example1.com/foo/ 'self' 'unsafe-inline'`, as
`'strict-dynamic'` means that  a script loaded from
`https://example1.com/foo/bar/script.js` could load
`https://evil.com/script.js`.

Updated so that 'strict-dynamic' is only subsumed by 'strict-dynamic'

[Mike West, 2016-11-30 21:55:51]

LGTM!

[amalika, 2016-12-02 13:41:47]

The CQ bit was checked by amalika@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 13:42:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 15:14:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 15:14:39]

Dry run: This issue passed the CQ dry run.

[amalika, 2016-12-02 16:48:37]

The CQ bit was checked by amalika@google.com

[amalika, 2016-12-02 16:48:37]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2538623003/#ps80001
(title: "Removing strict-dynamic from consideration")

[commit-bot: I haz the power, 2016-12-02 16:49:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 16:55:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480697317167670,
"parent_rev": "76cee6e570129642df4c76956f4499252e2b21e3", "commit_rev":
"97f5ead20a30186e15f46eb4ba12b2e4417ee7f9"}

[commit-bot: I haz the power, 2016-12-02 16:55:36]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-12-02 16:57:50]

Description was changed from

==========
Part 3.4: Is policy list subsumed under subsuming policy?

In this patch we consider special keywords such as `unsafe-eval`,
`unsafe-hashed-attributes`, `strict-dynamic`.

BUG=647588
==========

to

==========
Part 3.4: Is policy list subsumed under subsuming policy?

In this patch we consider special keywords such as `unsafe-eval`,
`unsafe-hashed-attributes`, `strict-dynamic`.

BUG=647588

Committed: https://crrev.com/522a12cdff7503f3ea42773e09976d84d230a9d9
Cr-Commit-Position: refs/heads/master@{#435957}
==========

[commit-bot: I haz the power, 2016-12-02 16:57:51]

Patchset 4 (id:??) landed as
https://crrev.com/522a12cdff7503f3ea42773e09976d84d230a9d9
Cr-Commit-Position: refs/heads/master@{#435957}