Enhance --enable-strict-site-isolation to prevent a site-isolated renderer

Enhance --enable-strict-site-isolation to prevent a site-isolated renderer
process from seeing or using cookies for other origins.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111310

[Charlie Reis, 2011-11-10 01:09:26]

Thanks Chris!  My main concern is keeping the cookie restrictions and process
restrictions separate, even if we combine them under the same flag for the time
being.

http://codereview.chromium.org/8496027/diff/3002/content/browser/child_proces...
File content/browser/child_process_security_policy.cc (right):

http://codereview.chromium.org/8496027/diff/3002/content/browser/child_proces...
content/browser/child_process_security_policy.cc:99: GURL site_gurl =
SiteInstance::GetSiteForURL(NULL, gurl);
Is NULL ok for the context?  I think that means we'll treat extension URLs (or
possibly hosted app URLs) incorrectly.

http://codereview.chromium.org/8496027/diff/3002/content/browser/child_proces...
content/browser/child_process_security_policy.cc:104: origin_lock_ =
gurl.GetOrigin();
Why use GetOrigin for one and GetSiteForURL for the other?

http://codereview.chromium.org/8496027/diff/3002/content/browser/child_proces...
File content/browser/child_process_security_policy.h (right):

http://codereview.chromium.org/8496027/diff/3002/content/browser/child_proces...
content/browser/child_process_security_policy.h:145: // the given origin.
Let's add a comment on these two that they're only currently used when
--enable-strict-site-isolation is specified, and possibly also that they're
experimental.  (I don't see us ever being able to turn them on by default, but
maybe they will be useful for things like isolated apps.)

http://codereview.chromium.org/8496027/diff/3002/content/public/common/conten...
File content/public/common/content_switches.cc (right):

http://codereview.chromium.org/8496027/diff/3002/content/public/common/conten...
content/public/common/content_switches.cc:246: // for the top-level site.
I think we're talking about two different things here.  With the changes in this
CL, this flag:
1) ensures a renderer process only has pages from a single site.
2) ensures a renderer process only has access to the cookies from that site.

If we're going to keep both of these behaviors under a single flag, we need to
at least describe them separately.

Personally, while I think it's fun to play with the cookie restriction stuff, I
don't think we'll ever be able to turn it on by default, unlike the
one-site-per-process logic.  As a result, I'm a little hesitant to bake in the
cookie isolation logic without clearly marking it as a separate experiment.

