Hmmm... alternatively, maybe AppCache's implementation of URLLoaderFactory can enforce its own |request_initiator_origin_lock|, before proxying requests into a network-facing URLLoaderFactory from the network service...

FWIW, issue 582750 tracks removing AppCache altogether (although there is currently no well-defined timeline for the removal).

I am starting to think that AppCacheSubresourceURLFactory should use a URLLoaderFactory obtained via RenderFrameHost::CreateNetworkServiceDefaultFactory - this will ensure that:

1) request_initiator_origin_lock is properly set
   and enforced from within the NetworkService
   (this bug)

2) site_for_cookies can be enforced in the future (see issue 911299)

3) WebRequest API is hooked properly - see
   https://chromium-review.googlesource.com/c/chromium/src/+/1134023/8#message-3b6be7c9f4e80b1daa21ec2c0f96845f99189f3d

I think the 2nd and 3rd points are also an arguments for associating
AppCacheSubresourceURLFactory with a RenderFrameHost[Impl] rather than
with an Origin.

wanderview@: cc-ed you because I mentioned you in the bug. Nothing actionable here.

#3: FWIW, we (storage team) would like to remove AppCache. Our intent to deprecate failed to get Blink API OWNERS approval, because current usage is too high and we don't have the resources to analyze this usage. At the very least, we know that Google Docs is still on AppCache and wanderview@ is working on moving them to Service Worker.

If we cannot remove AppCache, we definitely need to fix it.

I think mek@ is the best person on our end to review or help with the work.

WIP CL with an implementation of the idea from #c1 is at https://crrev.com/c/1410149

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

commit 288027b6b3bc6e41df98ae24d376907692669463
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Thu Jan 17 01:51:27 2019

Verify |request_initiator| in AppCacheSubresourceURLFactory.

Compromised renderers can spoof ResourceRequest::request_initiator and
pretend to be initiating a request on behalf of a victim origin.  This
CL ensures that such attempt will be caught and result in terminating
the renderer process (for requests handled by AppCacheSubresourceURLFactory).

Before this CL, URLLoaderInterceptor was intercepting requests as they were
sent from AppCacheSubresourceURLFactory to the NetworkService.  This wasn't
sufficient for testing the compromised renderer scenario, where the test needs
to inject a spoofed origin into a request made from the renderer, *before* the
request reaches AppCacheSubresourceURLFactory.  The CL accomplishes this, by
having RenderFrameHostImpl inject a g_create_network_factory_callback_for_test
both for 1) AppCache and 2) NetworkService factories.

The CL also takes the MojoBadMessageObserver class from
security_exploit_browertest.cc, makes it general-purpose and exposes it via
//mojo/public/cpp/test_support.

Bug: 910287
Change-Id: I665acf336e5c84533e2f5dd88769d88e3f9037ed
Reviewed-on: https://chromium-review.googlesource.com/c/1410149
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623505}
[modify] https://crrev.com/288027b6b3bc6e41df98ae24d376907692669463/content/browser/appcache/appcache_host.h
[modify] https://crrev.com/288027b6b3bc6e41df98ae24d376907692669463/content/browser/appcache/appcache_subresource_url_factory.cc
[modify] https://crrev.com/288027b6b3bc6e41df98ae24d376907692669463/content/browser/background_fetch/background_fetch_service_unittest.cc
[modify] https://crrev.com/288027b6b3bc6e41df98ae24d376907692669463/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/288027b6b3bc6e41df98ae24d376907692669463/content/browser/loader/cross_site_document_blocking_browsertest.cc
[modify] https://crrev.com/288027b6b3bc6e41df98ae24d376907692669463/content/browser/renderer_host/web_database_host_impl_unittest.cc
[modify] https://crrev.com/288027b6b3bc6e41df98ae24d376907692669463/content/browser/security_exploit_browsertest.cc
[modify] https://crrev.com/288027b6b3bc6e41df98ae24d376907692669463/content/browser/url_loader_factory_getter.cc
[modify] https://crrev.com/288027b6b3bc6e41df98ae24d376907692669463/content/browser/url_loader_factory_getter.h
[modify] https://crrev.com/288027b6b3bc6e41df98ae24d376907692669463/content/public/test/url_loader_interceptor.cc
[modify] https://crrev.com/288027b6b3bc6e41df98ae24d376907692669463/content/test/BUILD.gn
[modify] https://crrev.com/288027b6b3bc6e41df98ae24d376907692669463/mojo/public/cpp/test_support/BUILD.gn
[add] https://crrev.com/288027b6b3bc6e41df98ae24d376907692669463/mojo/public/cpp/test_support/lib/DEPS
[modify] https://crrev.com/288027b6b3bc6e41df98ae24d376907692669463/mojo/public/cpp/test_support/lib/test_utils.cc
[modify] https://crrev.com/288027b6b3bc6e41df98ae24d376907692669463/mojo/public/cpp/test_support/test_utils.h

