Protect sensistive chrome: and chrome-extension: schemes as not being able to be manipulated by b...

Protect sensistive chrome: and chrome-extension: schemes as not being able to be manipulated by bookmarklets and javascript: URLs typed into the omnibox.

BUG=93498
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98849

[Tom Sepez, 2011-08-25 18:20:49]

Chrome side CL for protecting chrome:// urls.

[abarth-chromium, 2011-08-25 18:25:48]

erikkay should see this CL.

[abarth-chromium, 2011-08-25 18:26:59]

Code change LGTM, assuming Erik is ok with the policy.

[Mihai Parparita -not on Chrome, 2011-08-25 18:30:56]

http://codereview.chromium.org/7748022/diff/1/chrome/renderer/chrome_content_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/7748022/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:215: extension_scheme);
This also includes packaged apps, users might expect bookmarklets to run on
those

[Tom Sepez, 2011-08-25 18:34:11]

Ok.  Erik suggested this on
http://code.google.com/p/chromium/issues/detail?id=93498 comment #6.  I'm happy
to do this either way.  Let me know.

[Tom Sepez, 2011-08-25 18:46:48]

Adding Mihai as cc.  See previous comment.

[Tom Sepez, 2011-08-25 19:24:59]

PS3 does chrome:// but not extensions.

[Tom Sepez, 2011-08-25 20:49:35]

The failures in CrossSiteNavigationErrorPage test are real.  When there is such
an error, RenderView::LoadNavigationErrorPage() loads the frame with an HTML
string supplying the URL as chrome://chromewebdata/, which is then subject to
the protection.  Subsequent invocation of JS on the page by the test then fails.

Worse, it appears that the call to tab->NavigateToURL() hangs rather than
completing in error, making it hard to add a test for the new blocking
behaviour.   

Hmmm ...

Might be desirable to run a bookmarklet against this page to do something useful
after the error.

Might be desirable to reduce the privilege of this page (IDR_NET_ERROR_HTML) by
not having the URL under chrome: scheme.

Need to find a way to get an indication returned to test suite when JS URL
navigation blocked.

[Tom Sepez, 2011-08-25 21:52:16]

Add chrome://newtab to the same list.  Though there doesn't appear to be a test
for it.

[Mihai Parparita -not on Chrome, 2011-08-26 01:12:44]

On Thu, Aug 25, 2011 at 2:52 PM, <tsepez@chromium.org> wrote:

> Might be desirable to run a bookmarklet against this page to do something
> useful
> after the error.


I can't think of a use-case for allowing the bookmarklet to run on the error
page.

Add chrome://newtab to the same list.  Though there doesn't appear to be a
> test
> for it.


Ditto, why would we allow bookmarklets to run here? The NTP has sensitive
data too (via sync and bookmarks).

Mihai

[tsepez (do not use), 2011-08-26 16:54:11]

I'd expect that clicking on a bookmarklet that navigates somewhere from the
NTP is a common case, no?

On Thu, Aug 25, 2011 at 6:12 PM, Mihai Parparita <mihaip@chromium.org>wrote:

> On Thu, Aug 25, 2011 at 2:52 PM, <tsepez@chromium.org> wrote:
>
>> Might be desirable to run a bookmarklet against this page to do something
>> useful
>> after the error.
>
>
> I can't think of a use-case for allowing the bookmarklet to run on the
> error page.
>
> Add chrome://newtab to the same list.  Though there doesn't appear to be a
>> test
>> for it.
>
>
> Ditto, why would we allow bookmarklets to run here? The NTP has sensitive
> data too (via sync and bookmarks).
>
> Mihai
>

[cevans, 2011-08-26 16:57:17]

On Fri, Aug 26, 2011 at 9:54 AM, Thomas Sepez <tsepez@google.com> wrote:

> I'd expect that clicking on a bookmarklet that navigates somewhere from the
> NTP is a common case, no?


Does the NTP have access to any privileged APIs? If so, we can't permit
bookemarklets on it without making the whole defense pointless, no?

I'd also worry about NTP gaining powers in the future and breaking our
defense.


