Add missing return statement after ReceivedBadMessage call.

Add missing return statement after ReceivedBadMessage call.

This prevents the scheme access being granted and used if the renderer is restarted.

BUG=726142
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2915813002
Cr-Commit-Position: refs/heads/master@{#476161}
Committed: https://chromium.googlesource.com/chromium/src/+/d5226ddf44a52a3f066755e8d32710f6a093aa30

[jam, 2017-05-31 14:55:18]

Description was changed from

==========
Reset the ChildProcessSecurityPolicy state for a renderer when it sends a bad
message.

This prevents a bug that continues processing a bad IPC from persisting after a
killed tab is reloaded. Also fix a missing return statement I added in r475043

BUG=726142
==========

to

==========
Reset the ChildProcessSecurityPolicy state for a renderer when it sends a bad
message.

This prevents a bug that continues processing a bad IPC from persisting after a
killed tab is reloaded. Also fix a missing return statement I added in r475043

BUG=726142
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-05-31 14:55:28]

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

[commit-bot: I haz the power, 2017-05-31 14:55:39]

Dry run: CQ is trying da patch.

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

[jam, 2017-05-31 14:56:42]

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

[jam, 2017-05-31 14:56:42]

Thanks for catching the earlier bug. This patch should hopefully make this not
possible again.

[commit-bot: I haz the power, 2017-05-31 15:09:11]

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

[commit-bot: I haz the power, 2017-05-31 15:09:12]

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...)

[jam, 2017-05-31 15:22:57]

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

[jam, 2017-05-31 15:23:05]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-05-31 15:23:16]

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, 2017-05-31 16:05:36]

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

[commit-bot: I haz the power, 2017-05-31 16:05:37]

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

[Charlie Reis, 2017-05-31 17:28:22]

On 2017/05/31 14:56:42, jam wrote:
> Thanks for catching the earlier bug. This patch should hopefully make this not
> possible again.

Thanks!  That's an interesting idea to reset on kills, but I think we should
stick with the early return for a CL that we plan to merge.  Clearing the CPSP
state is likely to cause additional kills that are harder to diagnose when the
user reloads the tab.  (If we want to try that, it should have a lot of bake
time on Dev.)

Can you also include a test for this case in
BrowserSideNavigationBrowserTest.ValidateBaseUrlForDataUrl, like
https://codereview.chromium.org/2905293002/diff/1/content/browser/browser_sid...

https://codereview.chromium.org/2915813002/diff/20001/content/browser/child_p...
File content/browser/child_process_security_policy_browsertest.cc (right):

https://codereview.chromium.org/2915813002/diff/20001/content/browser/child_p...
content/browser/child_process_security_policy_browsertest.cc:80:
ChildProcessSecurityPolicyImpl::GetInstance()->CanReadFile(child_id,
This is failing on the bots, but maybe that doesn't matter if we stick with the
early return approach for now.

[jam, 2017-05-31 23:14:37]

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

[commit-bot: I haz the power, 2017-05-31 23:15:19]

Dry run: CQ is trying da patch.

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

[jam, 2017-05-31 23:19:59]

Patchset #2 (id:40001) has been deleted

[jam, 2017-05-31 23:20:05]

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

[commit-bot: I haz the power, 2017-05-31 23:20:34]

Dry run: CQ is trying da patch.

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

[jam, 2017-05-31 23:25:36]

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

[jam, 2017-05-31 23:25:39]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-05-31 23:26:05]

Dry run: CQ is trying da patch.

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

[jam, 2017-05-31 23:26:15]

https://codereview.chromium.org/2915813002/diff/20001/content/browser/child_p...
File content/browser/child_process_security_policy_browsertest.cc (right):

https://codereview.chromium.org/2915813002/diff/20001/content/browser/child_p...
content/browser/child_process_security_policy_browsertest.cc:80:
ChildProcessSecurityPolicyImpl::GetInstance()->CanReadFile(child_id,
On 2017/05/31 17:28:23, Charlie Reis wrote:
> This is failing on the bots, but maybe that doesn't matter if we stick with
the
> early return approach for now.

ok, I'll do this separately since it's a good point we don't have to do it on
branch.

The test passes on windows, must be a race condition that it fails elsewhere (or
maybe the error code doesn't get signalled correctly there).

[jam, 2017-05-31 23:52:59]

Description was changed from

==========
Reset the ChildProcessSecurityPolicy state for a renderer when it sends a bad
message.

This prevents a bug that continues processing a bad IPC from persisting after a
killed tab is reloaded. Also fix a missing return statement I added in r475043

BUG=726142
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add missing return statement after ReceivedBadMessage call.

This prevents the scheme access being granted and used if the renderer is
restarted.

BUG=726142
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-05-31 23:54:12]

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

[commit-bot: I haz the power, 2017-05-31 23:54:13]

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...)

[jam, 2017-05-31 23:56:54]

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

[commit-bot: I haz the power, 2017-05-31 23:57:57]

Dry run: CQ is trying da patch.

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

[Charlie Reis, 2017-06-01 00:33:43]

Thanks!  Fix looks good.  One question about the data URL in the test.

