content/browser/frame_host/navigation_request.cc - Issue 2009453002: service worker: Don't control a subframe of an insecure context

Unified Diff: content/browser/frame_host/navigation_request.cc

Issue 2009453002: service worker: Don't control a subframe of an insecure context (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: getSecurityOrigin Created 4 years, 7 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« no previous file with comments | « no previous file | content/child/service_worker/service_worker_network_provider.h » ('j') | content/child/service_worker/service_worker_network_provider.h » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: content/browser/frame_host/navigation_request.cc

diff --git a/content/browser/frame_host/navigation_request.cc b/content/browser/frame_host/navigation_request.cc

index b8ef2b115fbab3577b2e5ba6512eb91817b09bde..17e42682553082f6df284c6416f8545bba084b4e 100644

--- a/content/browser/frame_host/navigation_request.cc

+++ b/content/browser/frame_host/navigation_request.cc

@@ -301,6 +301,7 @@ void NavigationRequest::OnResponseStarted(

}

// Update the service worker params of the request params.

+ // TODO(falken): This should do an isSecureContext check too.

jww 2016/05/24 18:06:03 To ask a dumb question, why not add that check rig

falken 2016/05/25 09:19:58 I didn't look too hard, but there's no WebFrame he

clamy 2016/05/25 11:21:46 If you're checking for the sandbox flags, you can

falken 2016/05/26 02:09:49 I want to do an ancestor walk over the frame tree,

clamy 2016/05/26 15:45:31 FrameTreeNodes do allow you to walk over the Frame

alexmos 2016/05/26 22:13:35 Yeah, you should be able to follow the FrameTreeNo

Yeah, you should be able to follow the FrameTreeNode::parent() chain and do the check on FrameTreeNode::current_origin(). You'd need to do some additional work to get isSecureContext's scheme whitelist here, but seems like secureContextBypassingSchemes() ultimately comes from GetSchemesBypassingSecureContextCheckWhitelist(), which is in chrome/common, so available in the browser process. So it's actually isn't that bad, but you'd need some extra delegate plumbing to get to it from content/. What seems harder is that the origin we have on FrameTreeNodes is a url::Origin, which doesn't have all the information from a SecurityOrigin. We do have a trustworthiness bit in FrameTreeNode::current_replication_state(), has_potentially_trustworthy_unique_origin, but it isn't actually equivalent to SecurityOrigin::isPotentiallyTrustworthy. It's only meaningful for unique origins (i.e., it indicates potential trustworthiness for a sandboxed iframe), and you'd still need to either do all the other work in SecurityOrigin::isPotentiallyTrustworthy() in the browser process, or replicate the full value of isPotentiallyTrustworthy() from the renderer. Overall, all this seems non-trivial and I'd vote for doing it in a separate CL. CC-ing estark@, who's worked on isSecureContext plumbing recently and may have more thoughts on getting it to work from the browser process.

falken 2016/05/27 05:47:03 Sorry yes I meant "FrameTreeNode doesn't have the