http://codereview.chromium.org/8496027/diff/3002/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8496027/diff/3002/content/renderer/render_view...
content/renderer/render_view_impl.cc:2028: if (frame_url.GetOrigin() !=
url.GetOrigin()) {
This doesn't agree with the logic the browser uses to decide when it's safe to
swap processes.  For example, two different subdomains of the same domain will
not cause a swap (but this logic will needlessly send it over to the browser),
while a hosted app that covers part of an origin should cause a swap (but this
logic would not send it over to the browser).  There's other things that catch
the hosted app case, but my point is that it's not quite the logic we want.

I think this origin comparison is only needed because POSTs don't work when we
punt them to the browser process yet (http://crbug.com/101395).  I agree that
this workaround is useful until then, so can you add a TODO to remove it when
101395 is fixed?

[Chris Evans, 2011-11-22 01:27:59]

On 2011/11/10 01:09:26, creis wrote:
> Thanks Chris!  My main concern is keeping the cookie restrictions and process
> restrictions separate, even if we combine them under the same flag for the
time
> being.
> 
>
http://codereview.chromium.org/8496027/diff/3002/content/browser/child_proces...
> File content/browser/child_process_security_policy.cc (right):
> 
>
http://codereview.chromium.org/8496027/diff/3002/content/browser/child_proces...
> content/browser/child_process_security_policy.cc:99: GURL site_gurl =
> SiteInstance::GetSiteForURL(NULL, gurl);
> Is NULL ok for the context?  I think that means we'll treat extension URLs (or
> possibly hosted app URLs) incorrectly.

I can't see that it affects anything. We should only go through this logic for
renderer processes, and if they do reference an extension resource, it shouldn't
be cookie-authenticated.

> 
>
http://codereview.chromium.org/8496027/diff/3002/content/browser/child_proces...
> content/browser/child_process_security_policy.cc:104: origin_lock_ =
> gurl.GetOrigin();
> Why use GetOrigin for one and GetSiteForURL for the other?

Done.

The GetSiteForURL() calculation has already been performed on the incoming URL.
I agree this precondition isn't clear, so I've added a couple of DCHECK()s and
rearranged to express what we expect here.

> 
>
http://codereview.chromium.org/8496027/diff/3002/content/browser/child_proces...
> File content/browser/child_process_security_policy.h (right):
> 
>
http://codereview.chromium.org/8496027/diff/3002/content/browser/child_proces...
> content/browser/child_process_security_policy.h:145: // the given origin.
> Let's add a comment on these two that they're only currently used when
> --enable-strict-site-isolation is specified, and possibly also that they're
> experimental.  (I don't see us ever being able to turn them on by default, but
> maybe they will be useful for things like isolated apps.)

Done.

> 
>
http://codereview.chromium.org/8496027/diff/3002/content/public/common/conten...
> File content/public/common/content_switches.cc (right):
> 
>
http://codereview.chromium.org/8496027/diff/3002/content/public/common/conten...
> content/public/common/content_switches.cc:246: // for the top-level site.
> I think we're talking about two different things here.  With the changes in
this
> CL, this flag:
> 1) ensures a renderer process only has pages from a single site.
> 2) ensures a renderer process only has access to the cookies from that site.
> 
> If we're going to keep both of these behaviors under a single flag, we need to
> at least describe them separately.

Ah, I was confused about the definition of "page". Duh. Fixed up, thanks.

> 
> Personally, while I think it's fun to play with the cookie restriction stuff,
I
> don't think we'll ever be able to turn it on by default, unlike the
> one-site-per-process logic.

One thing I realized, that I found interesting, is that this flag will gradually
get less broken over time. For example, once out-of-process iframes are
implemented, iGoogle's Gmail frame suddenly roars into life again. I see it as
potentially useful as a bellweather of how we're doing in this project :)

