Restricting redirects to chrome:

Restrict child processes from redirecting to chrome: URLs.

BUG=95374
TEST=content_unittests

[abarth-chromium, 2011-09-12 19:51:21]

http://codereview.chromium.org/7873007/diff/1/net/url_request/url_request_htt...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/7873007/diff/1/net/url_request/url_request_htt...
net/url_request/url_request_http_job.cc:1011: if (location.SchemeIs("chrome"))
The net module shouldn't really know anything about the "chrome" scheme.

http://codereview.chromium.org/7873007/diff/1/net/url_request/url_request_htt...
net/url_request/url_request_http_job.cc:1014: if
(!URLRequest::IsHandledURL(location))
Do we think that "chrome" is a handled URL?

http://codereview.chromium.org/7873007/diff/1/net/url_request/url_request_htt...
net/url_request/url_request_http_job.cc:1028: return false;
If so, it seems like we should be returning false here.

[kenrb, 2011-09-12 20:05:05]

http://codereview.chromium.org/7873007/diff/1/net/url_request/url_request_htt...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/7873007/diff/1/net/url_request/url_request_htt...
net/url_request/url_request_http_job.cc:1011: if (location.SchemeIs("chrome"))
On 2011/09/12 19:51:21, abarth wrote:
> The net module shouldn't really know anything about the "chrome" scheme.

I know. This is a hack at this point, but the alternatives seem messy. I have
some questions in the bug, including whether it might be worth trying to change
WebKit's handling of redirects.

http://codereview.chromium.org/7873007/diff/1/net/url_request/url_request_htt...
net/url_request/url_request_http_job.cc:1014: if
(!URLRequest::IsHandledURL(location))
On 2011/09/12 19:51:21, abarth wrote:
> Do we think that "chrome" is a handled URL?

Note the ! in the condition. It is not a handled URL, so this function returns
true. The actual behavior of this function is to blacklist about:, javascript:,
and view-source:.

[rvargas (doing something else), 2011-09-12 21:07:14]

http://codereview.chromium.org/7873007/diff/1/net/url_request/url_request_htt...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/7873007/diff/1/net/url_request/url_request_htt...
net/url_request/url_request_http_job.cc:1014: if
(!URLRequest::IsHandledURL(location))
On 2011/09/12 20:05:05, kenrb wrote:
> On 2011/09/12 19:51:21, abarth wrote:
> > Do we think that "chrome" is a handled URL?
> 
> Note the ! in the condition. It is not a handled URL, so this function returns
> true. The actual behavior of this function is to blacklist about:,
javascript:,
> and view-source:.

I don't think hard coding "chrome:" is a good idea. However, you mentioned
view-source: and javascript:, and they are not part of kBuiltinFactories, so if
IsHandledURL returns true for them, there must be a registered factory (I don't
see that). Maybe we can do something along that line.

[kenrb, 2011-09-13 00:27:52]

http://codereview.chromium.org/7873007/diff/1/net/url_request/url_request_htt...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/7873007/diff/1/net/url_request/url_request_htt...
net/url_request/url_request_http_job.cc:1014: if
(!URLRequest::IsHandledURL(location))
On 2011/09/12 21:07:14, rvargas wrote:
> On 2011/09/12 20:05:05, kenrb wrote:
> > On 2011/09/12 19:51:21, abarth wrote:
> > > Do we think that "chrome" is a handled URL?
> > 
> > Note the ! in the condition. It is not a handled URL, so this function
returns
> > true. The actual behavior of this function is to blacklist about:,
> javascript:,
> > and view-source:.
> 
> I don't think hard coding "chrome:" is a good idea. However, you mentioned
> view-source: and javascript:, and they are not part of kBuiltinFactories, so
if
> IsHandledURL returns true for them, there must be a registered factory (I
don't
> see that). Maybe we can do something along that line.

Sorry, my mistake. I was confusing this with a list elsewhere. It's about:,
file:, and data: that are blocked as redirect targets by this function.

[jam, 2011-09-15 20:47:07]