On 2016/05/26 22:13:35, alexmos wrote: > On 2016/05/26 15:45:31, clamy wrote: > > On 2016/05/26 02:09:49, falken wrote: > > > On 2016/05/25 11:21:46, clamy wrote: > > > > On 2016/05/25 09:19:58, falken wrote: > > > > > On 2016/05/24 18:06:03, jww wrote: > > > > > > To ask a dumb question, why not add that check right now? > > > > > > > > > > I didn't look too hard, but there's no WebFrame here. clamy, how hard > > would > > > it > > > > > be to add a check here like the one in > service_worker_navigation_provider? > > > > > > > > If you're checking for the sandbox flags, you can get them from the > > > > FrameTreeNode that NavigationRequest keeps a pointer to. > > > > > > I want to do an ancestor walk over the frame tree, checking their security > (I > > > think FrameTreeNode doesn't have that info). But I think the latest patch > > > changed this so service_worker_network_provider can handle the PlzNavigate > > case > > > well. > > > > FrameTreeNodes do allow you to walk over the FrameTree. Each of them has a > > FrameTreeNodeReplicationState that has some security info, including the > origin > > (and whether it's potentially trustworthy). The only thing missing to do a > check > > that matches the one in RenderFrame is the list of whitelisted urls. I'm not > > sure where you'd find that in the renderer. > > Yeah, you should be able to follow the FrameTreeNode::parent() chain and do the > check on FrameTreeNode::current_origin(). You'd need to do some additional work > to get isSecureContext's scheme whitelist here, but seems like > secureContextBypassingSchemes() ultimately comes from > GetSchemesBypassingSecureContextCheckWhitelist(), which is in chrome/common, so > available in the browser process. So it's actually isn't that bad, but you'd > need some extra delegate plumbing to get to it from content/. > > What seems harder is that the origin we have on FrameTreeNodes is a url::Origin, > which doesn't have all the information from a SecurityOrigin. We do have a > trustworthiness bit in FrameTreeNode::current_replication_state(), > has_potentially_trustworthy_unique_origin, but it isn't actually equivalent to > SecurityOrigin::isPotentiallyTrustworthy. It's only meaningful for unique > origins (i.e., it indicates potential trustworthiness for a sandboxed iframe), > and you'd still need to either do all the other work in > SecurityOrigin::isPotentiallyTrustworthy() in the browser process, or replicate > the full value of isPotentiallyTrustworthy() from the renderer. Overall, all > this seems non-trivial and I'd vote for doing it in a separate CL. > > CC-ing estark@, who's worked on isSecureContext plumbing recently and may have > more thoughts on getting it to work from the browser process.

Sorry yes I meant "FrameTreeNode doesn't have the security info". But I think the latest patchset is OK doing the walk renderer-side, for the PlzNavigation case?

clamy 2016/05/30 16:22:42 Probably. In any case, we'll have to make signific

On 2016/05/27 05:47:03, falken wrote: > On 2016/05/26 22:13:35, alexmos wrote: > > On 2016/05/26 15:45:31, clamy wrote: > > > On 2016/05/26 02:09:49, falken wrote: > > > > On 2016/05/25 11:21:46, clamy wrote: > > > > > On 2016/05/25 09:19:58, falken wrote: > > > > > > On 2016/05/24 18:06:03, jww wrote: > > > > > > > To ask a dumb question, why not add that check right now? > > > > > > > > > > > > I didn't look too hard, but there's no WebFrame here. clamy, how hard > > > would > > > > it > > > > > > be to add a check here like the one in > > service_worker_navigation_provider? > > > > > > > > > > If you're checking for the sandbox flags, you can get them from the > > > > > FrameTreeNode that NavigationRequest keeps a pointer to. > > > > > > > > I want to do an ancestor walk over the frame tree, checking their security > > (I > > > > think FrameTreeNode doesn't have that info). But I think the latest patch > > > > changed this so service_worker_network_provider can handle the PlzNavigate > > > case > > > > well. > > > > > > FrameTreeNodes do allow you to walk over the FrameTree. Each of them has a > > > FrameTreeNodeReplicationState that has some security info, including the > > origin > > > (and whether it's potentially trustworthy). The only thing missing to do a > > check > > > that matches the one in RenderFrame is the list of whitelisted urls. I'm not > > > sure where you'd find that in the renderer. > > > > Yeah, you should be able to follow the FrameTreeNode::parent() chain and do > the > > check on FrameTreeNode::current_origin(). You'd need to do some additional > work > > to get isSecureContext's scheme whitelist here, but seems like > > secureContextBypassingSchemes() ultimately comes from > > GetSchemesBypassingSecureContextCheckWhitelist(), which is in chrome/common, > so > > available in the browser process. So it's actually isn't that bad, but you'd > > need some extra delegate plumbing to get to it from content/. > > > > What seems harder is that the origin we have on FrameTreeNodes is a > url::Origin, > > which doesn't have all the information from a SecurityOrigin. We do have a > > trustworthiness bit in FrameTreeNode::current_replication_state(), > > has_potentially_trustworthy_unique_origin, but it isn't actually equivalent to > > SecurityOrigin::isPotentiallyTrustworthy. It's only meaningful for unique > > origins (i.e., it indicates potential trustworthiness for a sandboxed iframe), > > and you'd still need to either do all the other work in > > SecurityOrigin::isPotentiallyTrustworthy() in the browser process, or > replicate > > the full value of isPotentiallyTrustworthy() from the renderer. Overall, all > > this seems non-trivial and I'd vote for doing it in a separate CL. > > > > CC-ing estark@, who's worked on isSecureContext plumbing recently and may have > > more thoughts on getting it to work from the browser process. > > Sorry yes I meant "FrameTreeNode doesn't have the security info". But I think > the latest patchset is OK doing the walk renderer-side, for the PlzNavigation > case?

Probably. In any case, we'll have to make significant changes to how PlzNavigate works with ServiceWorker, so it's quite possible that this code will go away anyway.

request_params_.should_create_service_worker =

(frame_tree_node_->pending_sandbox_flags() &

blink::WebSandboxFlags::Origin) != blink::WebSandboxFlags::Origin;