Process-wide URLLoaderFactory going forward

[Łukasz Anforowicz]

Hello loading-dev@ / navigation-dev@ / network-service-dev@,

After r783649 the blink::Platform::CreateDefaultURLLoaderFactory is gone and therefore Blink code always has to go through a frame/origin-specific blink::LoaderFactoryForFrame::CreateURLLoader (or whatever equivalent is used for workers) - yay!  Nevertheless, the origin/frame/worker-agnostic content::RendererBlinkPlatformImpl::CreateNetworkURLLoaderFactory is still around, and may end up getting used in some circumstances - AFAICT the scenarios when this happens require that 1) a frame has not committed any navigation yet and 2) a frame doesn't have a creator/opener/parent to inherit a factory from.  https://crbug.com/1098938 mentions two scenarios where such a factory may be used (portals + DevTools), but it feels that there are quite likely many other similar, legitimate scenarios.

The main problem with content::RendererBlinkPlatformImpl::CreateNetworkURLLoaderFactory is that URLLoaderFactoryParamsHelper::CreateForRendererProcess has a hard time coming up with sensible values for URLLoaderFactoryParams.  My main objective is to initialize request_initiator_site_lock to a secure value, but there are other things that URLLoaderFactoryParamsHelper::CreateForRendererProcess has to "make up" in the absence of a frame to take the values from:

• |URLLoaderFactoryParams::request_initiator_site_lock| (currently based on renderer process lock, which may be base::nullopt on Android, which opens a way for an attacked to bypass CORB/CORP and/or spoof Sec-Fetch-Site - see "Android-specific protection gaps" in //docs/security/compromised-renderers.md)
• |origin| to communicate to ChromeContentBrowserClient::OverrideURLLoaderFactoryParams (used by extensions - shouldn't matter in frames that didn't yet commit an extension origin)
• |URLLoaderFactoryParams::top_frame_id| (currently always base::nullopt)
• |URLLoaderFactoryParams::isolation_info| (currently always net::IsolationInfo::CreateTransient())
  
• |URLLoaderFactoryParams::client_security_state| (currently always nullptr_
  
• |URLLoaderFactoryParams::coep_reporter| (currently always mojom::NullRemote())
  
• |URLLoaderFactoryParams::cookie_observer| (currently always mojom::NullRemote())
  
• ...

So far, whenever the "process-wide" factory is used, I've observed that network::ResourceRequest::request_initiator is always set to an opaque, unique origin.  Therefore, maybe I should try setting |URLLoaderFactoryParams::request_initiator_site_lock| of such "process-wide" factory to an opaque, unique origin?  (this would fix CORB/CORP bypasses and Sec-Fetch-Site spoofability on Android).

QUESTION1: Is it reasonable to expect that the "process-wide" factory will always have some legitimate uses?  Or should we try to stomp out all such uses and eventually get rid of the process-wide factory?

QUESTION2: Is it reasonable to set |request_initiator_site_lock| to an opaque/unique origin for the process-wide factory?  Are there any scenarios where a legitimate request_initiator would be non-null / non-opaque?

FWIW, in M85 we will attempt to ship |request_initiator_site_lock| enforcement to stable (see r781013) - the enforcement should hopefully be sufficient to flush out any bugs/scenarios where request_initiator is unexpectedly non-null / non-opaque.

Thanks,

Lukasz

[Kinuko Yasuda]

On Tue, Jun 30, 2020 at 6:43 AM Łukasz Anforowicz <luk...@chromium.org> wrote:

Hello loading-dev@ / navigation-dev@ / network-service-dev@,

After r783649 the blink::Platform::CreateDefaultURLLoaderFactory is gone and therefore Blink code always has to go through a frame/origin-specific blink::LoaderFactoryForFrame::CreateURLLoader (or whatever equivalent is used for workers) - yay!

This is really great!!

 

  Nevertheless, the origin/frame/worker-agnostic content::RendererBlinkPlatformImpl::CreateNetworkURLLoaderFactory is still around, and may end up getting used in some circumstances - AFAICT the scenarios when this happens require that 1) a frame has not committed any navigation yet and 2) a frame doesn't have a creator/opener/parent to inherit a factory from.  https://crbug.com/1098938 mentions two scenarios where such a factory may be used (portals + DevTools), but it feels that there are quite likely many other similar, legitimate scenarios.

The main problem with content::RendererBlinkPlatformImpl::CreateNetworkURLLoaderFactory is that URLLoaderFactoryParamsHelper::CreateForRendererProcess has a hard time coming up with sensible values for URLLoaderFactoryParams.  My main objective is to initialize request_initiator_site_lock to a secure value, but there are other things that URLLoaderFactoryParamsHelper::CreateForRendererProcess has to "make up" in the absence of a frame to take the values from:

