Does this mean the network service isn't currently doing the CanSetAsOriginHeader() check from ChildProcessSecurityPolicyImpl?

Doesn't look like it to me.

Network service is also not doing the extensions origin header checks, which are hooked up in non-network service when the ResourceDispatcherHost is created: https://cs.chromium.org/chromium/src/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc?l=1039&rcl=b390e82b77504a1d4a2c1a4308332e0f89ff0646

Another thing to note here, is that SecurityExploitBrowserTest.InvalidOriginHeaders, and possible some of the other security browser tests in content/browser/security_exploit_browsertest.cc are not running the network service codepath since they are using the ResourceMessageFilter instead of the network service URLLoaderFactory.

Thanks for the info. In the following CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1170312
I modified the SecurityExploitBrowserTest.InvalidOriginHeaders browser tests to be two tests for the two cases: CanSetAsOriginHeader() check and extensions origin header check.


I also updated the browser test to create the URLLoaderFactory from RenderFrameHost.

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

commit ae10116cc36d6b44868ccd4ae50589ff77b127e2
Author: Clark DuVall <cduvall@chromium.org>
Date: Wed Aug 15 01:26:46 2018

Fix SecurityExploitBrowserTest to use network service path

These tests were accessing ResourceMessageFilter directly, instead of
using the network service path when it is enabled.

Bug:  862176 ,  874227 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I3add3b588ad40c3bca265ad68adccca293ddae0d
Reviewed-on: https://chromium-review.googlesource.com/1175105
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583117}
[modify] https://crrev.com/ae10116cc36d6b44868ccd4ae50589ff77b127e2/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/ae10116cc36d6b44868ccd4ae50589ff77b127e2/content/browser/security_exploit_browsertest.cc
[modify] https://crrev.com/ae10116cc36d6b44868ccd4ae50589ff77b127e2/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter
[modify] https://crrev.com/ae10116cc36d6b44868ccd4ae50589ff77b127e2/testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter

Oops, didn't see your comment, I merged a change yesterday to fix the browser test. I also categorized the security test and an extension test failure under this bug in the filter files.

The existing enforcement that is driven by CanSetAsOriginHeader does not seem to be an important security boundary as it currently exists: the Origin header only appears when the requester is playing the CORS rules; a compromised renderer could just as easily omit the Origin header, which makes the request indistinguishable from a same-origin request.

In other words: spoofing another Origin is, as far as I know, a pretty lame thing to do with a compromised renderer, since you get a high privilege level just by not mentioning who you are at all in the first place.

What might be implementable today in the network service (with no extra process hops), would be to do the  Origin header check against the origin used by CORB. CORB already has a notion of a |frame_origin| is provided from elsewhere -- the idea is that if the Origin header is set in the outbound request, it really ought to match the |frame_origin| passed into CORB. Today the |frame_origin| is just a value passed from the renderer -- that suffices for the Spectre threat model. But we intend to make it come from a more trusted value.

Thanks Nick for looking into this.

Since this isn't a security bug regression, I'll remove canary blocking label.

Jun: can you please implement the suggestion Nick mentions? Also you can make the 2 failing tests for this feature early return if network service is enabled.

Thanks! Sure, I'll work on that.

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

commit 53c54b22b85d346e00b756c8cf175668d9eb97ec
Author: Jun Cai <juncai@chromium.org>
Date: Thu Aug 16 00:14:04 2018

Network Service: Make some checking Origin headers tests early return if network service is enabled

Based on the comment at:
https://bugs.chromium.org/p/chromium/issues/detail?id=862176
This CL makes:
SecurityExploitBrowserTest.InvalidOriginHeaders
ExtensionUnloadBrowserTest.UnloadWithContentScripts
early return if network service is enabled.

Bug:  862176 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I040ac9615a6deed786825d306a9192d14925a5cc
Reviewed-on: https://chromium-review.googlesource.com/1176479
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Jun Cai <juncai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583455}
[modify] https://crrev.com/53c54b22b85d346e00b756c8cf175668d9eb97ec/chrome/browser/extensions/extension_unload_browsertest.cc
[modify] https://crrev.com/53c54b22b85d346e00b756c8cf175668d9eb97ec/content/browser/security_exploit_browsertest.cc
[modify] https://crrev.com/53c54b22b85d346e00b756c8cf175668d9eb97ec/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter
[modify] https://crrev.com/53c54b22b85d346e00b756c8cf175668d9eb97ec/testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter

Per comment 11:
Just want to make sure I understand, does it mean that we need to store the |frame_origin| (which is provided to CrossOriginReadBlocking) somewhere, and use those |frame_origin| to check against the Origin header from the request in the URLLoaderFactory?

ping nick@ per comment 11, :).

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

commit 9e24959d5f3811c9764f0ff8d2e3e2dbb3c83be6
Author: Jun Cai <juncai@chromium.org>
Date: Mon Aug 27 22:56:04 2018

Network Service: Add UMA for Origin header sanity check for URLLoaderFactory

Bug:  862176 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I31885abe5eeb6eb56d775e2a80667d0fc30cafca
Reviewed-on: https://chromium-review.googlesource.com/1187274
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Jun Cai <juncai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586466}
[modify] https://crrev.com/9e24959d5f3811c9764f0ff8d2e3e2dbb3c83be6/services/network/url_loader_factory.cc
[modify] https://crrev.com/9e24959d5f3811c9764f0ff8d2e3e2dbb3c83be6/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/9e24959d5f3811c9764f0ff8d2e3e2dbb3c83be6/tools/metrics/histograms/histograms.xml