>
> On Thu, Aug 25, 2011 at 6:12 PM, Mihai Parparita <mihaip@chromium.org>wrote:
>
>> On Thu, Aug 25, 2011 at 2:52 PM, <tsepez@chromium.org> wrote:
>>
>>> Might be desirable to run a bookmarklet against this page to do something
>>> useful
>>> after the error.
>>
>>
>> I can't think of a use-case for allowing the bookmarklet to run on the
>> error page.
>>
>> Add chrome://newtab to the same list.  Though there doesn't appear to be a
>>> test
>>> for it.
>>
>>
>> Ditto, why would we allow bookmarklets to run here? The NTP has sensitive
>> data too (via sync and bookmarks).
>>
>> Mihai
>>
>
>

[tsepez (do not use), 2011-08-26 16:58:34]

APIs aside, it has the list of places you've been ...

On Fri, Aug 26, 2011 at 9:57 AM, Chris Evans <cevans@google.com> wrote:

> On Fri, Aug 26, 2011 at 9:54 AM, Thomas Sepez <tsepez@google.com> wrote:
>
>> I'd expect that clicking on a bookmarklet that navigates somewhere from
>> the NTP is a common case, no?
>
>
> Does the NTP have access to any privileged APIs? If so, we can't permit
> bookemarklets on it without making the whole defense pointless, no?
>
> I'd also worry about NTP gaining powers in the future and breaking our
> defense.
>
>
>>
>> On Thu, Aug 25, 2011 at 6:12 PM, Mihai Parparita <mihaip@chromium.org>wrote:
>>
>>> On Thu, Aug 25, 2011 at 2:52 PM, <tsepez@chromium.org> wrote:
>>>
>>>> Might be desirable to run a bookmarklet against this page to do
>>>> something useful
>>>> after the error.
>>>
>>>
>>> I can't think of a use-case for allowing the bookmarklet to run on the
>>> error page.
>>>
>>> Add chrome://newtab to the same list.  Though there doesn't appear to be
>>>> a test
>>>> for it.
>>>
>>>
>>> Ditto, why would we allow bookmarklets to run here? The NTP has sensitive
>>> data too (via sync and bookmarks).
>>>
>>> Mihai
>>>
>>
>>
>

[cevans, 2011-08-26 17:10:03]

On Fri, Aug 26, 2011 at 9:58 AM, Thomas Sepez <tsepez@google.com> wrote:

> APIs aside, it has the list of places you've been ...


And ever more and more stuff.

If I remember correctly, most of our UI used to be implemented as native UI.
The fact it's now HTML seems like an internal implementation detail. I think
it's fine (and necessary) to break bookmarklets on chrome://* -- there's
always the developer console.

Does chrome-extensions:// have anything sensitive mapped?


>
>
> On Fri, Aug 26, 2011 at 9:57 AM, Chris Evans <cevans@google.com> wrote:
>
>> On Fri, Aug 26, 2011 at 9:54 AM, Thomas Sepez <tsepez@google.com> wrote:
>>
>>> I'd expect that clicking on a bookmarklet that navigates somewhere from
>>> the NTP is a common case, no?
>>
>>
>> Does the NTP have access to any privileged APIs? If so, we can't permit
>> bookemarklets on it without making the whole defense pointless, no?
>>
>> I'd also worry about NTP gaining powers in the future and breaking our
>> defense.
>>
>>
>>>
>>> On Thu, Aug 25, 2011 at 6:12 PM, Mihai Parparita <mihaip@chromium.org>wrote:
>>>
>>>> On Thu, Aug 25, 2011 at 2:52 PM, <tsepez@chromium.org> wrote:
>>>>
>>>>> Might be desirable to run a bookmarklet against this page to do
>>>>> something useful
>>>>> after the error.
>>>>
>>>>
>>>> I can't think of a use-case for allowing the bookmarklet to run on the
>>>> error page.
>>>>
>>>> Add chrome://newtab to the same list.  Though there doesn't appear to be
>>>>> a test
>>>>> for it.
>>>>
>>>>
>>>> Ditto, why would we allow bookmarklets to run here? The NTP has
>>>> sensitive data too (via sync and bookmarks).
>>>>
>>>> Mihai
>>>>
>>>
>>>
>>
>

[tsepez (do not use), 2011-08-26 17:33:44]

re chrome:// - if we buy off on that, then I'm left with fixing an existing
test that relies on this to get its answer, and creating a test that doesn't
hang that proves the blocking works.

