Block top-level navigations to nested URLs with extension origins from non-extension processes.

Block top-level navigations to nested URLs with extension origins from non-extension processes.

Before this CL, it was possible for a web iframe with an unblessed
extension frame to exploit the renderer, create a blob: or filesystem:
URL in the extension frame context, then create a new top-level window
and navigate it to that URL, which could end up putting the new window
into a privileged extension process running attacker's code.

BUG=645028
Committed: https://crrev.com/4bfdc9292a6161980ba9a7a469d2d4515bebc6dd
Cr-Commit-Position: refs/heads/master@{#419019}

[alexmos, 2016-09-15 07:33:36]

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

[commit-bot: I haz the power, 2016-09-15 07:33:45]

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

[commit-bot: I haz the power, 2016-09-15 08:44:22]

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

[commit-bot: I haz the power, 2016-09-15 08:44:22]

Dry run: This issue passed the CQ dry run.

[alexmos, 2016-09-15 17:40:02]

Description was changed from

==========
Block top-level navigations to nested URLs with extension origins from
non-extension processes.

BUG=645028
==========

to

==========
Block top-level navigations to nested URLs with extension origins from
non-extension processes.

Before this CL, it was possible for a web iframe with an unblessed
extension frame to exploit the renderer, create a blob: or filesystem:
URL in the extension frame context, then create a new top-level window
and navigate it to that URL, which could end up putting the new window
into a privileged extension process running attacker's code.

BUG=645028
==========

[alexmos, 2016-09-15 17:43:01]

alexmos@chromium.org changed reviewers:
+ nasko@chromium.org, rdevlin.cronin@chromium.org

[alexmos, 2016-09-15 17:43:02]

Nasko, Devlin: please take a look.  This is the fix we discussed.

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:780:
OpenPopup(main_frame, GURL(url::kAboutBlankURL));
Unfortunately, I couldn't just pass the nested URL into window.open due to
https://crbug.com/644966#c4.

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/net/chro...
File chrome/browser/net/chrome_extensions_network_delegate.cc (right):

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/net/chro...
chrome/browser/net/chrome_extensions_network_delegate.cc:175: return
net::ERR_ABORTED;
I considered returning ERR_BLOCKED_BY_CLIENT here, but ERR_ABORTED seems to
behave a bit safer, as it commits the blocked page as an about:blank page with
the initiator's origin, whereas ERR_BLOCKED_BY_CLIENT shows a sad tab error page
and commits the blocked URL (i.e., blob:...) with a null origin (due to the
chromewebstore data URL used to show the error page), and having the blocked URL
as the last committed URL seems dangerous per our recent discussions with
Charlie and Nasko.

[alexmos, 2016-09-15 17:43:11]

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

[commit-bot: I haz the power, 2016-09-15 17:43:43]

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

[nasko, 2016-09-15 18:36:01]

LGTM

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:672:
EXPECT_EQ(IfExtensionsIsolated(1, 0), pm->GetAllFrames().size());
What about testing for not --isolate-extensions case?

[alexmos, 2016-09-15 18:40:12]

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:672:
EXPECT_EQ(IfExtensionsIsolated(1, 0), pm->GetAllFrames().size());
On 2016/09/15 18:36:01, nasko (slow) wrote:
> What about testing for not --isolate-extensions case?

Oh yeah, I should've mentioned this.  Up until uploading this, I tested both
tests locally both with and without --isolate-extensions (by reverting
IsIsolateExtensionsEnabled() to check the flag).  I was going to rely on the M53
stable bots to check this when I merge the fix back to M53, but do you think I
should find a way to turn off --isolate-extensions in the test itself and add
that coverage?

[nasko, 2016-09-15 18:50:03]

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:672:
EXPECT_EQ(IfExtensionsIsolated(1, 0), pm->GetAllFrames().size());
On 2016/09/15 18:40:12, alexmos wrote:
> On 2016/09/15 18:36:01, nasko (slow) wrote:
> > What about testing for not --isolate-extensions case?
> 
> Oh yeah, I should've mentioned this.  Up until uploading this, I tested both
> tests locally both with and without --isolate-extensions (by reverting
> IsIsolateExtensionsEnabled() to check the flag).  I was going to rely on the
M53
> stable bots to check this when I merge the fix back to M53, but do you think I
> should find a way to turn off --isolate-extensions in the test itself and add
> that coverage?

I don't think you need to turn of --isolate-extensions at all. In M55 it will
only test the --isolate-extensions case, but having the checks will allow M53 to
test the default case of --isolate-extensions being off. Once backported, we can
update the test to not even use IfExtensionsIsolated, since we know that is
always true.

[commit-bot: I haz the power, 2016-09-15 19:00:48]

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

[commit-bot: I haz the power, 2016-09-15 19:00:49]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[alexmos, 2016-09-15 20:51:42]

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:672:
EXPECT_EQ(IfExtensionsIsolated(1, 0), pm->GetAllFrames().size());
On 2016/09/15 18:50:03, nasko (slow) wrote:
> On 2016/09/15 18:40:12, alexmos wrote:
> > On 2016/09/15 18:36:01, nasko (slow) wrote:
> > > What about testing for not --isolate-extensions case?
> > 
> > Oh yeah, I should've mentioned this.  Up until uploading this, I tested both
> > tests locally both with and without --isolate-extensions (by reverting
> > IsIsolateExtensionsEnabled() to check the flag).  I was going to rely on the
> M53
> > stable bots to check this when I merge the fix back to M53, but do you think
I
> > should find a way to turn off --isolate-extensions in the test itself and
add
> > that coverage?
> 
> I don't think you need to turn of --isolate-extensions at all. In M55 it will
> only test the --isolate-extensions case, but having the checks will allow M53
to
> test the default case of --isolate-extensions being off. Once backported, we
can
> update the test to not even use IfExtensionsIsolated, since we know that is
> always true.

Agreed, let's plan on doing that.

[Devlin, 2016-09-15 21:05:10]

lgtm!

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:50: "var blob = new
Blob(['<html><body>" + content + "</body></html>'],"
nit: \n at the end (everywhere like this)

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/net/chro...
File chrome/browser/net/chrome_extensions_network_delegate.cc (right):

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/net/chro...
chrome/browser/net/chrome_extensions_network_delegate.cc:173:
url::Origin(url).scheme() == extensions::kExtensionScheme) {
nit: Is there a reason to prefer this over GURL::SchemeIs?

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/net/chro...
chrome/browser/net/chrome_extensions_network_delegate.cc:174: if
(!extension_info_map_->process_map().Contains(info->GetChildID()))
optional nit: I'd be fine using and && here with the above if, but up to you.

[alexmos, 2016-09-15 21:44:04]

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

[commit-bot: I haz the power, 2016-09-15 21:44:40]

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

[alexmos, 2016-09-15 21:45:59]

Thanks for reviewing!

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:50: "var blob = new
Blob(['<html><body>" + content + "</body></html>'],"
On 2016/09/15 21:05:10, Devlin wrote:
> nit: \n at the end (everywhere like this)

Done.

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/net/chro...
File chrome/browser/net/chrome_extensions_network_delegate.cc (right):

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/net/chro...
chrome/browser/net/chrome_extensions_network_delegate.cc:173:
url::Origin(url).scheme() == extensions::kExtensionScheme) {
On 2016/09/15 21:05:10, Devlin wrote:
> nit: Is there a reason to prefer this over GURL::SchemeIs?

Yes, this uses url::Origin(url).scheme() because we want to look at the inner
URL's scheme (i.e., "chrome-extension").  Sadly, GURL::SchemeIs(url) will just
return "blob" or "filesystem", and actually yesterday some of us here were
discussing how feasible it is to change that to return the inner scheme instead.
 Further, GURL::GetOrigin() doesn't work correctly for blob URLs (but works fine
for filesystem: URLs).  url::Origin is the only place where both blob and
filesystem are handled sanely.  It's a mess. :(

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/net/chro...
chrome/browser/net/chrome_extensions_network_delegate.cc:174: if
(!extension_info_map_->process_map().Contains(info->GetChildID()))
On 2016/09/15 21:05:10, Devlin wrote:
> optional nit: I'd be fine using and && here with the above if, but up to you.

Done.

[alexmos, 2016-09-15 21:51:40]

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/net/chro...
File chrome/browser/net/chrome_extensions_network_delegate.cc (right):

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/net/chro...
chrome/browser/net/chrome_extensions_network_delegate.cc:173:
url::Origin(url).scheme() == extensions::kExtensionScheme) {
On 2016/09/15 21:45:59, alexmos wrote:
> On 2016/09/15 21:05:10, Devlin wrote:
> > nit: Is there a reason to prefer this over GURL::SchemeIs?
> 
> Yes, this uses url::Origin(url).scheme() because we want to look at the inner
> URL's scheme (i.e., "chrome-extension").  Sadly, GURL::SchemeIs(url) will just
> return "blob" or "filesystem", and actually yesterday some of us here were
> discussing how feasible it is to change that to return the inner scheme
instead.
>  Further, GURL::GetOrigin() doesn't work correctly for blob URLs (but works
fine
> for filesystem: URLs).  url::Origin is the only place where both blob and
> filesystem are handled sanely.  It's a mess. :(

Also CC-ing mkwst@ for awareness and any thoughts on how we could improve
blob/filesystem handling in GURLs.

[Devlin, 2016-09-15 21:56:48]

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/net/chro...
File chrome/browser/net/chrome_extensions_network_delegate.cc (right):

https://codereview.chromium.org/2345473003/diff/40001/chrome/browser/net/chro...
chrome/browser/net/chrome_extensions_network_delegate.cc:173:
url::Origin(url).scheme() == extensions::kExtensionScheme) {
On 2016/09/15 21:45:59, alexmos wrote:
> On 2016/09/15 21:05:10, Devlin wrote:
> > nit: Is there a reason to prefer this over GURL::SchemeIs?
> 
> Yes, this uses url::Origin(url).scheme() because we want to look at the inner
> URL's scheme (i.e., "chrome-extension").  Sadly, GURL::SchemeIs(url) will just
> return "blob" or "filesystem", and actually yesterday some of us here were
> discussing how feasible it is to change that to return the inner scheme
instead.
>  Further, GURL::GetOrigin() doesn't work correctly for blob URLs (but works
fine
> for filesystem: URLs).  url::Origin is the only place where both blob and
> filesystem are handled sanely.  It's a mess. :(

