chrome-search: should not be display-isolated

chrome-search: should not be display-isolated

Makes chrome-search display-isolated only for non-Instant processes.
Also removes origin-access-whitelist additions for the specific hosts.

BUG=222499
TEST=InstantPolicyTest.ThemeBackgroundAccess, InstantExtendedTest.NoWebUIBindingsOn*
R=sreeram@chromium.org, creis@chromium.org


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

[dhollowa, 2013-03-20 21:32:22]

Hi Guys,

The fix here is to make chrome-schemed *not* display isolated for the Instant
process, and *not* add chrome-scheme: to the origin-access-whitelist.  I.e. the
Instant page should be able to display but not XHR the assets that live behind
chrome-search:.

[Shishir, 2013-03-20 21:35:51]

https://codereview.chromium.org/12621008/diff/1/chrome/renderer/chrome_conten...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/12621008/diff/1/chrome/renderer/chrome_conten...
chrome/renderer/chrome_content_renderer_client.cc:252:
WebSecurityPolicy::registerURLSchemeAsDisplayIsolated(chrome_search_scheme);
As sreeram pointed out, we should just block all chrome-search access for pages
out of the instant process.

https://codereview.chromium.org/12621008/diff/1/chrome/renderer/searchbox/sea...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/12621008/diff/1/chrome/renderer/searchbox/sea...
chrome/renderer/searchbox/searchbox.cc:12: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebSecurityPolicy.h"
Dont need this.

[dhollowa, 2013-03-20 22:15:08]

https://codereview.chromium.org/12621008/diff/1/chrome/renderer/chrome_conten...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/12621008/diff/1/chrome/renderer/chrome_conten...
chrome/renderer/chrome_content_renderer_client.cc:252:
WebSecurityPolicy::registerURLSchemeAsDisplayIsolated(chrome_search_scheme);
On 2013/03/20 21:35:52, Shishir wrote:
> As sreeram pointed out, we should just block all chrome-search access for
pages
> out of the instant process.

We're effectively doing that on the browser-side already.  Are you thinking of a
specific mechanism to do this on the renderer side (besides the one here that
is)?

https://codereview.chromium.org/12621008/diff/1/chrome/renderer/searchbox/sea...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/12621008/diff/1/chrome/renderer/searchbox/sea...
chrome/renderer/searchbox/searchbox.cc:12: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebSecurityPolicy.h"
On 2013/03/20 21:35:52, Shishir wrote:
> Dont need this.

Done.

[Shishir, 2013-03-20 22:20:49]

https://codereview.chromium.org/12621008/diff/1/chrome/renderer/chrome_conten...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/12621008/diff/1/chrome/renderer/chrome_conten...
chrome/renderer/chrome_content_renderer_client.cc:252:
WebSecurityPolicy::registerURLSchemeAsDisplayIsolated(chrome_search_scheme);
On 2013/03/20 22:15:08, dhollowa wrote:
> On 2013/03/20 21:35:52, Shishir wrote:
> > As sreeram pointed out, we should just block all chrome-search access for
> pages
> > out of the instant process.
> 
> We're effectively doing that on the browser-side already.  Are you thinking of
a
> specific mechanism to do this on the renderer side (besides the one here that
> is)?

One way I can think of doing that on the renderer side is blocking calls in
WillSendRequest in the same file. You can do what the extension system does for
requests where there is no access - rewrite the URL. We can do that in a
separate CL if you prefer.

[sreeram, 2013-03-20 22:26:24]

lgtm

[dhollowa, 2013-03-20 23:12:24]

https://codereview.chromium.org/12621008/diff/1/chrome/renderer/chrome_conten...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/12621008/diff/1/chrome/renderer/chrome_conten...
chrome/renderer/chrome_content_renderer_client.cc:252:
WebSecurityPolicy::registerURLSchemeAsDisplayIsolated(chrome_search_scheme);
On 2013/03/20 22:20:49, Shishir wrote:
> On 2013/03/20 22:15:08, dhollowa wrote:
> > On 2013/03/20 21:35:52, Shishir wrote:
> > > As sreeram pointed out, we should just block all chrome-search access for
> > pages
> > > out of the instant process.
> > 
> > We're effectively doing that on the browser-side already.  Are you thinking
of
> a
> > specific mechanism to do this on the renderer side (besides the one here
that
> > is)?
> 
> One way I can think of doing that on the renderer side is blocking calls in
> WillSendRequest in the same file. You can do what the extension system does
for
> requests where there is no access - rewrite the URL. We can do that in a
> separate CL if you prefer.

But, correct me if I'm wrong, adding a similar check to WillSendRequest() would
be redundant with the restrictions in place here with
registerURLSchemeAsDisplayIsolated().

If extensions were display-isolated they wouldn't need the WillSendRequest()
logic.

[Shishir, 2013-03-20 23:23:53]

