[jam, kinuko] So presumably this regressed in https://chromium-review.googlesource.com/c/chromium/src/+/882606.  That's Yuzhu's CL, though he's since left the team.  I guess that leaves one of his reviewers as being our best bet for a fix?

The first question I have (for jam or kinuko) is whether network::URLLoader::URLLoader() is ever supposed to be used if the network service is not enabled? (FWIW -- I also see a network::URLLoader::URLLoader() being instantiated when devtools starts up).

It is supposed to be used by some browser-process consumers but, to the extent of my knowledge, only through former URLFetcher consumers that have been switch to SimpleURLLoader.

(still catching up)

I'll look some more into this today in the afternoon. Per Matt, some URLLoader usage today is expected. If the cl linked above changed other uses cases to use URLLoader, that was probably not intended.

Can someone describe the security impact of this? It looks from just reading the bug this should not be a bug-security but rather security related feature/hardening work. What impact does this bug have on user safety?

If renderers have direct access to the network service's URLLoader, they can bypass basically all browser-side security checks normally made on requests issued from the renderer (Since those aren't hooked up to the network service's URLLoader yet).

Basically all checks not enforced by the network stack, that is (i.e., they can't redirect to a file URL or something silly like that).  If we restrict the URLs that can be request, upload source file paths, credentials mode, method, etc, based on renderer, those checks will be bypassed.  So this could give access to, for instance, file URLs, which seems pretty concerning.

okay, should this just be another blocker for issue 786673 then? it sounds like a security mitigation feature we need to implement in the network stack, rather than a security bug per-se.

@nick: are you 100% sure about the regression range identified in comment 0? I'm investigating now and I think it's from https://chromium-review.googlesource.com/c/chromium/src/+/830529.

btw I've synced to r532146 so I can confirm that even before the range given in comment 0, I don't see the EXPECTED BEHAVIOR listed above. Can you give me a revision that gives that?

John:  I'm in agreement that CL introduced a bug (Though not sure if the path was working before, haven't dug deep enough).

Seems like RenderFrameHostImpl::CreateNetworkServiceDefaultFactoryAndObserve needs to be creating a factory for the RDH when the network service is disabled.

I'd consider this a stable blocker.  As-is, seems like an attacker can access arbitrary file URLs, if they can gain access to the right URLLoaderFactory.

okay based on comments I'll tag this as High, it seems to only be in M65 if the revision range in #12 is correct.

+ awhalley@ (Security TPM)

Wonder if it's worth putting in a CHECK to make sure this doesn't happen in production (make network::URLLoaderFactory CHECK if it's passed a process ID when the network service is disabled, or have SimpleURLLoader somehow tell the factory it's being used, and fail when that's not the case)

@Matt: suggestion in comment 17 is good. We should also try to guard against a null process id as well? i.e. because that would be even more privileged, and could be a programmer error.

I've verified that this could lead to file reads, so thanks for bringing that example up. 

Regarding a factory for RDH, no need. We could just not pass in a bundle, which was old behavior, and then the renderer will see there isn't a per-frame factory and use the global renderer one (which is default without network service).

@Nick: I also synced before r524876 to try and repro the expected behavior you wrote, but it still wouldn't reproduce.


Here's a fix for not sending the URLLoaderFactory to the renderer: https://chromium-review.googlesource.com/c/chromium/src/+/935368

SimpleURLLoader is typically used with a null process id, so a CHECK for that won't work, unless we have some way to bypass it when SimpleURLLoader is in use (Or when using a URLLoaderFactory created with StorageParition::GetURLLoaderFactoryForBrowserProcess)

[Bulk Edit]: M65 Stable promotion is coming VERY soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Merge has to happen latest by 1:00 PM PT Tuesday (02/27/18) in order to make it to last M65 beta release next week. Thank you.

@Matt: yeah whatever way we check would have to let the SimpleURLLoader uses from the browser through.

another idea: use MOJO_HANDLE_SIGNAL_PEER_REMOTE to check in URLLoader that the other end is in the same process, if the network service is disabled.

That wouldn't catch it for navigations or downloads, but would for everything else - sounds like a great idea to me.

@Matt: true, I was mostly thinking about the developer-error case of sending a URLLoaderFactory to the renderer that goes to the network (i.e. network::URLLoader). I think for navigations and downloads, these wouldn't be security problems as even if we messed there, the initiating code is the browser?

I'll add a check for this as a test for this bug. I didn't want to do a one-off regression test for this specific symptom in comment 0, as it would miss other misuses.

Thanks Nick for finding this!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6bb1eba7d527f89d6d3eebec8fb1fb079b043060

commit 6bb1eba7d527f89d6d3eebec8fb1fb079b043060
Author: John Abd-El-Malek <jam@chromium.org>
Date: Sat Feb 24 01:20:27 2018

Don't send new subresource loader factories to the renderer for failed commits when the network service is disabled.

This was an unintended change in r524876.

Bug:  814913 
Change-Id: I60c45cc44a9295902fba553e0ddac704b6ac895a
Reviewed-on: https://chromium-review.googlesource.com/935368
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538963}
[modify] https://crrev.com/6bb1eba7d527f89d6d3eebec8fb1fb079b043060/content/browser/frame_host/render_frame_host_impl.cc

