Issue metadata
Sign in to add a comment
|
Compromised renderer can upload arbitrary files |
||||||||||||||||||||||
Issue descriptionREPRO STEPS: 1. Navigate to a page that has A) an OOPIF B) a form that i) targets the OOPIF above and ii) has a file field. 2. Fill the file field without a user permission / without going through the file dialog - This requires a compromised renderer process - A browser test below simulates this by filling out the file field and then revoking file access to the target process. 3. Submit the form. EXPECTED BEHAVIOR: POST body coming from the renderer is validated. ACTUAL BEHAVIOR: Browser grants (*) file access permissions to the OOPIF, without first checking if the originator of the request has access. OTHER NOTES: The bug was most likely introduced in r400442 (initially landed in 53.0.2771.0), which added support for passing POST data in OpenURL IPC and enabled http/tests/navigation/form-targets-cross-site-frame-post.html layout test. As far as I know the bug repros with and without PlzNavigate. I verified presence of the bug with a browser test included at the end of the bug report (**). Kudos to nasko@ for finding the bug when fixing a related PlzNavigate bug (in https://codereview.chromium.org/2902933002). (*) Callstack granting file access: content::ChildProcessSecurityPolicyImpl::SecurityState::GrantPermissionsForFile() content::ChildProcessSecurityPolicyImpl::GrantPermissionsForFile() content::RenderFrameHostImpl::GrantFileAccessFromResourceRequestBody() content::RenderFrameHostImpl::UpdatePermissionsForNavigation() content::RenderFrameHostImpl::Navigate() content::NavigatorImpl::NavigateToEntry() content::NavigatorImpl::RequestTransferURL() content::RenderFrameProxyHost::OnOpenURL() ... (**) Browser test showing the bug: // Test that verifies that if navigation originator doesn't have access to a // file, then no access is granted after a cross-process transfer of POST data. // // This test is somewhat similar to // http/tests/navigation/form-targets-cross-site-frame-post.html layout test // except that it 1) tests with files, 2) simulates a malicious scenario and 3) // verifies file acecss (all of these 3 things are not possible with layout // tests). // // This test is very similar to CrossSiteTransferTest.PostWithFileData above, // except that it simulates a malicious form / POST originator. IN_PROC_BROWSER_TEST_P(CrossSiteTransferTest, MaliciousPostWithFileData) { // Navigate to the page with form that posts via 307 redirection to // |redirect_target_url| (cross-site from |form_url|). Using 307 (rather than // 302) redirection is important to preserve the HTTP method and POST body. GURL form_url(embedded_test_server()->GetURL( "main.com", "/frame_tree/form_that_posts_to_cross_site_frame.html")); EXPECT_TRUE(NavigateToURL(shell(), form_url)); // Verify the current location and proces placement of the subframe. GURL initial_subframe_url(embedded_test_server()->GetURL( "frame.com", "/title1.html")); ASSERT_EQ(2u, shell()->web_contents()->GetAllFrames().size()); RenderFrameHost* main_frame = shell()->web_contents()->GetMainFrame(); RenderFrameHost* target_frame = shell()->web_contents()->GetAllFrames()[1]; EXPECT_NE(shell()->web_contents()->GetMainFrame(), target_frame); EXPECT_EQ(initial_subframe_url, target_frame->GetLastCommittedURL()); EXPECT_NE(main_frame->GetProcess()->GetID(), target_frame->GetProcess()->GetID()); // Prepare a file to upload. base::ThreadRestrictions::ScopedAllowIO allow_io_for_temp_dir; base::ScopedTempDir temp_dir; base::FilePath file_path; std::string file_content("test-file-content"); ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); ASSERT_TRUE(base::CreateTemporaryFileInDir(temp_dir.GetPath(), &file_path)); ASSERT_LT( 0, base::WriteFile(file_path, file_content.data(), file_content.size())); // Fill out the form to refer to the test file. std::unique_ptr<FileChooserDelegate> delegate( new FileChooserDelegate(file_path)); shell()->web_contents()->SetDelegate(delegate.get()); EXPECT_TRUE(ExecuteScript(shell()->web_contents(), "document.getElementById('file').click();")); EXPECT_TRUE(delegate->file_chosen()); // Simulate a malicious situation, where the renderer doesn't really have // access to the file. ChildProcessSecurityPolicyImpl* security_policy = ChildProcessSecurityPolicyImpl::GetInstance(); security_policy->RevokeAllPermissionsForFile(main_frame->GetProcess()->GetID(), file_path); EXPECT_FALSE(security_policy->CanReadFile(main_frame->GetProcess()->GetID(), file_path)); EXPECT_FALSE(security_policy->CanReadFile(target_frame->GetProcess()->GetID(), file_path)); // Submit the form. // TODO(lukasza): DO NOT SUBMIT: After fixing the security bug verify the // originator's process got killed (by replacing TestNavigationObserver below // with ProcessWatcher). TestNavigationObserver form_post_observer(shell()->web_contents(), 1); EXPECT_TRUE( ExecuteScript(shell(), "document.getElementById('file-form').submit();")); form_post_observer.Wait(); // The target frame should still be at the original location - the malicious // navigation should have been stopped. ASSERT_EQ(2u, shell()->web_contents()->GetAllFrames().size()); target_frame = shell()->web_contents()->GetAllFrames()[1]; EXPECT_EQ(initial_subframe_url, target_frame->GetLastCommittedURL()); // Both processes still shouldn't have access. EXPECT_FALSE(security_policy->CanReadFile(main_frame->GetProcess()->GetID(), file_path)); EXPECT_FALSE(security_policy->CanReadFile(target_frame->GetProcess()->GetID(), file_path)); // TODO(lukasza): DO NOT SUBMIT: The code below is used to verify that the // security bug is present - remove it after fixing the security bug. // Verify that POST body got preserved by 307 redirect. This expectation // comes from: https://tools.ietf.org/html/rfc7231#section-6.4.7 std::string actual_page_body; EXPECT_TRUE(ExecuteScriptAndExtractString( target_frame, "window.domAutomationController.send(" "document.getElementsByTagName('pre')[0].innerText);", &actual_page_body)); LOG(ERROR) << "actual_page_body=" << actual_page_body; }
,
May 24 2017
Awesome find. Is an OOPIF required, or could a page target any cross-process window?
,
May 24 2017
,
May 24 2017
I don't think we need OOPIFs necessarily. The but exists because the target of the form submission is a RemoteFrame. As such, I think this can be easily pulled of by doing a window.open(), which navigates cross-process.
,
May 24 2017
That might be worth verifying also, it significantly affects the severity of this bug. If it only works on OOPIFs, then the pre-requisite is a compromised extension process, which is a higher bar than a compromised renderer process. I'm marking this as Sev-Medium because I have doubts about whether it works in the window.open() case -- I don't think a window reference is supposed to be a valid form submission target. If I'm wrong this would be Sev-High.
,
May 24 2017
,
May 24 2017
,
May 24 2017
AFAICT, to repro the issue, an attacker needs to control a renderer process that contains a RenderFrameProxy. In this case, the attacker can send FrameHostMsg_OpenURL IPC to RenderFrameProxyHost and ask the browser to navigate the proxy to an attacker controlled URL, while including in the POST body any local file chosen by the attacker. I don't think there are any other conditions needed for the attack to succeed - just the presence of a RenderFrameProxy in a compromised renderer is sufficient. In particular, the currently committed location of the RenderFrameProxy doesn't matter, because 1) the attacker controls the form's action URL - the posted data will reach the attacker even if the proxy is originally at an unrelated URL 2) the attack doesn't require a process transfer (i.e. as seen in comment #0, the file access is granted before any potential process transfer happens later). It seems that with --isolate-extensions many renderers might contain a RenderFrameProxy without requiring any specific steps by the attacker. This probably means we should increase the severity of this bug. FWIW, I've verified that the issue repros without subframes / via window.open, but I've run this test with --site-per-process so I am not 100% sure if an attacker can make sure the new window is cross-process in the default Chrome. OTOH, I hear that this might be possible by navigating the opened window to google.com (or other search provider).
,
May 24 2017
This can be done without any extension involvement. From evil.com, do: w = window.open("https://www.google.com"); // Default search provider == process swap w.location.href = "http://www.evil2.com"; // Different process than evil.com. Then target a form at window w with files you don't have access to. Raising severity to High as a result.
,
May 25 2017
I agree, Severity-High sounds right. Thanks for the explanation.
,
May 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4ec2e7573759e15f79a06198dd2cca6d72f6b7a4 commit 4ec2e7573759e15f79a06198dd2cca6d72f6b7a4 Author: lukasza <lukasza@chromium.org> Date: Fri May 26 23:18:10 2017 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} [modify] https://crrev.com/4ec2e7573759e15f79a06198dd2cca6d72f6b7a4/content/browser/bad_message.h [modify] https://crrev.com/4ec2e7573759e15f79a06198dd2cca6d72f6b7a4/content/browser/child_process_security_policy_impl.cc [modify] https://crrev.com/4ec2e7573759e15f79a06198dd2cca6d72f6b7a4/content/browser/child_process_security_policy_impl.h [modify] https://crrev.com/4ec2e7573759e15f79a06198dd2cca6d72f6b7a4/content/browser/cross_site_transfer_browsertest.cc [modify] https://crrev.com/4ec2e7573759e15f79a06198dd2cca6d72f6b7a4/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/4ec2e7573759e15f79a06198dd2cca6d72f6b7a4/content/browser/frame_host/render_frame_proxy_host.cc [modify] https://crrev.com/4ec2e7573759e15f79a06198dd2cca6d72f6b7a4/content/browser/security_exploit_browsertest.cc [modify] https://crrev.com/4ec2e7573759e15f79a06198dd2cca6d72f6b7a4/tools/metrics/histograms/enums.xml
,
May 30 2017
The fix from #c11 initially landed in 61.0.3113.0. I've looked at the crash data and AFAICT there are no RF[P]H_ILLEGAL_UPLOAD_PARAMS kills in the wild.
,
May 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5e8bef0e04a637592c3443eb58a32091449d987a commit 5e8bef0e04a637592c3443eb58a32091449d987a Author: lukasza <lukasza@chromium.org> Date: Tue May 30 23:41:45 2017 Deduplicating CanReadRequestBody code. The same validation of ResourceRequestBody needs to be done from - RenderFrameHostImpl::OnBeginNavigation (renderer-initiated navigations via PlzNavigate) - RenderFrameProxyHost::OnOpenURL (renderer-initiated navigations that navigate a remote frame; this is used with and without PlzNavigate). - ResourceDispatcherHostImpl::ShouldServiceRequest (before PlzNavigate this applies to navigations; after PlzNavigate this will only be used for XHRs) BUG= 726067 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2905763002 Cr-Commit-Position: refs/heads/master@{#475693} [modify] https://crrev.com/5e8bef0e04a637592c3443eb58a32091449d987a/content/browser/child_process_security_policy_impl.h [modify] https://crrev.com/5e8bef0e04a637592c3443eb58a32091449d987a/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/5e8bef0e04a637592c3443eb58a32091449d987a/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/5e8bef0e04a637592c3443eb58a32091449d987a/content/browser/loader/resource_dispatcher_host_impl.cc
,
May 30 2017
Requesting a merge of the fix from #c11 to M60
,
May 31 2017
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 31 2017
Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 1 2017
,
Jun 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4d785e9281d5bcf83b730c6cb2c3ddabeac2910e commit 4d785e9281d5bcf83b730c6cb2c3ddabeac2910e Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Thu Jun 01 16:14:12 2017 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-Original-Commit-Position: refs/heads/master@{#475179} Review-Url: https://codereview.chromium.org/2920833003 . Cr-Commit-Position: refs/branch-heads/3112@{#91} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/4d785e9281d5bcf83b730c6cb2c3ddabeac2910e/content/browser/bad_message.h [modify] https://crrev.com/4d785e9281d5bcf83b730c6cb2c3ddabeac2910e/content/browser/child_process_security_policy_impl.cc [modify] https://crrev.com/4d785e9281d5bcf83b730c6cb2c3ddabeac2910e/content/browser/child_process_security_policy_impl.h [modify] https://crrev.com/4d785e9281d5bcf83b730c6cb2c3ddabeac2910e/content/browser/cross_site_transfer_browsertest.cc [modify] https://crrev.com/4d785e9281d5bcf83b730c6cb2c3ddabeac2910e/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/4d785e9281d5bcf83b730c6cb2c3ddabeac2910e/content/browser/frame_host/render_frame_proxy_host.cc [modify] https://crrev.com/4d785e9281d5bcf83b730c6cb2c3ddabeac2910e/content/browser/security_exploit_browsertest.cc [modify] https://crrev.com/4d785e9281d5bcf83b730c6cb2c3ddabeac2910e/tools/metrics/histograms/enums.xml
,
Jun 5 2017
,
Jun 6 2017
Security sheriffs: Any advise on the merge schedule? The merge from #c18 ended up in 60.0.3112.12 (currently on the dev channel, soon to be promoted to beta). I don't see any crash reports with magic signature containing either "kill 170" or "kill 172". At the same time, I am not sure if I should request a merge to M59 at this time, given that the fix from #c18 didn't yet have bake time on the Beta channel (and so the merge *today* would go straight from Dev to soon-to-be-Stable).
,
Jun 6 2017
Adding awhalley@ and abdulsyed@ to recommend a timeline for merging to M59, per comment 20.
,
Jun 6 2017
There are no imminent respins of M59 I'm aware of, so we can re-visit after #18 has baked a bit.
,
Jun 12 2017
,
Jun 16 2017
I still don't see kills for 172 in crash (*) and the only kill (**) for 170 comes from PlzNavigate-specific part of code (so - unaffected by the fix in #c11). So - given severity of this bug as well as plenty of bake time that the merge from #c18 got on M60, I think we should merge the fix to M59. (*) https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.magic_signature_1.name%20CONTAINS%20%27kill%20170%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D and https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.magic_signature_1.name%20CONTAINS%20%27kill%20172%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D (**) https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.magic_signature_1.name%20CONTAINS%20%27kill%20170%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D&stbtiq=&reportid=3833e1c198000000&index=0
,
Jun 16 2017
,
Jun 16 2017
The fix is much larger than I'd like to take right now - we have a bunch of major product issues that are already causing us to take a lot more change than we should in a post stable release, and I really don't want to add additional complexity. Given that it was internally disclosed, only sev sec high, and the bug has existed since M53, I don't think we *have* to take it - I'll let awhalley@ and asymmetric@ override me if they want. Happy to discuss further if you want, ping me.
,
Jul 24 2017
,
Sep 6 2017
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by lukasza@chromium.org
, May 24 2017