r623505 resulted in some new kills that are tracked in issue 922953 :-(

So - in issue 922953 I learned that AppCacheHost::origin_in_use_ is not always populated - for example AppCacheHost created for shared workers...

    UI thread:
    #1 0x7fd0be72d401 content::AppCacheNavigationHandle::AppCacheNavigationHandle()
    #2 0x7fd0bedbcf43 content::SharedWorkerServiceImpl::CreateWorker()
    #3 0x7fd0bedbc7f1 content::SharedWorkerServiceImpl::ConnectToWorker()
    #4 0x7fd0bedb6d3a content::SharedWorkerConnectorImpl::Connect()
    #5 0x7fd0bc3dd3fb blink::mojom::SharedWorkerConnectorStubDispatch::Accept()

    IO thread later:
    #1 0x7faa78fb52a3 content::AppCacheHost::AppCacheHost()
    #2 0x7faa78fc1abe content::AppCacheNavigationHandleCore::Initialize()
    #3 0x7faa7ac3ca79 base::debug::TaskAnnotator::RunTask()
    #4 0x7faa7ac6a1bf base::MessageLoopImpl::RunTask()

... won't have AppCacheHost::origin_in_use_ set when AppCacheSubresourceURLFactory::CreateLoaderAndStart runs (e.g. when trigerred by WorkerGlobalScope.importScripts).  I hoped that maybe such AppCacheHost would have |is_for_dedicated_worker()| return true and I could grab the origin via GetParentAppCacheHost but apparently  |is_for_dedicated_worker()| returns false in this scenario.

So - I guess I need to go back to the drawing board and figure out if AppCacheSubresourceURLFactory::CreateLoaderAndStart can know either 1) the expected request_initiator origin or 2) the renderer_process_id.

jsbell@ / pwnall@ - any hints or suggestions wrt the question in #c12 above (how to figure out the expected request_initiator origin or renderer_process_id in AppCacheSubresourceURLFactory::CreateLoaderAndStart)?

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

commit 912f47b4715551b4db57f8ed8bed05a83144c2f7
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Thu Jan 17 23:59:45 2019

Partial revert: "Verify |request_initiator| in AppCacheSubresourceURLFactory."

This CL partially reverts commit
288027b6b3bc6e41df98ae24d376907692669463 (r623505) which has caused
renderer kills both in the wild and on Mojo bots (when running
external/wpt/html/browsers/offline/appcache/workers/appcache-worker.https.html
test with --enable-features=NetworkService).

Bug: 922953, 910287
Change-Id: I776898e16ca22cb2b3e3a07b37a7941f6d8ce32c
Reviewed-on: https://chromium-review.googlesource.com/c/1418400
Auto-Submit: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623917}
[modify] https://crrev.com/912f47b4715551b4db57f8ed8bed05a83144c2f7/content/browser/appcache/appcache_subresource_url_factory.cc
[modify] https://crrev.com/912f47b4715551b4db57f8ed8bed05a83144c2f7/content/browser/loader/cross_site_document_blocking_browsertest.cc

Re origin_in_use_ and shared workers, probably part of that is because of the difference between SelectCache and SelectCacheForSharedWorker, where only the first one sets the field? Although even for the first case thinking about it more I wonder how safe it is to use that for security decisions, since it is just a URL passed in by the renderer in the first place, and one that afaict isn't checked against any origin locks itself.

