Appetite for removing RenderFrame::GetRoutingID

[Dave Tapuska]

I've been working on removing legacy IPC (extensions has had the new mojo channel for about a week now in Canary) and Android/Android WebView started a finch trial today.

I've been thinking about the RenderFrame::GetRoutingID and the sync IPC call GenerateFrameRoutingID

I think there are roughly 20 callees left that call GetRoutingID and I believe we can move them to some of them to blink::LocalFrameToken if we want.

Roughly 10 of them are NaCl/PPAPI related (as it is the only real remaining legacy IPC channel now). We will have to support NaCl/PPAPI until Jan 2025 for ChromeOS still.

What do you think about conditionally supporting Legacy IPC/GetRoutingID behind a compilation flag? I imagine something BUILDFLAG(CONTENT_LEGACY_IPC_SUPPORT). 

If we no longer support the GetRoutingID we can actually move iframe creation to be entirely async but that does mean we would have a slightly different flow.

I also feel that we'd likely be able to disable legacy IPC between the renderer and the browser with this flag once we've found appropriate replacements for RenderThread::GetChannel and RenderThreadObserver::RegisterMojoInterfaces which inherently are also problematic for MBI.

dave.

[Kentaro Hara]

I think there are roughly 20 callees left that call GetRoutingID and I believe we can move them to some of them to blink::LocalFrameToken if we want.

We are (slowly) moving in the direction of replacing the routing ID with a frame token, so +1 to this change.

What do you think about conditionally supporting Legacy IPC/GetRoutingID behind a compilation flag? I imagine something BUILDFLAG(CONTENT_LEGACY_IPC_SUPPORT). 

I love this idea!

This is a moment -- thank you very much for removing the legacy IPC finally. You have been doing heroic work leading Igalia TVCs and then working on the remaining ones yourself :)

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

--

Kentaro Hara, Tokyo

[Daniel Cheng]

On Thu, 9 Nov 2023 at 21:38, Kentaro Hara <har...@chromium.org> wrote:

I think there are roughly 20 callees left that call GetRoutingID and I believe we can move them to some of them to blink::LocalFrameToken if we want.

We are (slowly) moving in the direction of replacing the routing ID with a frame token, so +1 to this change.

What do you think about conditionally supporting Legacy IPC/GetRoutingID behind a compilation flag? I imagine something BUILDFLAG(CONTENT_LEGACY_IPC_SUPPORT). 

I love this idea!

This is a moment -- thank you very much for removing the legacy IPC finally. You have been doing heroic work leading Igalia TVCs and then working on the remaining ones yourself :)

On Thu, Nov 9, 2023 at 11:11 PM Dave Tapuska <dtap...@chromium.org> wrote:

I've been working on removing legacy IPC (extensions has had the new mojo channel for about a week now in Canary) and Android/Android WebView started a finch trial today.

I've been thinking about the RenderFrame::GetRoutingID and the sync IPC call GenerateFrameRoutingID

I think there are roughly 20 callees left that call GetRoutingID and I believe we can move them to some of them to blink::LocalFrameToken if we want.

Roughly 10 of them are NaCl/PPAPI related (as it is the only real remaining legacy IPC channel now). We will have to support NaCl/PPAPI until Jan 2025 for ChromeOS still.

What do you think about conditionally supporting Legacy IPC/GetRoutingID behind a compilation flag? I imagine something BUILDFLAG(CONTENT_LEGACY_IPC_SUPPORT). 

Will this help us ensure we don't reintroduce GetRoutingID() uses? If so, that seems like a good thing to do.

 

If we no longer support the GetRoutingID we can actually move iframe creation to be entirely async but that does mean we would have a slightly different flow.

This is actually already doable, but it was technically a bit icky. Using LocalFrameToken would make this a lot easier.

 

I also feel that we'd likely be able to disable legacy IPC between the renderer and the browser with this flag once we've found appropriate replacements for RenderThread::GetChannel and RenderThreadObserver::RegisterMojoInterfaces which inherently are also problematic for MBI.

dave.

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

--

Kentaro Hara, Tokyo

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jxatqoLL4dMekaYW6woBXKPU_5%2BxHPbf0rtwRfF0H_r1g%40mail.gmail.com.

[Dave Tapuska]

So to move iframe creation to be async, we'd need to pass the LocalFrameToken in from the renderer and validate it somehow in the browser. The frame token needs to be present at the time of creation of the WebLocalFrame, so an async response from the browser I don't think would work.

Thinking about how to validate I'd love some ideas.

We want to make sure that we didn't receive a FrameToken for frame that previously existed in the renderer. (We store the RenderProcessHost::ID in the GlobalRenderFrameHostToken so it's uniqueness per RenderProcessHost should be good). But what we want to prevent is someone storing a GlobalRenderFrameHostToken somewhere, and then the RenderFrame getting deleted and then a new one getting created with the exact same FrameToken from a compromised renderer.

The sync IPC design for the routing ID prevents this because the CreateChildFrame (which takes a routing_id) ensures that the handled out routing_id is in a table of pending routing_ids

Some ideas I thought of:

1) Store every encountered LocalFrameToken (unbounded memory) on a RenderProcessHost table.

2) Store the routing_id in GlobalRenderFrameHostFrameToken as well. Don't allow creation of GlobalRenderFrameHostFrameToken without a routing_id but do allow lookup of a RenderFrameHost from a process ID and frame token. This still seems problematic if people store the process ID and frame token separately, they could get the wrong frame again. So I don't care for this idea.

3) Other ideas?

dave. 

[Łukasz Anforowicz]

On Thu, Nov 16, 2023 at 2:11 PM Dave Tapuska <dtap...@chromium.org> wrote:

