The change in #51 has seemed to introduce a flakiness in the webkit_tests virtual/mojo-loading/http/tests/security/xss-DENIED-iframe-src-alias.html
Failing build: https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Win7%20%28dbg%29/builds/9171
The offending portion had previously been reverted in #48 when it last caused these test errors.
I'm hesitant to revert a larger change like this for a test flake. Could someone look into the cause of the error?
The current architecture trusts the process ID sent by the (untrusted) renderer process. I don't think we can ship this until we fix that. If requests are coming from the renderer through ResourceMessageFilter instead of URLLoaderFactoryImpl, this should be a simple fix, since the filter already knows the child ID to expect.
#58- I thought that's how things are implemented today (RMF implements URLLoaderFactory and that's where we get requester_info for requests), but I'm maybe missing something?
Wow...I'm shocked, but it certainly looks to me like you're right.
That's completely bizarre - it's a 1 line change to rely on the child ID from the MessageFilter code, but for some reason we're not doing that. If https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatcher_host_impl.cc?gsn=OnRequestResource&l=858 used requester_info->child_id() instead of routing_id, we'd be using a trusted ID instead of an untrusted one, right? Or am I missing something? Most other methods get child ID that way, too.
Seems like a similarly simple change to make the Mojo path use the ID from the ResourceMessageFilter.
If I'm right, the old API let a hostile renderer issue requests on behalf of other processes, but not get the result back. The Mojo API lets the hostile renderer issues requests on behalf of other processes, and get the result back, too, so I think it's still a regression?
Oh, it's not? I had thought it was the same as child ID, seeing as it's presumably used for...um...routing communication for a request back to the renderer process.
Anyhow, sorry for the false alarm - you're correct. I think there used to be another name for child id (Process ID? Host ID?) that was used in a number of places in loader, and even if it's gone now, I remain eternally confused.
route_id is associated with frames today. In mojo case we don't rely on route_id for any communication purposes but just for some boookkeepings, and I think neither traditional IPC code does (it basically relies on RMF::Send)?
Mojo code uses mojo::URLLoaderClient and its message pipe (that's established when we get request in RMF), so I *think* it's not possible for hostile renderer to let the response back to wrong process.
One other concern: Cookies. With IPC, if you do document.cookie to set a cookie, and then send an XHR or add a subresource to the document, presumably the cookie will be set before the request is issued (I don't know renderer code, so this is just an assumption about ordering, and also seems like something webdevs probably expect).
With Mojo, we don't have that guarantee, unless we move cookies over to the same pipe as issuing new requests.
There's a similar race with providing cookies, where permissions are set when devtools is opened (I guess?), but that's already racy (I think?) and the failure case isn't as problematic - we just don't send cookie headers to the renderer for that request. Not sure about any other races.
Sorry for raising these issues late in the game, I've just started thinking about how the network process will work in a Mojo world.
Currently, renderer => browser messages (i.e., URLLoader and URLLoaderFactory method calls) are channel-associated, which means they are delivered using the same message channel that the existing ChromeIPC messages (and other channel-associated interfaces) are using. Does this address your concern?
Comment 1 by bugdroid1@chromium.org
, Apr 25 2016