Verify all files in the request body are accessible by the renderer process.

Verify all files in the request body are accessible by the renderer process.

This CL adds verification to RenderFrameHostImpl::OnBeginNavigation to
ensure that any files referenced in the request body have been properly
granted access to the renderer process. If the validation fails, the
process is terminated.

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

Review-Url: https://codereview.chromium.org/2902933002
Cr-Commit-Position: refs/heads/master@{#474478}
Committed: https://chromium.googlesource.com/chromium/src/+/5d30e832a1ce833818a76134b6fdd62238ec82c6

[nasko, 2017-05-24 07:18:10]

Description was changed from

==========
Verify all files in the request body are accessible by the renderer process.

This CL adds verification to RenderFrameHostImpl::OnBeginNavigation to
ensure that any files referenced in the request body have been properly
granted access to the renderer process. If the validation fails, the
process is terminated.

BUG=725689
==========

to

==========
Verify all files in the request body are accessible by the renderer process.

This CL adds verification to RenderFrameHostImpl::OnBeginNavigation to
ensure that any files referenced in the request body have been properly
granted access to the renderer process. If the validation fails, the
process is terminated.

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

[nasko, 2017-05-24 07:20:53]

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

[commit-bot: I haz the power, 2017-05-24 07:21: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-24 07:53:16]

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

[commit-bot: I haz the power, 2017-05-24 07:53:16]

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

[nasko, 2017-05-24 17:28:49]

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

[commit-bot: I haz the power, 2017-05-24 17:29:20]

Dry run: CQ is trying da patch.

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

[nasko, 2017-05-24 17:29:43]

nasko@chromium.org changed reviewers:
+ nick@chromium.org

[nasko, 2017-05-24 17:29:44]

Hey Nick,
Can you review this CL for me? It implements the verification allowed access of
upload files as part of BeginNavigate IPC as we chatted offline.
Thanks in advance!
Nasko

[commit-bot: I haz the power, 2017-05-24 18:15:05]

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

[commit-bot: I haz the power, 2017-05-24 18:15:06]

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

[ncarter (slow), 2017-05-24 20:06:19]

lgtm w/ nits and a Windows compile fix

https://codereview.chromium.org/2902933002/diff/20001/content/browser/browser...
File content/browser/browser_side_navigation_browsertest.cc (right):

https://codereview.chromium.org/2902933002/diff/20001/content/browser/browser...
content/browser/browser_side_navigation_browsertest.cc:368: base::FilePath
wrong_file(file_path.value() + "-foobarz");
base::FilePath wrong_file(file_path.value() + FILE_PATH_LITERAL("-foobarz"));

https://codereview.chromium.org/2902933002/diff/20001/content/browser/browser...
content/browser/browser_side_navigation_browsertest.cc:392: base::WrapUnique(new
FrameHostMsg_BeginNavigation(
nit: this could be created on the stack, or as a temporary expression in the
call to PwnMessageReceived.

https://codereview.chromium.org/2902933002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2902933002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:958: // Ensures that the
upload parameters sent by the renderer process are
"Ensures that" -> "Returns true if".

[Łukasz Anforowicz, 2017-05-24 20:24:10]

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

[Łukasz Anforowicz, 2017-05-24 20:24:11]

Drive-by comments...

https://codereview.chromium.org/2902933002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2902933002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2178: 
This is unrelated to the bug you are fixing and therefore optional and can be
done in a follow-up CL.  I am wondering if (here or underneath
frame_tree_node()->navigator()->OnBeginNavigation called below) we could also:

    if (common_params.method != "POST") {
      DCHECK(!common_params.post_data);
      common_params.post_data = nullptr;
    }

PS. I vaguely remember that we cannot do
    if (common_params.method == "POST")
      DCHECK(common_params.url.SchemeIsHTTPOrHTTPS())
because you can post to chrome-extension:// URI (AFAIR under the covers this
will be like a GET - the POST body will be lost).  I forgot other details... :-/

https://codereview.chromium.org/2902933002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:3940: // Check if the
renderer is permitted to upload the requested files.
Is there any chance the code below can be abstracted away (and shared with
ResourceDispatcherHostImpl::ShouldServiceRequest and
RenderFrameProxyHost::OnOpenURL)?

I am not sure what a good place would be for the shared function - maybe a new
instance method in ChildProcessSecurityPolicyImpl - bool CanReadBody(const
ResourceRequestBodyImpl&) ?

PS. Interestingly, just for the files, there is
ChildProcessSecurityPolicyImpl::CanReadAllFiles,
 but it doesn't handle filesystem things (but I think these don't survive inside
PageState?).

[nasko, 2017-05-24 20:55:16]

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

[commit-bot: I haz the power, 2017-05-24 20:56:01]

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-24 21:00:05]

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

