Extra browser-side validation of transferred_request_child_id / request_id.

Extra browser-side validation of transferred_request_child_id / request_id.

BUG=656179
Committed: https://crrev.com/1f59c2aac5f896e23613af618fbe8444a63911f8
Cr-Commit-Position: refs/heads/master@{#427510}

[Łukasz Anforowicz, 2016-10-21 17:21:43]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 17:22:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 17:26:07]

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

[commit-bot: I haz the power, 2016-10-21 17:26:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Łukasz Anforowicz, 2016-10-21 17:27:46]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 17:28:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2016-10-21 17:29:08]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-10-21 17:35:08]

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

[commit-bot: I haz the power, 2016-10-21 17:35:09]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Łukasz Anforowicz, 2016-10-21 17:38:48]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 17:39:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2016-10-21 17:39:50]

The CQ bit was unchecked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-10-21 17:40:14]

Patchset #3 (id:60001) has been deleted

[Łukasz Anforowicz, 2016-10-21 17:40:23]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 17:41:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 19:03:16]

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

[commit-bot: I haz the power, 2016-10-21 19:03:17]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Łukasz Anforowicz, 2016-10-21 21:35:41]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 21:36:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2016-10-21 22:57:38]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[Łukasz Anforowicz, 2016-10-21 22:57:49]

lukasza@chromium.org changed reviewers:
+ creis@chromium.org

[Łukasz Anforowicz, 2016-10-21 22:57:50]

Charlie, can you take a look please?

Even without any extra tests, the bots were red in patchset #2 (i.e. without
corresponding renderer-side change to avoid sending transferred_request_xxx, the
new browser-side renderer kills would kick in).  Therefore, I think the changes
have sufficient test coverage as-is.  This is great, as I wasn't really looking
forward to adding a fragile, implementation-coupled unit test (or a browser test
that influences timing / race between navigational-vs-subresource requests in
the renderer).

The bots are green in patchset #3 (after adding renderer-side code to avoid
sending transferred_request_xxx for non-navigational requests), but I've
realized a mistake in rebasing/merging bad_message.h, so I've respun tryjobs on
patchset #4 as well.

[commit-bot: I haz the power, 2016-10-21 22:58:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-22 01:53:44]

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

[commit-bot: I haz the power, 2016-10-22 01:53:45]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-10-25 20:19:12]

Sorry for the delay-- this LGTM!

Sanity check: If I understand correctly, the renderer kills before the
RenderFrameImpl change were because subresources (in a page that transferred)
were tacking the transfer IDs onto their requests?

[Łukasz Anforowicz, 2016-10-25 20:20:43]

On 2016/10/25 20:19:12, Charlie Reis wrote:
> Sorry for the delay-- this LGTM!

Thanks for reviewing.

> Sanity check: If I understand correctly, the renderer kills before the
> RenderFrameImpl change were because subresources (in a page that transferred)
> were tacking the transfer IDs onto their requests?

Yes - correct.

[Łukasz Anforowicz, 2016-10-25 20:20:56]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-10-25 20:21:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-10-25 20:23:46]

mmenke@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2016-10-25 20:23:47]

https://codereview.chromium.org/2442793002/diff/100001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2442793002/diff/100001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1239: }
Random drive-by question:  How concerned are we about a compromised renderer
stealing requests this way?  If an attacker can guess a child_id and request_id
(Latter isn't hard, if opening a new tab) of a new navigation that's currently
transferring a request, can't it steal the request?

[commit-bot: I haz the power, 2016-10-25 20:24:51]

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

[commit-bot: I haz the power, 2016-10-25 20:24:52]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Reis, 2016-10-25 20:30:08]

https://codereview.chromium.org/2442793002/diff/100001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2442793002/diff/100001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1239: }
On 2016/10/25 20:23:47, mmenke wrote:
> Random drive-by question:  How concerned are we about a compromised renderer
> stealing requests this way?  If an attacker can guess a child_id and
request_id
> (Latter isn't hard, if opening a new tab) of a new navigation that's currently
> transferring a request, can't it steal the request?

Yes, good point.  Łukasz was asking about this the other day.  I think we have
some logic in RenderFrameHostManager that will only accept a commit if it comes
from the expected pending RenderFrameHost, but that won't really defend against
this (which happens earlier).  Specifically, the response would be send to the
first renderer that asks for it, and that would matter from a cross-site
document blocking perspective for Site Isolation.

We should file a bug to track that.  Not sure if there's much impact today,
though-- probably mainly for --site-per-process.

[Łukasz Anforowicz, 2016-10-25 20:48:17]

On 2016/10/25 20:30:08, Charlie Reis wrote:
>
https://codereview.chromium.org/2442793002/diff/100001/content/browser/loader...
> File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2442793002/diff/100001/content/browser/loader...
> content/browser/loader/resource_dispatcher_host_impl.cc:1239: }
> On 2016/10/25 20:23:47, mmenke wrote:
> > Random drive-by question:  How concerned are we about a compromised renderer
> > stealing requests this way?  If an attacker can guess a child_id and
> request_id
> > (Latter isn't hard, if opening a new tab) of a new navigation that's
currently
> > transferring a request, can't it steal the request?
> 
> Yes, good point.  Łukasz was asking about this the other day.  I think we have
> some logic in RenderFrameHostManager that will only accept a commit if it
comes
> from the expected pending RenderFrameHost, but that won't really defend
against
> this (which happens earlier).  Specifically, the response would be send to the
> first renderer that asks for it, and that would matter from a cross-site
> document blocking perspective for Site Isolation.
> 
> We should file a bug to track that.  Not sure if there's much impact today,
> though-- probably mainly for --site-per-process.

I've opened https://crbug.com/659299 to track this.

[Łukasz Anforowicz, 2016-10-25 21:12:40]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-10-25 21:12:40]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2442793002/#ps120001
(title: "Rebasing... (renumbering bad_message.h)")

[commit-bot: I haz the power, 2016-10-25 21:13:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 22:22:15]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-10-25 22:25:52]

Description was changed from

==========
Extra browser-side validation of transferred_request_child_id / request_id.

BUG=656179
==========

to

==========
Extra browser-side validation of transferred_request_child_id / request_id.

BUG=656179

Committed: https://crrev.com/1f59c2aac5f896e23613af618fbe8444a63911f8
Cr-Commit-Position: refs/heads/master@{#427510}
==========

[commit-bot: I haz the power, 2016-10-25 22:25:53]

Patchset 5 (id:??) landed as
https://crrev.com/1f59c2aac5f896e23613af618fbe8444a63911f8
Cr-Commit-Position: refs/heads/master@{#427510}