Experimental Feature: Allow-CSP-From header

This is part of an experimental feature in Content Security Policy.

This patch introduces a new header Allow-CSP-From that allows the embedded iframe
to use a whitelist to enforce upon itself embedder's CSP.

BUG=647588
Committed: https://crrev.com/69cc847f5802c6d871836695d5f984db682af776
Cr-Commit-Position: refs/heads/master@{#425955}

[amalika, 2016-10-11 19:00:41]

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

[amalika, 2016-10-11 19:06:46]

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

https://codereview.chromium.org/2404373003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:314: bool
ContentSecurityPolicy::checkAllowBlanketEnforcement(
This is the most important function.

After I ran git cl format however, it made me make all the style corrections or
otherwise I could not submit...

https://codereview.chromium.org/2404373003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2404373003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:456:
toLocalFrame(frame()->tree().parent())->document()->url())) {
This function is after x-frames checks where it seems like non local frames are
blocked? 
The tests are passing for cross origin iframes, but not totally sure about
remote frames and what's the correct way to get url() of the parent.

https://codereview.chromium.org/2404373003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:461: // TODO(amalika):
Under this flag up to this pointS, we only support
nit: will fix the wording

[amalika, 2016-10-13 10:33:49]

it turns out someone already committed "blink formatting", I just had to rebase

[Mike West, 2016-10-13 10:40:18]

On 2016/10/13 at 10:33:49, amalika wrote:
> it turns out someone already committed "blink formatting", I just had to
rebase

Much nicer, thank you. :)

[Mike West, 2016-10-13 11:01:42]

Thanks! Here's some feedback!

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

https://codereview.chromium.org/2404373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:318:
response.url().protocolIsAbout() || response.url().protocolIs("blob") ||
Nit: One of these `protocolIsAbout` should probably be `protocolIsData`.

https://codereview.chromium.org/2404373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:327: }
If you pass in an origin, you can change this to
`parentOrigin->canAccess(SecurityOrigin::create(childURL))`.

https://codereview.chromium.org/2404373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:333: it !=
response.httpHeaderFields().end() ? it->value : nullAtom;
You can simplify this check down to something like `String header =
response.httpHeaderField(HTTPNames::Allow_CSP_From)`;

https://codereview.chromium.org/2404373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:339:
header.split(',', headers);
I think we probably don't want to look at all the headers. Let's just grab the
first one. Would you mind filing a bug against the spec to make sure we record
that change?

https://codereview.chromium.org/2404373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:342: if
(equalIgnoringCase(currentHeader, "*")) {
No need for case-folding here: `*` is not a cased character.

https://codereview.chromium.org/2404373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:350: }
This should also be an origin check. That is,
`parentOrigin->canAccess(SecurityOrigin::createFromString(currentHeader)` (which
does the URL parsing internally).

https://codereview.chromium.org/2404373003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/2404373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:356: static
bool checkAllowBlanketEnforcement(const ResourceResponse&,
Nit: Perhaps naming this something like `shouldEnforceEmbeddersPolicy()` would
be clearer? We can adjust the spec as well, as I'm no longer sure that "blanket
enforcement" is the right way to talk about this. :)

Nit2: Can you add a comment here describing what the method does, and perhaps
link to https://w3c.github.io/webappsec-csp/embedded/#origin-allowed as well?

https://codereview.chromium.org/2404373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:357: const
KURL&);
1. As discussed below, passing an origin here would probably be better.
2. Please add a variable name for things like this, as it's not clear what
URL/Origin we're talking about.

https://codereview.chromium.org/2404373003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2404373003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:456:
toLocalFrame(frame()->tree().parent())->document()->url())) {
1. You can't know that the parent is a local frame; it might be remote, at which
point this code would explode.

2. Checking against the URL misses some cases where the URL and the origin don't
match (consider `<iframe sandbox="">` for instance).

I'd suggest just grabbing the parent frame's security context and looking at its
origin for the comparison. Something like:

```
frame()->tree().parent()->securityContext()->getSecurityContext()
```

[amalika, 2016-10-14 08:21:26]

Should we support multiple URLs returned from Allow-CSP-From header?
As per discussion on github, seemed like original proposal was to allow

"allow-csp-from: google.com facebook.com etc..."

[Mike West, 2016-10-14 08:43:40]

On 2016/10/14 at 08:21:26, amalika wrote:
> Should we support multiple URLs returned from Allow-CSP-From header?
> As per discussion on github, seemed like original proposal was to allow
> 
> "allow-csp-from: google.com facebook.com etc..."

I think that's something to consider for step 4 or 5. For the moment, let's just
match the `Origin` header's syntax because it's both consistent and simple to
implement. If we hear from developers that they're serving static content and
really just want X and Y and Z, then we can extend the syntax.

[Mike West, 2016-10-14 09:01:18]

Looking good! A little more feedback, and I'm looking at the layout tests now.

https://codereview.chromium.org/2404373003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2404373003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:314: bool
ContentSecurityPolicy::shouldEnforceEmbeddersPolicy(
Can you add some unit tests for this method in `ContentSecurityPolicyTest.cpp`?

https://codereview.chromium.org/2404373003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:333:
SecurityOrigin::createFromString(headerValue).get()))
I think you can simplify the logic here a bit with something like:

```
String header = response.httpHeaderField(HTTPNames::Allow_CSP_From);
header = header.stripWhiteSpace();
if (header == "*")
  return true;
if (RefPtr<SecurityOrigin> childOrigin =
SecurityOrigin::createFromString(header))
  return parentOrigin->canAccess(childOrigin);

return false;
```

https://codereview.chromium.org/2404373003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:334: return
true;
Nit: `{}` around the body if the `if` clause is multi-line.

[Mike West, 2016-10-14 09:08:20]