[commit-bot: I haz the power, 2017-05-24 21:00:06]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[nasko, 2017-05-24 21:16:33]

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

[commit-bot: I haz the power, 2017-05-24 21:17:05]

Dry run: CQ is trying da patch.

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

[ncarter (slow), 2017-05-24 21:24:38]

https://codereview.chromium.org/2902933002/diff/60001/content/browser/browser...
File content/browser/browser_side_navigation_browsertest.cc (right):

https://codereview.chromium.org/2902933002/diff/60001/content/browser/browser...
content/browser/browser_side_navigation_browsertest.cc:330: //
security_exploit_browsertest.cc, so move it there once PlzNavigate is on
If we're no longer using pwnmessagerecieved, do we still want to move this to
security_exploit_browsertest?

https://codereview.chromium.org/2902933002/diff/60001/content/browser/browser...
content/browser/browser_side_navigation_browsertest.cc:365: // should be
terminanted.
"terminanted" typo

https://codereview.chromium.org/2902933002/diff/60001/content/browser/browser...
content/browser/browser_side_navigation_browsertest.cc:373:
ExecuteScript(shell(), "document.getElementById('file-form').submit();"));
Possibly paranoid: could the wait and EXPECT_TRUE here be a source of flakiness?

In particular, it's not clear to me why the FrameHostMsg_BeginNavigation IPC
(which causes the process kill) would necessarily arrive after the
FrameHostMsg_DomOperationResponse ?

because, in js, the submit() call seems to happen before the send() call.

[nasko, 2017-05-24 21:43:58]

https://codereview.chromium.org/2902933002/diff/60001/content/browser/browser...
File content/browser/browser_side_navigation_browsertest.cc (right):

https://codereview.chromium.org/2902933002/diff/60001/content/browser/browser...
content/browser/browser_side_navigation_browsertest.cc:330: //
security_exploit_browsertest.cc, so move it there once PlzNavigate is on
On 2017/05/24 21:24:38, ncarter (slow) wrote:
> If we're no longer using pwnmessagerecieved, do we still want to move this to
> security_exploit_browsertest?

Yeah, no need to move. Removed.

https://codereview.chromium.org/2902933002/diff/60001/content/browser/browser...
content/browser/browser_side_navigation_browsertest.cc:365: // should be
terminanted.
On 2017/05/24 21:24:38, ncarter (slow) wrote:
> "terminanted" typo

Done.

https://codereview.chromium.org/2902933002/diff/60001/content/browser/browser...
content/browser/browser_side_navigation_browsertest.cc:373:
ExecuteScript(shell(), "document.getElementById('file-form').submit();"));
On 2017/05/24 21:24:38, ncarter (slow) wrote:
> Possibly paranoid: could the wait and EXPECT_TRUE here be a source of
flakiness?
> 
> In particular, it's not clear to me why the FrameHostMsg_BeginNavigation IPC
> (which causes the process kill) would necessarily arrive after the
> FrameHostMsg_DomOperationResponse ?
> 
> because, in js, the submit() call seems to happen before the send() call.

Rewritten to ensure response is sent back before the submit() happens.

[nasko, 2017-05-24 21:44:20]

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

[commit-bot: I haz the power, 2017-05-24 21:45:19]

Dry run: CQ is trying da patch.

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

[ncarter (slow), 2017-05-24 21:49:18]

lgtm

[nasko, 2017-05-24 21:52:03]

The CQ bit was unchecked by nasko@chromium.org

[nasko, 2017-05-24 22:10:12]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2017-05-24 22:11:10]

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-24 23:43:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495663812064850,
"parent_rev": "e5a1d494ad6984b2a9ebed2100d97c703166a9c8", "commit_rev":
"5d30e832a1ce833818a76134b6fdd62238ec82c6"}

[commit-bot: I haz the power, 2017-05-24 23:43:20]

Description was changed from

==========
Verify all files in the request body are accessible by the renderer process.

This CL adds verification to RenderFrameHostImpl::OnBeginNavigation to
ensure that any files referenced in the request body have been properly
granted access to the renderer process. If the validation fails, the
process is terminated.

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

to

==========
Verify all files in the request body are accessible by the renderer process.

This CL adds verification to RenderFrameHostImpl::OnBeginNavigation to
ensure that any files referenced in the request body have been properly
granted access to the renderer process. If the validation fails, the
process is terminated.

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

Review-Url: https://codereview.chromium.org/2902933002
Cr-Commit-Position: refs/heads/master@{#474478}
Committed:
https://chromium.googlesource.com/chromium/src/+/5d30e832a1ce833818a76134b6fd...
==========

[commit-bot: I haz the power, 2017-05-24 23:43:21]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/5d30e832a1ce833818a76134b6fd...