• |URLLoaderFactoryParams::request_initiator_site_lock| (currently based on renderer process lock, which may be base::nullopt on Android, which opens a way for an attacked to bypass CORB/CORP and/or spoof Sec-Fetch-Site - see "Android-specific protection gaps" in //docs/security/compromised-renderers.md)
• |origin| to communicate to ChromeContentBrowserClient::OverrideURLLoaderFactoryParams (used by extensions - shouldn't matter in frames that didn't yet commit an extension origin)
• |URLLoaderFactoryParams::top_frame_id| (currently always base::nullopt)
• |URLLoaderFactoryParams::isolation_info| (currently always net::IsolationInfo::CreateTransient())
  
• |URLLoaderFactoryParams::client_security_state| (currently always nullptr_
  
• |URLLoaderFactoryParams::coep_reporter| (currently always mojom::NullRemote())
  
• |URLLoaderFactoryParams::cookie_observer| (currently always mojom::NullRemote())
  
• ...

So far, whenever the "process-wide" factory is used, I've observed that network::ResourceRequest::request_initiator is always set to an opaque, unique origin.  Therefore, maybe I should try setting |URLLoaderFactoryParams::request_initiator_site_lock| of such "process-wide" factory to an opaque, unique origin?  (this would fix CORB/CORP bypasses and Sec-Fetch-Site spoofability on Android).

QUESTION1: Is it reasonable to expect that the "process-wide" factory will always have some legitimate uses?  Or should we try to stomp out all such uses and eventually get rid of the process-wide factory?

Ideally I'd really like to get rid of this, but it's unclear to me if we can do so for all cases (e.g. on about:blank, and when no factories can be derived).  I wonder if we could at least get rid of blanket process-wide factory callsites... e.g. it feels it's better if we have reason/purpose-specific methods and do the validation on the browser side too.

 

QUESTION2: Is it reasonable to set |request_initiator_site_lock| to an opaque/unique origin for the process-wide factory?  Are there any scenarios where a legitimate request_initiator would be non-null / non-opaque?

From what we know it seems it could be legitimate?  Could we maybe add DumpWithoutCrashing to see if there could be use cases where non-opaque origin is expected?

[Nasko Oskov]

On Tue, Jun 30, 2020 at 1:34 AM Kinuko Yasuda <kin...@chromium.org> wrote:

On Tue, Jun 30, 2020 at 6:43 AM Łukasz Anforowicz <luk...@chromium.org> wrote:

Hello loading-dev@ / navigation-dev@ / network-service-dev@,

After r783649 the blink::Platform::CreateDefaultURLLoaderFactory is gone and therefore Blink code always has to go through a frame/origin-specific blink::LoaderFactoryForFrame::CreateURLLoader (or whatever equivalent is used for workers) - yay!

This is really great!!

 

  Nevertheless, the origin/frame/worker-agnostic content::RendererBlinkPlatformImpl::CreateNetworkURLLoaderFactory is still around, and may end up getting used in some circumstances - AFAICT the scenarios when this happens require that 1) a frame has not committed any navigation yet and 2) a frame doesn't have a creator/opener/parent to inherit a factory from.  https://crbug.com/1098938 mentions two scenarios where such a factory may be used (portals + DevTools), but it feels that there are quite likely many other similar, legitimate scenarios.

The main problem with content::RendererBlinkPlatformImpl::CreateNetworkURLLoaderFactory is that URLLoaderFactoryParamsHelper::CreateForRendererProcess has a hard time coming up with sensible values for URLLoaderFactoryParams.  My main objective is to initialize request_initiator_site_lock to a secure value, but there are other things that URLLoaderFactoryParamsHelper::CreateForRendererProcess has to "make up" in the absence of a frame to take the values from:

