Improve trustworthiness of browser's notion of committed origin (e.g. via extra CanCommitURL/Origin checks) |
||||||||||
Issue description
Today, the browser mostly trusts the origin and URL reported in DidCommitProvisionalLoad IPC. This was a natural state of things without Site Isolation - in this old world the renderer could create a subframe with any origin and the browser had to accept it. However, the new world of Site Isolation enables additional checks in the browser process, to ensure that the browser has a trustworthy notion of the committed origin.
Some initial ideas:
1. Improve validation of the URL/origin send by the renderer in DidCommitProvisionalLoad IPC
1.1. Consult extensions::ProcessMap in most cases
(today it is only consulted for Chrome Web Store)
1.2. Verify that the origin sent by the renderer matches the pending navigation known to the browser.
The origin should be either:
*) a unique origin
*) the origin of the navigation initiator known to the browser (e.g. when navigating to about:blank)
*) the origin of the URL the browser knows we are navigating to
1.3. Reject DidCommitProvisionalLoad received while there is no pending navigation
(modulo same-document navigations)
2. Move origin calculations entirely into the browser
(this might be undesirable if it ends up duplicating some code across renderer and the browser)
,
Oct 2 2017
1.1. Seems doable - I have a WIP CL @ https://chromium-review.googlesource.com/c/chromium/src/+/683312 1.2. Seems difficult. Consider the race from NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSameDocumentHistoryNavigation which leads to the following sequence of events: - a.com/title1.html commits - a.com/title1.html#foo commits - navigation to suborigin.a.com/title2.html starts at this point RFHI::navigation_handle_.GetURL() will be suborigin.a.com/title2.html - just before the navigation to suborigin.a.com commits a back history navigation is trigerred at this point RFHI::navigation_handle_.GetURL() will be a.com/title1.html (#foo is ignored by navigation handle) - when the commit to suborigin.a.com is handled, it doesn't match a.com from the navigation handle 1.3. Seems doable - I have a WIP CL @ https://chromium-review.googlesource.com/c/chromium/src/+/692623 I think the race from 1.2. doesn't have any negative effects here. OTOH, I am not sure if this is a meaningful protection - the attacker can easily start a navigation before sending a malicious DidCommitProvisionalLoad IPC. :-/ To protect isolated origins (and future isolation policies), I think that we have to either A) figure out what to do with races from 1.2 A.1. maybe cancel navigations in case of mismatch (instead of killing the renderer)? A.2. or maybe remember both the real and the history navigation (and compare both in CanCommitOrigin / DidCommitProvisionalLoad) or B) make CanCommitOrigin aware of isolated origins (and future isolation policies)
,
Oct 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/75cffa99885f66cafd7bcdef1c10c6b83a6e7af9 commit 75cffa99885f66cafd7bcdef1c10c6b83a6e7af9 Author: Alex Moshchuk <alexmos@chromium.org> Date: Wed Oct 11 20:24:02 2017 Set crash keys to help debug ChildProcessSecurityPolicy renderer kills. Two of these crash keys are set any time the CanAccessDataForOrigin access check fails. The other two are set for all renderer kills. They should help understand classes of kills that we are currently seeing, as well as aid in understanding any kills resulting from tightening other site isolation security checks, such as CanCommitURL. This would especially help with the kills that are only seen in Dev trial and don't have minidumps. Bug: 773140, 770239 Change-Id: I11cf76f50a59d86ccae7894322bea9b307bdb433 Reviewed-on: https://chromium-review.googlesource.com/707801 Reviewed-by: Robert Sesek <rsesek@chromium.org> Reviewed-by: Jay Civelli <jcivelli@chromium.org> Reviewed-by: Luke Halliwell <halliwell@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Commit-Queue: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#508093} [modify] https://crrev.com/75cffa99885f66cafd7bcdef1c10c6b83a6e7af9/android_webview/common/crash_reporter/crash_keys.cc [modify] https://crrev.com/75cffa99885f66cafd7bcdef1c10c6b83a6e7af9/chrome/app/chrome_crash_reporter_client_win.cc [modify] https://crrev.com/75cffa99885f66cafd7bcdef1c10c6b83a6e7af9/chrome/common/crash_keys.cc [modify] https://crrev.com/75cffa99885f66cafd7bcdef1c10c6b83a6e7af9/chromecast/crash/cast_crash_keys.cc [modify] https://crrev.com/75cffa99885f66cafd7bcdef1c10c6b83a6e7af9/content/browser/child_process_security_policy_impl.cc [modify] https://crrev.com/75cffa99885f66cafd7bcdef1c10c6b83a6e7af9/content/browser/child_process_security_policy_impl.h [modify] https://crrev.com/75cffa99885f66cafd7bcdef1c10c6b83a6e7af9/content/browser/renderer_host/render_process_host_impl.cc
,
Oct 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3a7cc34f95c7a8bff01dede69940018cc1f4bdfa commit 3a7cc34f95c7a8bff01dede69940018cc1f4bdfa Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Tue Oct 31 21:33:19 2017 Extension URLs should only commit in an extension process. Since --isolate-extensions has shipped in M56, we should be able to enforce stricter CanCommitURL checks and disallow extension frames from committing outside of an extension process. This CL tightens the checks done by ChromeContentBrowserClientExtensionsPart::CanCommitURL so that extension URLs are only allowed to commit in an extension process. Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: I2c83659c5eba1a1a45c94876e1b254c52175f7b9 Bug: 770239 Reviewed-on: https://chromium-review.googlesource.com/683312 Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> Reviewed-by: Nick Carter <nick@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/heads/master@{#512959} [modify] https://crrev.com/3a7cc34f95c7a8bff01dede69940018cc1f4bdfa/chrome/browser/chromeos/accessibility/accessibility_manager.cc [modify] https://crrev.com/3a7cc34f95c7a8bff01dede69940018cc1f4bdfa/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc [modify] https://crrev.com/3a7cc34f95c7a8bff01dede69940018cc1f4bdfa/chrome/browser/extensions/process_manager_browsertest.cc [modify] https://crrev.com/3a7cc34f95c7a8bff01dede69940018cc1f4bdfa/content/browser/frame_host/navigation_request.cc [modify] https://crrev.com/3a7cc34f95c7a8bff01dede69940018cc1f4bdfa/content/browser/frame_host/render_frame_host_impl.cc
,
Nov 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a23619bf86f9324f9ef2f43cf05fa5eea976d553 commit a23619bf86f9324f9ef2f43cf05fa5eea976d553 Author: Łukasz Anforowicz <lukasza@chromium.org> Date: Fri Nov 03 18:35:18 2017 Revert "Extension URLs should only commit in an extension process." This reverts commit 3a7cc34f95c7a8bff01dede69940018cc1f4bdfa. Reason for revert: Caused https://crbug.com/780707 Original change's description: > Extension URLs should only commit in an extension process. > > Since --isolate-extensions has shipped in M56, we should be able to > enforce stricter CanCommitURL checks and disallow extension frames from > committing outside of an extension process. This CL tightens the checks > done by ChromeContentBrowserClientExtensionsPart::CanCommitURL so > that extension URLs are only allowed to commit in an extension process. > > Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation > Change-Id: I2c83659c5eba1a1a45c94876e1b254c52175f7b9 > Bug: 770239 > Reviewed-on: https://chromium-review.googlesource.com/683312 > Reviewed-by: Devlin <rdevlin.cronin@chromium.org> > Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> > Reviewed-by: Nick Carter <nick@chromium.org> > Reviewed-by: Alex Moshchuk <alexmos@chromium.org> > Reviewed-by: Charlie Reis <creis@chromium.org> > Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> > Cr-Commit-Position: refs/heads/master@{#512959} TBR=dmazzoni@chromium.org,msw@chromium.org,creis@chromium.org,nick@chromium.org,rdevlin.cronin@chromium.org,alexmos@chromium.org,lukasza@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 770239 Change-Id: I92393a58e9a3df77a321e4ef5d9c2cfbe2155640 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Reviewed-on: https://chromium-review.googlesource.com/752059 Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/heads/master@{#513848} [modify] https://crrev.com/a23619bf86f9324f9ef2f43cf05fa5eea976d553/chrome/browser/chromeos/accessibility/accessibility_manager.cc [modify] https://crrev.com/a23619bf86f9324f9ef2f43cf05fa5eea976d553/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc [modify] https://crrev.com/a23619bf86f9324f9ef2f43cf05fa5eea976d553/chrome/browser/extensions/process_manager_browsertest.cc [modify] https://crrev.com/a23619bf86f9324f9ef2f43cf05fa5eea976d553/content/browser/frame_host/navigation_request.cc [modify] https://crrev.com/a23619bf86f9324f9ef2f43cf05fa5eea976d553/content/browser/frame_host/render_frame_host_impl.cc
,
Nov 18 2017
Marking this as started. When relanding r512959, we'll need to make sure issue 780707 stays fixed as well.
,
Nov 18 2017
,
Dec 7 2017
,
Dec 12 2017
,
Dec 13 2017
,
Dec 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e53acacf151ea5eafc31282be15c7b11e0e56f22 commit e53acacf151ea5eafc31282be15c7b11e0e56f22 Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Tue Dec 26 19:02:33 2017 CanCommitOrigin tightening: should match origin lock (if present). This CL makes RenderFrameHostImpl::CanCommitOrigin check if the current renderer process has an origin lock - if yes, then the lock has to match the origin reported by the renderer in the DidCommitProvisionalLoad IPC. This CL makes additional tweaks to support LoadDataWithBaseURL. Such navigations commit with origin = <base_url argument>. Before this CL, such navigations would use site_url = data:... and in site-per-process mode would lock the renderer to the data: origin - this would lead to renderer kills after the CanCommitOrigin changes done by this CL. Therefore this CL also makes sure that LoadDataWithBaseURL navigations use site_url = <base url>. This is accomplished by taking into account the base url in NavigationRequest::OnResponseStarted and NavigationControllerImpl::Reload. This CL also tweaks unit tests, to make sure the simulated navigations are compatible with the new CanCommitOrigin tightening. Bug: 770239 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: Ie2a3adfa29d971bd32ba6d3311aed9d79f986c3d Reviewed-on: https://chromium-review.googlesource.com/769647 Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#526198} [modify] https://crrev.com/e53acacf151ea5eafc31282be15c7b11e0e56f22/content/browser/frame_host/navigation_controller_impl_browsertest.cc [modify] https://crrev.com/e53acacf151ea5eafc31282be15c7b11e0e56f22/content/browser/frame_host/navigation_controller_impl_unittest.cc [modify] https://crrev.com/e53acacf151ea5eafc31282be15c7b11e0e56f22/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/e53acacf151ea5eafc31282be15c7b11e0e56f22/content/browser/message_port_provider_browsertest.cc [modify] https://crrev.com/e53acacf151ea5eafc31282be15c7b11e0e56f22/content/browser/security_exploit_browsertest.cc [modify] https://crrev.com/e53acacf151ea5eafc31282be15c7b11e0e56f22/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/e53acacf151ea5eafc31282be15c7b11e0e56f22/content/browser/web_contents/web_contents_impl_unittest.cc
,
Dec 28 2017
commit e53acacf151ea5eafc31282be15c7b11e0e56f22 was initially in 65.0.3306.0 I see somewhat elevated levels of the following kills in 3306 (they are missing when looking at 3300-3305 [inclusive]): - [Renderer kill 114] content::RenderFrameHostImpl::DidCommitProvisionalLoad (RFH_INVALID_ORIGIN_ON_COMMIT, control: 0, site-per-process: 8, currently ranked around #8) - [Renderer kill 169] content::RenderWidgetHostImpl::SubmitCompositorFrame (RWH_SURFACE_INVARIANTS_VIOLATION, control: 0, isolate-origins: 5, site-per-process: 6, currently ranked around #10-#11) It is quite possible that the RFH_INVALID_ORIGIN_ON_COMMIT kill is related to r526198 from #c11 above. I've opened issue 797968 to investigate this in more depth.
,
Dec 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/38563bf4c5da1b11fd639c9156eb6b63c4b702a9 commit 38563bf4c5da1b11fd639c9156eb6b63c4b702a9 Author: Łukasz Anforowicz <lukasza@chromium.org> Date: Thu Dec 28 20:53:13 2017 Revert "CanCommitOrigin tightening: should match origin lock (if present)." This reverts commit e53acacf151ea5eafc31282be15c7b11e0e56f22. Reason for revert: Caused more kills - see https://crbug.com/797968 Original change's description: > CanCommitOrigin tightening: should match origin lock (if present). > > This CL makes RenderFrameHostImpl::CanCommitOrigin check if the current > renderer process has an origin lock - if yes, then the lock has to match > the origin reported by the renderer in the DidCommitProvisionalLoad IPC. > > This CL makes additional tweaks to support LoadDataWithBaseURL. Such > navigations commit with origin = <base_url argument>. Before this CL, > such navigations would use site_url = data:... and in site-per-process > mode would lock the renderer to the data: origin - this would lead to > renderer kills after the CanCommitOrigin changes done by this CL. > Therefore this CL also makes sure that LoadDataWithBaseURL navigations > use site_url = <base url>. This is accomplished by taking into account > the base url in NavigationRequest::OnResponseStarted and > NavigationControllerImpl::Reload. > > This CL also tweaks unit tests, to make sure the simulated navigations > are compatible with the new CanCommitOrigin tightening. > > Bug: 770239 > Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation > Change-Id: Ie2a3adfa29d971bd32ba6d3311aed9d79f986c3d > Reviewed-on: https://chromium-review.googlesource.com/769647 > Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> > Reviewed-by: Alex Moshchuk <alexmos@chromium.org> > Cr-Commit-Position: refs/heads/master@{#526198} TBR=alexmos@chromium.org,lukasza@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 770239 , 797968 Change-Id: I2e0142a637b9a976fd7e8ea3e46c8a0f7ba52cf2 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Reviewed-on: https://chromium-review.googlesource.com/846059 Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/heads/master@{#526324} [modify] https://crrev.com/38563bf4c5da1b11fd639c9156eb6b63c4b702a9/content/browser/frame_host/navigation_controller_impl_browsertest.cc [modify] https://crrev.com/38563bf4c5da1b11fd639c9156eb6b63c4b702a9/content/browser/frame_host/navigation_controller_impl_unittest.cc [modify] https://crrev.com/38563bf4c5da1b11fd639c9156eb6b63c4b702a9/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/38563bf4c5da1b11fd639c9156eb6b63c4b702a9/content/browser/message_port_provider_browsertest.cc [modify] https://crrev.com/38563bf4c5da1b11fd639c9156eb6b63c4b702a9/content/browser/security_exploit_browsertest.cc [modify] https://crrev.com/38563bf4c5da1b11fd639c9156eb6b63c4b702a9/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/38563bf4c5da1b11fd639c9156eb6b63c4b702a9/content/browser/web_contents/web_contents_impl_unittest.cc
,
Jan 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7359e0bedad0cc21fffd3063d1a69f4672895b48 commit 7359e0bedad0cc21fffd3063d1a69f4672895b48 Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Thu Jan 04 17:49:37 2018 Call SetActiveURL while handling IPCs on the *browser* side. Knowing the URL is very helpful not only for diagnosing renderer crashes, but also for handling browser crashes, and DwoCs (e.g. ones issued for renderer kills). This CL introduces a call to ContentClient::SetActiveURL in 4 places in the browser process: - RenderViewHostImpl::OnMessageReceived - RenderFrameProxyHost::OnMessageReceived - RenderFrameHostImpl::OnMessageReceived - RenderFrameHostImpl::DidCommitProvisionalLoad Bug: 797968, 770239 Change-Id: I78481d0fcb40e7c0548eb5ee3bd1ff00c7703592 Reviewed-on: https://chromium-review.googlesource.com/846109 Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#527031} [modify] https://crrev.com/7359e0bedad0cc21fffd3063d1a69f4672895b48/content/browser/BUILD.gn [modify] https://crrev.com/7359e0bedad0cc21fffd3063d1a69f4672895b48/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/7359e0bedad0cc21fffd3063d1a69f4672895b48/content/browser/frame_host/render_frame_proxy_host.cc [modify] https://crrev.com/7359e0bedad0cc21fffd3063d1a69f4672895b48/content/browser/renderer_host/render_view_host_impl.cc [add] https://crrev.com/7359e0bedad0cc21fffd3063d1a69f4672895b48/content/browser/scoped_active_url.cc [add] https://crrev.com/7359e0bedad0cc21fffd3063d1a69f4672895b48/content/browser/scoped_active_url.h
,
Jan 6 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3d58b9396cebb451f88f432796afe88695c7ed6f commit 3d58b9396cebb451f88f432796afe88695c7ed6f Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Sat Jan 06 00:37:17 2018 Preserve |origin| in RFH_INVALID_ORIGIN_ON_COMMIT kill dumps. This CL makes sure that the origin responsible for the RFH_INVALID_ORIGIN_ON_COMMIT kill is copied into a stack-allocated char array (and therefore preserved in the DwoC dumps associated with the kill). Bug: 770239 , 797968 Change-Id: I062f500c85d8b5935a7f85c36e0ec4675b2f7132 Reviewed-on: https://chromium-review.googlesource.com/846259 Reviewed-by: Brett Wilson <brettw@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/heads/master@{#527458} [modify] https://crrev.com/3d58b9396cebb451f88f432796afe88695c7ed6f/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/3d58b9396cebb451f88f432796afe88695c7ed6f/url/origin.h [modify] https://crrev.com/3d58b9396cebb451f88f432796afe88695c7ed6f/url/origin_unittest.cc
,
Jan 11 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/931f616caaad085f34e0d8df0b77789e3f9c8146 commit 931f616caaad085f34e0d8df0b77789e3f9c8146 Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Thu Jan 11 01:14:15 2018 [reland] CanCommitOrigin tightening: match origin lock (if present). This CL makes RenderFrameHostImpl::CanCommitOrigin check if the current renderer process has an origin lock - if yes, then the lock has to match the origin reported by the renderer in the DidCommitProvisionalLoad IPC. This CL makes additional tweaks to support LoadDataWithBaseURL. Such navigations commit with origin = <base_url argument>. Before this CL, such navigations would use site_url = data:... and in site-per-process mode would lock the renderer to the data: origin - this would lead to renderer kills after the CanCommitOrigin changes done by this CL. Therefore this CL also makes sure that LoadDataWithBaseURL navigations use site_url = <base url>. This is accomplished by taking into account the base url in NavigationRequest::OnResponseStarted and NavigationControllerImpl::Reload. This CL also tweaks unit tests, to make sure the simulated navigations are compatible with the new CanCommitOrigin tightening. This is a reland of r526198, after adding extra logging of requested_site_url from CanCommitOrigin. Bug: 770239 , 797968 Change-Id: I5783a3e32e6313fa5db5c88f55ebdc389843afc0 Reviewed-on: https://chromium-review.googlesource.com/846342 Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/heads/master@{#528514} [modify] https://crrev.com/931f616caaad085f34e0d8df0b77789e3f9c8146/content/browser/frame_host/navigation_controller_impl_browsertest.cc [modify] https://crrev.com/931f616caaad085f34e0d8df0b77789e3f9c8146/content/browser/frame_host/navigation_controller_impl_unittest.cc [modify] https://crrev.com/931f616caaad085f34e0d8df0b77789e3f9c8146/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/931f616caaad085f34e0d8df0b77789e3f9c8146/content/browser/message_port_provider_browsertest.cc [modify] https://crrev.com/931f616caaad085f34e0d8df0b77789e3f9c8146/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/931f616caaad085f34e0d8df0b77789e3f9c8146/content/browser/web_contents/web_contents_impl_unittest.cc
,
Jan 11 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d8ec893735d0bc1424f0f16a8efcf4c158a8fa43 commit d8ec893735d0bc1424f0f16a8efcf4c158a8fa43 Author: Łukasz Anforowicz <lukasza@chromium.org> Date: Thu Jan 11 23:06:07 2018 Revert "[reland] CanCommitOrigin tightening: match origin lock (if present)." This reverts commit 931f616caaad085f34e0d8df0b77789e3f9c8146. Reason for revert: We've gathered 20 crash reports - let's investigate them before relanding again. Original change's description: > [reland] CanCommitOrigin tightening: match origin lock (if present). > > This CL makes RenderFrameHostImpl::CanCommitOrigin check if the current > renderer process has an origin lock - if yes, then the lock has to match > the origin reported by the renderer in the DidCommitProvisionalLoad IPC. > > This CL makes additional tweaks to support LoadDataWithBaseURL. Such > navigations commit with origin = <base_url argument>. Before this CL, > such navigations would use site_url = data:... and in site-per-process > mode would lock the renderer to the data: origin - this would lead to > renderer kills after the CanCommitOrigin changes done by this CL. > Therefore this CL also makes sure that LoadDataWithBaseURL navigations > use site_url = <base url>. This is accomplished by taking into account > the base url in NavigationRequest::OnResponseStarted and > NavigationControllerImpl::Reload. > > This CL also tweaks unit tests, to make sure the simulated navigations > are compatible with the new CanCommitOrigin tightening. > > This is a reland of r526198, after adding extra logging > of requested_site_url from CanCommitOrigin. > > Bug: 770239 , 797968 > Change-Id: I5783a3e32e6313fa5db5c88f55ebdc389843afc0 > Reviewed-on: https://chromium-review.googlesource.com/846342 > Reviewed-by: Alex Moshchuk <alexmos@chromium.org> > Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> > Cr-Commit-Position: refs/heads/master@{#528514} TBR=alexmos@chromium.org,lukasza@chromium.org Change-Id: Id2245895c1ba8dfa0f6d9a03bbefba1b3c49c9c2 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 770239 , 797968 Reviewed-on: https://chromium-review.googlesource.com/862165 Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/heads/master@{#528798} [modify] https://crrev.com/d8ec893735d0bc1424f0f16a8efcf4c158a8fa43/content/browser/frame_host/navigation_controller_impl_browsertest.cc [modify] https://crrev.com/d8ec893735d0bc1424f0f16a8efcf4c158a8fa43/content/browser/frame_host/navigation_controller_impl_unittest.cc [modify] https://crrev.com/d8ec893735d0bc1424f0f16a8efcf4c158a8fa43/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/d8ec893735d0bc1424f0f16a8efcf4c158a8fa43/content/browser/message_port_provider_browsertest.cc [modify] https://crrev.com/d8ec893735d0bc1424f0f16a8efcf4c158a8fa43/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/d8ec893735d0bc1424f0f16a8efcf4c158a8fa43/content/browser/web_contents/web_contents_impl_unittest.cc
,
Jun 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e2c2c49f75dc103e7f49c0f3fcb7f0283ee12801 commit e2c2c49f75dc103e7f49c0f3fcb7f0283ee12801 Author: Charlie Reis <creis@chromium.org> Date: Fri Jun 15 21:34:04 2018 Do not allow extension origins or URLs to commit in web processes. This CL revives some of the checks from r512959 to ensure that most extension URLs only commit in the correct extension processes. There are several exceptions that must be accounted for. BUG= 770239 , 840857 Change-Id: Id3dd2a7814041186d4de6f61e2dee440939b57d9 Reviewed-on: https://chromium-review.googlesource.com/1025075 Commit-Queue: Charlie Reis <creis@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Nasko Oskov <nasko@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/heads/master@{#567799} [modify] https://crrev.com/e2c2c49f75dc103e7f49c0f3fcb7f0283ee12801/chrome/browser/chrome_security_exploit_browsertest.cc [modify] https://crrev.com/e2c2c49f75dc103e7f49c0f3fcb7f0283ee12801/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc [modify] https://crrev.com/e2c2c49f75dc103e7f49c0f3fcb7f0283ee12801/chrome/browser/extensions/process_management_browsertest.cc [modify] https://crrev.com/e2c2c49f75dc103e7f49c0f3fcb7f0283ee12801/content/browser/frame_host/navigation_request.cc [modify] https://crrev.com/e2c2c49f75dc103e7f49c0f3fcb7f0283ee12801/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/e2c2c49f75dc103e7f49c0f3fcb7f0283ee12801/content/browser/security_exploit_browsertest.cc [modify] https://crrev.com/e2c2c49f75dc103e7f49c0f3fcb7f0283ee12801/content/public/test/browser_test_utils.cc [modify] https://crrev.com/e2c2c49f75dc103e7f49c0f3fcb7f0283ee12801/content/public/test/browser_test_utils.h [modify] https://crrev.com/e2c2c49f75dc103e7f49c0f3fcb7f0283ee12801/extensions/browser/url_request_util.cc [modify] https://crrev.com/e2c2c49f75dc103e7f49c0f3fcb7f0283ee12801/extensions/browser/url_request_util.h
,
Dec 15
,
Dec 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8aa429781b1f7dc7fc4f32de609dfc3b0838d8a4 commit 8aa429781b1f7dc7fc4f32de609dfc3b0838d8a4 Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Thu Dec 20 21:42:00 2018 Avoid using LoadDataWithBaseURL in browser tests. LoadDataWithBaseURL should be avoided in browser tests, because 1. It doesn't fully, correctly simulate navigating to a real website. 2. It is only used in production in Android WebView and in GuestView (both of which don't run with site-per-process, which is the default mode on desktop platforms). Cleaning up test usage of LoadDataWithBaseURL will help proceed with enforcement of CanCommit origin checks - LoadDataWithBaseURL is problematic because it commits https://foo.com origin in a SiteInstance associated with data: site URL. Bug: 770239 Change-Id: Ic68697be28cace0d8ff8abc0951e12cb5f81ac68 Reviewed-on: https://chromium-review.googlesource.com/c/1385471 Reviewed-by: Nasko Oskov <nasko@chromium.org> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/heads/master@{#618336} [modify] https://crrev.com/8aa429781b1f7dc7fc4f32de609dfc3b0838d8a4/content/browser/display_cutout/display_cutout_browsertest.cc [modify] https://crrev.com/8aa429781b1f7dc7fc4f32de609dfc3b0838d8a4/content/browser/fileapi/file_system_chooser_browsertest.cc [modify] https://crrev.com/8aa429781b1f7dc7fc4f32de609dfc3b0838d8a4/content/browser/message_port_provider_browsertest.cc [modify] https://crrev.com/8aa429781b1f7dc7fc4f32de609dfc3b0838d8a4/content/public/test/content_browser_test_utils.cc [modify] https://crrev.com/8aa429781b1f7dc7fc4f32de609dfc3b0838d8a4/content/public/test/content_browser_test_utils.h
,
Dec 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/86e3a9f6efedf945f86250ab9188f8c2ea9a300e commit 86e3a9f6efedf945f86250ab9188f8c2ea9a300e Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Fri Dec 21 00:25:18 2018 Tweak navigation commit unit tests, to more closely match product code. The following tweaks were made: - Use NavigationSimulator where possible (instead of manually calling individual functions of TestRenderFrameHost). - Target the correct RenderFrameHost (which might change during a navigation that requires a process swap). This is achieved by introducing the GetNavigatingRenderFrameHost helper method. - Fix several tests that used a different url 1) when initiating the navigation and 2) when committing the navigation. - Change interstitial tests so that the target page doesn't change while the interstitial is shown. - Make sure that Forward_GeneratesNewPage and Reload_GeneratesNewPage unit tests pass the correct value of |did_create_new_entry| (which is |false| in browser tests / in production). This changes the expectations of Reload_GeneratesNewPage to match the production (reload should replace the history entry, instead of creating a new entry) and Forward_GeneratesNewPage (no pruning needs to take place). This change is supported by introducing a new browser test that replicates the scenario of Reload_GeneratesNewPage. Cleaning up unit test will help proceed with enforcement of CanCommit origin checks (which might have otherwise introduce "renderer kills" during the tests). Bug: 770239 , 916783 Change-Id: I0af7e1049376a0d0834d48f4de05fb0fe38d56df Reviewed-on: https://chromium-review.googlesource.com/c/1387196 Reviewed-by: Nasko Oskov <nasko@chromium.org> Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Cr-Commit-Position: refs/heads/master@{#618400} [modify] https://crrev.com/86e3a9f6efedf945f86250ab9188f8c2ea9a300e/content/browser/frame_host/navigation_controller_impl_unittest.cc [modify] https://crrev.com/86e3a9f6efedf945f86250ab9188f8c2ea9a300e/content/browser/frame_host/render_frame_host_manager_browsertest.cc [modify] https://crrev.com/86e3a9f6efedf945f86250ab9188f8c2ea9a300e/content/browser/web_contents/web_contents_impl_unittest.cc [modify] https://crrev.com/86e3a9f6efedf945f86250ab9188f8c2ea9a300e/content/public/test/navigation_simulator.cc
,
Dec 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1da30d7c7d76076d9ab9b5e5b0af6c984d8ebd8f commit 1da30d7c7d76076d9ab9b5e5b0af6c984d8ebd8f Author: Lukasz Anforowicz <lukasza@chromium.org> Date: Fri Dec 21 00:44:27 2018 Consult CPSPI::CanAccessDataForOrigin from RFHI::CanCommitOrigin. In presence of Strict Site Isolation (aka site-per-process) this CL should prevent a compromised renderer from being able to commit an origin incompatible with the process lock. Note that more work is still needed to protect other isolation modes (citadel-style protection is needed to prevent non-locked processes from committing an origin that requires a dedicated process). Change-Id: Ifc39a81d8a1ded184c2ceb64c943f7ed2babb858 Bug: 770239 Reviewed-on: https://chromium-review.googlesource.com/c/1383293 Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org> Reviewed-by: Nasko Oskov <nasko@chromium.org> Cr-Commit-Position: refs/heads/master@{#618405} [modify] https://crrev.com/1da30d7c7d76076d9ab9b5e5b0af6c984d8ebd8f/content/browser/frame_host/navigation_controller_impl_browsertest.cc [modify] https://crrev.com/1da30d7c7d76076d9ab9b5e5b0af6c984d8ebd8f/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/1da30d7c7d76076d9ab9b5e5b0af6c984d8ebd8f/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/1da30d7c7d76076d9ab9b5e5b0af6c984d8ebd8f/content/public/browser/child_process_security_policy.h
,
Dec 21
Commit 1da30d7c... (the one in #c22 above) initially landed in 73.0.3647.0
,
Dec 21
,
Dec 27
AFAICT there were no reports of RFH_INVALID_ORIGIN_ON_COMMIT kills in the wild since the CL in #c22 landed.
,
Dec 27
I wonder if this bug should be marked as Fixed, and we can follow-up on remaining stuff in other bugs:
- "Move origin calculations entirely into the browser"
- this is tracked by issue 888079
- "Reject DidCommitProvisionalLoad received while there is no pending navigation"
- this will be fixed as a side-effect of mojo-ifying navigations (issue 784904)
- "Consult extensions::ProcessMap" (extensions vs nonisolated webpages)
- nonisolated vs isolated webpages (aka citadel-style protection)
- WebUI bindings
- CWS
- etc.
- we want to fix these not just for CanCommitOrigin/URL, but also for all other
places where CanAccessDataForOrigin is used - this is sort of tracked in issue 764958
|
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by dcheng@chromium.org
, Oct 1 2017