Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2908433003:
RenderFrameProxyHost::OnOpenURL needs to validate resource request body. (Closed)
|
Created:
3 years, 7 months ago by Łukasz Anforowicz Modified:
3 years, 7 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, creis+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFrameHostMsg_OpenURL_Params.resource_request_body needs to be validated.
FrameHostMsg_OpenURL_Params comes from an untrusted source and needs to
be validated to check whether the sender of the IPC really has access to
the contents of the resource request body.
BUG=726067
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2908433003
Cr-Commit-Position: refs/heads/master@{#475179}
Committed: https://chromium.googlesource.com/chromium/src/+/4ec2e7573759e15f79a06198dd2cca6d72f6b7a4
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Rebasing... #Patch Set 4 : . #
Total comments: 6
Patch Set 5 : Covering RenderFrameHostImpl as well and rearranging the test to pass on Android. #
Total comments: 15
Patch Set 6 : Fixed a typo in a comment #Patch Set 7 : Addressed CR comments from alexmos@ #Patch Set 8 : Making CanReadRequestBody an instance method of ChildProcessSecurityPolicyImpl. #Patch Set 9 : Rebasing... #Dependent Patchsets: Messages
Total messages: 46 (36 generated)
Description was changed from ========== RenderFrameProxyHost::OnOpenURL needs to validate resource request body. RenderFrameProxyHost::OnOpenURL needs to validate whether the sender of FrameHostMsg_OpenURL really has access to the contents of the resource request body. BUG=726067 ========== to ========== RenderFrameProxyHost::OnOpenURL needs to validate resource request body. RenderFrameProxyHost::OnOpenURL needs to validate whether the sender of FrameHostMsg_OpenURL really has access to the contents of the resource request body. BUG=726067 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) 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_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lukasza@chromium.org changed reviewers: + alexmos@chromium.org
alexmos@, can you PTAL? https://codereview.chromium.org/2908433003/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_proxy_host.cc (right): https://codereview.chromium.org/2908433003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_proxy_host.cc:43: // ResourceDispatcherHostImpl::ShouldServiceRequest. I've duplicated the code, so that this CL is (hopefully) easier to merge upstream. I am trying to deduplicate in a follow-up CL @ https://codereview.chromium.org/2905763002, but this will need some discussion about content/browser/loader/DEPS
Thanks for fixing this! One question below, and I only looked at the test on a high level for now (and it looks good) since it might change to address the Android failure. https://codereview.chromium.org/2908433003/diff/60001/content/browser/cross_s... File content/browser/cross_site_transfer_browsertest.cc (right): https://codereview.chromium.org/2908433003/diff/60001/content/browser/cross_s... content/browser/cross_site_transfer_browsertest.cc:525: // verifies file acecss (all of these 3 things are not possible with layout nit: access https://codereview.chromium.org/2908433003/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_proxy_host.cc (right): https://codereview.chromium.org/2908433003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_proxy_host.cc:59: switch (element.type()) { Thanks for being thorough and investigating all the element types! https://codereview.chromium.org/2908433003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_proxy_host.cc:318: if (!CanReadRequestBody(GetSiteInstance(), params.resource_request_body)) { Sanity check: is the same check needed in RenderFrameHostImpl::OnOpenURL? I think that path also eventually hits NavigateToEntry -> Navigate -> UpdatePermissionsForNavigation and grants file access. Or is that validated somewhere else on that path? The validation in RenderFrameHostImpl::ValidateUploadParams() is only called by RenderFrameHostImpl::OnBeginNavigation, which is only for PlzNavigate.
Description was changed from ========== RenderFrameProxyHost::OnOpenURL needs to validate resource request body. RenderFrameProxyHost::OnOpenURL needs to validate whether the sender of FrameHostMsg_OpenURL really has access to the contents of the resource request body. BUG=726067 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== FrameHostMsg_OpenURL_Params.resource_request_body needs to be validated. FrameHostMsg_OpenURL_Params comes from an untrusted source and needs to be validated to check whether the sender of the IPC really has access to the contents of the resource request body. BUG=726067 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Alex, can you take another look please? https://codereview.chromium.org/2908433003/diff/60001/content/browser/cross_s... File content/browser/cross_site_transfer_browsertest.cc (right): https://codereview.chromium.org/2908433003/diff/60001/content/browser/cross_s... content/browser/cross_site_transfer_browsertest.cc:525: // verifies file acecss (all of these 3 things are not possible with layout On 2017/05/25 18:17:33, alexmos wrote: > nit: access Done. https://codereview.chromium.org/2908433003/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_proxy_host.cc (right): https://codereview.chromium.org/2908433003/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_proxy_host.cc:318: if (!CanReadRequestBody(GetSiteInstance(), params.resource_request_body)) { On 2017/05/25 18:17:34, alexmos wrote: > Sanity check: is the same check needed in RenderFrameHostImpl::OnOpenURL? I > think that path also eventually hits NavigateToEntry -> Navigate -> > UpdatePermissionsForNavigation and grants file access. Or is that validated > somewhere else on that path? The validation in > RenderFrameHostImpl::ValidateUploadParams() is only called by > RenderFrameHostImpl::OnBeginNavigation, which is only for PlzNavigate. Thanks for catching this. Done. https://codereview.chromium.org/2908433003/diff/80001/content/browser/cross_s... File content/browser/cross_site_transfer_browsertest.cc (right): https://codereview.chromium.org/2908433003/diff/80001/content/browser/cross_s... content/browser/cross_site_transfer_browsertest.cc:544: "', 'form-window');")); I've rearranged the order of windows - now the window.open-ed window (the one that shows "on top" in Android) contains the form - this allows the test to select a file into the form. I've also considered testing with an OOPIF, but in the original OOPIF-based test the subframe is lost when the parent frame is killed (making it difficult to verify "the target frame should still be at the original location"). Hmmm... not that I think about it more, I guess this can be fixed by also putting the form into another subframe. At any rate, I think the current, window.open-based test is also okay. https://codereview.chromium.org/2908433003/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2908433003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1227: bad_message::RFH_ILLEGAL_UPLOAD_PARAMS); This reuses the bad_message flavour added yesterday for OnBeginNavigation. I think the reuse should be fine? https://codereview.chromium.org/2908433003/diff/80001/content/browser/resourc... File content/browser/resource_request_body_browser_utils.h (right): https://codereview.chromium.org/2908433003/diff/80001/content/browser/resourc... content/browser/resource_request_body_browser_utils.h:20: const scoped_refptr<ResourceRequestBodyImpl>& body); Does this look okay? I had some comments about the file/dir for this new function in https://codereview.chromium.org/2905763002/diff/20001/content/browser/resourc... mmenke@ had some (optional) suggestions about possibly putting the new function into a static-only class or namespace. I'll rebase https://codereview.chromium.org/2905763002 in a minute.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! LGTM % nits and a couple of thoughts below. https://codereview.chromium.org/2908433003/diff/80001/content/browser/cross_s... File content/browser/cross_site_transfer_browsertest.cc (right): https://codereview.chromium.org/2908433003/diff/80001/content/browser/cross_s... content/browser/cross_site_transfer_browsertest.cc:543: EXPECT_TRUE(ExecuteScript(target_contents, "window.open('" + form_url.spec() + optional: There's a OpenPopup in content_browser_test_utils_internal.h which could simplify this. https://codereview.chromium.org/2908433003/diff/80001/content/browser/cross_s... content/browser/cross_site_transfer_browsertest.cc:575: EXPECT_TRUE(delegate->file_chosen()); Might as well sanity-check that at this point, EXPECT_TRUE(security_policy->CanReadFile(form_contents->GetMainFrame()->GetProcess()->GetID(), file_path)); https://codereview.chromium.org/2908433003/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2908433003/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1227: bad_message::RFH_ILLEGAL_UPLOAD_PARAMS); On 2017/05/25 19:56:01, Łukasz A. wrote: > This reuses the bad_message flavour added yesterday for OnBeginNavigation. I > think the reuse should be fine? I think so. If we ever get a problem with a lot of these kills, we'll still get DWOC reports from ShutdownForBadMessage, which we can use to tell which variety is triggering them. https://codereview.chromium.org/2908433003/diff/80001/content/browser/resourc... File content/browser/resource_request_body_browser_utils.cc (right): https://codereview.chromium.org/2908433003/diff/80001/content/browser/resourc... content/browser/resource_request_body_browser_utils.cc:20: const scoped_refptr<ResourceRequestBodyImpl>& body) { This one looks like a ChildProcessSecurityPolicyImpl kind of method - any thoughts about putting it there? Looks like we've already got similar helpers, e.g. ChildProcessSecurityPolicyImpl::CanReadAllFiles(). https://codereview.chromium.org/2908433003/diff/80001/content/browser/resourc... File content/browser/resource_request_body_browser_utils.h (right): https://codereview.chromium.org/2908433003/diff/80001/content/browser/resourc... content/browser/resource_request_body_browser_utils.h:15: // Checks if |site_instance| can read all elements of |body|. nit: Maybe elaborate a little bit? I.e., validate that the renderer process for |site_instance| is allowed to access files in the POST upload data specified by |body|. Also, probably more for your other CL, but maybe also mention what can be called on what thread (since you'll need the deduplicated helper on both UI and IO threads, though this one takes a SiteInstance so should be UI thread). https://codereview.chromium.org/2908433003/diff/80001/content/browser/resourc... content/browser/resource_request_body_browser_utils.h:20: const scoped_refptr<ResourceRequestBodyImpl>& body); On 2017/05/25 19:56:01, Łukasz A. wrote: > Does this look okay? > > I had some comments about the file/dir for this new function in > https://codereview.chromium.org/2905763002/diff/20001/content/browser/resourc... > > mmenke@ had some (optional) suggestions about possibly putting the new function > into a static-only class or namespace. > > I'll rebase https://codereview.chromium.org/2905763002 in a minute. Hmm, this might be ok. A static-only class might make it easier to discover, but I think it'd make more sense if you had multiple of these helpers that it could help group together. But maybe also run this by nick@?
Thanks Alex. https://codereview.chromium.org/2908433003/diff/80001/content/browser/cross_s... File content/browser/cross_site_transfer_browsertest.cc (right): https://codereview.chromium.org/2908433003/diff/80001/content/browser/cross_s... content/browser/cross_site_transfer_browsertest.cc:543: EXPECT_TRUE(ExecuteScript(target_contents, "window.open('" + form_url.spec() + On 2017/05/25 23:44:05, alexmos wrote: > optional: There's a OpenPopup in content_browser_test_utils_internal.h which > could simplify this. Done. https://codereview.chromium.org/2908433003/diff/80001/content/browser/cross_s... content/browser/cross_site_transfer_browsertest.cc:575: EXPECT_TRUE(delegate->file_chosen()); On 2017/05/25 23:44:06, alexmos wrote: > Might as well sanity-check that at this point, > EXPECT_TRUE(security_policy->CanReadFile(form_contents->GetMainFrame()->GetProcess()->GetID(), > file_path)); Done. https://codereview.chromium.org/2908433003/diff/80001/content/browser/resourc... File content/browser/resource_request_body_browser_utils.cc (right): https://codereview.chromium.org/2908433003/diff/80001/content/browser/resourc... content/browser/resource_request_body_browser_utils.cc:20: const scoped_refptr<ResourceRequestBodyImpl>& body) { On 2017/05/25 23:44:06, alexmos wrote: > This one looks like a ChildProcessSecurityPolicyImpl kind of method - any > thoughts about putting it there? Looks like we've already got similar helpers, > e.g. ChildProcessSecurityPolicyImpl::CanReadAllFiles(). nick@: Any concerns with making CanReadRequestBody an instance method of ChildProcessSecurityPolicyImpl? - It uses storage::FileSystemContext::CrackURL and BrowserContext::GetStoragePartition and storage::StoragePartition->GetFileSystemContext, but this might be fine, given that child_process_security_policy_impl.cc already includes things from //storage. - Some time ago (*) you were concerned about making PrepareDropDataForChildProcess an instance method of ChildProcessSecurityPolicyImpl, but this seems different. (*) https://codereview.chromium.org/2830743004/diff/40001/content/browser/child_p... https://codereview.chromium.org/2908433003/diff/80001/content/browser/resourc... File content/browser/resource_request_body_browser_utils.h (right): https://codereview.chromium.org/2908433003/diff/80001/content/browser/resourc... content/browser/resource_request_body_browser_utils.h:15: // Checks if |site_instance| can read all elements of |body|. On 2017/05/25 23:44:06, alexmos wrote: > nit: Maybe elaborate a little bit? I.e., validate that the renderer process for > |site_instance| is allowed to access files in the POST upload data specified by > |body|. Done. > > Also, probably more for your other CL, but maybe also mention what can be called > on what thread (since you'll need the deduplicated helper on both UI and IO > threads, though this one takes a SiteInstance so should be UI thread). Done. https://codereview.chromium.org/2908433003/diff/80001/content/browser/resourc... content/browser/resource_request_body_browser_utils.h:20: const scoped_refptr<ResourceRequestBodyImpl>& body); On 2017/05/25 23:44:06, alexmos wrote: > On 2017/05/25 19:56:01, Łukasz A. wrote: > > Does this look okay? > > > > I had some comments about the file/dir for this new function in > > > https://codereview.chromium.org/2905763002/diff/20001/content/browser/resourc... > > > > mmenke@ had some (optional) suggestions about possibly putting the new > function > > into a static-only class or namespace. > > > > I'll rebase https://codereview.chromium.org/2905763002 in a minute. > > Hmm, this might be ok. A static-only class might make it easier to discover, > but I think it'd make more sense if you had multiple of these helpers that it > could help group together. But maybe also run this by nick@? Ok - see https://codereview.chromium.org/2908433003/diff/80001/content/browser/resourc...
lukasza@chromium.org changed reviewers: + nick@chromium.org
nick@, could you please comment on the idea to make CanReadRequestBody an instance method of ChildProcessSecurityPolicyImpl? Initially I thought that it might be a bit like PrepareDropDataForChildProcess and should be a separate function, but I am starting to think it might be okay (and cleaner) to move CanReadRequestBody to ChildProcessSecurityPolicyImpl. WDYT? Feel free to reply inline @ https://codereview.chromium.org/2908433003/diff/80001/content/browser/resourc...
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2908433003/diff/80001/content/browser/resourc... File content/browser/resource_request_body_browser_utils.cc (right): https://codereview.chromium.org/2908433003/diff/80001/content/browser/resourc... content/browser/resource_request_body_browser_utils.cc:20: const scoped_refptr<ResourceRequestBodyImpl>& body) { On 2017/05/26 00:05:14, Łukasz A. wrote: > On 2017/05/25 23:44:06, alexmos wrote: > > This one looks like a ChildProcessSecurityPolicyImpl kind of method - any > > thoughts about putting it there? Looks like we've already got similar > helpers, > > e.g. ChildProcessSecurityPolicyImpl::CanReadAllFiles(). > > nick@: Any concerns with making CanReadRequestBody an instance method of > ChildProcessSecurityPolicyImpl? > > - It uses storage::FileSystemContext::CrackURL and > BrowserContext::GetStoragePartition and > storage::StoragePartition->GetFileSystemContext, but this might be fine, given > that child_process_security_policy_impl.cc already includes things from > //storage. > > - Some time ago (*) you were concerned about making > PrepareDropDataForChildProcess an instance method of > ChildProcessSecurityPolicyImpl, but this seems different. > > (*) > https://codereview.chromium.org/2830743004/diff/40001/content/browser/child_p... My gut sense: it doesn't totally feel like it fits in with the other methods, but it's easier to use than the alternative, so I'm okay with it. lgtm
The CQ bit was unchecked by lukasza@chromium.org
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2908433003/#ps160001 (title: "Rebasing...")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lukasza@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1495839651490190,
"parent_rev": "954e46372100531c3e780f6c64da5fd91e988f4b", "commit_rev":
"4ec2e7573759e15f79a06198dd2cca6d72f6b7a4"}
Message was sent while issue was closed.
Description was changed from ========== FrameHostMsg_OpenURL_Params.resource_request_body needs to be validated. FrameHostMsg_OpenURL_Params comes from an untrusted source and needs to be validated to check whether the sender of the IPC really has access to the contents of the resource request body. BUG=726067 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== FrameHostMsg_OpenURL_Params.resource_request_body needs to be validated. FrameHostMsg_OpenURL_Params comes from an untrusted source and needs to be validated to check whether the sender of the IPC really has access to the contents of the resource request body. BUG=726067 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2908433003 Cr-Commit-Position: refs/heads/master@{#475179} Committed: https://chromium.googlesource.com/chromium/src/+/4ec2e7573759e15f79a06198dd2c... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/4ec2e7573759e15f79a06198dd2c... |