I'm not that familiar with this code, so I defer to other owners who are.

[Avi (use Gerrit), 2011-09-15 20:51:21]

On 2011/09/15 20:47:07, John Abd-El-Malek wrote:
> I'm not that familiar with this code, so I defer to other owners who are.

Ditto. I'm ready to rubber-stamp approve if needed, though.

[kenrb, 2011-09-15 21:09:09]

On 2011/09/15 20:51:21, Avi wrote:
> On 2011/09/15 20:47:07, John Abd-El-Malek wrote:
> > I'm not that familiar with this code, so I defer to other owners who are.
> 
> Ditto. I'm ready to rubber-stamp approve if needed, though.

Unfortunate. Out of 6 owners I pull the 2 who most recent appeared in git log
for that file... I'm looking for a real review so do you know who would be
better to have a look at that?

[kenrb, 2011-09-17 02:29:08]

I've added brettw, sky, darin and ben to the cc: line, who are all OWNERS for
this. Is someone who is familiar with ChildProcessSecurityPolicy able to review
this change?

[darin (slow to review), 2011-09-19 06:16:56]

+abarth

On Fri, Sep 16, 2011 at 7:29 PM, <kenrb@chromium.org> wrote:

> I've added brettw, sky, darin and ben to the cc: line, who are all OWNERS
> for
> this. Is someone who is familiar with ChildProcessSecurityPolicy able to
> review
> this change?
>
>
>
>
>
http://codereview.chromium.**org/7873007/<http://codereview.chromium.org/7873...
>

[abarth-chromium, 2011-09-19 06:46:39]

It's hard to understand what you're trying to achieve with this patch because
the description says what you're doing but not why you're doing it.  I've left
some specific comments below, but I suspect you might need to use a different
approach to solve whatever problem you're having.

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

http://codereview.chromium.org/7873007/diff/8001/content/browser/child_proces...
content/browser/child_process_security_policy.cc:141:
RegisterWebUIScheme(chrome::kChromeUIScheme);
In the past, access to this scheme happened naturally because we navigated
render processes to chrome: URLs when we added the WebUI bindings.  Does this
mean we're granting WebUI bindings to URLs that lack the chrome: scheme?

http://codereview.chromium.org/7873007/diff/8001/content/browser/child_proces...
content/browser/child_process_security_policy.cc:417: return
CanRequestURL(child_id, url) && !IsWebUIScheme(url.scheme());
If you can request a URL, why can't you redirect to it?  I suspect you're trying
to solve this problem at the wrong layer.

The ChildProcessSecurityPolicy is only meant to be a line of defense against a
compromised rendering engine.  If a render process CanRequestURL, then that
process can navigate itself to that URL and there's no security to be gained by
blocking redirects.

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

http://codereview.chromium.org/7873007/diff/8001/content/browser/child_proces...
content/browser/child_process_security_policy.h:136: bool CanRedirectURL(int
child_id, const GURL& url);
I'm not sure I understand why CanRedirectURL is any different than
CanRequestURL.

[abarth-chromium, 2011-09-19 06:53:04]

From reading the bug, it sounds like you need to expose
WebCore::SecurityOrigin::canDisplay in WebKit::WebSecurityOrigin and call that
function on the appropriate security origin.  In this case, that's probably the
origin you construct from the URL that gave you the redirect.

Trying to handle this case in ChildProcessSecurityPolicy is incorrect.

[kenrb, 2011-09-19 16:02:30]

On 2011/09/19 06:53:04, abarth wrote:
> From reading the bug, it sounds like you need to expose
> WebCore::SecurityOrigin::canDisplay in WebKit::WebSecurityOrigin and call that
> function on the appropriate security origin.  In this case, that's probably
the
> origin you construct from the URL that gave you the redirect.
> 
> Trying to handle this case in ChildProcessSecurityPolicy is incorrect.

Thanks for the review and suggestions, Adam. I was asking about this possibility
in c#6 of the bug (because canDisplay is where the scheme filtering is done
during meta refresh and document.location redirects), but it's not clear to me
how this would be done. Where would the call to
WebCore::SecurityOrigin::canDisplay go? Trying to check that from the browser
would need a new IPC message, and renderer code path that is used during HTTP
redirects is very generic.