• |URLLoaderFactoryParams::request_initiator_site_lock| (currently based on renderer process lock, which may be base::nullopt on Android, which opens a way for an attacked to bypass CORB/CORP and/or spoof Sec-Fetch-Site - see "Android-specific protection gaps" in //docs/security/compromised-renderers.md)
• |origin| to communicate to ChromeContentBrowserClient::OverrideURLLoaderFactoryParams (used by extensions - shouldn't matter in frames that didn't yet commit an extension origin)
• |URLLoaderFactoryParams::top_frame_id| (currently always base::nullopt)
• |URLLoaderFactoryParams::isolation_info| (currently always net::IsolationInfo::CreateTransient())
  
• |URLLoaderFactoryParams::client_security_state| (currently always nullptr_
  
• |URLLoaderFactoryParams::coep_reporter| (currently always mojom::NullRemote())
  
• |URLLoaderFactoryParams::cookie_observer| (currently always mojom::NullRemote())
  
• ...

So far, whenever the "process-wide" factory is used, I've observed that network::ResourceRequest::request_initiator is always set to an opaque, unique origin.  Therefore, maybe I should try setting |URLLoaderFactoryParams::request_initiator_site_lock| of such "process-wide" factory to an opaque, unique origin?  (this would fix CORB/CORP bypasses and Sec-Fetch-Site spoofability on Android).

QUESTION1: Is it reasonable to expect that the "process-wide" factory will always have some legitimate uses?  Or should we try to stomp out all such uses and eventually get rid of the process-wide factory?

Ideally I'd really like to get rid of this, but it's unclear to me if we can do so for all cases (e.g. on about:blank, and when no factories can be derived). 

Can we clarify what is the scenario where we have about:blank and have no way of deriving factories? The only case I can think of is browser-initiated navigation to about:blank that the user explicitly does through the omnibox. In that case, however, we generate factories from the browser process and can give it opaque and unique origin. The initial empty document is a special case of about:blank, which requires either having a creator/initiator or should be equivalent to the explicit omnibox navigation in terms of using opaque and unique origin.

 

I wonder if we could at least get rid of blanket process-wide factory callsites... e.g. it feels it's better if we have reason/purpose-specific methods and do the validation on the browser side too.

+100!

[Łukasz Anforowicz]

On Tue, Jun 30, 2020 at 11:22 AM Nasko Oskov <na...@chromium.org> wrote:

On Tue, Jun 30, 2020 at 1:34 AM Kinuko Yasuda <kin...@chromium.org> wrote:

On Tue, Jun 30, 2020 at 6:43 AM Łukasz Anforowicz <luk...@chromium.org> wrote:

Hello loading-dev@ / navigation-dev@ / network-service-dev@,

After r783649 the blink::Platform::CreateDefaultURLLoaderFactory is gone and therefore Blink code always has to go through a frame/origin-specific blink::LoaderFactoryForFrame::CreateURLLoader (or whatever equivalent is used for workers) - yay!

This is really great!!

 

  Nevertheless, the origin/frame/worker-agnostic content::RendererBlinkPlatformImpl::CreateNetworkURLLoaderFactory is still around, and may end up getting used in some circumstances - AFAICT the scenarios when this happens require that 1) a frame has not committed any navigation yet and 2) a frame doesn't have a creator/opener/parent to inherit a factory from.  https://crbug.com/1098938 mentions two scenarios where such a factory may be used (portals + DevTools), but it feels that there are quite likely many other similar, legitimate scenarios.

The main problem with content::RendererBlinkPlatformImpl::CreateNetworkURLLoaderFactory is that URLLoaderFactoryParamsHelper::CreateForRendererProcess has a hard time coming up with sensible values for URLLoaderFactoryParams.  My main objective is to initialize request_initiator_site_lock to a secure value, but there are other things that URLLoaderFactoryParamsHelper::CreateForRendererProcess has to "make up" in the absence of a frame to take the values from:

