Re: Getting a process id from a frame_tree_node_id in //chrome

[Daniel Cheng]

Can you give some more background on what you're trying to do? The comment specifically notes this doesn't take a RFH or RPH ID because that can change.

Can you verify the security properties when the request to navigate is being made (i.e. somewhere in the control flow reached by RenderFrameHostImpl::BeginNavigation)? Navigation requests are always routed through the browser process.

Daniel

On Thu, 18 Apr 2024 at 14:11, 'Robbie McElrath' via navigation-dev <navigat...@chromium.org> wrote:

Hello!

I'm working on a URLLoaderFactory in //chrome that has a FrameTreeNode id (from CreateNonNetworkNavigationURLLoaderFactory), and I want to validate some security properties of the process the request is coming from. I can get the process id via FrameTreeNode::GloballyFindByID(ftn_id)->current_frame_host()->GetProcess(), but FrameTreeNode isn't part of //content/public.

I've seen other code look up the WebContents and iterate over all RFH's to find the one with a matching FTN, but I'm hoping there's something more performant :)

Is there a way to get to a RPH from a frame_tree_node_id in //chrome?

--
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/CAChyaWUb6f2s1Q5SsriPeCuPuqoBDVb9Ki%3DLH91cxe2b-a3cZQ%40mail.gmail.com.

[Robbie McElrath]

Sorry for the noise, sending again hopefully with permission on loading-dev now.

The URLLoaderFactory in question is IsolatedWebAppURLLoaderFactory, which handles all isolated-app: requests. These resources should only be accessible from within the Isolated Web App (IWA) corresponding to the origin of the request, but that isn't actually checked in the loader. Our existing tests worked because we have other mechanisms to block navigations/requests, but we recently realized regular non-IWA pages can request isolated-app: resources.

I was hoping to add a CPSP::HostsOrigin call in IsolatedWebAppURLLoaderFactory::CreateLoaderAndStart (and I suppose on redirects, but our Throttle already blocks those), but that requires knowing which process the request is coming from. I have a prototype of a fix here, which relies on an invalid //content call. We could potentially check for permission during Navigations, but we want this to be checked for all loads, including fetch, service workers, etc... not just navigations.

On Fri, Apr 19, 2024 at 5:19 PM Robbie McElrath <rmce...@google.com> wrote:

The URLLoaderFactory in question is IsolatedWebAppURLLoaderFactory, which handles all isolated-app: requests. These resources should only be accessible from within the Isolated Web App (IWA) corresponding to the origin of the request, but that isn't actually checked in the loader. Our existing tests worked because we have other mechanisms to block navigations/requests, but we recently realized regular non-IWA pages can request isolated-app: resources.

I was hoping to add a CPSP::HostsOrigin call in IsolatedWebAppURLLoaderFactory::CreateLoaderAndStart (and I suppose on redirects, but our Throttle already blocks those), but that requires knowing which process the request is coming from. I have a prototype of a fix here, which relies on an invalid //content call. We could potentially check for permission during Navigations, but we want this to be checked for all loads, including fetch, service workers, etc... not just navigations.

[Robbie McElrath]

Thanks for the response!

When you say current_frame_host() might not be correct, I assume you just mean that if I stored the RFH it might not be correct over the duration of the request? Presumably at any given instant in time, current_frame_host() will return the correct current value?

Also, you suggested plumbing a process id into the URLLoaderFactory. Does that mean that while the RFH can change, the RPH can't?

On Fri, Apr 19, 2024 at 6:00 PM Alex Moshchuk <ale...@chromium.org> wrote:

I wouldn't recommend trying to get a process from a FTN ID here, since as Daniel mentioned, the FTN could've moved on to a different RFH since the request was made, and current_frame_host() may not be correct.  In this case, I wonder if it's possible to plumb the request's process ID from a bit higher up the stack? For example, it looks like it may be available in NavigationURLLoaderImpl::CreateNonNetworkLoaderFactory() as request_info.initiator_process_id (and used there for resolving the document token).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/CAChyaWWF6GhJNsmi3NiWriatJOXmYh-cqKQLJjEmJ67E%2BEZpyg%40mail.gmail.com.

