ExtensionURLLoaderFactory holds on to a render process host for navigations which may be wrong |
|||||
Issue descriptionI just noticed that in the case of navigations, ExtensionURLLoaderFactory is initialized with the render process of the initial process. However this can change later, if the navigation redirects cross origin for example and we switch the RenderFrameHost that is used. In that case, ExtensionURLLoaderFactory will have the wrong render process host and that could lead to security checks being incorrect. Since this class is also used by service workers (and soon shared workers), we will need it to either have a RenderProcessHost* (for service worker/shared worker case) OR a FrameTreeNode* (for subresource in a committed frame case).
,
May 8 2018
Is there a reason why it needs to keep process information at all? Isn't it supposed to just vend URLLoaders and be abstracted away from processes?
,
May 8 2018
I've added a security section in https://docs.google.com/document/d/1wAHLw9h7gGuqJNCgG1mP1BmLtCGfZ2pys-PdZQ1vg7M/edit#, please comment there so it's the source of truth. The summary is that for subresources, there are checks that need to happen to know if a particular renderer can request an extension URL.
,
May 8 2018
+Ken as well
,
May 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ea006300beceb1e9750792bc953401ef984d60ab commit ea006300beceb1e9750792bc953401ef984d60ab Author: John Abd-El-Malek <jam@chromium.org> Date: Thu May 10 05:50:46 2018 Don't hold on to process or frame IDs in ExtensionURLLoaderFactory for navigations. The FrameTreeNode's initial RenderFrameHost might change during navigations if a cross-origin redirect happens. So caching the initial RPH/RFH IDs would be incorrect. For the navigation case, the only thing that is needed is the BrowserContext so pass that in directly. The security check (AllowExtensionResourceLoad) isn't needed for navigations, as that's done in ExtensionNavigationThrottle per the comment in the file. This makes ExtensionURLLoaderFactory pass in -1 for the procss ID when the network service is running, to match what ExtensionProtocolHandler does for the non-network-service case. Bug: 840600 Change-Id: Ie23ed9e78f69fd4a8bb7a1a4aa964303db2102cf Reviewed-on: https://chromium-review.googlesource.com/1050812 Commit-Queue: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#557462} [modify] https://crrev.com/ea006300beceb1e9750792bc953401ef984d60ab/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/ea006300beceb1e9750792bc953401ef984d60ab/chrome/browser/chrome_content_browser_client.h [modify] https://crrev.com/ea006300beceb1e9750792bc953401ef984d60ab/chrome/browser/extensions/extension_protocols_unittest.cc [modify] https://crrev.com/ea006300beceb1e9750792bc953401ef984d60ab/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/ea006300beceb1e9750792bc953401ef984d60ab/content/browser/loader/navigation_url_loader_impl.cc [modify] https://crrev.com/ea006300beceb1e9750792bc953401ef984d60ab/content/browser/service_worker/embedded_worker_instance.cc [modify] https://crrev.com/ea006300beceb1e9750792bc953401ef984d60ab/content/public/browser/content_browser_client.cc [modify] https://crrev.com/ea006300beceb1e9750792bc953401ef984d60ab/content/public/browser/content_browser_client.h [modify] https://crrev.com/ea006300beceb1e9750792bc953401ef984d60ab/extensions/browser/extension_protocols.cc [modify] https://crrev.com/ea006300beceb1e9750792bc953401ef984d60ab/extensions/browser/extension_protocols.h [modify] https://crrev.com/ea006300beceb1e9750792bc953401ef984d60ab/extensions/shell/browser/shell_content_browser_client.cc [modify] https://crrev.com/ea006300beceb1e9750792bc953401ef984d60ab/extensions/shell/browser/shell_content_browser_client.h
,
May 10 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by jam@chromium.org
, May 8 2018Status: start (was: Assigned)