Having said that, getting the render_process_id in an AppCacheHost should be trivial. AppCacheBackendImpl afaik stores the process id in its process_id_ field, and that class is responsible for creating (and owning) the AppCacheHost instances. As such it should be trivial to just pass process_id_ into AppCacheHost.

(and AppCacheSubresourceURLFactory has a reference to the AppCacheHost, which would then get you the id there as well)

RE: origin_in_use_ [...] is just a URL passed in by the renderer

Ooops... this seems like a major mistake.  Thanks for catching this.  I should have checked where |origin_in_use_| comes from more thoroughly.


RE: AppCacheBackendImpl [...] stores the process id [...] and that class is responsible for creating (and owning) the AppCacheHost instances.

AppCacheHost is indeed created by AppCacheBackendImpl in the following callstack:

    IO thread
    AppCacheBackendImpl::RegisterHost
    AppCacheDispatcherHost::RegisterHost
    --- mojo browser/renderer boundary ---
    AppCacheBackendProxy::RegisterHost

OTOH, AppCacheHost may also be created by AppCacheNavigationHandle:

    IO thread
    #1 content::AppCacheHost::AppCacheHost()
    #2 content::AppCacheNavigationHandleCore::Initialize()

    The above is a test posted from the UI thread by content::AppCacheNavigationHandle's ctor.

In some cases, it is easy to know the process_id when constructing content::AppCacheNavigationHandle:

    UI thread
    #1 0x7fd0be72d401 content::AppCacheNavigationHandle::AppCacheNavigationHandle()
    #2 0x7fd0bedbcf43 content::SharedWorkerServiceImpl::CreateWorker() **** has |process_id| arg - yay!
    #3 0x7fd0bedbc7f1 content::SharedWorkerServiceImpl::ConnectToWorker()
    #4 0x7fd0bedb6d3a content::SharedWorkerConnectorImpl::Connect()
    #5 0x7fd0bc3dd3fb blink::mojom::SharedWorkerConnectorStubDispatch::Accept()

But in some cases, we might not yet have the process_id (I think).  For example, when a navigation *starts*, we might not have *yet* picked a renderer process for hosting the web page:

    UI thread
    #1 content::AppCacheNavigationHandle::AppCacheNavigationHandle()
    #2 content::NavigationHandleImpl::InitAppCacheHandle()
    #3 content::NavigationRequest::OnStartChecksComplete()
    #4 ...
    #5 content::NavigationHandleImpl::RunCompleteCallback()
    #6 content::NavigationHandleImpl::WillStartRequest()
    #7 content::NavigationRequest::BeginNavigation()
    #8 content::NavigatorImpl::Navigate()
    #9 content::NavigationControllerImpl::NavigateWithoutEntry()
    #10 content::NavigationControllerImpl::LoadURLWithParams()

I guess I could introduce AppCacheNavigationHandle::SetProcessId and call it from NavigationHandleImpl::WillProcessResponse.  Hmmm... let me try that...

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

commit 1b43ef7815413edac4400dadfe3701d7818cfd12
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Mon Jan 21 22:29:50 2019

CanAccessDataFromOrigin(..., initiator) in AppCacheSubresourceURLFactory

This reverts commit 912f47b4715551b4db57f8ed8bed05a83144c2f7 and tries
another approach for protecting network::ResourceRequest::request_initiator
against compromised renderers.  In the new approach:
1. AppCacheHost stores the renderer process id it is associated with.
2. AppCacheSubresourceURLFactory uses the process id to verify
   request_initiator via
   ChildProcessSecurityPolicyImpl::CanAccessDataFromOrigin.