• |URLLoaderFactoryParams::request_initiator_site_lock| (currently based on renderer process lock, which may be base::nullopt on Android, which opens a way for an attacked to bypass CORB/CORP and/or spoof Sec-Fetch-Site - see "Android-specific protection gaps" in //docs/security/compromised-renderers.md)
• |origin| to communicate to ChromeContentBrowserClient::OverrideURLLoaderFactoryParams (used by extensions - shouldn't matter in frames that didn't yet commit an extension origin)
• |URLLoaderFactoryParams::top_frame_id| (currently always base::nullopt)
• |URLLoaderFactoryParams::isolation_info| (currently always net::IsolationInfo::CreateTransient())
  
• |URLLoaderFactoryParams::client_security_state| (currently always nullptr_
  
• |URLLoaderFactoryParams::coep_reporter| (currently always mojom::NullRemote())
  
• |URLLoaderFactoryParams::cookie_observer| (currently always mojom::NullRemote())
  
• ...

So far, whenever the "process-wide" factory is used, I've observed that network::ResourceRequest::request_initiator is always set to an opaque, unique origin.  Therefore, maybe I should try setting |URLLoaderFactoryParams::request_initiator_site_lock| of such "process-wide" factory to an opaque, unique origin?  (this would fix CORB/CORP bypasses and Sec-Fetch-Site spoofability on Android).

QUESTION1: Is it reasonable to expect that the "process-wide" factory will always have some legitimate uses?  Or should we try to stomp out all such uses and eventually get rid of the process-wide factory?

Ideally I'd really like to get rid of this, but it's unclear to me if we can do so for all cases (e.g. on about:blank, and when no factories can be derived). 

Can we clarify what is the scenario where we have about:blank and have no way of deriving factories?

Please see "https://crbug.com/1098938 mentions two scenarios where such a factory may be used (portals + DevTools)" mentioned above :-).  The bug links to a document that lists callstacks.

 

The only case I can think of is browser-initiated navigation to about:blank that the user explicitly does through the omnibox. In that case, however, we generate factories from the browser process and can give it opaque and unique origin. The initial empty document is a special case of about:blank, which requires either having a creator/initiator or should be equivalent to the explicit omnibox navigation in terms of using opaque and unique origin.

 

I wonder if we could at least get rid of blanket process-wide factory callsites... e.g. it feels it's better if we have reason/purpose-specific methods and do the validation on the browser side too.

+100!

I guess the CSP violations (and other reporting situations) can initiate reporting HTTP requests from the browser-side.

I think the DevTools requests end up targeting a provisional frame - maybe their timing can be changed?  Or maybe we can get rid of provisional frames altogether?

QUESTION2: Is it reasonable to set |request_initiator_site_lock| to an opaque/unique origin for the process-wide factory?  Are there any scenarios where a legitimate request_initiator would be non-null / non-opaque?

From what we know it seems it could be legitimate?  Could we maybe add DumpWithoutCrashing to see if there could be use cases where non-opaque origin is expected?

Based on https://crbug.com/1102347 it seems that indeed, the process-wide factory is sometimes used with a non-opaque initiator... :-(

Unfortunately, the nature of the CHECKs / the nature of mojo means that we didn't get full renderer-side callstacks that initiate such requests.

 

FWIW, in M85 we will attempt to ship |request_initiator_site_lock| enforcement to stable (see r781013) - the enforcement should hopefully be sufficient to flush out any bugs/scenarios where request_initiator is unexpectedly non-null / non-opaque.

Thanks,

Lukasz

--
You received this message because you are subscribed to the Google Groups "navigation-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to navigation-de...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/CAA%3DmyAuH6Eog9YDMfUn0HWjRZV6e2ebPvNX6gPUzXGSv2Zeaww%40mail.gmail.com.

[Łukasz Anforowicz]

Let me share my current plan (hoping that people will give feedback and stop me if there are any concerns with the plan):

It seems that a considerable usage of the process-wide factory comes from provisional frames (and in general RenderFrameImpls that have been created but did not yet commit any navigation).  Given this, I wonder if at the frame creation time (when calling mojom::Renderer::CreateFrame and/or mojom::Renderer::CreateView) the browser process could create a URLLoaderFactory and hand such factory to the renderer process.  This would mean that a RenderFrameImpl is *always* guaranteed to have a factory (and therefore that there is no need to fallback to a process-wide factory (i.e. CreateDefaultURLLoaderFactoryBundle) in RenderFrameImpl::CreateLoaderFactoryBundle).

To create such URLLoaderFactory, the browser process would need to calculate |request_initiator_site_lock| and other URLLoaderFactoryParams.  This seems doable if GetOriginForURLLoaderFactory becomes an instance method of NavigationRequest (I have a CL for this here).  OTOH, currently GetOriginForURLLoaderFactory can only be called at READY_TO_COMMIT or later - this is a challenge, but is feels fixable (currently the final frame is used to 1) find the origin of the parent frame [used for about:srcdoc frames] and 2) double-check the origin via CanAccessDataForOrigin.

If the above doesn't work then I may consider the following fallback:

Once request_initiator_site_lock is enforced, having a mismatched request_initiator may lead to renderer kills (e.g. on Android;  on desktop platforms mojo::ReportBadMessage in NetworkService doesn't lead to renderer kills).  This means that we can't have the process-wide factory set |request_initiator_site_lock| to an opaque origin, unless we can guarantee that request_initiator will always be an opaque origin (and https://crbug.com/1102347 shows that requests with non-opaque initiator exist today).  But, maybe as a starting point I could put together a CL that reports callstacks responsible for using the process-wide-factory with a non-opaque request_initiator origin.

[Łukasz Anforowicz]

Small clarification, because I realized that it isn't quite obvious from my previous emails: By "process-wide" factory, I mean a factory that is used by a renderer process and not associated with a specific frame / worker / execution context (i.e. a factory that is obtained via RenderProcessHostImpl::CreateURLLoaderFactoryForRendererProcess + CreateDefaultURLLoaderFactory-from-render_frame_impl.cc).  In particular, this email is *not* about StoragePartitionImpl::GetURLLoaderFactoryForBrowserProcess in the browser process (which is also not associated with a specific origin / frame / etc and therefore kind of process-widestorage-partition-wide).