|
|
Created:
4 years, 3 months ago by ncarter (slow) Modified:
4 years, 3 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisallow navigations to blob URLs with non-canonical origins.
BUG=646278
TEST=content_browsertests, included
Committed: https://crrev.com/654b9b9e4b7bdec1366e1ab378b7a8f5c63fc697
Cr-Commit-Position: refs/heads/master@{#420103}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add tests. #
Total comments: 14
Patch Set 3 : Fix issues and add unittest. #
Total comments: 6
Patch Set 4 : Charlie fixes. #Patch Set 5 : Fix XR to blob in isolated world. #
Messages
Total messages: 38 (23 generated)
The CQ bit was checked by nick@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.
nick@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org
Nasko or Charlie -- What do you think of this approach? I'm working on tests; I'm mostly trying to figure out whether I need whitelisting for any weird URLs like blob:blobdata.
https://codereview.chromium.org/2347163004/diff/1/content/browser/child_proce... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2347163004/diff/1/content/browser/child_proce... content/browser/child_process_security_policy_impl.cc:607: Probably need this here too.
On 2016/09/16 21:18:13, ncarter wrote: > Nasko or Charlie -- > > What do you think of this approach? > > I'm working on tests; I'm mostly trying to figure out whether I need > whitelisting for any weird URLs like blob:blobdata. I like this approach. I don't know any corner cases except PlzNavigate, but that might get intercepted and handled before we get to CanRequestURL. Nasko, do you know how the PlzNavigate stream works? (If so, we should sanity check that it's not exploitable if a compromised renderer sends a PlzNavigate-like blob URL.) https://codereview.chromium.org/2347163004/diff/1/content/browser/child_proce... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2347163004/diff/1/content/browser/child_proce... content/browser/child_process_security_policy_impl.cc:607: On 2016/09/16 21:21:03, ncarter wrote: > Probably need this here too. Agreed.
On 2016/09/16 21:45:03, Charlie Reis (slow) wrote: > On 2016/09/16 21:18:13, ncarter wrote: > > Nasko or Charlie -- > > > > What do you think of this approach? > > > > I'm working on tests; I'm mostly trying to figure out whether I need > > whitelisting for any weird URLs like blob:blobdata. > > I like this approach. I like this too. > I don't know any corner cases except PlzNavigate, but that might get intercepted > and handled before we get to CanRequestURL. Nasko, do you know how the > PlzNavigate stream works? (If so, we should sanity check that it's not > exploitable if a compromised renderer sends a PlzNavigate-like blob URL.) Stream uses blob URLs, but those don't commit. They are used only as a transport mechanism for the response body and shouldn't be hitting the UI thread. The Stream blob for PlzNavigate is also created with the proper origin, so it shouldn't fail this check. > https://codereview.chromium.org/2347163004/diff/1/content/browser/child_proce... > File content/browser/child_process_security_policy_impl.cc (right): > > https://codereview.chromium.org/2347163004/diff/1/content/browser/child_proce... > content/browser/child_process_security_policy_impl.cc:607: > On 2016/09/16 21:21:03, ncarter wrote: > > Probably need this here too. > > Agreed.
Description was changed from ========== Disallow navigations to blob URLs with non-canonical origins. Things need to test: - Weird media URLs - Nasko mentioned some usage of weird blob: URLs inside of something something? - Consistency re: IDNs/punycode. - Behavior of null origins. BUG=646278 ========== to ========== Disallow navigations to blob URLs with non-canonical origins. BUG=646278 TEST=content_browsertests, included ==========
The CQ bit was checked by nick@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...
PTAL. Test added.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! LGTM with nits. https://codereview.chromium.org/2347163004/diff/20001/content/browser/blob_st... File content/browser/blob_storage/blob_url_browsertest.cc (right): https://codereview.chromium.org/2347163004/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_url_browsertest.cc:70: // Using an http page, click a link that open a popup to a same-origin blob. nit: opens https://codereview.chromium.org/2347163004/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_url_browsertest.cc:96: EXPECT_EQ(page_content, origin.Serialize() + " potato"); nit: Reverse order (expected, actual) https://codereview.chromium.org/2347163004/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_url_browsertest.cc:101: // Using an http page, click a link that open a popup to a same-origin blob nit: opens https://codereview.chromium.org/2347163004/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_url_browsertest.cc:130: EXPECT_EQ(page_content, origin.Serialize() + " "); // no potato nit: Reverse order (expected, actual) Also, can you elaborate, either here or in the comment on line 121, why "potato" isn't shown? At first it sounded like the page would display but we would omit the authority from the visible URL. But this indicates that we prevent the page from loading. That behavior is be fine, but I'm not sure what form it takes-- is it an error page? A blank page? A renderer kill? Worth mentioning in the comment. https://codereview.chromium.org/2347163004/diff/20001/content/browser/child_p... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2347163004/diff/20001/content/browser/child_p... content/browser/child_process_security_policy_impl.cc:566: bool ChildProcessSecurityPolicyImpl::CanRequestURL( We have pretty extensive unit tests for this in child_process_security_policy_unittest.cc. Can you toss one in for the spoof case for completeness? https://codereview.chromium.org/2347163004/diff/20001/content/browser/child_p... content/browser/child_process_security_policy_impl.cc:584: if (url.SchemeIsBlob() && Sanity check: Do we have the same problem for filesystem URLs?
https://codereview.chromium.org/2347163004/diff/20001/content/browser/child_p... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2347163004/diff/20001/content/browser/child_p... content/browser/child_process_security_policy_impl.cc:584: if (url.SchemeIsBlob() && On 2016/09/20 16:28:55, Charlie Reis (slow) wrote: > Sanity check: Do we have the same problem for filesystem URLs? Potentially but the spoof doesn't directly translate; since for filesystem URLs, GURL actually parses past the scheme. The behavior I see is that authorities are preserved in inner URLs of filesystem URLs, but the inner URL is parsed and canonicalized, so spaces get escaped to %20. We probably should not allow authorities inside of filesystem URLs, but it may be a spec question.
The CQ bit was checked by nick@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...
Significant changes since approval. PTAL https://codereview.chromium.org/2347163004/diff/20001/content/browser/blob_st... File content/browser/blob_storage/blob_url_browsertest.cc (right): https://codereview.chromium.org/2347163004/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_url_browsertest.cc:70: // Using an http page, click a link that open a popup to a same-origin blob. On 2016/09/20 16:28:55, Charlie Reis (slow) wrote: > nit: opens Done. https://codereview.chromium.org/2347163004/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_url_browsertest.cc:96: EXPECT_EQ(page_content, origin.Serialize() + " potato"); On 2016/09/20 16:28:55, Charlie Reis (slow) wrote: > nit: Reverse order (expected, actual) Done. https://codereview.chromium.org/2347163004/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_url_browsertest.cc:101: // Using an http page, click a link that open a popup to a same-origin blob On 2016/09/20 16:28:55, Charlie Reis (slow) wrote: > nit: opens Done. https://codereview.chromium.org/2347163004/diff/20001/content/browser/blob_st... content/browser/blob_storage/blob_url_browsertest.cc:130: EXPECT_EQ(page_content, origin.Serialize() + " "); // no potato On 2016/09/20 16:28:55, Charlie Reis (slow) wrote: > nit: Reverse order (expected, actual) > > Also, can you elaborate, either here or in the comment on line 121, why "potato" > isn't shown? At first it sounded like the page would display but we would omit > the authority from the visible URL. But this indicates that we prevent the page > from loading. That behavior is be fine, but I'm not sure what form it takes-- > is it an error page? A blank page? A renderer kill? Worth mentioning in the > comment. Done. https://codereview.chromium.org/2347163004/diff/20001/content/browser/child_p... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2347163004/diff/20001/content/browser/child_p... content/browser/child_process_security_policy_impl.cc:566: bool ChildProcessSecurityPolicyImpl::CanRequestURL( On 2016/09/20 16:28:55, Charlie Reis (slow) wrote: > We have pretty extensive unit tests for this in > child_process_security_policy_unittest.cc. Can you toss one in for the spoof > case for completeness? Thanks for suggesting this! It uncovered the fact that I meant to fix up CanCommitURL too. I copypasted that clause there too.
Thanks! Updated patch LGTM with minor nits. https://codereview.chromium.org/2347163004/diff/20001/content/browser/child_p... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2347163004/diff/20001/content/browser/child_p... content/browser/child_process_security_policy_impl.cc:566: bool ChildProcessSecurityPolicyImpl::CanRequestURL( On 2016/09/20 23:01:43, ncarter wrote: > On 2016/09/20 16:28:55, Charlie Reis (slow) wrote: > > We have pretty extensive unit tests for this in > > child_process_security_policy_unittest.cc. Can you toss one in for the spoof > > case for completeness? > > Thanks for suggesting this! It uncovered the fact that I meant to fix up > CanCommitURL too. I copypasted that clause there too. Great! I agree that having the check in CanCommitURL as well seems better. https://codereview.chromium.org/2347163004/diff/20001/content/browser/child_p... content/browser/child_process_security_policy_impl.cc:584: if (url.SchemeIsBlob() && On 2016/09/20 17:46:55, ncarter wrote: > On 2016/09/20 16:28:55, Charlie Reis (slow) wrote: > > Sanity check: Do we have the same problem for filesystem URLs? > > Potentially but the spoof doesn't directly translate; since for filesystem URLs, > GURL actually parses past the scheme. > > The behavior I see is that authorities are preserved in inner URLs of filesystem > URLs, but the inner URL is parsed and canonicalized, so spaces get escaped to > %20. We probably should not allow authorities inside of filesystem URLs, but it > may be a spec question. Acknowledged. https://codereview.chromium.org/2347163004/diff/40001/content/browser/blob_st... File content/browser/blob_storage/blob_url_browsertest.cc (right): https://codereview.chromium.org/2347163004/diff/40001/content/browser/blob_st... content/browser/blob_storage/blob_url_browsertest.cc:141: // Location.replace shouldn't be allowed . location.replace is different than history.replaceState, which this test appears to be testing. Should this say replaceState? (Also, there's an extra space before the period.) https://codereview.chromium.org/2347163004/diff/40001/content/browser/blob_st... content/browser/blob_storage/blob_url_browsertest.cc:171: // about:blank. Please also mention that the page loads anyway. (I think that's ok for the time being, in the hopes that we can start showing error pages for FilterURL cases in the future.) https://codereview.chromium.org/2347163004/diff/40001/content/browser/child_p... File content/browser/child_process_security_policy_unittest.cc (right): https://codereview.chromium.org/2347163004/diff/40001/content/browser/child_p... content/browser/child_process_security_policy_unittest.cc:213: p->CanRequestURL(kRendererID, GURL("blob:http://localhost/some-guid"))); nit: Let's put the CanRequest block above the CanCommit block, to be consistent with the other tests in this file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2347163004/diff/40001/content/browser/blob_st... File content/browser/blob_storage/blob_url_browsertest.cc (right): https://codereview.chromium.org/2347163004/diff/40001/content/browser/blob_st... content/browser/blob_storage/blob_url_browsertest.cc:141: // Location.replace shouldn't be allowed . On 2016/09/20 23:19:06, Charlie Reis (slow) wrote: > location.replace is different than history.replaceState, which this test appears > to be testing. Should this say replaceState? > > (Also, there's an extra space before the period.) Done. https://codereview.chromium.org/2347163004/diff/40001/content/browser/blob_st... content/browser/blob_storage/blob_url_browsertest.cc:171: // about:blank. On 2016/09/20 23:19:06, Charlie Reis (slow) wrote: > Please also mention that the page loads anyway. (I think that's ok for the time > being, in the hopes that we can start showing error pages for FilterURL cases in > the future.) Done. https://codereview.chromium.org/2347163004/diff/40001/content/browser/child_p... File content/browser/child_process_security_policy_unittest.cc (right): https://codereview.chromium.org/2347163004/diff/40001/content/browser/child_p... content/browser/child_process_security_policy_unittest.cc:213: p->CanRequestURL(kRendererID, GURL("blob:http://localhost/some-guid"))); On 2016/09/20 23:19:06, Charlie Reis (slow) wrote: > nit: Let's put the CanRequest block above the CanCommit block, to be consistent > with the other tests in this file. Done.
The CQ bit was checked by nick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2347163004/#ps60001 (title: "Charlie fixes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Disallow navigations to blob URLs with non-canonical origins. BUG=646278 TEST=content_browsertests, included ========== to ========== Disallow navigations to blob URLs with non-canonical origins. BUG=646278 TEST=content_browsertests, included ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Disallow navigations to blob URLs with non-canonical origins. BUG=646278 TEST=content_browsertests, included ========== to ========== Disallow navigations to blob URLs with non-canonical origins. BUG=646278 TEST=content_browsertests, included Committed: https://crrev.com/654b9b9e4b7bdec1366e1ab378b7a8f5c63fc697 Cr-Commit-Position: refs/heads/master@{#420103} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/654b9b9e4b7bdec1366e1ab378b7a8f5c63fc697 Cr-Commit-Position: refs/heads/master@{#420103}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2358193002/ by dewittj@chromium.org. The reason for reverting is: Likely breaks this layout test: http/tests/xmlhttprequest/xhr-to-blob-in-isolated-world.html e.g. https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux --- /mnt/data/b/rr/tmpVD1Qdr/w/layout-test-results/http/tests/xmlhttprequest/xhr-to-blob-in-isolated-world-expected.txt +++ /mnt/data/b/rr/tmpVD1Qdr/w/layout-test-results/http/tests/xmlhttprequest/xhr-to-blob-in-isolated-world-actual.txt @@ -1,3 +1,4 @@ CONSOLE WARNING: line 1: Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user's experience. For more help, check https://xhr.spec.whatwg.org/. +CONSOLE ERROR: line 1: Uncaught NetworkError: Failed to execute 'send' on 'XMLHttpRequest': Failed to load 'blob:chrome-extension://123/456789'. This tests an isolated script's ability to XHR a blob that is in its security origin, which is not the same as the document's security origin. We pass if there are no console errors..
Message was sent while issue was closed.
Thanks for the revert. Definitely this is the reason for the layouttest failures.
Message was sent while issue was closed.
Description was changed from ========== Disallow navigations to blob URLs with non-canonical origins. BUG=646278 TEST=content_browsertests, included Committed: https://crrev.com/654b9b9e4b7bdec1366e1ab378b7a8f5c63fc697 Cr-Commit-Position: refs/heads/master@{#420103} ========== to ========== Disallow navigations to blob URLs with non-canonical origins. BUG=646278 TEST=content_browsertests, included Committed: https://crrev.com/654b9b9e4b7bdec1366e1ab378b7a8f5c63fc697 Cr-Commit-Position: refs/heads/master@{#420103} ==========
The CQ bit was checked by nick@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. |