Make Document::isSecureContext() work for OOPIFs

Make Document::isSecureContext() work for OOPIFs

The inputs needed for isSecureContext() were, previously:
a.) the origins of the current frame and its ancestors
b.) for any sandboxed origins in the ancestor chain, the URLs of those frames

(a) is easily accessible in --site-per-process, but (b) is not. For
sandboxed frames, the URL was used to construct a stand-in origin. The
sandboxed frame was considered potentially trustworthy if the stand-in
origin was potentially trustworthy.

In --site-per-process, instead of using the URL for a sandboxed origin,
we save the necessary information (the potential-trustworthiness) at the
time that the origin becomes sandboxed.

Based on https://codereview.chromium.org/1716303002/

BUG=571079
TEST=secureContexts/* layout tests with --site-per-process
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/bd8e26fa87f192482bc35f5417a2616f85a5bde5
Cr-Commit-Position: refs/heads/master@{#381587}

[estark, 2016-02-23 00:46:18]

Description was changed from

==========
Make Document::isSecureContext() work for OOPIFs

The inputs needed for isSecureContext() were, previously:
a.) the origins of the current frame and its ancestors
b.) for any sandboxed origins in the ancestor chain, the URLs of those frames

(a) is easily accessible in --site-per-process, but (b) is not. For
sandboxed frames, the URL was used to construct a stand-in origin. The
sandboxed frame was considered potentially trustworthy if the stand-in
origin was potentially trustworthy. The protocol of the stand-in origin
was also used to bypass the secure context check for chrome-extension://
pages.

In --site-per-process, instead of using the URL for a sandboxed origin,
we save the necessary information at the time that the origin becomes
sandboxed: namely, the potential-trustworthiness of the original origin
and whether its scheme should bypass secure context checks.

Based on https://codereview.chromium.org/1716303002/

BUG=571079
==========

to

==========
Make Document::isSecureContext() work for OOPIFs

The inputs needed for isSecureContext() were, previously:
a.) the origins of the current frame and its ancestors
b.) for any sandboxed origins in the ancestor chain, the URLs of those frames

(a) is easily accessible in --site-per-process, but (b) is not. For
sandboxed frames, the URL was used to construct a stand-in origin. The
sandboxed frame was considered potentially trustworthy if the stand-in
origin was potentially trustworthy. The protocol of the stand-in origin
was also used to bypass the secure context check for chrome-extension://
pages.

In --site-per-process, instead of using the URL for a sandboxed origin,
we save the necessary information at the time that the origin becomes
sandboxed: namely, the potential-trustworthiness of the original origin
and whether its scheme should bypass secure context checks.

Based on https://codereview.chromium.org/1716303002/

BUG=571079
TEST=secureContexts/* layout tests with --site-per-process
==========

[estark, 2016-02-23 21:45:51]

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

[estark, 2016-02-23 21:45:52]

Mike, Alex, could you please take a look at this CL for OOPIF-ifying
Document::isSecureContext()?

The main challenge is having enough information about sandboxed unique-origin
OOPIFs that might be in the ancestor chain of a document. To deal with this, I
am storing some the necessary pieces of information on the SecurityContext at
the point when an origin becomes sandboxed. These pieces of information can then
be read during isSecureContext() checks.

https://codereview.chromium.org/1723753002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):

https://codereview.chromium.org/1723753002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:81:
ASSERT(securityOrigin());
I'm not sure if this will actually work... I looked through the
enforceSandboxFlags() call sites and it looked like this was a reasonable thing
to do, and the trybots seem happy, but I'm not sure.

[Mike West, 2016-02-24 08:40:59]

https://codereview.chromium.org/1723753002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):

https://codereview.chromium.org/1723753002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:81:
ASSERT(securityOrigin());
On 2016/02/23 at 21:45:52, estark wrote:
> I'm not sure if this will actually work... I looked through the
enforceSandboxFlags() call sites and it looked like this was a reasonable thing
to do, and the trybots seem happy, but I'm not sure.

This should be the case, but there might be some weirdness around `about:blank`
documents. Should already be present in layout tests, however, so if the bots
are happy, adding this ASSERT seems totally reasonable.

https://codereview.chromium.org/1723753002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:91:
setSecurityOrigin(SecurityOrigin::createUnique());
Following on from above, this might look something like:

~~~
bool wasTrustworthy = securityOrigin()->isPotentiallyTrustworthy();
bool wasBypassing =
SchemeRegistry::schemeShouldBypassSecureContextCheck(securityOrigin()->protocol());
setSecurityOrigin(SecurityOrigin::createUnique());
securityOrigin()->setTrustworthynesses(wasTrustworthy, wasBypassing);
~~~

and the `Document::isSecureContextImpl` could be simplified to just use
`context->securityOrigin()->isPotentiallyTrustworthy()` throughout.

WDYT?

https://codereview.chromium.org/1723753002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/SecurityContext.h (right):

https://codereview.chromium.org/1723753002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/SecurityContext.h:88: bool
isPotentiallyTrustworthySandboxedOrigin() const { return
m_isPotentiallyTrustworthySandboxedOrigin; }
Did you consider putting this information into SecurityOrigin instead? Since
that's the object responsible for determining trustworthyness, it might makes
sense for it to take responsibility for this kind of calculation.

[estark, 2016-02-24 21:59:51]

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

[estark, 2016-02-24 21:59:52]

Thanks Mike.

I'm also adding dcheng: I'm curious what you think of this patch because I think
it's in nontrivial conflict with your CL
https://codereview.chromium.org/1685003002/, which I happened to notice on IRC.

https://codereview.chromium.org/1723753002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):

https://codereview.chromium.org/1723753002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:91:
setSecurityOrigin(SecurityOrigin::createUnique());
On 2016/02/24 08:40:59, Mike West wrote:
> Following on from above, this might look something like:
> 
> ~~~
> bool wasTrustworthy = securityOrigin()->isPotentiallyTrustworthy();
> bool wasBypassing =
>
SchemeRegistry::schemeShouldBypassSecureContextCheck(securityOrigin()->protocol());
> setSecurityOrigin(SecurityOrigin::createUnique());
> securityOrigin()->setTrustworthynesses(wasTrustworthy, wasBypassing);
> ~~~
> 
> and the `Document::isSecureContextImpl` could be simplified to just use
> `context->securityOrigin()->isPotentiallyTrustworthy()` throughout.
> 
> WDYT?
> 

Done.

https://codereview.chromium.org/1723753002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/SecurityContext.h (right):

https://codereview.chromium.org/1723753002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/SecurityContext.h:88: bool
isPotentiallyTrustworthySandboxedOrigin() const { return
m_isPotentiallyTrustworthySandboxedOrigin; }
On 2016/02/24 08:40:59, Mike West wrote:
> Did you consider putting this information into SecurityOrigin instead? Since
> that's the object responsible for determining trustworthyness, it might makes
> sense for it to take responsibility for this kind of calculation.

That seems reasonable. I moved these same flags on to SecurityOrigin and made a
version of createUnique() that takes the flag values as input; let me know what
you think of this version.

[dcheng, 2016-02-24 22:11:30]

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4929: // will adjust it to be a
unique origin if necessary.
"if necessary": does that mean there are combinations of sandbox flags that
could result in the origin being shared?

[estark, 2016-02-24 22:14:51]

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4929: // will adjust it to be a
unique origin if necessary.
On 2016/02/24 22:11:30, dcheng wrote:
> "if necessary": does that mean there are combinations of sandbox flags that
> could result in the origin being shared?

I might be misunderstanding the question, but the origin could be non-unique any
time SandboxOrigin is not set, right? i.e. sandbox="allow-same-origin"?

[dcheng, 2016-02-24 22:17:26]

On 2016/02/24 at 22:14:51, estark wrote:
>
https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/Document.cpp:4929: // will adjust it to be
a unique origin if necessary.
> On 2016/02/24 22:11:30, dcheng wrote:
> > "if necessary": does that mean there are combinations of sandbox flags that
> > could result in the origin being shared?
> 
> I might be misunderstanding the question, but the origin could be non-unique
any time SandboxOrigin is not set, right? i.e. sandbox="allow-same-origin"?

Ah I see. Patch seems reasonable, but I'll defer the review to alexmos@/mkwst@…
the conflicts shouldn't be too hard to resolve elegantly I hope =)

[Mike West, 2016-02-25 11:58:11]

LGTM % nits. Thanks for taking another pass, Emily!

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated_sandbox.html
(right):

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated_sandbox.html:4:
<title>Unauthenticated origin with sandbox iframe is insecure</title>
Nit: s/Unauthenticated/Authenticated/, s/insecure/secure/

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated_srcdoc.html
(right):

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated_srcdoc.html:4:
<title>Unauthenticated origin with srcdoc iframe is insecure</title>
Nit: s/Unauthenticated/Authenticated/, s/insecure/secure/

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:78: // The security
origin must be set already in order to determine a sandboxed origin's potential
trustworthiness.
We apparently don't have any unit tests for this method (nor do we have a
SecurityContextTest.cpp at all, which is worrisome :) ).

Would you mind adding a small check for `enforceSandboxFlags` in
`DocumentTest.cpp` to verify that we're doing the right thing? If you wanted to
add tests for isSecureContext at the same time, I certainly wouldn't complain.
:)

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h (right):

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h:151: bool
bypassSecureContextCheck() const;
Would you mind adding unit tests for these new methods in
SecurityOriginTest.cpp?

[alexmos, 2016-02-26 02:02:50]

I've started going through this and will send other comments a bit later, but I
think some secureContexts tests might be broken?  I was initially surprised that
unauthenticated.html test passes under --site-per-process even without this CL
(it's not in our filter, LayoutTests/FlagExpectations/site-per-process).  That
test has an HTTP page that embeds HTTP and HTTPS iframes, and since today we
skip the check and just return true in isSecureContext for remote parent frames,
the test should fail for the HTTPS iframe, as it should incorrectly return true.
 I dug a bit into this, and the frame is doing this:

    ["top", "parent", "opener"].forEach(function (x) {
        if (window[x] && window[x] != window)
            window[x].postMessage({ "isSecureContext": window.isSecureContext },
"*");
    });

which sends two messages to the parent frame (one via "top", one via "parent");
opener is null so it's skipped.

There are two frames in the parent page, so four messages total.  But the parent
page only waits for two:

    if (messages >= 2)
        t.done();

The first two are from the HTTP frame, so we never check the messages from the
HTTPS frame.  Changing that to "if (messages >= 4)" makes the test fail the
assert_false(e.data.isSecureContext) under --site-per-process, as I would
expect.

Any reason for sending the message to all of top/parent/opener, as opposed to
just top?  I don't see it used anywhere where the opener would matter, and in
the new tests where top and parent differ (due to srcdoc middle frames), the
middle frame doesn't handle the message anyway, while the top frame won't know
how many messages to expect depending on the test.

[alexmos, 2016-02-26 19:21:57]

These changes look good, but I'm a bit confused about whether we also need to do
some additional work to replicate the two new bits in SecurityOrigin across
processes.  Were you perhaps planning to do that in a separate CL?

More specifically, suppose you have an HTTPS page from site A that has a
sandboxed frame also from site A.  After initializing its SecurityContext, the
frame's origin will be unique with the m_isUniqueOriginPotentiallyTrustworthy
bit set.  However, when we send that origin over to the browser process on
commit, the bit would get lost (during translation to url::Origin).  Now suppose
the sandboxed frame embeds another grandchild frame, also HTTPS but from site B.
 We'll kick off a new process for site B, where the middle frame's replicated
origin would just be unique, with m_isUniqueOriginPotentiallyTrustworthy set to
false by default.  So when the bottom frame evals isSecureContext, it will get
false because of the middle frame, even though it should get true.

Does that reasoning make sense?  To double-check, I patched in your CL and
modified the srcdoc in authenticated_sandbox.html to use another HTTPS origin
for the bottom frame:
    i1.srcdoc = "<iframe src='" + get_host_info().HTTPS_REMOTE_ORIGIN +
"/security/secureContexts/resources/post-securecontext-status.html" +
"'></iframe>";

And that test fails with --site-per-process but passes without.

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated_sandbox.html
(right):

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated_sandbox.html:31:
i1.srcdoc = "<iframe src='" + get_host_info().AUTHENTICATED_ORIGIN +
"/security/secureContexts/resources/post-securecontext-status.html" +
"'></iframe>";
nit: wrong indent (and line below).  Also for other files on these lines.

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4986: // load local resources.
This lets about:blank iframes in file://
nit: perhaps s/This/The latter/ or something similar to clarify that the second
sentence pertains only to ability to load local resources, not to origin
trustworthiness.

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:87:
setSecurityOrigin(SecurityOrigin::createUnique(securityOrigin()->isPotentiallyTrustworthy(),
securityOrigin()->isPotentiallyTrustworthy() &&
SchemeRegistry::schemeShouldBypassSecureContextCheck(securityOrigin()->protocol())));
Would it make sense for the scheme check to just use
securityOrigin()->bypassSecureContextCheck() that you've added?

[estark, 2016-02-29 23:35:13]

Patchset #4 (id:60001) has been deleted

[estark, 2016-03-01 00:15:46]

Patchset #5 (id:100001) has been deleted

[estark, 2016-03-01 02:40:39]

Patchset #6 (id:140001) has been deleted

[estark, 2016-03-01 02:59:26]

Ok, thanks for the comments Alex and Mike. I've made some changes:

1.) As Alex pointed out, the secureContexts layout tests are buggy. I mailed a
fix for that in a separate CL (https://codereview.chromium.org/1745333002/) and
rebased this one on top of that.

2.) Addressed inline comments, including some new unit tests.

3.) Also as Alex pointed out, this CL didn't really work. :( Mostly due to the
fact that I neglected to update the layout tests to use OOPIFs, so my changes
didn't actually get tested. (Not sure how I convinced myself that this would
work without replicating the flags to other processes...) So now I've updated
the CL to replicate the flags through url::Origin, but, blargh, it feels pretty
gross. Mike: is this the kind of thing that you intended to live on url::Origin?
It feels weird to have such a basic class know about very Blink-y things like
secure context checks, but if its main purpose is to communicate origin thingies
between Blink and content, maybe that's what it's intended for.

If not, then another option would be to pass these flags around between
processes alongside the sandbox flags; for example, maybe
WebRemoteFrame::setReplicatedSandboxFlags() would take two booleans in addition
to the sandbox flags themselves. Or they could just be replicated on their own,
with their own set of plumbing methods, a la
setReplicatedShouldEnforceStrictMixedContentChecking(). Either one of those
would work, I think, and would feel less weird to me than putting them in
url::Origin, but would also be a lot more boilerplate plumbing.

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated_sandbox.html
(right):

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated_sandbox.html:4:
<title>Unauthenticated origin with sandbox iframe is insecure</title>
On 2016/02/25 11:58:11, Mike West wrote:
> Nit: s/Unauthenticated/Authenticated/, s/insecure/secure/

Done.

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated_sandbox.html:31:
i1.srcdoc = "<iframe src='" + get_host_info().AUTHENTICATED_ORIGIN +
"/security/secureContexts/resources/post-securecontext-status.html" +
"'></iframe>";
On 2016/02/26 19:21:57, alexmos wrote:
> nit: wrong indent (and line below).  Also for other files on these lines.

Done.

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated_srcdoc.html
(right):

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated_srcdoc.html:4:
<title>Unauthenticated origin with srcdoc iframe is insecure</title>
On 2016/02/25 11:58:11, Mike West wrote:
> Nit: s/Unauthenticated/Authenticated/, s/insecure/secure/

Done.

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4986: // load local resources.
This lets about:blank iframes in file://
On 2016/02/26 19:21:57, alexmos wrote:
> nit: perhaps s/This/The latter/ or something similar to clarify that the
second
> sentence pertains only to ability to load local resources, not to origin
> trustworthiness.

Done.

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/SecurityContext.cpp (right):

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:78: // The security
origin must be set already in order to determine a sandboxed origin's potential
trustworthiness.
On 2016/02/25 11:58:11, Mike West wrote:
> We apparently don't have any unit tests for this method (nor do we have a
> SecurityContextTest.cpp at all, which is worrisome :) ).
> 
> Would you mind adding a small check for `enforceSandboxFlags` in
> `DocumentTest.cpp` to verify that we're doing the right thing? If you wanted
to
> add tests for isSecureContext at the same time, I certainly wouldn't complain.
> :)

Done. I'm happy to add isSecureContext() tests but I'll do it in a separate CL,
since this one's already gotten a bit big now that I've added the url::Origin
stuff.

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:87:
setSecurityOrigin(SecurityOrigin::createUnique(securityOrigin()->isPotentiallyTrustworthy(),
securityOrigin()->isPotentiallyTrustworthy() &&
SchemeRegistry::schemeShouldBypassSecureContextCheck(securityOrigin()->protocol())));
On 2016/02/26 19:21:57, alexmos wrote:
> Would it make sense for the scheme check to just use
> securityOrigin()->bypassSecureContextCheck() that you've added?

Done.

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h (right):

https://codereview.chromium.org/1723753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h:151: bool
bypassSecureContextCheck() const;
On 2016/02/25 11:58:11, Mike West wrote:
> Would you mind adding unit tests for these new methods in
> SecurityOriginTest.cpp?

Done.

[alexmos, 2016-03-01 23:32:33]

Thanks for taking another pass, Emily!  I'll defer to Mike on whether this is ok
to plumb through url::Origin, but otherwise LGTM.  Is this something that might
eventually be used for security checks in the browser process?

https://codereview.chromium.org/1723753002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/1723753002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:397: bool
SecurityOrigin::bypassSecureContextCheck() const
nit: bypassesSecureContextCheck or shouldBypassSecureContextCheck might sound a
bit clearer.  This is probably fine though.

https://codereview.chromium.org/1723753002/diff/180001/url/origin.cc
File url/origin.cc (right):

https://codereview.chromium.org/1723753002/diff/180001/url/origin.cc#newcode20
url/origin.cc:20: Origin::Origin() : unique_(true) {
Does this one also need to init the new bits?

[Mike West, 2016-03-02 14:02:57]

https://codereview.chromium.org/1723753002/diff/180001/url/origin.h
File url/origin.h (right):

https://codereview.chromium.org/1723753002/diff/180001/url/origin.h#newcode132
url/origin.h:132: }
Not LGTM, sorry. This isn't going to fly. `url::Origin` should know about
RFC6454, and a little bit of Fetch, but we're going to have to find another way
to smuggle this data up the stack. Passing around another set of flags sounds
like a little more work, but a much cleaner result.

Actually, now that I think about it, you might want to take a look at moving
this data to something like `SecurityOrigin::PrivilegeData`, and piping that up
to the browser rather than passing a million individual flags.

[estark, 2016-03-06 17:47:40]

Description was changed from

==========
Make Document::isSecureContext() work for OOPIFs

The inputs needed for isSecureContext() were, previously:
a.) the origins of the current frame and its ancestors
b.) for any sandboxed origins in the ancestor chain, the URLs of those frames

(a) is easily accessible in --site-per-process, but (b) is not. For
sandboxed frames, the URL was used to construct a stand-in origin. The
sandboxed frame was considered potentially trustworthy if the stand-in
origin was potentially trustworthy. The protocol of the stand-in origin
was also used to bypass the secure context check for chrome-extension://
pages.

In --site-per-process, instead of using the URL for a sandboxed origin,
we save the necessary information at the time that the origin becomes
sandboxed: namely, the potential-trustworthiness of the original origin
and whether its scheme should bypass secure context checks.

Based on https://codereview.chromium.org/1716303002/

BUG=571079
TEST=secureContexts/* layout tests with --site-per-process
==========

to

==========
Make Document::isSecureContext() work for OOPIFs

The inputs needed for isSecureContext() were, previously:
a.) the origins of the current frame and its ancestors
b.) for any sandboxed origins in the ancestor chain, the URLs of those frames

(a) is easily accessible in --site-per-process, but (b) is not. For
sandboxed frames, the URL was used to construct a stand-in origin. The
sandboxed frame was considered potentially trustworthy if the stand-in
origin was potentially trustworthy. The protocol of the stand-in origin
was also used to bypass the secure context check for chrome-extension://
pages.

In --site-per-process, instead of using the URL for a sandboxed origin,
we save the necessary information at the time that the origin becomes
sandboxed: namely, the potential-trustworthiness of the original origin
and whether its scheme should bypass secure context checks.

Based on https://codereview.chromium.org/1716303002/

BUG=571079
TEST=secureContexts/* layout tests with --site-per-process
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[estark, 2016-03-06 22:56:34]

Patchset #9 (id:220001) has been deleted

[estark, 2016-03-07 05:24:21]

Patchset #9 (id:240001) has been deleted

[estark, 2016-03-08 06:15:18]

Ok, I've taken another crack at this following Mike's stamp of doom. :)

I realized that we don't actually need to replicate the bypassSecureContextCheck
bit in other processes; we only ever check that on the current document because
that flag doesn't get inherited to other frames. So that simplifies things a
bit, and I'm now just threading through the isUniqueOriginPotentiallyTrustworthy
flag similarly to how strict mixed content checking is replicated. Please take a
look.

As Mike pointed out, there is some other data on SecurityOrigins that may need
to be replicated as well (the stuff that goes into PrivilegeData, and maybe
other stuff too). It would be a somewhat larger refactor to replicate all of
that, though, as we have to make sure that whenever those flags are set, a
message is sent to the browser. (I did that by overriding
SecurityContext::enforceSandboxFlags() in Document here in this CL.) I'm also
getting a bit weirded out by the fact that all this plumbing in content/ has to
know about all these Blink-y things like strict mixed content checking and
potential trustworthiness. I'm wondering if there's some way that we can
replicate stuff to frame proxies without content/ needing to know exactly what
the "stuff" is.

Regardless, though, my preference would be to go ahead with the
isUniqueOriginPotentiallyTrustworthy plumbing to un-break isSecureContext(), and
then think about a cleaner way that we can replicate Blink frame state in
general.

[Mike West, 2016-03-08 11:04:58]

On 2016/03/08 at 06:15:18, estark wrote:
> Ok, I've taken another crack at this following Mike's stamp of doom. :)

Oof. Sorry this exploded the size of the patch. :/

> I realized that we don't actually need to replicate the
bypassSecureContextCheck bit in other processes; we only ever check that on the
current document because that flag doesn't get inherited to other frames. So
that simplifies things a bit, and I'm now just threading through the
isUniqueOriginPotentiallyTrustworthy flag similarly to how strict mixed content
checking is replicated. Please take a look.

The Blinky bits LGTM, as do the IPC bits. I'd like alexmos to take another look
at the //content side, however. I think I agree with your assessment that this
is a reasonable fix for the acute problem, but I agree with you that it's
getting a little bit strange in terms of layering.

[alexmos, 2016-03-09 18:45:06]

This looks good, though a couple of high-level questions about the new
replication IPCs:

- do we really need a separate renderer->browser IPC for dynamically updating
the new bit, in addition to the update in DidCommitProvisionalLoad?  Normal
sandbox flags in <iframe>'s take effect on next navigation, so a commit-time
update is enough.  The one scenario I can think of is if the page inserts a
<meta> tag with a CSP that sets sandbox flags which make the origin unique.  Are
there other cases I'm forgetting about?  And for sandbox flags in <meta>, do we
really want to support that? :)  The spec explicitly says to discard sandbox
flags in that case (https://www.w3.org/TR/CSP2/#delivery-html-meta-element), but
it seems we still might support it, similarly to frame-ancestors?  I think Mike
mentioned we eventually might want to drop frame-ancestors in <meta> - what
about sandbox flags?

- It seems the new bit can only change when the origin changes, so depending on
the answer above, I'm wondering if it might be simpler to piggyback the flag on
the existing DidUpdateOrigin IPC and setReplicatedOrigin, instead of the new
browser->renderer IPC, if the two are going to be sent at the same time?

I think this is fine for now (and will let us unblock the trial), but
longer-term, I agree that it'd be nice to have a separate struct with bits like
this and PrivilegeData on FrameReplicationState and a cleaner way to keep it
updated.  I'm fine if we do it as a follow-up.

Also, description still mentions scheme bypassing security checks. :)

https://codereview.chromium.org/1723753002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentTest.cpp (right):

https://codereview.chromium.org/1723753002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentTest.cpp:379:
EXPECT_FALSE(document().securityOrigin()->isPotentiallyTrustworthy());
It seems this part of the test still might have been useful to verify that
putting a scheme on the bypass list won't make the origin potentially
trustworthy, no?

https://codereview.chromium.org/1723753002/diff/360001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1723753002/diff/360001/content/common/frame_m...
content/common/frame_messages.h:388:
IPC_STRUCT_TRAITS_MEMBER(is_potentially_trustworthy_unique_origin)
nit: maybe that should be named has_potentialy_trustworthy_unique_origin?  Since
these are frame's properties rather than origin's, and "a frame has a
potentially trustworthy unique origin" sounds better than "a frame is a
potentially trustworthy unique origin".

https://codereview.chromium.org/1723753002/diff/360001/content/common/frame_m...
content/common/frame_messages.h:774: // set to or unset from a unique,
potentially trustworthy origin.
Maybe add another sentence here to explain under what conditions we might unset
the flag?  Presumably, this might happen when a frame loses sandbox flags and
commits a new navigation (triggering this update message from
NavigatorImpl::DidNavigate)?

https://codereview.chromium.org/1723753002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (left):

https://codereview.chromium.org/1723753002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5004: if
(securityOrigin()->hasSuborigin())
Weird that there were two identical enforceSuborigin calls previously.  That was
presumably a bug?

https://codereview.chromium.org/1723753002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h (right):

https://codereview.chromium.org/1723753002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h:253: void
setUniqueOriginIsPotentiallyTrustworthy(bool
isUniqueOriginPotentiallyTrustworthy) { m_isUniqueOriginPotentiallyTrustworthy =
isUniqueOriginPotentiallyTrustworthy; }
Would it make sense to also assert here that the origin is always unique when
this is called?

[estark, 2016-03-10 00:39:08]

Description was changed from

==========
Make Document::isSecureContext() work for OOPIFs

The inputs needed for isSecureContext() were, previously:
a.) the origins of the current frame and its ancestors
b.) for any sandboxed origins in the ancestor chain, the URLs of those frames

(a) is easily accessible in --site-per-process, but (b) is not. For
sandboxed frames, the URL was used to construct a stand-in origin. The
sandboxed frame was considered potentially trustworthy if the stand-in
origin was potentially trustworthy. The protocol of the stand-in origin
was also used to bypass the secure context check for chrome-extension://
pages.

In --site-per-process, instead of using the URL for a sandboxed origin,
we save the necessary information at the time that the origin becomes
sandboxed: namely, the potential-trustworthiness of the original origin
and whether its scheme should bypass secure context checks.

Based on https://codereview.chromium.org/1716303002/

BUG=571079
TEST=secureContexts/* layout tests with --site-per-process
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make Document::isSecureContext() work for OOPIFs

The inputs needed for isSecureContext() were, previously:
a.) the origins of the current frame and its ancestors
b.) for any sandboxed origins in the ancestor chain, the URLs of those frames

(a) is easily accessible in --site-per-process, but (b) is not. For
sandboxed frames, the URL was used to construct a stand-in origin. The
sandboxed frame was considered potentially trustworthy if the stand-in
origin was potentially trustworthy.

In --site-per-process, instead of using the URL for a sandboxed origin,
we save the necessary information (the potential-trustworthiness) at the
time that the origin becomes sandboxed.

Based on https://codereview.chromium.org/1716303002/

BUG=571079
TEST=secureContexts/* layout tests with --site-per-process
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[estark, 2016-03-10 00:53:22]

Thanks for taking another look.

On 2016/03/09 18:45:06, alexmos wrote:
> This looks good, though a couple of high-level questions about the new
> replication IPCs:
> 
> - do we really need a separate renderer->browser IPC for dynamically updating
> the new bit, in addition to the update in DidCommitProvisionalLoad?  Normal
> sandbox flags in <iframe>'s take effect on next navigation, so a commit-time
> update is enough.  The one scenario I can think of is if the page inserts a
> <meta> tag with a CSP that sets sandbox flags which make the origin unique. 
Are
> there other cases I'm forgetting about?  And for sandbox flags in <meta>, do
we
> really want to support that? :)  The spec explicitly says to discard sandbox
> flags in that case (https://www.w3.org/TR/CSP2/#delivery-html-meta-element),
but
> it seems we still might support it, similarly to frame-ancestors?  I think
Mike
> mentioned we eventually might want to drop frame-ancestors in <meta> - what
> about sandbox flags?

Oh, interesting... dynamically-added CSP meta was the thing that I had in mind
when I made it dynamically-updateable, but I didn't realize that sandbox flags
in meta tags are supposed to be discarded. But I think it would be confusing for
<meta>-delivered sandbox flags to work in some cases, but not in others. Maybe
we should leave it dynamically-updateable now, and file a bug to remove support
for sandbox flags in <meta>? (Or at least measure to see if we can remove it
without breaking too many things.)

(There's also this enforceSandboxFlags() callsite that I don't really understand
and that might or might not happen after DidCommitProvisionalLoad:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)

> 
> - It seems the new bit can only change when the origin changes, so depending
on
> the answer above, I'm wondering if it might be simpler to piggyback the flag
on
> the existing DidUpdateOrigin IPC and setReplicatedOrigin, instead of the new
> browser->renderer IPC, if the two are going to be sent at the same time?

Yes, I think that would make sense to do if and when we can get rid of
dynamically-updated sandbox flags.

> 
> I think this is fine for now (and will let us unblock the trial), but
> longer-term, I agree that it'd be nice to have a separate struct with bits
like
> this and PrivilegeData on FrameReplicationState and a cleaner way to keep it
> updated.  I'm fine if we do it as a follow-up.
> 
> Also, description still mentions scheme bypassing security checks. :)
> 
>
https://codereview.chromium.org/1723753002/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/DocumentTest.cpp (right):
> 
>
https://codereview.chromium.org/1723753002/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/DocumentTest.cpp:379:
> EXPECT_FALSE(document().securityOrigin()->isPotentiallyTrustworthy());
> It seems this part of the test still might have been useful to verify that
> putting a scheme on the bypass list won't make the origin potentially
> trustworthy, no?
> 
>
https://codereview.chromium.org/1723753002/diff/360001/content/common/frame_m...
> File content/common/frame_messages.h (right):
> 
>
https://codereview.chromium.org/1723753002/diff/360001/content/common/frame_m...
> content/common/frame_messages.h:388:
> IPC_STRUCT_TRAITS_MEMBER(is_potentially_trustworthy_unique_origin)
> nit: maybe that should be named has_potentialy_trustworthy_unique_origin? 
Since
> these are frame's properties rather than origin's, and "a frame has a
> potentially trustworthy unique origin" sounds better than "a frame is a
> potentially trustworthy unique origin".
> 
>
https://codereview.chromium.org/1723753002/diff/360001/content/common/frame_m...
> content/common/frame_messages.h:774: // set to or unset from a unique,
> potentially trustworthy origin.
> Maybe add another sentence here to explain under what conditions we might
unset
> the flag?  Presumably, this might happen when a frame loses sandbox flags and
> commits a new navigation (triggering this update message from
> NavigatorImpl::DidNavigate)?
> 
>
https://codereview.chromium.org/1723753002/diff/360001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/Document.cpp (left):
> 
>
https://codereview.chromium.org/1723753002/diff/360001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/Document.cpp:5004: if
> (securityOrigin()->hasSuborigin())
> Weird that there were two identical enforceSuborigin calls previously.  That
was
> presumably a bug?
> 
>
https://codereview.chromium.org/1723753002/diff/360001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h (right):
> 
>
https://codereview.chromium.org/1723753002/diff/360001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h:253: void
> setUniqueOriginIsPotentiallyTrustworthy(bool
> isUniqueOriginPotentiallyTrustworthy) { m_isUniqueOriginPotentiallyTrustworthy
=
> isUniqueOriginPotentiallyTrustworthy; }
> Would it make sense to also assert here that the origin is always unique when
> this is called?

[estark, 2016-03-10 00:53:44]

https://codereview.chromium.org/1723753002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentTest.cpp (right):

https://codereview.chromium.org/1723753002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentTest.cpp:379:
EXPECT_FALSE(document().securityOrigin()->isPotentiallyTrustworthy());
On 2016/03/09 18:45:06, alexmos wrote:
> It seems this part of the test still might have been useful to verify that
> putting a scheme on the bypass list won't make the origin potentially
> trustworthy, no?

Done.

https://codereview.chromium.org/1723753002/diff/360001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1723753002/diff/360001/content/common/frame_m...
content/common/frame_messages.h:388:
IPC_STRUCT_TRAITS_MEMBER(is_potentially_trustworthy_unique_origin)
On 2016/03/09 18:45:06, alexmos wrote:
> nit: maybe that should be named has_potentialy_trustworthy_unique_origin? 
Since
> these are frame's properties rather than origin's, and "a frame has a
> potentially trustworthy unique origin" sounds better than "a frame is a
> potentially trustworthy unique origin".

Done.

https://codereview.chromium.org/1723753002/diff/360001/content/common/frame_m...
content/common/frame_messages.h:774: // set to or unset from a unique,
potentially trustworthy origin.
On 2016/03/09 18:45:06, alexmos wrote:
> Maybe add another sentence here to explain under what conditions we might
unset
> the flag?  Presumably, this might happen when a frame loses sandbox flags and
> commits a new navigation (triggering this update message from
> NavigatorImpl::DidNavigate)?

Done.

https://codereview.chromium.org/1723753002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (left):

https://codereview.chromium.org/1723753002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5004: if
(securityOrigin()->hasSuborigin())
On 2016/03/09 18:45:06, alexmos wrote:
> Weird that there were two identical enforceSuborigin calls previously.  That
was
> presumably a bug?

Huh, I actually didn't notice that I deleted this; I think I must have done it
accidentally while resolving a conflict. I asked jww@ though and he thinks it
probably was indeed a mistake to have it twice. And no tests are failing so I
think it's okay to leave it out.

https://codereview.chromium.org/1723753002/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h (right):

https://codereview.chromium.org/1723753002/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h:253: void
setUniqueOriginIsPotentiallyTrustworthy(bool
isUniqueOriginPotentiallyTrustworthy) { m_isUniqueOriginPotentiallyTrustworthy =
isUniqueOriginPotentiallyTrustworthy; }
On 2016/03/09 18:45:06, alexmos wrote:
> Would it make sense to also assert here that the origin is always unique when
> this is called?

Done.

[alexmos, 2016-03-10 06:13:47]

> On 2016/03/09 18:45:06, alexmos wrote:
> > This looks good, though a couple of high-level questions about the new
> > replication IPCs:
> > 
> > - do we really need a separate renderer->browser IPC for dynamically
updating
> > the new bit, in addition to the update in DidCommitProvisionalLoad?  Normal
> > sandbox flags in <iframe>'s take effect on next navigation, so a commit-time
> > update is enough.  The one scenario I can think of is if the page inserts a
> > <meta> tag with a CSP that sets sandbox flags which make the origin unique. 
> Are
> > there other cases I'm forgetting about?  And for sandbox flags in <meta>, do
> we
> > really want to support that? :)  The spec explicitly says to discard sandbox
> > flags in that case (https://www.w3.org/TR/CSP2/#delivery-html-meta-element),
> but
> > it seems we still might support it, similarly to frame-ancestors?  I think
> Mike
> > mentioned we eventually might want to drop frame-ancestors in <meta> - what
> > about sandbox flags?
> 
> Oh, interesting... dynamically-added CSP meta was the thing that I had in mind
> when I made it dynamically-updateable, but I didn't realize that sandbox flags
> in meta tags are supposed to be discarded. But I think it would be confusing
for
> <meta>-delivered sandbox flags to work in some cases, but not in others. Maybe
> we should leave it dynamically-updateable now, and file a bug to remove
support
> for sandbox flags in <meta>? (Or at least measure to see if we can remove it
> without breaking too many things.)

Sure, filing a bug and leaving it dynamically-updateable sounds good for now.  

There might still be a problem though, because currently we only update origins
in the browser process during commit, so in this scenario, no one is going to
update the origin to unique in the FTN and in proxies.  So a proxy might end up
hitting your assert in
WebRemoteFrameImpl::setReplicatedPotentiallyTrustworthyUniqueOrigin.  Do you
mind trying out the <meta> sandbox in a page with OOPIFs to see if this is
actually the case?  If so, perhaps we need to call
FrameTreeNode::SetCurrentOrigin with a unique origin from
FrameTreeNode::SetUniqueOriginPotentiallyTrustworthy, to keep everything in
sync?  And then folding the new IPC into a flag on DidUpdateOrigin might
simplify things, since the flag update IPC won't ever be sent without the origin
update.

> (There's also this enforceSandboxFlags() callsite that I don't really
understand
> and that might or might not happen after DidCommitProvisionalLoad:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)

Interesting.  This seems to be about MHTML, and it does look like a loaded MHTML
document might be sandboxed after commit.  It seems we should be able to fix
this to sandbox before commit though.

https://codereview.chromium.org/1723753002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h (right):

https://codereview.chromium.org/1723753002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h:257: }
nit: move to .cpp file, now that it's a bit more than a straightforward
assignment.

[estark, 2016-03-14 19:48:57]

On 2016/03/10 06:13:47, alexmos wrote:
> > On 2016/03/09 18:45:06, alexmos wrote:
> > > This looks good, though a couple of high-level questions about the new
> > > replication IPCs:
> > > 
> > > - do we really need a separate renderer->browser IPC for dynamically
> updating
> > > the new bit, in addition to the update in DidCommitProvisionalLoad? 
Normal
> > > sandbox flags in <iframe>'s take effect on next navigation, so a
commit-time
> > > update is enough.  The one scenario I can think of is if the page inserts
a
> > > <meta> tag with a CSP that sets sandbox flags which make the origin
unique. 
> > Are
> > > there other cases I'm forgetting about?  And for sandbox flags in <meta>,
do
> > we
> > > really want to support that? :)  The spec explicitly says to discard
sandbox
> > > flags in that case
(https://www.w3.org/TR/CSP2/#delivery-html-meta-element),
> > but
> > > it seems we still might support it, similarly to frame-ancestors?  I think
> > Mike
> > > mentioned we eventually might want to drop frame-ancestors in <meta> -
what
> > > about sandbox flags?
> > 
> > Oh, interesting... dynamically-added CSP meta was the thing that I had in
mind
> > when I made it dynamically-updateable, but I didn't realize that sandbox
flags
> > in meta tags are supposed to be discarded. But I think it would be confusing
> for
> > <meta>-delivered sandbox flags to work in some cases, but not in others.
Maybe
> > we should leave it dynamically-updateable now, and file a bug to remove
> support
> > for sandbox flags in <meta>? (Or at least measure to see if we can remove it
> > without breaking too many things.)
> 
> Sure, filing a bug and leaving it dynamically-updateable sounds good for now. 

> 
> There might still be a problem though, because currently we only update
origins
> in the browser process during commit, so in this scenario, no one is going to
> update the origin to unique in the FTN and in proxies.  So a proxy might end
up
> hitting your assert in
> WebRemoteFrameImpl::setReplicatedPotentiallyTrustworthyUniqueOrigin.  Do you
> mind trying out the <meta> sandbox in a page with OOPIFs to see if this is
> actually the case?  If so, perhaps we need to call
> FrameTreeNode::SetCurrentOrigin with a unique origin from
> FrameTreeNode::SetUniqueOriginPotentiallyTrustworthy, to keep everything in
> sync?  And then folding the new IPC into a flag on DidUpdateOrigin might
> simplify things, since the flag update IPC won't ever be sent without the
origin
> update.

Ahh, you're right, origins weren't getting replicated when updated from a
dynamic meta tag. Sorry for the delay, but the latest patchset sends the flag
along with the DidUpdateOrigin IPC (and sends an UpdateOrigin IPC from
renderer->browser when the origin is changed due to sandbox flags). I also added
layout tests for the dynamic-meta-sandbox case.

It pains me to introduce an IPC to change the origin after a navigation commits
(which seems like a weird/wrong thing to do), but I guess I was already doing
this, effectively, with a different name, and I think I should be able to clean
it up soon so that we don't need to change the flag dynamically and thus don't
need the renderer->browser IPC (crbug.com/594645).

> 
> > (There's also this enforceSandboxFlags() callsite that I don't really
> understand
> > and that might or might not happen after DidCommitProvisionalLoad:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> 
> Interesting.  This seems to be about MHTML, and it does look like a loaded
MHTML
> document might be sandboxed after commit.  It seems we should be able to fix
> this to sandbox before commit though.
> 
>
https://codereview.chromium.org/1723753002/diff/400001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h (right):
> 
>
https://codereview.chromium.org/1723753002/diff/400001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h:257: }
> nit: move to .cpp file, now that it's a bit more than a straightforward
> assignment.

[estark, 2016-03-14 19:49:11]

https://codereview.chromium.org/1723753002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h (right):

https://codereview.chromium.org/1723753002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h:257: }
On 2016/03/10 06:13:47, alexmos wrote:
> nit: move to .cpp file, now that it's a bit more than a straightforward
> assignment.

Done.

[alexmos, 2016-03-14 22:20:35]

alexmos@chromium.org changed reviewers:
+ nasko@chromium.org

[alexmos, 2016-03-14 22:20:37]

Thanks, Emily! Adding Nasko for my question below and to start a content/ owners
review.

https://codereview.chromium.org/1723753002/diff/440001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1723753002/diff/440001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1482:
frame_tree_node()->SetCurrentOrigin(origin,
I know this is temporary until we fix issue 594645, but I'm still wary of adding
this in unrestricted form, as it basically lets a compromised renderer set any
origin it wishes.  Nasko recently added commit-time verification of the renderer
origin to ensure it's valid: https://codereview.chromium.org/1775543002/, so we
don't want to undermine that with this IPC.  That said, this IPC isn't entirely
weird, as we could need a renderer->browser origin update IPC later on for other
things, e.g. document.domain updates, PrivilegeData bits, etc. (though atm I
don't have a use case that requires it in mind).

A couple of things we could do:
- restrict this function to only work if |origin| is unique, i.e., for the only
use case today.
- call Nasko's RenderFrameHostImpl::CanCommitOrigin here to validate the origin.
 That has an early-exit for unique origins, so it should allow our use case, and
it'd be safe for potentially other use cases in the future, but I'm not sure we
want to add a broad hole for updating origins without motivating use cases.
- Go back to having a dedicated IPC for this, something like
OnDidSetUniqueOrigin that takes the trustworthiness bool.  That's safe as it's
always ok for renderer to restrict itself to a unique origin.  It'd be nice to
eventually figure out the trustworthiness in the browser process as well (we do
have the URL here, but I guess we'd need to make 
SchemeRegistry::shouldTreatURLSchemeAsSecure and
SecurityPolicy::isOriginWhiteListedTrustworthy available on the browser side).

+Nasko for opinions on this, and also to start doing an owner's review for
content/, since everything else looks good to me. :)

https://codereview.chromium.org/1723753002/diff/440001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated_sandbox.html
(right):

https://codereview.chromium.org/1723753002/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated_sandbox.html:24:
console.log(e);
nit: remove?

https://codereview.chromium.org/1723753002/diff/440001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1723753002/diff/440001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:184: // This frame has been
navigated to a new |origin|, which is a unique origin that should be considered
potentially trustworthy if |isPotentiallyTrustworthyUniqueOrigin| is true.
"has been navigated" doesn't sound correct here, as there was no navigation,
just setting the sandbox flags.

And I'm also realizing that effective sandbox flags will be wrong in the browser
process for the <meta> use case...  which might be problematic as we're starting
to use those in some browser-side security checks.  Probably fine if we fix that
separately, since this CL is already long.

[estark, 2016-03-15 01:04:41]

https://codereview.chromium.org/1723753002/diff/440001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1723753002/diff/440001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1482:
frame_tree_node()->SetCurrentOrigin(origin,
On 2016/03/14 22:20:36, alexmos wrote:
> I know this is temporary until we fix issue 594645, but I'm still wary of
adding
> this in unrestricted form, as it basically lets a compromised renderer set any
> origin it wishes.  Nasko recently added commit-time verification of the
renderer
> origin to ensure it's valid: https://codereview.chromium.org/1775543002/, so
we
> don't want to undermine that with this IPC.  That said, this IPC isn't
entirely
> weird, as we could need a renderer->browser origin update IPC later on for
other
> things, e.g. document.domain updates, PrivilegeData bits, etc. (though atm I
> don't have a use case that requires it in mind).
> 
> A couple of things we could do:
> - restrict this function to only work if |origin| is unique, i.e., for the
only
> use case today.
> - call Nasko's RenderFrameHostImpl::CanCommitOrigin here to validate the
origin.
>  That has an early-exit for unique origins, so it should allow our use case,
and
> it'd be safe for potentially other use cases in the future, but I'm not sure
we
> want to add a broad hole for updating origins without motivating use cases.
> - Go back to having a dedicated IPC for this, something like
> OnDidSetUniqueOrigin that takes the trustworthiness bool.  That's safe as it's
> always ok for renderer to restrict itself to a unique origin.  It'd be nice to
> eventually figure out the trustworthiness in the browser process as well (we
do
> have the URL here, but I guess we'd need to make 
> SchemeRegistry::shouldTreatURLSchemeAsSecure and
> SecurityPolicy::isOriginWhiteListedTrustworthy available on the browser side).
> 
> +Nasko for opinions on this, and also to start doing an owner's review for
> content/, since everything else looks good to me. :)

Oh, I wasn't aware that we were already validating the origin on navigation. I
think in that case it makes sense to make it OnUpdateToUniqueOrigin() or
something. I've gone ahead and done that, though I'm happy to change it to
something else if Nasko ends up having another opinion.

https://codereview.chromium.org/1723753002/diff/440001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated_sandbox.html
(right):

https://codereview.chromium.org/1723753002/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/secureContexts/authenticated_sandbox.html:24:
console.log(e);
On 2016/03/14 22:20:36, alexmos wrote:
> nit: remove?

Done.

https://codereview.chromium.org/1723753002/diff/440001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1723753002/diff/440001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:184: // This frame has been
navigated to a new |origin|, which is a unique origin that should be considered
potentially trustworthy if |isPotentiallyTrustworthyUniqueOrigin| is true.
On 2016/03/14 22:20:36, alexmos wrote:
> "has been navigated" doesn't sound correct here, as there was no navigation,
> just setting the sandbox flags.
> 
> And I'm also realizing that effective sandbox flags will be wrong in the
browser
> process for the <meta> use case...  which might be problematic as we're
starting
> to use those in some browser-side security checks.  Probably fine if we fix
that
> separately, since this CL is already long.

Urgh. I think we should just kill the <meta> sandbox flags ASAP. (I can work on
it as soon as I finish this CL.) Changing origin, or sandbox flags for that
matter, without navigating just seems too weird.

[alexmos, 2016-03-15 06:44:34]

LGTM.  I'm ok landing this now and ripping out the <meta> sandbox support next. 
Thanks for all the iterations on this!

https://codereview.chromium.org/1723753002/diff/460001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1723753002/diff/460001/content/common/frame_m...
content/common/frame_messages.h:959: // Sent when the frame is set to a unique
origin.
nit: consider expanding this comment a bit to explain why this IPC is needed,
that it's temporary, and give a link to the bug you filed.

https://codereview.chromium.org/1723753002/diff/460001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1723753002/diff/460001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:184: // This frame has been
updated to a unique origin, which should be considered potentially trustworthy
if |isPotentiallyTrustworthyUniqueOrigin| is true.
Might be good to mention the bug you filed here as well.

[nasko, 2016-03-16 18:28:46]

LGTM once alexmos@'s nits are addressed.

[estark, 2016-03-16 20:02:52]

Thanks for all the reviews. Especially alexmos!

https://codereview.chromium.org/1723753002/diff/460001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1723753002/diff/460001/content/common/frame_m...
content/common/frame_messages.h:959: // Sent when the frame is set to a unique
origin.
On 2016/03/15 06:44:34, alexmos wrote:
> nit: consider expanding this comment a bit to explain why this IPC is needed,
> that it's temporary, and give a link to the bug you filed.

Done.

https://codereview.chromium.org/1723753002/diff/460001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1723753002/diff/460001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:184: // This frame has been
updated to a unique origin, which should be considered potentially trustworthy
if |isPotentiallyTrustworthyUniqueOrigin| is true.
On 2016/03/15 06:44:34, alexmos wrote:
> Might be good to mention the bug you filed here as well.

Done.

[estark, 2016-03-16 20:20:59]

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

[estark, 2016-03-16 20:21:00]

csharrison: can you please review changes in media/?

[estark, 2016-03-16 20:31:17]

estark@chromium.org changed reviewers:
+ chcunningham@chromium.org
- csharrison@chromium.org

[estark, 2016-03-16 20:31:19]

Oops, I added the wrong person.

chcunningham: can you please review changes in media/?

[chcunningham, 2016-03-16 21:36:04]

Media LGTM

[estark, 2016-03-16 22:03:19]

The CQ bit was checked by estark@chromium.org

[estark, 2016-03-16 22:03:20]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
alexmos@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1723753002/#ps540001
(title: "more rebase fixups")

[commit-bot: I haz the power, 2016-03-16 22:03:53]

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

[commit-bot: I haz the power, 2016-03-16 23:30:51]

Description was changed from

==========
Make Document::isSecureContext() work for OOPIFs

The inputs needed for isSecureContext() were, previously:
a.) the origins of the current frame and its ancestors
b.) for any sandboxed origins in the ancestor chain, the URLs of those frames

(a) is easily accessible in --site-per-process, but (b) is not. For
sandboxed frames, the URL was used to construct a stand-in origin. The
sandboxed frame was considered potentially trustworthy if the stand-in
origin was potentially trustworthy.

In --site-per-process, instead of using the URL for a sandboxed origin,
we save the necessary information (the potential-trustworthiness) at the
time that the origin becomes sandboxed.

Based on https://codereview.chromium.org/1716303002/

BUG=571079
TEST=secureContexts/* layout tests with --site-per-process
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make Document::isSecureContext() work for OOPIFs

The inputs needed for isSecureContext() were, previously:
a.) the origins of the current frame and its ancestors
b.) for any sandboxed origins in the ancestor chain, the URLs of those frames

(a) is easily accessible in --site-per-process, but (b) is not. For
sandboxed frames, the URL was used to construct a stand-in origin. The
sandboxed frame was considered potentially trustworthy if the stand-in
origin was potentially trustworthy.

In --site-per-process, instead of using the URL for a sandboxed origin,
we save the necessary information (the potential-trustworthiness) at the
time that the origin becomes sandboxed.

Based on https://codereview.chromium.org/1716303002/

BUG=571079
TEST=secureContexts/* layout tests with --site-per-process
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-03-16 23:30:53]

Committed patchset #23 (id:540001)

[commit-bot: I haz the power, 2016-03-16 23:31:58]

Description was changed from

==========
Make Document::isSecureContext() work for OOPIFs

The inputs needed for isSecureContext() were, previously:
a.) the origins of the current frame and its ancestors
b.) for any sandboxed origins in the ancestor chain, the URLs of those frames

(a) is easily accessible in --site-per-process, but (b) is not. For
sandboxed frames, the URL was used to construct a stand-in origin. The
sandboxed frame was considered potentially trustworthy if the stand-in
origin was potentially trustworthy.

In --site-per-process, instead of using the URL for a sandboxed origin,
we save the necessary information (the potential-trustworthiness) at the
time that the origin becomes sandboxed.

Based on https://codereview.chromium.org/1716303002/

BUG=571079
TEST=secureContexts/* layout tests with --site-per-process
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make Document::isSecureContext() work for OOPIFs

The inputs needed for isSecureContext() were, previously:
a.) the origins of the current frame and its ancestors
b.) for any sandboxed origins in the ancestor chain, the URLs of those frames

(a) is easily accessible in --site-per-process, but (b) is not. For
sandboxed frames, the URL was used to construct a stand-in origin. The
sandboxed frame was considered potentially trustworthy if the stand-in
origin was potentially trustworthy.

In --site-per-process, instead of using the URL for a sandboxed origin,
we save the necessary information (the potential-trustworthiness) at the
time that the origin becomes sandboxed.

Based on https://codereview.chromium.org/1716303002/

BUG=571079
TEST=secureContexts/* layout tests with --site-per-process
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-03-16 23:32:00]

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