[Daniel Cheng]

I think I'm a little confused here. The original post referenced `CreateNonNetworkNavigationURLLoaderFactory()`. Presumably, this is used to create URL loaders for *navigation* requests. But some of the replies after this discuss URL loaders for *resources*. Is this referring to loading subresources (e.g. loading the src of an <img> tag) or navigations?

If we're concerned about navigations, shouldn't this be checkable somewhere in the BeginNavigation() path? Those checks should happen before any URL loader factory is ever created.

If it's for *subresources*, we shouldn't be providing a URL loader factory for isolated-app: subresources to non-isolated-app pages, right? So why would we need to check?

Daniel

On Mon, 22 Apr 2024 at 18:59, Alex Moshchuk <ale...@chromium.org> wrote:

On Mon, Apr 22, 2024 at 2:43 PM Robbie McElrath <rmce...@google.com> wrote:

Thanks for the response!

When you say current_frame_host() might not be correct, I assume you just mean that if I stored the RFH it might not be correct over the duration of the request? Presumably at any given instant in time, current_frame_host() will return the correct current value?

I just meant that the RFH that started the resource load may no longer be FTN's corrent_frame_host(), since another navigation could've committed and made that RFH pending deletion. If we want to complete the resource load in that case, we should be using the original RFH (now pending deletion) rather than the current one.

 

Also, you suggested plumbing a process id into the URLLoaderFactory. Does that mean that while the RFH can change, the RPH can't?

I'd need to double-check, but I'm assuming in a situation like the one above, request_info.initiator_process_id would correspond to the original RFH's process ID (the one that's making the request), rather than the current_frame_host()'s process (which could be different).

[Robbie McElrath]

Sorry, my explanation wasn't very clear. The goal here is to block all isolated-app: requests that don't come from the app itself, which includes navigations, subresources, and service worker related requests. We register IsolatedWebAppURLLoaderFactory in several places right now because of the different types of isolated-app: requests we need, specifically:

• ContentBrowserClient::CreateNonNetworkNavigationURLLoaderFactory
  
• ContentBrowserClient::RegisterNonNetworkSubresourceURLLoaderFactories
  
• ContentBrowserClient::RegisterNonNetworkWorkerMainResourceURLLoaderFactories
  
• ContentBrowserClient::RegisterNonNetworkServiceWorkerUpdateURLLoaderFactories
• ExtensionsBrowserClient::GetControlledFrameEmbedderURLLoader
  

All of these except RegisterNonNetworkServiceWorkerUpdateURLLoaderFactories have an RPH id and sometimes an FTN id available (RegisterNonNetworkWorkerMainResourceURLLoaderFactories doesn't but I added one in my prototype CL).

We currently always create the URLoaderFactory in each of those locations, but you're right that we don't need to universally create it for subresources. We can fix that.

I'd still feel more confident with a CPSP check though. In addition to fixing the subresource loader creation logic, how about the following:

• Add a initiator_process_id argument to CreateNonNetworkNavigationURLLoaderFactory, initialized from request_info.initiator_process_id
• Add a initiator_process_id argument to RegisterNonNetworkWorkerMainResourceURLLoaderFactories, initialized from worker_process_id in WorkerScriptFetcher::CreateFactoryBundle

IsolatedWebAppURLLoaderFactory would then always have an initiator process id (unless it's CreateForBrowser), which it can save and use for a CPSP::HostsOrigin check at the start of each request it handles.

[Robbie McElrath]

So after thinking more about this last week, I think Daniel's right that I should just only create the subresource URLLoaderFactory if we're within an IWA, and always create it for navigation and rely on our existing throttle to enforce navigation rules.

I updated crrev.com/c/5463431 to conditionally create the URLLoaderFactory and optionally lock it to an origin instead of using CPSP checks.