On 2013/03/20 23:12:24, dhollowa wrote:
>
https://codereview.chromium.org/12621008/diff/1/chrome/renderer/chrome_conten...
> File chrome/renderer/chrome_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/12621008/diff/1/chrome/renderer/chrome_conten...
> chrome/renderer/chrome_content_renderer_client.cc:252:
> WebSecurityPolicy::registerURLSchemeAsDisplayIsolated(chrome_search_scheme);
> On 2013/03/20 22:20:49, Shishir wrote:
> > On 2013/03/20 22:15:08, dhollowa wrote:
> > > On 2013/03/20 21:35:52, Shishir wrote:
> > > > As sreeram pointed out, we should just block all chrome-search access
for
> > > pages
> > > > out of the instant process.
> > > 
> > > We're effectively doing that on the browser-side already.  Are you
thinking
> of
> > a
> > > specific mechanism to do this on the renderer side (besides the one here
> that
> > > is)?
> > 
> > One way I can think of doing that on the renderer side is blocking calls in
> > WillSendRequest in the same file. You can do what the extension system does
> for
> > requests where there is no access - rewrite the URL. We can do that in a
> > separate CL if you prefer.
> 
> But, correct me if I'm wrong, adding a similar check to WillSendRequest()
would
> be redundant with the restrictions in place here with
> registerURLSchemeAsDisplayIsolated().
> 
> If extensions were display-isolated they wouldn't need the WillSendRequest()
> logic.

Sounds right. DisplayIsolated should prevent all cross origin access on
chrome-search.

[Shishir, 2013-03-20 23:26:48]

lgtm

[dhollowa, 2013-03-20 23:32:15]

+ jam      -> OWNERS chrome/renderer/chrome_content_renderer_client.cc
+ palmer -> OWNERS chrome/common/render_messages.h

Thanks.

[Charlie Reis, 2013-03-21 00:08:17]

I'm not entirely clear what this means.  Can you elaborate on how
DisplayIsolated is used?

https://codereview.chromium.org/12621008/diff/10001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/12621008/diff/10001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:251: if
(!command_line->HasSwitch(switches::kInstantProcess))
Why would we register this (or the one above) at all in a non-instant process?

[dhollowa, 2013-03-21 01:30:19]

> I'm not entirely clear what this means.  Can you elaborate on how
DisplayIsolated is used?

Yes, the problem we were seeing was that with the origin-access-whitelist
approach it was possible for search providers within the Instant process to XHR
to read asset data originating within chrome-search:.  This is not a big deal if
you trust the search provider to play nice with your data, but we don't want to
be that trusting.  So by making the chrome-search: assets displayable (aka not
display-isolated) by search providers but revoking their cross-origin access, we
shut down their ability to *read* asset data via XHR.

Non-Instant renderers are further restricted as described in my inline comment.

https://codereview.chromium.org/12621008/diff/10001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/12621008/diff/10001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:251: if
(!command_line->HasSwitch(switches::kInstantProcess))
On 2013/03/21 00:08:17, creis wrote:
> Why would we register this (or the one above) at all in a non-instant process?

The one above, chrome:, is linked from external places I suppose, like help
pages.  But has nothing to do with Instant.

The registering of the chrome-search: protocol in general is a somewhat global
affair, ProfileIOData::IsHandledProtocol for example, so is cumbersome at best
to avoid.

A cleaner approach is to use this display-isolated setting here to disallow
access except from chrome-search: and Instant pages.

