Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2536713002:
Part 3.3: Is policy list subsumed under subsuming policy? (Closed)
DescriptionPart 3.3: Is policy list subsumed under subsuming policy?
In this patch we consider special case of `allow all inline`. In
particular, if the returned CSP allows all inline behavior but
Embedding-CSP does not, then the latter does not subsume the former.
BUG=647588
Committed: https://crrev.com/48b3b7a226affaa6e4f009da74252f185238e366
Cr-Commit-Position: refs/heads/master@{#435930}
Patch Set 1 #Patch Set 2 : Rebasing #Patch Set 3 : Rebasing #
Total comments: 3
Patch Set 4 : Using allowAllInline() #
Total comments: 8
Patch Set 5 : Removing checkInline() #Patch Set 6 : Removing upstream part3.2 #Patch Set 7 : Rebasing on master (that includes part3.2 changes) #
Dependent Patchsets: Messages
Total messages: 29 (17 generated)
amalika@google.com changed reviewers: + jochen@chromium.org, mkwst@chromium.org
In this patch we do *not* yet consider `allowEval`, etc attributes but only check if A subsumes B if at least one of them has `unsafe-inline`. https://codereview.chromium.org/2536713002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp (right): https://codereview.chromium.org/2536713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirectiveTest.cpp:631: {false, Note that 'strict-dynamic' is not effective here because we consider `style-src`.
https://codereview.chromium.org/2536713002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): https://codereview.chromium.org/2536713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:589: return m_allowInline && !isHashOrNoncePresent() && Can we use this when responding to `allowInline()` in general? That is, it would be nice if you integrated this into the main CSP check, as I don't think we're currently ignoring `strict-dynamic` for things that aren't `script-src`.
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2536713002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): https://codereview.chromium.org/2536713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:589: return m_allowInline && !isHashOrNoncePresent() && On 2016/11/28 at 15:38:16, Mike West (slow) wrote: > Can we use this when responding to `allowInline()` in general? That is, it would be nice if you integrated this into the main CSP check, as I don't think we're currently ignoring `strict-dynamic` for things that aren't `script-src`. Made changes to call allowAllInline with comments below! https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right): https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:391: if (directive->allowInline() && directive->isHashOrNoncePresent()) { Left this as it is so that we display an appropriate message. Otherwise, allowInline() gets called only by: - this function - and `checkInline` which I changed to use allowAllInline() https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:585: if (type != ContentSecurityPolicy::DirectiveType::DefaultSrc && Added default-src since otherwise unspecified script-src will be failing. https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.h (right): https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirective.h:48: bool allowAllInline(); Changing it to be public
https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp (right): https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/CSPDirectiveList.cpp:166: bool CSPDirectiveList::checkInline(SourceListDirective* directive) const { Can you just remove this method entirely, since it boils down to a single call against the directive that you pass in? https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:609: bool allowInlineOther = other[0]->m_allowInline; Why do you need these three variables? Don't they boil down to calling `allowAllInline()` on all the items?
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:609: bool allowInlineOther = other[0]->m_allowInline; On 2016/11/29 at 12:00:50, Mike West (slow) wrote: > Why do you need these three variables? Don't they boil down to calling `allowAllInline()` on all the items? To call `allowAllInline()` is a method on SourceListDirective, so we can only call it on Embedding-CSP. For returned CSP, we create these variables to keep track of `normalized` values, i.e. normalized policy allows `unsafe-eval` only if all returned policies allow it. Then we check `allowAllInlineOther` for returned CSP on lin2 623 here.
LGTM, other than the question about how you'd like to spell the various variables. I'm fine either way, but exiting early seems cleaner to me. https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:609: bool allowInlineOther = other[0]->m_allowInline; On 2016/11/29 at 12:27:33, amalika wrote: > On 2016/11/29 at 12:00:50, Mike West (slow) wrote: > > Why do you need these three variables? Don't they boil down to calling `allowAllInline()` on all the items? > > To call `allowAllInline()` is a method on SourceListDirective, so we can only call it on Embedding-CSP. > For returned CSP, we create these variables to keep track of `normalized` values, i.e. normalized policy allows `unsafe-eval` only if all returned policies allow it. Then we check `allowAllInlineOther` for returned CSP on lin2 623 here. Hrm. Ok. But you only need them to check `allowAllInlineOther`, right? It seems like you could simplify this (maybe simpler?) with something like the following: ``` for (const auto& directive : other) { if (!allowAllInline() && (directive->m_allowInline && !((type == ContentSecurityPolicy::DirectiveType::ScriptSrc &&directive->m_allowDynamic) || directive->isHashOrNoncePresent())) return false; ... } ``` WDYT?
Please, let me know if my thinking if right! Thanks! https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:609: bool allowInlineOther = other[0]->m_allowInline; On 2016/11/30 at 09:57:19, Mike West (slow) wrote: > On 2016/11/29 at 12:27:33, amalika wrote: > > On 2016/11/29 at 12:00:50, Mike West (slow) wrote: > > > Why do you need these three variables? Don't they boil down to calling `allowAllInline()` on all the items? > > > > To call `allowAllInline()` is a method on SourceListDirective, so we can only call it on Embedding-CSP. > > For returned CSP, we create these variables to keep track of `normalized` values, i.e. normalized policy allows `unsafe-eval` only if all returned policies allow it. Then we check `allowAllInlineOther` for returned CSP on lin2 623 here. > > Hrm. Ok. But you only need them to check `allowAllInlineOther`, right? > > It seems like you could simplify this (maybe simpler?) with something like the following: > > ``` > for (const auto& directive : other) { > if (!allowAllInline() && (directive->m_allowInline && !((type == ContentSecurityPolicy::DirectiveType::ScriptSrc &&directive->m_allowDynamic) || directive->isHashOrNoncePresent())) > return false; > > ... > } > ``` > > WDYT? I dont think this would give an expected behavior because we need to find `effective` allow*() values? For example, if there are two policies in returned CSP: Content-Security-Policy: script-src 'unsafe-inline' http://example.com Content-Security-Policy: script-src 'strict-dynamic' http://example.com Then the `effective` policy should be Content-Security-Policy: script-src http://example.com (is that correct?) Then we can't judge allowAllInline() based on individual policies? Otherwise, we would return `false` after the first policy. So we have to iterate over all of them, saving if all of them allow*() that attribute and then compare with the Embedding-CSP?
The CQ bit was checked by amalika@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by amalika@google.com
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by amalika@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/30 at 10:06:07, amalika wrote: > Please, let me know if my thinking if right! > Thanks! > > https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): > > https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:609: bool allowInlineOther = other[0]->m_allowInline; > On 2016/11/30 at 09:57:19, Mike West (slow) wrote: > > On 2016/11/29 at 12:27:33, amalika wrote: > > > On 2016/11/29 at 12:00:50, Mike West (slow) wrote: > > > > Why do you need these three variables? Don't they boil down to calling `allowAllInline()` on all the items? > > > > > > To call `allowAllInline()` is a method on SourceListDirective, so we can only call it on Embedding-CSP. > > > For returned CSP, we create these variables to keep track of `normalized` values, i.e. normalized policy allows `unsafe-eval` only if all returned policies allow it. Then we check `allowAllInlineOther` for returned CSP on lin2 623 here. > > > > Hrm. Ok. But you only need them to check `allowAllInlineOther`, right? > > > > It seems like you could simplify this (maybe simpler?) with something like the following: > > > > ``` > > for (const auto& directive : other) { > > if (!allowAllInline() && (directive->m_allowInline && !((type == ContentSecurityPolicy::DirectiveType::ScriptSrc &&directive->m_allowDynamic) || directive->isHashOrNoncePresent())) > > return false; > > > > ... > > } > > ``` > > > > WDYT? > > I dont think this would give an expected behavior because we need to find `effective` allow*() values? > For example, if there are two policies in returned CSP: > Content-Security-Policy: script-src 'unsafe-inline' http://example.com > Content-Security-Policy: script-src 'strict-dynamic' http://example.com > > Then the `effective` policy should be Content-Security-Policy: script-src http://example.com > (is that correct?) The effective policy here would be empty, as `'strict-dynamic'` drops host-source expressions. Change the example to `script-src 'strict-dynamic' 'nonce-a'`, and you have a policy that can't actually be expressed as a single policy. Hrm.
On 2016/12/01 at 15:17:07, Mike West (slow) wrote: > On 2016/11/30 at 10:06:07, amalika wrote: > > Please, let me know if my thinking if right! > > Thanks! > > > > https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp (right): > > > > https://codereview.chromium.org/2536713002/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/frame/csp/SourceListDirective.cpp:609: bool allowInlineOther = other[0]->m_allowInline; > > On 2016/11/30 at 09:57:19, Mike West (slow) wrote: > > > On 2016/11/29 at 12:27:33, amalika wrote: > > > > On 2016/11/29 at 12:00:50, Mike West (slow) wrote: > > > > > Why do you need these three variables? Don't they boil down to calling `allowAllInline()` on all the items? > > > > > > > > To call `allowAllInline()` is a method on SourceListDirective, so we can only call it on Embedding-CSP. > > > > For returned CSP, we create these variables to keep track of `normalized` values, i.e. normalized policy allows `unsafe-eval` only if all returned policies allow it. Then we check `allowAllInlineOther` for returned CSP on lin2 623 here. > > > > > > Hrm. Ok. But you only need them to check `allowAllInlineOther`, right? > > > > > > It seems like you could simplify this (maybe simpler?) with something like the following: > > > > > > ``` > > > for (const auto& directive : other) { > > > if (!allowAllInline() && (directive->m_allowInline && !((type == ContentSecurityPolicy::DirectiveType::ScriptSrc &&directive->m_allowDynamic) || directive->isHashOrNoncePresent())) > > > return false; > > > > > > ... > > > } > > > ``` > > > > > > WDYT? > > > > I dont think this would give an expected behavior because we need to find `effective` allow*() values? > > For example, if there are two policies in returned CSP: > > Content-Security-Policy: script-src 'unsafe-inline' http://example.com > > Content-Security-Policy: script-src 'strict-dynamic' http://example.com > > > > Then the `effective` policy should be Content-Security-Policy: script-src http://example.com > > (is that correct?) > > The effective policy here would be empty, as `'strict-dynamic'` drops host-source expressions. Change the example to `script-src 'strict-dynamic' 'nonce-a'`, and you have a policy that can't actually be expressed as a single policy. Hrm. (Land it, we can come back and work out optimizations to the code later. Let's get something working first, even if we don't consider every line a work of art. :) )
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #7 (id:200001) has been deleted
The CQ bit was checked by amalika@google.com
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/2536713002/#ps220001 (title: "Rebasing on master (that includes part3.2 changes)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1480674731271440,
"parent_rev": "97cb2c187af6e4f5a8c975b831415b3330ba7839", "commit_rev":
"f2fd3ad27079e18f1266181cbd1b865cedd6b30e"}
Message was sent while issue was closed.
Committed patchset #7 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Part 3.3: Is policy list subsumed under subsuming policy? In this patch we consider special case of `allow all inline`. In particular, if the returned CSP allows all inline behavior but Embedding-CSP does not, then the latter does not subsume the former. BUG=647588 ========== to ========== Part 3.3: Is policy list subsumed under subsuming policy? In this patch we consider special case of `allow all inline`. In particular, if the returned CSP allows all inline behavior but Embedding-CSP does not, then the latter does not subsume the former. BUG=647588 Committed: https://crrev.com/48b3b7a226affaa6e4f009da74252f185238e366 Cr-Commit-Position: refs/heads/master@{#435930} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/48b3b7a226affaa6e4f009da74252f185238e366 Cr-Commit-Position: refs/heads/master@{#435930} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