re chrome-extension:// - not sure what privs it has.


On Fri, Aug 26, 2011 at 10:09 AM, Chris Evans <cevans@google.com> wrote:

> On Fri, Aug 26, 2011 at 9:58 AM, Thomas Sepez <tsepez@google.com> wrote:
>
>> APIs aside, it has the list of places you've been ...
>
>
> And ever more and more stuff.
>
> If I remember correctly, most of our UI used to be implemented as native
> UI. The fact it's now HTML seems like an internal implementation detail. I
> think it's fine (and necessary) to break bookmarklets on chrome://* --
> there's always the developer console.
>
> Does chrome-extensions:// have anything sensitive mapped?
>
>
>>
>>
>> On Fri, Aug 26, 2011 at 9:57 AM, Chris Evans <cevans@google.com> wrote:
>>
>>> On Fri, Aug 26, 2011 at 9:54 AM, Thomas Sepez <tsepez@google.com> wrote:
>>>
>>>> I'd expect that clicking on a bookmarklet that navigates somewhere from
>>>> the NTP is a common case, no?
>>>
>>>
>>> Does the NTP have access to any privileged APIs? If so, we can't permit
>>> bookemarklets on it without making the whole defense pointless, no?
>>>
>>> I'd also worry about NTP gaining powers in the future and breaking our
>>> defense.
>>>
>>>
>>>>
>>>> On Thu, Aug 25, 2011 at 6:12 PM, Mihai Parparita
<mihaip@chromium.org>wrote:
>>>>
>>>>> On Thu, Aug 25, 2011 at 2:52 PM, <tsepez@chromium.org> wrote:
>>>>>
>>>>>> Might be desirable to run a bookmarklet against this page to do
>>>>>> something useful
>>>>>> after the error.
>>>>>
>>>>>
>>>>> I can't think of a use-case for allowing the bookmarklet to run on the
>>>>> error page.
>>>>>
>>>>> Add chrome://newtab to the same list.  Though there doesn't appear to
>>>>>> be a test
>>>>>> for it.
>>>>>
>>>>>
>>>>> Ditto, why would we allow bookmarklets to run here? The NTP has
>>>>> sensitive data too (via sync and bookmarks).
>>>>>
>>>>> Mihai
>>>>>
>>>>
>>>>
>>>
>>
>

[Erik does not do reviews, 2011-08-29 18:03:20]

What UI do users see when they click on a bookmarklet when it's disabled?

[Tom Sepez, 2011-08-29 18:10:23]

Request is silently ignored.

[Mihai Parparita -not on Chrome, 2011-08-29 20:10:28]

On 2011/08/29 18:10:23, Tom Sepez wrote:
> Request is silently ignored.

Is there still a "Refused to execute inline script because of
Content-Security-Policy."-like message to the console?

[abarth-chromium, 2011-08-29 20:16:45]

On Mon, Aug 29, 2011 at 1:10 PM,  <mihaip@chromium.org> wrote:
> On 2011/08/29 18:10:23, Tom Sepez wrote:
>>
>> Request is silently ignored.
>
> Is there still a "Refused to execute inline script because of
> Content-Security-Policy."-like message to the console?

Nope, but we could add such a message on the WebKit side pretty easily.

Adam

[Tom Sepez, 2011-08-29 23:07:15]

Adding john and brett for review of RDH_uitest.cc

[brettw, 2011-08-30 06:19:59]

I'll defer my review to creis (LGTM to make the commit bot happy for owners)

[Charlie Reis, 2011-08-30 16:55:59]

LGTM

http://codereview.chromium.org/7748022/diff/11001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host_uitest.cc (right):

http://codereview.chromium.org/7748022/diff/11001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_uitest.cc:364: // kick
off the navigation, and wait to see that the tab loads.
This ExecuteAndExtractBool approach looks great.  We don't even need the "We
would like to..." sentence, since that was just one way to get the renderer to
initiate the navigation, and your approach works equally well.

http://codereview.chromium.org/7748022/diff/11001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_uitest.cc:372:
EXPECT_TRUE(WaitUntilJavaScriptCondition(
I'm a little concerned about this wait being flaky, but I'm not sure what would
be better.