Block renderer-initiated navigation to other chrome URLs on signin page

Block renderer-initiated navigation to other chrome URLs on signin page.

BUG=366127
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266721

[guohui, 2014-04-23 19:43:08]

Hey,

could you please take a look at the CL? Will add unit test after the main code
is approved.

Thanks,

Hui

[Charlie Reis, 2014-04-23 20:54:03]

Yes, that check looks like a good starting point.

What happens if you force RenderFrameImpl::DecidePolicyForNavigation to return
default_policy and then click a link (or use a script to navigate) to
chrome://settings?  That should simulate an exploited renderer which decides not
to use OpenURL.  Hopefully we'll catch it already in something like FilterURL,
but if not we can think about how.

And thanks, I agree we'll need a test.

[guohui, 2014-04-24 21:40:54]

On 2014/04/23 20:54:03, Charlie Reis wrote:
> Yes, that check looks like a good starting point.
> 
> What happens if you force RenderFrameImpl::DecidePolicyForNavigation to return
> default_policy and then click a link (or use a script to navigate) to
> chrome://settings?  That should simulate an exploited renderer which decides
not
> to use OpenURL.  Hopefully we'll catch it already in something like FilterURL,
> but if not we can think about how.
Tested on a local build with DecidePolicyForNavigation always returning
default_policy, i am able to trigger navigation to chrome://settings, but the
settings page is not loaded and instead shows the standard error "this webpage
is not available".

Is this good enough?

> 
> And thanks, I agree we'll need a test.

[Charlie Reis, 2014-04-24 21:53:35]

On 2014/04/24 21:40:54, guohui wrote:
> On 2014/04/23 20:54:03, Charlie Reis wrote:
> > Yes, that check looks like a good starting point.
> > 
> > What happens if you force RenderFrameImpl::DecidePolicyForNavigation to
return
> > default_policy and then click a link (or use a script to navigate) to
> > chrome://settings?  That should simulate an exploited renderer which decides
> not
> > to use OpenURL.  Hopefully we'll catch it already in something like
FilterURL,
> > but if not we can think about how.
> Tested on a local build with DecidePolicyForNavigation always returning
> default_policy, i am able to trigger navigation to chrome://settings, but the
> settings page is not loaded and instead shows the standard error "this webpage
> is not available".
> 
> Is this good enough?

Interesting.  I'm curious what's preventing us from loading other chrome:// URLs
in that process.  Do you see any clues?

Anyway, the AllowOpenURL check should be sufficient to prevent any process swap
to another chrome:// URL, which is the main goal.


> 
> > 
> > And thanks, I agree we'll need a test.

[guohui, 2014-04-24 22:27:23]

On 2014/04/24 21:53:35, Charlie Reis wrote:
> On 2014/04/24 21:40:54, guohui wrote:
> > On 2014/04/23 20:54:03, Charlie Reis wrote:
> > > Yes, that check looks like a good starting point.
> > > 
> > > What happens if you force RenderFrameImpl::DecidePolicyForNavigation to
> return
> > > default_policy and then click a link (or use a script to navigate) to
> > > chrome://settings?  That should simulate an exploited renderer which
decides
> > not
> > > to use OpenURL.  Hopefully we'll catch it already in something like
> FilterURL,
> > > but if not we can think about how.
> > Tested on a local build with DecidePolicyForNavigation always returning
> > default_policy, i am able to trigger navigation to chrome://settings, but
the
> > settings page is not loaded and instead shows the standard error "this
webpage
> > is not available".
> > 
> > Is this good enough?
> 
> Interesting.  I'm curious what's preventing us from loading other chrome://
URLs
> in that process.  Do you see any clues?

URLDataManagerBackend::GetDataSourceFromURL returns NULL for the URL
chrome://settings from the chrome-signin process, thus
URLRequestChromeJob::StartAsyn failed with net::ERR_INVALID_URL. I guess it
worked without my fix because of process swap.

> 
> Anyway, the AllowOpenURL check should be sufficient to prevent any process
swap
> to another chrome:// URL, which is the main goal.
> 
> 
> > 
> > > 
> > > And thanks, I agree we'll need a test.

[guohui, 2014-04-25 14:30:54]

@charlie, are you ok with the main code? if yes i ll go ahead with adding unit
test.

[Charlie Reis, 2014-04-25 17:31:43]

On 2014/04/25 14:30:54, guohui wrote:
> @charlie, are you ok with the main code? if yes i ll go ahead with adding unit
> test.

Yes, it sounds like the sign-in process doesn't have the ability to load other
chrome pages itself, and we can block process swaps to them using AllowOpenURL. 
Thanks for investigating it.

In the future, you don't need to wait to write the test until the main code is
fully approved.  In many cases (this included), the test should be somewhat
agnostic of how the fix works (e.g., trying to navigate to a chrome:// URL in
the sign-in process).

[guohui, 2014-04-25 19:54:30]

Added test, please take another look, thanks!

[Charlie Reis, 2014-04-25 20:45:47]

Thanks!  LGTM.

[guohui, 2014-04-28 17:02:51]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-04-28 17:03:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/246533006/70001

[guohui, 2014-04-28 18:01:05]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-04-28 18:02:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/246533006/90001

[guohui, 2014-04-28 18:34:12]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-04-28 18:35:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/246533006/110001

[commit-bot: I haz the power, 2014-04-28 18:51:07]

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

[commit-bot: I haz the power, 2014-04-28 18:51:07]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[guohui, 2014-04-28 18:51:40]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-04-28 18:53:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/246533006/110001

[commit-bot: I haz the power, 2014-04-28 19:00:23]

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

[commit-bot: I haz the power, 2014-04-28 19:00:23]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[guohui, 2014-04-28 22:48:46]

The CQ bit was checked by guohui@chromium.org

[commit-bot: I haz the power, 2014-04-28 22:49:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/guohui@chromium.org/246533006/110001

[commit-bot: I haz the power, 2014-04-29 00:53:56]

Change committed as 266721