Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1775543002:
Validate params.origin in the browser process at commit time. (Closed)
|
Created:
4 years, 9 months ago by nasko Modified:
4 years, 9 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionValidate params.origin in the browser process at commit time.
The renderer process sends both an URL and an origin in its commit IPC
and they should match. This CL adds such validation at commit time.
BUG=559324
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/402cc291d225747dca8594602f59e139b1684d57
Cr-Commit-Position: refs/heads/master@{#380585}
Patch Set 1 #Patch Set 2 : Quite a bit of refactoring and a basic browser test. #
Total comments: 24
Patch Set 3 : Fixes based on Charlie's review. #
Total comments: 20
Patch Set 4 : Addressing another round of comments. #
Messages
Total messages: 29 (12 generated)
Description was changed from ========== Validate params.origin in the browser process at commit time. The renderer process sends both an URL and an origin in its commit IPC and they should match. This CL adds such validation at commit time. BUG=559324 ========== to ========== Validate params.origin in the browser process at commit time. The renderer process sends both an URL and an origin in its commit IPC and they should match. This CL adds such validation at commit time. BUG=559324 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
nasko@chromium.org changed reviewers: + creis@chromium.org
Hey Charlie, Can you poke at this CL for me? It lacks more tests, but the main logic in the verification code should be there. Thanks! Nasko
Cool! Looks pretty close, with a few questions below. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:991: if (!CanCommitOrigin(validated_params)) { Maybe we should put this after the CanCommitURL call, since we depend on that URL in CanCommitOrigin. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1946: if (validated_params.was_within_same_page) What made this check necessary? It doesn't look safe at first glance-- a compromised renderer could pass any origin it wanted during an in-page navigation. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1949: // It is fine to commit into unique origin, regardless of the URL, as it is nit: s/fine/safe/ nit: a unique origin https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1954: // Non-unique origins must be valid URLs. If the URL is not standard, the Let's be a little more explicit in phrasing here, since we're talking about two different URLs (the converted origin and params.url) and valid vs standard. I'm not sure how the first and second sentences relate to each other yet. (Maybe they're separate thoughts?) Maybe something like: // Non-unique origins can be converted to valid URLs. // // If the actual URL from the parameters is not standard... https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1955: // origin can be treated as URL and can be checked whether it is allowed to nit: as a URL https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1959: return CanCommitURL(GURL(validated_params.origin.Serialize())); Can we do this check for standard URLs as well? I see that we kind of transitively get it from IsSameOriginWith + CanCommitURL(params.url), though I'm not sure those are 100% equivalent. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1961: url::Origin url_origin(validated_params.url); Can you add a comment about why we don't do this check for non-standard URLs? https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:682: // RenderFrameHost. Let's add a sentence similar to the second one for CanCommitURL. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:684: const FrameHostMsg_DidCommitProvisionalLoad_Params& validated_params); nit: Probably better to just take in the parameters that we care about, to reduce the dependencies on this large struct. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl_browsertest.cc (right): https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl_browsertest.cc:166: // process. nit: No new line here. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl_browsertest.cc:167: IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest, Would it make sense to put this in SecurityExploitBrowserTest instead, especially since it's a PwnMessageReceived test? We have a few related tests of CanCommitURL there: https://codereview.chromium.org/1270663002/diff/200001/content/browser/securi... That also shows some other tests to consider, like chrome:// origins in web processes. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl_browsertest.cc:193: FrameHostMsg_DidCommitProvisionalLoad_Params params; // Create commit params with different origins in params.url and params.origin.
https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:991: if (!CanCommitOrigin(validated_params)) { On 2016/03/10 00:23:03, Charlie Reis wrote: > Maybe we should put this after the CanCommitURL call, since we depend on that > URL in CanCommitOrigin. Done. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1946: if (validated_params.was_within_same_page) On 2016/03/10 00:23:03, Charlie Reis wrote: > What made this check necessary? It doesn't look safe at first glance-- a > compromised renderer could pass any origin it wanted during an in-page > navigation. It was a left over that happened to catch a few failing cases and I forgot to remove :(. Replaced it with explicit test for --disable-web-security and universal file access. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1949: // It is fine to commit into unique origin, regardless of the URL, as it is On 2016/03/10 00:23:03, Charlie Reis wrote: > nit: s/fine/safe/ > nit: a unique origin Done. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1954: // Non-unique origins must be valid URLs. If the URL is not standard, the On 2016/03/10 00:23:03, Charlie Reis wrote: > Let's be a little more explicit in phrasing here, since we're talking about two > different URLs (the converted origin and params.url) and valid vs standard. I'm > not sure how the first and second sentences relate to each other yet. (Maybe > they're separate thoughts?) > > Maybe something like: > > // Non-unique origins can be converted to valid URLs. > // > // If the actual URL from the parameters is not standard... The bit about non-unique origins is meant to clarify why it is safe to perform the conversion and check. Reworded a bit, hopefully it is better. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1955: // origin can be treated as URL and can be checked whether it is allowed to On 2016/03/10 00:23:03, Charlie Reis wrote: > nit: as a URL Done. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1959: return CanCommitURL(GURL(validated_params.origin.Serialize())); On 2016/03/10 00:23:03, Charlie Reis wrote: > Can we do this check for standard URLs as well? I see that we kind of > transitively get it from IsSameOriginWith + CanCommitURL(params.url), though I'm > not sure those are 100% equivalent. Sure. Being on the defensive is fine with me. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1961: url::Origin url_origin(validated_params.url); On 2016/03/10 00:23:03, Charlie Reis wrote: > Can you add a comment about why we don't do this check for non-standard URLs? I added a note to the previous comment. Let me know if that makes it better or you still think it makes sense to add something here. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:682: // RenderFrameHost. On 2016/03/10 00:23:03, Charlie Reis wrote: > Let's add a sentence similar to the second one for CanCommitURL. Done. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:684: const FrameHostMsg_DidCommitProvisionalLoad_Params& validated_params); On 2016/03/10 00:23:03, Charlie Reis wrote: > nit: Probably better to just take in the parameters that we care about, to > reduce the dependencies on this large struct. Done. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl_browsertest.cc (right): https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl_browsertest.cc:166: // process. On 2016/03/10 00:23:04, Charlie Reis wrote: > nit: No new line here. Done. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl_browsertest.cc:167: IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest, On 2016/03/10 00:23:04, Charlie Reis wrote: > Would it make sense to put this in SecurityExploitBrowserTest instead, > especially since it's a PwnMessageReceived test? We have a few related tests of > CanCommitURL there: > https://codereview.chromium.org/1270663002/diff/200001/content/browser/securi... > > That also shows some other tests to consider, like chrome:// origins in web > processes. Sure. Moving it over. https://codereview.chromium.org/1775543002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl_browsertest.cc:193: FrameHostMsg_DidCommitProvisionalLoad_Params params; On 2016/03/10 00:23:04, Charlie Reis wrote: > // Create commit params with different origins in params.url and params.origin. Done.
Looking nicer! A few additional thoughts below. https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1966: // Non-unique origin must be a valid URL, which allows us to safely do a nit: A non-unique origin https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1981: return CanCommitURL(origin_url); Feels weird to call CanCommitURL twice. Maybe we can rephrase lines 1970-1981 as the following? // Standard URLs must match the reported origin. if (url.IsStandard() && !origin.IsSameOriginWith(url::Origin(url))) { return false; } // Verify that the origin is allowed to commit in this process. // Note: This also handles non-standard cases for |url|, such as // about:blank, data, and blob URLs. return CanCommitURL(origin_url); https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:684: bool CanCommitOrigin(const GURL& url, const url::Origin& origin); nit: Reverse order (since |origin| is the main thing we care about) and explain why |url| is needed as well in the comment. (Something about making sure they match?) https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:2982: kUrlBar, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string()); Maybe this should be inside the loop with test_case.url? (To the extent it matters, I don't think we're aiming to test CanCommitURL kills here.) https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:3001: { "http://a.com/", "http://a.com", false }, Maybe add something to the path of the |url|? https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:3011: // about:blank. Let's add a comment about why this isn't a kill (which is the right behavior). https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:3012: { "about:blank", "http://a.com", false }, Can we add a test to ensure that "about:blank" + "chrome://settings" results in a kill? https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:3022: int expected_bad_msg_count = process()->bad_msg_count();; nit: Remove extra semicolon. https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:3028: main_test_rfh()->OnMessageReceived(msg); Maybe use SendNavigateWithParams, as you suggested to me? It looks like we may also want to call PrepareForCommit first. (I'm not 100% sure, but that was introduced for PlzNavigate.)
https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1966: // Non-unique origin must be a valid URL, which allows us to safely do a On 2016/03/10 22:28:02, Charlie Reis wrote: > nit: A non-unique origin Done. https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:1981: return CanCommitURL(origin_url); On 2016/03/10 22:28:02, Charlie Reis wrote: > Feels weird to call CanCommitURL twice. Maybe we can rephrase lines 1970-1981 > as the following? > > // Standard URLs must match the reported origin. > if (url.IsStandard() && > !origin.IsSameOriginWith(url::Origin(url))) { > return false; > } > > // Verify that the origin is allowed to commit in this process. > // Note: This also handles non-standard cases for |url|, such as > // about:blank, data, and blob URLs. > return CanCommitURL(origin_url); Done. https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:684: bool CanCommitOrigin(const GURL& url, const url::Origin& origin); On 2016/03/10 22:28:02, Charlie Reis wrote: > nit: Reverse order (since |origin| is the main thing we care about) and explain > why |url| is needed as well in the comment. (Something about making sure they > match?) Done. https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:2982: kUrlBar, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string()); On 2016/03/10 22:28:02, Charlie Reis wrote: > Maybe this should be inside the loop with test_case.url? (To the extent it > matters, I don't think we're aiming to test CanCommitURL kills here.) I didn't do it purposefully. If I move it in the loop, starting a navigation for each test case causes cross-process navigations for some cases, which means I need to add some complexity in checking whether there is a pending RFH and using its process to count the bad_msg_count(). https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:3001: { "http://a.com/", "http://a.com", false }, On 2016/03/10 22:28:02, Charlie Reis wrote: > Maybe add something to the path of the |url|? Done. https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:3011: // about:blank. On 2016/03/10 22:28:02, Charlie Reis wrote: > Let's add a comment about why this isn't a kill (which is the right behavior). Done. https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:3012: { "about:blank", "http://a.com", false }, On 2016/03/10 22:28:02, Charlie Reis wrote: > Can we add a test to ensure that "about:blank" + "chrome://settings" results in > a kill? I don't think we have enough support for this kill quite yet. Probably will be added as we add support for checking WebUI bindings. https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:3022: int expected_bad_msg_count = process()->bad_msg_count();; On 2016/03/10 22:28:02, Charlie Reis wrote: > nit: Remove extra semicolon. Done. https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:3028: main_test_rfh()->OnMessageReceived(msg); On 2016/03/10 22:28:02, Charlie Reis wrote: > Maybe use SendNavigateWithParams, as you suggested to me? It looks like we may > also want to call PrepareForCommit first. (I'm not 100% sure, but that was > introduced for PlzNavigate.) Done.
Thanks! LGTM. https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:2982: kUrlBar, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string()); On 2016/03/11 00:19:37, nasko (slow) wrote: > On 2016/03/10 22:28:02, Charlie Reis wrote: > > Maybe this should be inside the loop with test_case.url? (To the extent it > > matters, I don't think we're aiming to test CanCommitURL kills here.) > > I didn't do it purposefully. If I move it in the loop, starting a navigation for > each test case causes cross-process navigations for some cases, which means I > need to add some complexity in checking whether there is a pending RFH and using > its process to count the bad_msg_count(). Acknowledged. https://codereview.chromium.org/1775543002/diff/40001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:3012: { "about:blank", "http://a.com", false }, On 2016/03/11 00:19:38, nasko (slow) wrote: > On 2016/03/10 22:28:02, Charlie Reis wrote: > > Can we add a test to ensure that "about:blank" + "chrome://settings" results > in > > a kill? > > I don't think we have enough support for this kill quite yet. Probably will be > added as we add support for checking WebUI bindings. Acknowledged.
The CQ bit was checked by nasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775543002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775543002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775543002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775543002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775543002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775543002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775543002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775543002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775543002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775543002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Validate params.origin in the browser process at commit time. The renderer process sends both an URL and an origin in its commit IPC and they should match. This CL adds such validation at commit time. BUG=559324 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Validate params.origin in the browser process at commit time. The renderer process sends both an URL and an origin in its commit IPC and they should match. This CL adds such validation at commit time. BUG=559324 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/402cc291d225747dca8594602f59e139b1684d57 Cr-Commit-Position: refs/heads/master@{#380585} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/402cc291d225747dca8594602f59e139b1684d57 Cr-Commit-Position: refs/heads/master@{#380585} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