[abarth-chromium, 2011-09-19 17:25:20]

In most situations, we route redirects through the renderer, for example to
check for mixed content and other security-related properties.  Maybe the best
approach is to block the request at that time.  Is this redirect not flowing
through the renderer somehow?  Does that mean we're not detecting mixed content
and CORS restrictions properly?

[kenrb, 2011-09-19 18:10:11]

Yes, it does, that's what I meant by the generic code path in the renderer. That
check is done in WebCore::MainResourceLoader::willSendRequest(). I experimented
with changing that function last week when I asked if this would be a good idea.
I wasn't sure about this because changing redirect behavior for other browsers
may or may not be desirable. I can go ahead with this if you think there won't
be an issue getting it into WebKit.

[abarth-chromium, 2011-09-19 18:15:31]

Can this same approach be used to redirect to a file URL?  If so, we
can check whether the same bug occurs in other WebKit-based browsers,
like Safari.  If it does, that's a good argument for making the change
in WebKit.

It seems like WebKit needs to do a canDisplay check before displaying
a web page in the main frame.  If we're missing that, we should add
it.  If we have one, but it's not working in this case, it would be
helpful to understand why it's not working.

Adam


On Mon, Sep 19, 2011 at 11:10 AM,  <kenrb@chromium.org> wrote:
> Yes, it does, that's what I meant by the generic code path in the renderer.
> That
> check is done in WebCore::MainResourceLoader::willSendRequest(). I
> experimented
> with changing that function last week when I asked if this would be a good
> idea.
> I wasn't sure about this because changing redirect behavior for other
> browsers
> may or may not be desirable. I can go ahead with this if you think there
> won't
> be an issue getting it into WebKit.
>
>
> http://codereview.chromium.org/7873007/
>

[kenrb, 2011-09-19 18:48:24]

file: redirects are not blocked at that layer. They are blocked in two places
within Chromium -- in ChildProcessSecurityPolicy, and in URLRequestHttpJob at
the net layer (after the renderer has processed the redirect).

In WebKit, WebCore::MainResourceLoader::willSendRequest() calls
SecurityOrigin::canRequest() but not canDisplay(). canRequest() is used to
determine if it is a cross-origin redirect, but it doesn't block the request
based on that because, obviously, cross-origin redirects are normal. I can write
a WebKit patch for this and see if it breaks anything.

[abarth-chromium, 2011-09-19 19:01:16]

On Mon, Sep 19, 2011 at 11:48 AM,  <kenrb@chromium.org> wrote:
> file: redirects are not blocked at that layer. They are blocked in two
> places
> within Chromium -- in ChildProcessSecurityPolicy, and in URLRequestHttpJob
> at
> the net layer (after the renderer has processed the redirect).

How does that work in Safari, for example, which doesn't link in
either of those two objects?

Adam


> In WebKit, WebCore::MainResourceLoader::willSendRequest() calls
> SecurityOrigin::canRequest() but not canDisplay(). canRequest() is used to
> determine if it is a cross-origin redirect, but it doesn't block the request
> based on that because, obviously, cross-origin redirects are normal. I can
> write a WebKit patch for this and see if it breaks anything.
>
>
> http://codereview.chromium.org/7873007/
>

[kenrb, 2011-09-19 19:07:18]

On 2011/09/19 19:01:16, abarth wrote:
> On Mon, Sep 19, 2011 at 11:48 AM,  <mailto:kenrb@chromium.org> wrote:
> > file: redirects are not blocked at that layer. They are blocked in two
> > places
> > within Chromium -- in ChildProcessSecurityPolicy, and in URLRequestHttpJob
> > at
> > the net layer (after the renderer has processed the redirect).
> 
> How does that work in Safari, for example, which doesn't link in
> either of those two objects?
> 

Lacking both a Mac and access to Safari source code, I find it difficult to
answer that question.