https://codereview.chromium.org/2404373003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/embeddedEnforcement/allow_csp_header-same-origin.html
(right):

https://codereview.chromium.org/2404373003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/embeddedEnforcement/allow_csp_header-same-origin.html:10:
injectFrameWithCSP(url, csp, EXPECT_LOAD, SAME_ORIGIN);
We'll also need to test that the policy was applied to the embedded document.
That probably requires a bit more work that we should talk about. :)

[amalika, 2016-10-17 13:21:53]

I did not know how to add two t.done() testing for two post messages. So I just
tested them separately...

https://codereview.chromium.org/2404373003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2404373003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:462: "' because CSP
does not satisfy Embedding-CSP: " +
Not sure what would be a good message?

[Mike West, 2016-10-17 14:30:12]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 14:30:30]

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

[Mike West, 2016-10-17 14:54:28]

Looking good!

https://codereview.chromium.org/2404373003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/embeddedEnforcement/allow_csp_from-header.html
(right):

https://codereview.chromium.org/2404373003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/embeddedEnforcement/allow_csp_from-header.html:15:
url = urlWithAlloCspFrom(SAME_ORIGIN, "");
s/Allo/Allow/g

https://codereview.chromium.org/2404373003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/resources/child-csp-test.js
(right):

https://codereview.chromium.org/2404373003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/resources/child-csp-test.js:21:
window.onerror = t.unreached_func("Error should not be triggered.");
Why `window.onerror`?

https://codereview.chromium.org/2404373003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/resources/child-csp-test.js:36:
console.log("IFrame load event fired: the IFrame's location is '" +
ev.target.contentWindow.location.href + "'.");
This is always going to throw for cross-origin frames. It's not clear whether
you're accounting for that.

https://codereview.chromium.org/2404373003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/resources/child-csp-test.js:46:
function urlWithAlloCspFrom(useCrossOrigin, allowCspFrom) {
Nit: Perhaps `generateUrlWith...` for clarity?

https://codereview.chromium.org/2404373003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp
(right):

https://codereview.chromium.org/2404373003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicyTest.cpp:873: //
Same origin automatically should accept required CSP.
I think this test would be clearer if it didn't rely so much on each piece
building upon the piece before it. Really, it seems like the only thing that
differs is the URL of the resource we're loading. That is, perhaps something
like the following could be clearer:

```
struct TestCase {
  const char* resourceURL;
  const bool inherits;
} cases[] = {
  // Same-origin
  {"https://example.test/index.html", true},
  // Cross-origin
  {"http://example.test/index.html", false},
  {"http://example.test:8443/index.html", false},
  {"https://example.test:8443/index.html", false},
  {"http://not.example.test/index.html", false},
  {"https://not.example.test/index.html", false},
  {"https://not.example.test:8443/index.html", false},
  
  // Inherit
  {"about:blank", true},
  {"data:text/html,yay", true},
  {"blob:https://example.test/bbe708f3-defd-4852-93b6-cf94e032f08d", true},
  {"filesystem:http://example.test/temporary/index.html", true},
};

for (const auto& test : cases) {
  // Set up response with the URL and no header.

  // Make some assertions about `shouldEnforceEmbeddersPolicy`

  // Add a header of `*`
  // Assert.

  // Switch to an invalid header.
  // Assert.

  // Switch to a cross-origin header.
  // Assert.

  // Switch to a matching header.
  // Assert.
}
```

WDYT?

https://codereview.chromium.org/2404373003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2404373003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:462: "' because CSP
does not satisfy Embedding-CSP: " +
On 2016/10/17 at 13:21:53, amalika wrote:
> Not sure what would be a good message?

Perhaps "' because it has not opted-into the following policy required by its
embedder: '"?

[commit-bot: I haz the power, 2016-10-17 16:20:44]

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

[commit-bot: I haz the power, 2016-10-17 16:20:45]

Dry run: This issue passed the CQ dry run.

[amalika, 2016-10-18 08:35:34]

Patchset #6 (id:100001) has been deleted

[Mike West, 2016-10-18 08:39:53]

The CQ bit was checked by mkwst@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 08:40:20]

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

[Mike West, 2016-10-18 08:41:17]

LGTM, if the bots are happy. Thanks for going through a few iterations on this!

I'd suggest that once this lands, it would be a good idea for you to ping the
blink-dev@ and public-webappsec@ threads with a quick note that the core of the
feature is testable. We can start widening the net of folks that might be
interested in poking at this.

[commit-bot: I haz the power, 2016-10-18 09:54:37]

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

[commit-bot: I haz the power, 2016-10-18 09:54:38]

Dry run: This issue passed the CQ dry run.

[amalika, 2016-10-18 14:30:37]

The CQ bit was checked by amalika@google.com

[commit-bot: I haz the power, 2016-10-18 14:30:50]

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-10-18 14:35:24]

Committed patchset #6 (id:110001)

[commit-bot: I haz the power, 2016-10-18 14:37:31]

Description was changed from

==========
This is part of an experimental feature in Content Security Policy.

This patch introduces a new header Allow-CSP-From that allows the embedded
iframe
to use a whitelist to enforce upon itself embedder's CSP.

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

to

==========
This is part of an experimental feature in Content Security Policy.

This patch introduces a new header Allow-CSP-From that allows the embedded
iframe
to use a whitelist to enforce upon itself embedder's CSP.

BUG=647588

Committed: https://crrev.com/69cc847f5802c6d871836695d5f984db682af776
Cr-Commit-Position: refs/heads/master@{#425955}
==========

[commit-bot: I haz the power, 2016-10-18 14:37:32]

Patchset 6 (id:??) landed as
https://crrev.com/69cc847f5802c6d871836695d5f984db682af776
Cr-Commit-Position: refs/heads/master@{#425955}