https://codereview.chromium.org/2915813002/diff/100001/content/browser/browse...
File content/browser/browser_side_navigation_browsertest.cc (right):

https://codereview.chromium.org/2915813002/diff/100001/content/browser/browse...
content/browser/browser_side_navigation_browsertest.cc:475: data_url,
Referrer(), ui::PAGE_TRANSITION_LINK,
Just curious, why did you need to use a data URL?  I would think that the
previous web URL would also not be allowed to access the file URL, and would
thus be an ok test case without needing
kAllowContentInitiatedDataUrlNavigations.  But maybe there's a problem I'm
overlooking?

https://codereview.chromium.org/2915813002/diff/100001/content/browser/browse...
content/browser/browser_side_navigation_browsertest.cc:495:
process_exit_observer.Wait();
Does the CanReadFile check work?  We should add it if so.  Something like:

EXPECT_FALSE(ChildProcessSecurityPolicyImpl::GetInstance()->CanReadFile(rfh->GetProcess()->GetID(),
file_url));

[jam, 2017-06-01 00:46:01]

Patchset #2 (id:80001) has been deleted

[jam, 2017-06-01 00:51:19]

https://codereview.chromium.org/2915813002/diff/100001/content/browser/browse...
File content/browser/browser_side_navigation_browsertest.cc (right):

https://codereview.chromium.org/2915813002/diff/100001/content/browser/browse...
content/browser/browser_side_navigation_browsertest.cc:475: data_url,
Referrer(), ui::PAGE_TRANSITION_LINK,
On 2017/06/01 00:33:43, Charlie Reis wrote:
> Just curious, why did you need to use a data URL?  I would think that the
> previous web URL would also not be allowed to access the file URL, and would
> thus be an ok test case without needing
> kAllowContentInitiatedDataUrlNavigations.  But maybe there's a problem I'm
> overlooking?

The call to ChildProcessSecurityPolicy that uses base_url_for_data_url in
UpdatePermissionsForNavigation() is only done if the request is for a data url
(see line 3643 in the other file)

https://codereview.chromium.org/2915813002/diff/100001/content/browser/browse...
content/browser/browser_side_navigation_browsertest.cc:495:
process_exit_observer.Wait();
On 2017/06/01 00:33:43, Charlie Reis wrote:
> Does the CanReadFile check work?  We should add it if so.  Something like:
> 
>
EXPECT_FALSE(ChildProcessSecurityPolicyImpl::GetInstance()->CanReadFile(rfh->GetProcess()->GetID(),
> file_url));

Done.

[jam, 2017-06-01 00:52:00]

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

[commit-bot: I haz the power, 2017-06-01 00:52:42]

Dry run: CQ is trying da patch.

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

[Charlie Reis, 2017-06-01 01:00:09]

LGTM

https://codereview.chromium.org/2915813002/diff/100001/content/browser/browse...
File content/browser/browser_side_navigation_browsertest.cc (right):

https://codereview.chromium.org/2915813002/diff/100001/content/browser/browse...
content/browser/browser_side_navigation_browsertest.cc:475: data_url,
Referrer(), ui::PAGE_TRANSITION_LINK,
On 2017/06/01 00:51:19, jam wrote:
> On 2017/06/01 00:33:43, Charlie Reis wrote:
> > Just curious, why did you need to use a data URL?  I would think that the
> > previous web URL would also not be allowed to access the file URL, and would
> > thus be an ok test case without needing
> > kAllowContentInitiatedDataUrlNavigations.  But maybe there's a problem I'm
> > overlooking?
> 
> The call to ChildProcessSecurityPolicy that uses base_url_for_data_url in
> UpdatePermissionsForNavigation() is only done if the request is for a data url
> (see line 3643 in the other file)

Ah, thanks.  So the original test wouldn't have gotten as far as the exploit
without the fix.  Sounds good!

[jam, 2017-06-01 01:05:15]

The CQ bit was unchecked by jam@chromium.org

[jam, 2017-06-01 01:05:24]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2017-06-01 01:06:25]

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, 2017-06-01 02:34:39]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496279124417870,
"parent_rev": "8f09864d8e5e1702e719d594f05bad3e098f0a52", "commit_rev":
"777f581901a971b72f945a8108a2c0bbdcdf2f92"}

[commit-bot: I haz the power, 2017-06-01 02:34:54]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496279124417870,
"parent_rev": "0ba9bbc1468d08e9268699a76d542913ea13c758", "commit_rev":
"d5226ddf44a52a3f066755e8d32710f6a093aa30"}

[commit-bot: I haz the power, 2017-06-01 02:35:03]

Description was changed from

==========
Add missing return statement after ReceivedBadMessage call.

This prevents the scheme access being granted and used if the renderer is
restarted.

BUG=726142
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add missing return statement after ReceivedBadMessage call.

This prevents the scheme access being granted and used if the renderer is
restarted.

BUG=726142
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2915813002
Cr-Commit-Position: refs/heads/master@{#476161}
Committed:
https://chromium.googlesource.com/chromium/src/+/d5226ddf44a52a3f066755e8d327...
==========

[commit-bot: I haz the power, 2017-06-01 02:35:04]

Committed patchset #3 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/d5226ddf44a52a3f066755e8d327...