3. The process id needs to be provided to AppCacheHost in 3 scenarios
   (not counting unit tests):
   3.1. SharedWorkerServiceImpl::CreateWorker already has a process id
   3.2. AppCacheBackendImpl::RegisterHost also already has a process id
   3.3. NavigationHandleImpl::InitAppCacheHandle runs when navigation
        starts and doesn't have a process yet.  In this scenario
        ChildProcessHost::kInvalidUniqueID is passed to AppCacheHost's
        constructor and the real process id is provided later, once
        NavigationHandleImpl::ReadyToCommitNavigation knows the target
        process for the navigation.
        - This delay is okay, because AppCacheHost::process_id() is only
          consulted from AppCache*Subresource*URLFactory which is only
          used for subresources and not for navigations.
        - The delay involves a thread hop - this is also okay, because
          the task that will set the process id is posted to the IO
          thread *before* a Commit IPC is sent down to the renderer (and
          therefore subresource requests from the renderer will come
          *after* that task).

Bug: 910287
Change-Id: Icb59e7247231373490c6fc79ae0c39a5af1c1a87
Reviewed-on: https://chromium-review.googlesource.com/c/1419015
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624685}
[modify] https://crrev.com/1b43ef7815413edac4400dadfe3701d7818cfd12/content/browser/appcache/appcache_backend_impl.cc
[modify] https://crrev.com/1b43ef7815413edac4400dadfe3701d7818cfd12/content/browser/appcache/appcache_group_unittest.cc
[modify] https://crrev.com/1b43ef7815413edac4400dadfe3701d7818cfd12/content/browser/appcache/appcache_host.cc
[modify] https://crrev.com/1b43ef7815413edac4400dadfe3701d7818cfd12/content/browser/appcache/appcache_host.h
[modify] https://crrev.com/1b43ef7815413edac4400dadfe3701d7818cfd12/content/browser/appcache/appcache_host_unittest.cc
[modify] https://crrev.com/1b43ef7815413edac4400dadfe3701d7818cfd12/content/browser/appcache/appcache_navigation_handle.cc
[modify] https://crrev.com/1b43ef7815413edac4400dadfe3701d7818cfd12/content/browser/appcache/appcache_navigation_handle.h
[modify] https://crrev.com/1b43ef7815413edac4400dadfe3701d7818cfd12/content/browser/appcache/appcache_navigation_handle_core.cc
[modify] https://crrev.com/1b43ef7815413edac4400dadfe3701d7818cfd12/content/browser/appcache/appcache_navigation_handle_core.h
[modify] https://crrev.com/1b43ef7815413edac4400dadfe3701d7818cfd12/content/browser/appcache/appcache_subresource_url_factory.cc
[modify] https://crrev.com/1b43ef7815413edac4400dadfe3701d7818cfd12/content/browser/appcache/appcache_unittest.cc
[modify] https://crrev.com/1b43ef7815413edac4400dadfe3701d7818cfd12/content/browser/appcache/appcache_update_job_unittest.cc
[modify] https://crrev.com/1b43ef7815413edac4400dadfe3701d7818cfd12/content/browser/frame_host/navigation_handle_impl.cc
[modify] https://crrev.com/1b43ef7815413edac4400dadfe3701d7818cfd12/content/browser/loader/cross_site_document_blocking_browsertest.cc
[modify] https://crrev.com/1b43ef7815413edac4400dadfe3701d7818cfd12/content/browser/worker_host/shared_worker_service_impl.cc

Commit 1b43ef78... initially landed in 73.0.3680.0.

So far there is 1 kill (so, ranked #70+) present :-/ (with the following magic signature: [Renderer kill 123] mojo::`anonymous namespace\'::RunErrorCallback - APPCACHE_SUBRESOURCE_URL_FACTORY_INVALID_INITIATO).

killed_process_origin_lock = https://amazon.com/

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

commit adef07f5fb6e55b01f5396631a90d00faf8b0f8b
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Tue Jan 22 19:23:15 2019

Diagnostics for the APP_CACHE_SUBRESOURCE_URL_FACTORY... kill.

This CL adds crash keys that should hopefully make it easier to
understand the kills that were apparently introduced by r623505.

Bug: 910287
Change-Id: Ia70fb4d89bb9a6b7b414bb44602e2f86160bb544
Reviewed-on: https://chromium-review.googlesource.com/c/1418391
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Auto-Submit: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624867}
[modify] https://crrev.com/adef07f5fb6e55b01f5396631a90d00faf8b0f8b/content/browser/appcache/appcache_subresource_url_factory.cc