Also thanks Matt for quickly realizing this is a high severity bug!

Requesting merge to 65. If we get it in today it can make the final beta.

This bug requires manual review: We are only 7 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

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

Approving merge to M65 branch 3325 based on comment #28. Please merge ASAP so we can pick it up for tomorrow's last beta release. Thank you.

jam: thanks for the fix. I actually already wrote a regression test for this specific issue that'll land as part of https://chromium-review.googlesource.com/c/chromium/src/+/927561 (it's an interesting case for cross site document blocking, as we've seen ambiguous handling of origin behavior of error pages being used in exploit chains before)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fb7861501ec0411aca13dd475a1fca2cf38a0e27

commit fb7861501ec0411aca13dd475a1fca2cf38a0e27
Author: John Abd-El-Malek <jam@chromium.org>
Date: Mon Feb 26 18:22:01 2018

Add a check that network::URLLoader is only used by the browser process when network service is disabled.

Bug:  814913 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I7a3f7746491bdd2a6e65ea532af38147098a4c1a
Reviewed-on: https://chromium-review.googlesource.com/936011
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539202}
[modify] https://crrev.com/fb7861501ec0411aca13dd475a1fca2cf38a0e27/services/network/url_loader.cc

btw the test I added in comment 33 would have triggered before the fix in comment 26 (which can be seen in its first patchset).

The merge has landed but it's not showing up in the bug yet. I'm building locally to verify it's fine as a simple merge wasn't possible. https://chromium-review.googlesource.com/c/chromium/src/+/937814

M65 merge - https://chromium.googlesource.com/chromium/src.git/+/55ab9a65bcc8d02fcb6069d2a391996f781c4501

https://chromium.googlesource.com/chromium/src.git/+log/refs/branch-heads/3325?n=1000

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/702e84f69e4d6741867d4ab4465c49a67fde601f

commit 702e84f69e4d6741867d4ab4465c49a67fde601f
Author: Nick Carter <nick@chromium.org>
Date: Mon Feb 26 23:08:33 2018

Remove LinkDoctorBaseURL special case for cross origin read blocking.

This special case would have been hard to port to the network service.
It's easier to just force the net_error_helper to issue a CORS-enabled
request, which https://www.googleapis.com/rpc readily understands.

A browsertest is added to explicitly test that CORB is applied to
subresources loaded by error pages. Currently this test fails; we'll
fix that in a separate CL.

Manual testing: in an official/branded Chrome build, navigate to
"http://blog.thestranger.com" and on the resulting DNS error page,
observe a suggested correction link (of "http://thestranger.com/blog").
Repeat these steps with a chrome://net-internals trace running, and
observe a request to "https://www.googleapis.com/rpc" that has the
"Origin: null" request header, and which includes an
"Access-Control-Allow-Origin: *" header in the response.

BUG= 814913 , 792546 

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Ib7434bd52a27909dd67c5e9a867db1dab7090d59
Reviewed-on: https://chromium-review.googlesource.com/927561
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Nick Carter <nick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539309}
[modify] https://crrev.com/702e84f69e4d6741867d4ab4465c49a67fde601f/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/702e84f69e4d6741867d4ab4465c49a67fde601f/chrome/renderer/net/net_error_helper.cc
[modify] https://crrev.com/702e84f69e4d6741867d4ab4465c49a67fde601f/chrome/test/data/mock-link-doctor.json.mock-http-headers
[modify] https://crrev.com/702e84f69e4d6741867d4ab4465c49a67fde601f/content/browser/loader/cross_site_document_blocking_browsertest.cc
[modify] https://crrev.com/702e84f69e4d6741867d4ab4465c49a67fde601f/testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter

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