So to move iframe creation to be async, we'd need to pass the LocalFrameToken in from the renderer and validate it somehow in the browser. The frame token needs to be present at the time of creation of the WebLocalFrame, so an async response from the browser I don't think would work.

Thinking about how to validate I'd love some ideas.

We want to make sure that we didn't receive a FrameToken for frame that previously existed in the renderer. (We store the RenderProcessHost::ID in the GlobalRenderFrameHostToken so it's uniqueness per RenderProcessHost should be good). But what we want to prevent is someone storing a GlobalRenderFrameHostToken somewhere, and then the RenderFrame getting deleted and then a new one getting created with the exact same FrameToken from a compromised renderer.

The sync IPC design for the routing ID prevents this because the CreateChildFrame (which takes a routing_id) ensures that the handled out routing_id is in a table of pending routing_ids

Some ideas I thought of:

1) Store every encountered LocalFrameToken (unbounded memory) on a RenderProcessHost table.

2) Store the routing_id in GlobalRenderFrameHostFrameToken as well. Don't allow creation of GlobalRenderFrameHostFrameToken without a routing_id but do allow lookup of a RenderFrameHost from a process ID and frame token. This still seems problematic if people store the process ID and frame token separately, they could get the wrong frame again. So I don't care for this idea.

Can we include a renderer/process-specific snippet in the frame token to work around that concern?  For example, the browser process could generate a random renderer-unique-prefix at runtime and pass it to the associated renderer process.  Then we can require that frame tokens from this specific renderer process include the renderer-unique-prefix.

 

3) Other ideas?

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/CAHgVhZX1r16TksgXNphJeM1%2BzjN1R9OD1L-ubf-qczFC%3DdnO7Q%40mail.gmail.com.

[Dave Tapuska]

The concern is not that another renderer can forge tokens matching some other process. We already prevent that by having the render process id used part of the identifier. It is that a renderer could replay the same token if we change the ipc such that the renderer provides the token. 

Currently with the current ipcs this is prevented as I indicated. But we'd ideally like to remove the sync ipc (to the browser io thread) when iframes are created. 

Dave

[Łukasz Anforowicz]

Can we require and enforce that the renderer-provided tokens are monotonically increasing?  E.g. the token can include 2 parts:

1. renderer-process-specific-part (to prevent developers from stashing the token and accidentally using it with another process_id)

2. renderer-provided-monotonically-increasing-part (to prevent the renderer from being able to reissue the token;  I assume that the browser can remember the highest monotonically-increasing-part it has so far received from the given RenderProcessHost)

?

[Kentaro Hara]

Can we require and enforce that the renderer-provided tokens are monotonically increasing?  E.g. the token can include 2 parts:
1. renderer-process-specific-part (to prevent developers from stashing the token and accidentally using it with another process_id)
2. renderer-provided-monotonically-increasing-part (to prevent the renderer from being able to reissue the token;  I assume that the browser can remember the highest monotonically-increasing-part it has so far received from the given RenderProcessHost)
?

This will work for routing IDs, but doesn't the frame token need to be generated by the browser process in an ungessable way?

The frame token needs to be present at the time of creation of the WebLocalFrame, so an async response from the browser I don't think would work.

Would you help me understand why WebLocalFrame needs the frame token in the constructor? Would there be any option to split some part of the constructor into WebLocalFrame::Initialize() (which is invoked asynchronously when the renderer gets the frame token) and initialize the frame token there?

--

Kentaro Hara, Tokyo

[Dave Tapuska]

We pass the LocalFrameToken into blink::LocalFrame constructor which I presume needs to be created so we can synchronously continue executing the javascript. 

We use LocalFrameToken / FrameToken in a number of spots and I think an asynchronous API there would make it difficult to reason about. 

I don't mind the idea of using a monotonically increasing FrameToken. It gets away from it being unguessable because we decrease the number of bits, currently it is 128 bits, but if we could reserve the top 32 or so for a counter, then it could uniquely be enforced by the browser.

dave.

[Kevin McNee]

Would hashing a counter salted with a random value generated by the browser on renderer process creation help? Then the browser and renderer can independently generate the same sequence of tokens without an obvious pattern.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/CAHgVhZVxii0jMTPVUkygXf_WVkm4q2UqrZypMi%2B%2BECZ-ww-Vaw%40mail.gmail.com.

[Dave Tapuska]

I've summarized my thoughts in a design document so they don't get lost before any of my upcoming vacation time. Feel free to leave comments on the document. 

dave. 

[Dave Tapuska]

Going to ping this thread again. I haven't received any comments on my document. I've almost removed all instances of routing IDs in the renderer when legacy IPC isn't used (modulo the 3 changes in flight). That means if we had a system where the FrameToken was generated on the renderer side (and validated by the browser, we could indeed remove the sync IPC). So I'm looking for some feedback.

The remaining fixes are:

https://chromium-review.googlesource.com/c/chromium/src/+/5200555

https://chromium-review.googlesource.com/c/chromium/src/+/5201195

https://chromium-review.googlesource.com/c/chromium/src/+/5182794

dave.

[Kentaro Hara]

I fully support the work of removing the sync IPC from the frame creation path. I defer the decision about the technical token design to the CSA team :)

--

Kentaro Hara, Tokyo

[Charlie Reis]

Thanks for the ping.  I think it would be incredible to remove routing IDs and especially the slow sync IPCs used to get them.  I've chimed in on the doc about token design, and would be curious for @Daniel Cheng's input there as well.  Hopefully we can use something that doesn't make collisions any more likely.

Charlie

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/navigation-dev/CABg10jwRHeFfCm3PFC7YvsqTXXnJ3Uou-ssJXbsOjDpp4tS-Ew%40mail.gmail.com.