Prevent unauthorized commits of the Chrome Web Store URL.

Prevent unauthorized commits of the Chrome Web Store URL.
Second line of defense, in case renderer process is exploited.

BUG=172119
TEST=none

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

[Charlie Reis, 2013-05-09 22:56:36]

Nasko, can you take a look?

I was trying to think of a way to test this in ChromeSecurityExploitBrowserTest,
but I need ChromeContentRendererClient::ShouldFork to return false.  I can't
think of a way to do that in a browser_test.  I thought about simulating the
commit message, but the test has to be in chrome/ to be meaningful, and I can't
simulate a ViewHostMsg_FrameNavigate message from outside content/.

[nasko, 2013-05-10 20:37:34]

Just a couple of comments.

In general, I don't see any clean solution for the test. For tests which are
mimicking exploits, we should investigate ways to cross chrome/content
boundaries in a "clean" way. I don't know what is the best way to do so.

https://codereview.chromium.org/15017018/diff/4001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/15017018/diff/4001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:899: const Extension*
new_extension =
Nit: any reason why this is called new_extension? What is "new" about it?

https://codereview.chromium.org/15017018/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/15017018/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.cc:1213:
process->GetHandle(), content::RESULT_CODE_KILLED, false);
Why not call ReceivedBadMessage? It checks for single process mode and possibly
other things in the future.

[Charlie Reis, 2013-05-10 22:35:01]

https://codereview.chromium.org/15017018/diff/4001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/15017018/diff/4001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:899: const Extension*
new_extension =
On 2013/05/10 20:37:34, nasko wrote:
> Nit: any reason why this is called new_extension? What is "new" about it?

It distinguishes it from any current extension that might be in |process_host|
(i.e., it refers to the extension of the destination URL).  Same convention as
in ShouldSwapProcessesForNavigation below, but a slightly different check.

https://codereview.chromium.org/15017018/diff/4001/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/15017018/diff/4001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.cc:1213:
process->GetHandle(), content::RESULT_CODE_KILLED, false);
On 2013/05/10 20:37:34, nasko wrote:
> Why not call ReceivedBadMessage? It checks for single process mode and
possibly
> other things in the future.

Good idea.  Fixed.

(The one downside is that it has a NOTREACHED and kills the browser process in
debug builds, but that's arguably a good thing.  Catch it early if it's a bug,
and be aware if it's an exploit.  Might make it trickier to test, though.)

[Charlie Reis, 2013-05-13 18:02:58]

After chatting offline, I agree that removing the NOTREACHED seems sensible. 
The browser process should be able to survive an exploited renderer.

[nasko, 2013-05-13 18:14:27]

One last thing and a small nit.

https://codereview.chromium.org/15017018/diff/27001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/15017018/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:724: // We kill the
renderer but don't include a NOTREACHED, because it is possible
nit: "it is possible" doesn't seem as strong messaging to me. Chrome better be
able to survive a malformed messages/rogue renderers.

https://codereview.chromium.org/15017018/diff/27001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/15017018/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.cc:1213:
process->ReceivedBadMessage();
Do we need to return here? Why proceed with the rest if we just killed the
renderer?

[Charlie Reis, 2013-05-13 20:10:51]

Good questions!  Responses below.

https://codereview.chromium.org/15017018/diff/27001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/15017018/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:724: // We kill the
renderer but don't include a NOTREACHED, because it is possible
On 2013/05/13 18:14:27, nasko wrote:
> nit: "it is possible" doesn't seem as strong messaging to me. Chrome better be
> able to survive a malformed messages/rogue renderers.

We can't always survive: renderers can kill the browser process using resource
exhaustion, etc.  But you're right that we should try to survive.  Comment
updated.

https://codereview.chromium.org/15017018/diff/27001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/15017018/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.cc:1213:
process->ReceivedBadMessage();
On 2013/05/13 18:14:27, nasko wrote:
> Do we need to return here? Why proceed with the rest if we just killed the
> renderer?

If we returned early here, we would leave the tab spinning and the address bar
would reflect the previous page, which may not even be in the same SiteInstance.


For example, suppose you're showing a.com and navigate to b.com in a different
process.  b.com's process is already hacked by another tab and tries to commit
the CWS.  We would kill the process for b.com but never stop the spinner, commit
the navigation, or show the sad tab, and the user would be left wondering what
happened while a.com is still visible.  Even if we called Stop instead of
committing, I think it makes sense to give some visual evidence that something
went wrong.

By leaving it this way, we'll commit an "about:blank" entry and show the sad
tab, giving the user some chance to report a bug or investigate.

[nasko, 2013-05-13 20:14:59]

Thanks for the detailed answers!

LGTM

[commit-bot: I haz the power, 2013-05-13 20:22:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/15017018/33001

[commit-bot: I haz the power, 2013-05-13 21:10:53]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-05-13 23:19:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/15017018/33001

[commit-bot: I haz the power, 2013-05-14 01:08:26]

Change committed as 199881