To reiterate, a non-Instant process, when trying to access
chrome-search://something, will bump up against the following:

  1. Renderer: the display-isolated check in WebKit, so will be denied,
  2. Browser: (Assuming they got by #1), the scheme checks in
URLDataSource::ShouldServiceRequest,
  3. Browser: for specific sub-classes of URLDataSource, like ThemeSource there
are additional Insant-PID checks that make sure the request is coming from a
blessed Instant process.

So with all this, I believe we're good.

[Charlie Reis, 2013-03-21 18:06:42]

Thanks.  LGTM once you add comments explaining the reasoning.

https://codereview.chromium.org/12621008/diff/10001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/12621008/diff/10001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:251: if
(!command_line->HasSwitch(switches::kInstantProcess))
On 2013/03/21 01:30:19, dhollowa wrote:
> On 2013/03/21 00:08:17, creis wrote:
> > Why would we register this (or the one above) at all in a non-instant
process?
> 
> The one above, chrome:, is linked from external places I suppose, like help
> pages.  But has nothing to do with Instant.

My mistake; I was misreading that.

> The registering of the chrome-search: protocol in general is a somewhat global
> affair, ProfileIOData::IsHandledProtocol for example, so is cumbersome at best
> to avoid.
> 
> A cleaner approach is to use this display-isolated setting here to disallow
> access except from chrome-search: and Instant pages.
> 
> To reiterate, a non-Instant process, when trying to access
> chrome-search://something, will bump up against the following:
> 
>   1. Renderer: the display-isolated check in WebKit, so will be denied,
>   2. Browser: (Assuming they got by #1), the scheme checks in
> URLDataSource::ShouldServiceRequest,
>   3. Browser: for specific sub-classes of URLDataSource, like ThemeSource
there
> are additional Insant-PID checks that make sure the request is coming from a
> blessed Instant process.
> 
> So with all this, I believe we're good.

Thanks for the explanation.  I don't think it's obvious from reading this code,
so could you add a comment here explaining that the Instant process can *only*
display the content (not read it) and other processes can't display it or read
it.

I think it would also be worthwhile to add some of this context to the
kChromeSearchScheme declaration in url_constants.h, to avoid confusion about how
we expect this scheme to be handled.

[dhollowa, 2013-03-21 19:14:07]

On 2013/03/21 18:06:42, creis wrote:
> Thanks.  LGTM once you add comments explaining the reasoning.
> 

Thanks.  Done.

[dhollowa, 2013-03-21 19:14:48]

+ jam -> OWNERS chrome/renderer/chrome_content_renderer_client.cc
+ palmer -> OWNERS chrome/common/render_messages.h

[palmer, 2013-03-21 20:33:08]

lgtm

[jam, 2013-03-21 20:47:47]

On 2013/03/21 19:14:48, dhollowa wrote:
> + jam -> OWNERS chrome/renderer/chrome_content_renderer_client.cc
please see chrome/OWNERS
> + palmer -> OWNERS chrome/common/render_messages.h

[dhollowa, 2013-03-21 20:52:04]

On 2013/03/21 20:47:47, jam wrote:
> On 2013/03/21 19:14:48, dhollowa wrote:
> > + jam -> OWNERS chrome/renderer/chrome_content_renderer_client.cc
> please see chrome/OWNERS
> > + palmer -> OWNERS chrome/common/render_messages.h

John, sorry, I'm not understanding.  chrome/render does not have an OWNERS file.
 You are listed in chrome/OWNERS and are most familiar with this file.  Am I
mistaken in my reasoning for picking you?

[commit-bot: I haz the power, 2013-03-21 22:08:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dhollowa@chromium.org/12621008/14001

[commit-bot: I haz the power, 2013-03-21 22:08:47]

Presubmit check for 12621008-14001 failed and returned exit status 1.

INFO:root:Found 8 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/renderer/chrome_content_renderer_client.cc

Presubmit checks took 1.4s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[dhollowa, 2013-03-21 22:32:55]

Hi Scott, looking for an OWNERS stamp for:
  chrome/renderer/chrome_content_renderer_client.cc

Was reviewed by creis@ already for sec, and is a simple change.  Thanks!

[sky, 2013-03-22 04:01:06]

LGTM

[commit-bot: I haz the power, 2013-03-22 16:08:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dhollowa@chromium.org/12621008/14001

[commit-bot: I haz the power, 2013-03-22 18:27:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dhollowa@chromium.org/12621008/14001

[commit-bot: I haz the power, 2013-03-23 15:11:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dhollowa@chromium.org/12621008/14001

[commit-bot: I haz the power, 2013-03-23 22:57:49]

Failed to apply patch for chrome/renderer/searchbox/searchbox.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/renderer/searchbox/searchbox.h
  Hunk #1 FAILED at 119.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/renderer/searchbox/searchbox.h.rej

Patch:       chrome/renderer/searchbox/searchbox.h
Index: chrome/renderer/searchbox/searchbox.h
diff --git a/chrome/renderer/searchbox/searchbox.h
b/chrome/renderer/searchbox/searchbox.h
index
124331835a020c3bc5f5f9ba6a8d228df45f4f53..f9578046321dd0871a7238918581f2e1f8c498f6
100644
--- a/chrome/renderer/searchbox/searchbox.h
+++ b/chrome/renderer/searchbox/searchbox.h
@@ -119,7 +119,6 @@ class SearchBox : public content::RenderViewObserver,
   void OnThemeAreaHeightChanged(int height);
   void OnFontInformationReceived(const string16& omnibox_font,
                                  size_t omnibox_font_size);
-  void OnGrantChromeSearchAccessFromOrigin(const GURL& origin_url);
   void OnMostVisitedChanged(const std::vector<InstantMostVisitedItem>& items);
 
   // Returns the current zoom factor of the render view or 1 on failure.

[Shishir, 2013-03-24 16:40:10]

Rebased to HEAD and verified that the diffs are same as PS4. Sending to CQ.

[commit-bot: I haz the power, 2013-03-24 16:48:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dhollowa@chromium.org/12621008/45001

[commit-bot: I haz the power, 2013-03-24 17:54:57]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-24 17:56:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dhollowa@chromium.org/12621008/45001

[commit-bot: I haz the power, 2013-03-24 18:05:21]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-03-24 18:07:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dhollowa@chromium.org/12621008/45001

[commit-bot: I haz the power, 2013-03-24 19:22:52]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-03-24 19:24:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dhollowa@chromium.org/12621008/45001

[commit-bot: I haz the power, 2013-03-24 19:49:39]

Change committed as 190282

[unblockableq, 2016-12-04 15:35:24]

unblockableq@gmail.com changed reviewers:
+ unblockableq@gmail.com

[unblockableq, 2016-12-04 15:35:24]

aaaaaaaaa

[unblockableq, 2016-12-04 15:36:41]

lgtm

aaaaaaaaa