> As a result, I'm a little hesitant to bake in the
> cookie isolation logic without clearly marking it as a separate experiment.
> 
>
http://codereview.chromium.org/8496027/diff/3002/content/renderer/render_view...
> File content/renderer/render_view_impl.cc (right):
> 
>
http://codereview.chromium.org/8496027/diff/3002/content/renderer/render_view...
> content/renderer/render_view_impl.cc:2028: if (frame_url.GetOrigin() !=
> url.GetOrigin()) {
> This doesn't agree with the logic the browser uses to decide when it's safe to
> swap processes.  For example, two different subdomains of the same domain will
> not cause a swap (but this logic will needlessly send it over to the browser),
> while a hosted app that covers part of an origin should cause a swap (but this
> logic would not send it over to the browser).  There's other things that catch
> the hosted app case, but my point is that it's not quite the logic we want.
> 
> I think this origin comparison is only needed because POSTs don't work when we
> punt them to the browser process yet (http://crbug.com/101395).  I agree that
> this workaround is useful until then, so can you add a TODO to remove it when
> 101395 is fixed?

Done.

[Charlie Reis, 2011-11-22 19:06:58]

Thanks-- I'm much happier with the comments!  A few notes inline.

On 2011/11/22 01:27:59, Chris Evans wrote:
> One thing I realized, that I found interesting, is that this flag will
gradually
> get less broken over time. For example, once out-of-process iframes are
> implemented, iGoogle's Gmail frame suddenly roars into life again. I see it as
> potentially useful as a bellweather of how we're doing in this project :)

That's a good point.  In the long term, the only thing it would break is
authenticated subresources like JSON files, but it would be nice to see people
move away from that sort of thing if possible.  Anyway, I'm happy with it as an
experimental flag.

http://codereview.chromium.org/8496027/diff/9001/content/browser/child_proces...
File content/browser/child_process_security_policy.cc (right):

http://codereview.chromium.org/8496027/diff/9001/content/browser/child_proces...
content/browser/child_process_security_policy.cc:99: GURL site_gurl =
SiteInstance::GetSiteForURL(NULL, gurl);
> > Is NULL ok for the context?  I think that means we'll treat extension URLs
(or
> > possibly hosted app URLs) incorrectly.
> 
> I can't see that it affects anything. We should only go through this logic for
> renderer processes, and if they do reference an extension resource, it
shouldn't
> be cookie-authenticated.

Hosted apps and extensions do run in renderer processes.  Looking closer, I
think this actually does the right thing for hosted apps (since we don't want
GetEffectiveURL to return the hosted app's chrome-extension://... URL).  This
code might end up preventing extensions from accessing their own cookies, but
I'd have to test it to be sure.

Since this is just experimental, I'm not too concerned for the time being-- it's
ok if it has a bug there for now.  Feel free to give it a try if you're curious,
though.

http://codereview.chromium.org/8496027/diff/9001/content/browser/child_proces...
content/browser/child_process_security_policy.cc:474: DCHECK(gurl.GetOrigin() ==
gurl);
Interesting.  I think line 475 implies line 474, but not vice versa.  In other
words, anything you get back from GetSiteForURL is indeed an origin, but you can
get back something from GetOrigin that isn't a site (e.g., something with a
subdomain or port).

It works, but it's tricky to read.  Maybe flip these two lines for clarity?

http://codereview.chromium.org/8496027/diff/9001/content/browser/renderer_hos...
File content/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/8496027/diff/9001/content/browser/renderer_hos...
content/browser/renderer_host/render_message_filter.cc:464: return;
Wow, lacking a return statement there was a real bug, wasn't it?

http://codereview.chromium.org/8496027/diff/9001/content/browser/site_instanc...
File content/browser/site_instance.cc (right):

http://codereview.chromium.org/8496027/diff/9001/content/browser/site_instanc...
content/browser/site_instance.cc:96: policy->LockToOrigin(process_->id(),
site_);
Hmm, a SiteInstance's site_ isn't always set by the time GetProcess() is called
(which helps deal with redirects).  For example, if you open a bookmark in a new
tab, GetProcess gets called before SetSite.  If you open a bookmark in the
current tab, SetSite gets called before GetProcess.

I think we might have to move this to a helper and call it from both GetProcess
and SetSite, whichever comes first (e.g., checking if has_site here, and if
process_ below).

http://codereview.chromium.org/8496027/diff/9001/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8496027/diff/9001/content/renderer/render_view...
content/renderer/render_view_impl.cc:2028: // TODO(cevans): revisit whether this
workaround is still necessary once
nit: workaround -> origin check

[Chris Evans, 2011-11-22 23:54:46]

On 2011/11/22 19:06:58, creis wrote:
> Thanks-- I'm much happier with the comments!  A few notes inline.
> 
> On 2011/11/22 01:27:59, Chris Evans wrote:
> > One thing I realized, that I found interesting, is that this flag will
> gradually
> > get less broken over time. For example, once out-of-process iframes are
> > implemented, iGoogle's Gmail frame suddenly roars into life again. I see it
as
> > potentially useful as a bellweather of how we're doing in this project :)
> 
> That's a good point.  In the long term, the only thing it would break is
> authenticated subresources like JSON files, but it would be nice to see people
> move away from that sort of thing if possible.  Anyway, I'm happy with it as
an
> experimental flag.
> 
>
http://codereview.chromium.org/8496027/diff/9001/content/browser/child_proces...
> File content/browser/child_process_security_policy.cc (right):
> 
>
http://codereview.chromium.org/8496027/diff/9001/content/browser/child_proces...
> content/browser/child_process_security_policy.cc:99: GURL site_gurl =
> SiteInstance::GetSiteForURL(NULL, gurl);
> > > Is NULL ok for the context?  I think that means we'll treat extension URLs
> (or
> > > possibly hosted app URLs) incorrectly.
> > 
> > I can't see that it affects anything. We should only go through this logic
for
> > renderer processes, and if they do reference an extension resource, it
> shouldn't
> > be cookie-authenticated.
> 
> Hosted apps and extensions do run in renderer processes.  Looking closer, I
> think this actually does the right thing for hosted apps (since we don't want
> GetEffectiveURL to return the hosted app's chrome-extension://... URL).  This
> code might end up preventing extensions from accessing their own cookies, but
> I'd have to test it to be sure.
> 
> Since this is just experimental, I'm not too concerned for the time being--
it's
> ok if it has a bug there for now.  Feel free to give it a try if you're
curious,
> though.
> 
>
http://codereview.chromium.org/8496027/diff/9001/content/browser/child_proces...
> content/browser/child_process_security_policy.cc:474: DCHECK(gurl.GetOrigin()
==
> gurl);
> Interesting.  I think line 475 implies line 474, but not vice versa.  In other
> words, anything you get back from GetSiteForURL is indeed an origin, but you
can
> get back something from GetOrigin that isn't a site (e.g., something with a
> subdomain or port).
> 
> It works, but it's tricky to read.  Maybe flip these two lines for clarity?

Since 475 is a superset, I kept that and removed the other one.

Done.

>
http://codereview.chromium.org/8496027/diff/9001/content/browser/renderer_hos...
> File content/browser/renderer_host/render_message_filter.cc (right):
> 
>
http://codereview.chromium.org/8496027/diff/9001/content/browser/renderer_hos...
> content/browser/renderer_host/render_message_filter.cc:464: return;
> Wow, lacking a return statement there was a real bug, wasn't it?

Definitely. Probably would allow a compromised renderer to read HttpOnly
cookies. Not actually a huge deal but worth fixing.

> 
>
http://codereview.chromium.org/8496027/diff/9001/content/browser/site_instanc...
> File content/browser/site_instance.cc (right):
> 
>
http://codereview.chromium.org/8496027/diff/9001/content/browser/site_instanc...
> content/browser/site_instance.cc:96: policy->LockToOrigin(process_->id(),
> site_);
> Hmm, a SiteInstance's site_ isn't always set by the time GetProcess() is
called
> (which helps deal with redirects).  For example, if you open a bookmark in a
new
> tab, GetProcess gets called before SetSite.  If you open a bookmark in the
> current tab, SetSite gets called before GetProcess.
> 
> I think we might have to move this to a helper and call it from both
GetProcess
> and SetSite, whichever comes first (e.g., checking if has_site here, and if
> process_ below).

Confirmed with a DCHECK(). Launching dev tools repros this most reliably.

Done.

> 
>
http://codereview.chromium.org/8496027/diff/9001/content/renderer/render_view...
> File content/renderer/render_view_impl.cc (right):
> 
>
http://codereview.chromium.org/8496027/diff/9001/content/renderer/render_view...
> content/renderer/render_view_impl.cc:2028: // TODO(cevans): revisit whether
this
> workaround is still necessary once
> nit: workaround -> origin check

Done.

[Charlie Reis, 2011-11-23 00:22:43]

LGTM.  You'll probably need owner's approval as well.

[cevans, 2011-11-23 00:33:32]

Looks like I need jam@ due to the command line switch comment change in
content/public.

On Tue, Nov 22, 2011 at 4:22 PM, <creis@chromium.org> wrote:

> LGTM.  You'll probably need owner's approval as well.
>
>
http://codereview.chromium.**org/8496027/<http://codereview.chromium.org/8496...
>

[jam, 2011-11-23 01:38:10]

lgtm