...yuck.  Regardless, fine for this CL, but...wow.

[alexmos, 2016-09-15 22:04:39]

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

[alexmos, 2016-09-15 22:04:40]

sky@: can you please review chrome_extensions_network_delegate.cc for OWNERS?

[alexmos, 2016-09-15 22:05:26]

alexmos@chromium.org changed reviewers:
+ agl@chromium.org
- sky@chromium.org

[alexmos, 2016-09-15 22:05:27]

Actually, agl@ will review it for OWNERS.

[agl, 2016-09-15 22:11:15]

RS LGTM

[alexmos, 2016-09-15 22:13:05]

The CQ bit was unchecked by alexmos@chromium.org

[alexmos, 2016-09-15 22:13:06]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2016-09-15 22:13:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2345473003/#ps60001
(title: "Devlin's comments")

[commit-bot: I haz the power, 2016-09-15 22:13:17]

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-09-15 22:41:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-15 22:45:08]

Description was changed from

==========
Block top-level navigations to nested URLs with extension origins from
non-extension processes.

Before this CL, it was possible for a web iframe with an unblessed
extension frame to exploit the renderer, create a blob: or filesystem:
URL in the extension frame context, then create a new top-level window
and navigate it to that URL, which could end up putting the new window
into a privileged extension process running attacker's code.

BUG=645028
==========

to

==========
Block top-level navigations to nested URLs with extension origins from
non-extension processes.

Before this CL, it was possible for a web iframe with an unblessed
extension frame to exploit the renderer, create a blob: or filesystem:
URL in the extension frame context, then create a new top-level window
and navigate it to that URL, which could end up putting the new window
into a privileged extension process running attacker's code.

BUG=645028

Committed: https://crrev.com/4bfdc9292a6161980ba9a7a469d2d4515bebc6dd
Cr-Commit-Position: refs/heads/master@{#419019}
==========

[commit-bot: I haz the power, 2016-09-15 22:45:09]

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