[Connection-Allowlist] Enforcing fetch restrictions in the network service [chromium/src : main]
Cesar Fonseca (Gerrit)
Cesar Fonseca added 1 comment![]( "Open in Gerrit")
Related details
- Shivani Sharma
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shivani Sharma (Gerrit)
Shivani Sharma added 1 comment![]( "Open in Gerrit")
- Mike West
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shivani Sharma (Gerrit)
Shivani Sharma added 1 comment![]( "Open in Gerrit")
// CommitNavigation is sent to the renderer.My understanding is that since this is being sent to the network service before CommitNavigation is sent to the renderer, we are fine with not waiting on a callback. After this the renderer will receive CommitNavigation, process it, send back DidCommitNavigation and only then will use the URLLoaderFactory for fetches. But if there is a chance that the fetch could happen before this is processed by the n/w service, I can add the callback here.
Related details
- Mike West
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West added 8 comments![]( "Open in Gerrit")
Looks reasonable all in all. I left some high level comments and we can talk about it later in the day.
{{document_associated_data_->token().value(),1. You're using the `DocumentAssociatedData::token()` for the non-fenced case, and the `FencedFrameProperties::partition_nonce()` otherwise. It's not clear to me how those interact. What if a document in a fenced frame asserts an allowlist?
2. https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/document_associated_data.h;drc=5be0bdcfcde6f70ecafa6ec854c246d58d3fc21e;l=62 suggests that there might be some cases in which the RFH and Document might disagree about the token, which might turn into a bypass? Looking into the lifetimes here seems necessary if you're going to do the revocation prior to committing the navigation. We might also need to look into things like `Link` headers that can cause requests prior to commit (which y'all probably already poked at for FF)?
URLLoaderCompletionStatus(net::ERR_NETWORK_ACCESS_REVOKED));TODO: Different net error, and probably console issues once we're further along.
allowlisted_urls.end());As noted elsewhere, it seems possible for a fenced frame's document to assert a connection-allowlist, and it doesn't seem like this map would be able to handle that (one would overwrite the other).
That's not a huge issue for this prototype, but it's not clear to me that this is actually the right structure for the kind of thing we're building. Or maybe the map is fine, but we need to do conflict resolution while building it to make sure that the allowlist that will be checked represents the right set of constraints?
// For connection allowlist feature, network_revocation_nonces_ contains thePerhaps wrap all this in ` if (base::FeatureList::IsEnabled(network::features::kConnectionAllowlists)) {`?
Or `CHECK(base::FeatureList::IsEnabled(network::features::kConnectionAllowlists) || allowlisted_urls.empty())`?
for (const GURL& allowed_url : allowlisted_urls) {Nit: Probably worth a comment somewhere (perhaps here and in the mojo file?) that it's intentional that an empty allowlist means all requests should be disallowed. And distinguishing the exception mechanism below, which I'm not familiar with, but doesn't seem like a thing that `connection-allowlist` would take into consideration?
// `allowlisted_urls` and blocked otherwise.I find this whole comment somewhat confusing, especially now that we're not just dealing with a list of nonces, but a list of pairs of nonces and URLs. I'm not sure it's worth addressing in this CL, but I'd suggest leaving at least a TODO to rename the method now that the control it offers will be substantially more granular: we're not "revoking network access" anymore, we're creating another gate on top of outgoing requests.
Also, framing things at this layer in terms of "nonces" is certainly technically accurate, but it's not terribly explanatory. I wonder if "network access token" or something would have been clarifying?
#endif // THIRD_PARTY_BLINK_RENDERER_CORE_FRAME_CONNECTION_ALLOWLIST_H_Removing this from Blink for the moment is pretty reasonable, but I do wonder whether we're going to need it or something like it for some of the edge cases we've discussed (devtools-triggered requests, if we end up considering that in scope, as those would be triggered from a distinct context that didn't itself set the policy). Maybe things like favicon as well? I don't recall if those are tied to the page's context or issued from the central cache the browser stores. Y'all have probably looked into these already in the docs I haven't gotten to yet. :)
Related details
- Shivani Sharma
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shivani Sharma (Gerrit)
Shivani Sharma added 6 comments![]( "Open in Gerrit")
{{document_associated_data_->token().value(),1. You're using the `DocumentAssociatedData::token()` for the non-fenced case, and the `FencedFrameProperties::partition_nonce()` otherwise. It's not clear to me how those interact. What if a document in a fenced frame asserts an allowlist?
2. https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/document_associated_data.h;drc=5be0bdcfcde6f70ecafa6ec854c246d58d3fc21e;l=62 suggests that there might be some cases in which the RFH and Document might disagree about the token, which might turn into a bypass? Looking into the lifetimes here seems necessary if you're going to do the revocation prior to committing the navigation. We might also need to look into things like `Link` headers that can cause requests prior to commit (which y'all probably already poked at for FF)?
1. To be safe, adding an early return with a TODO from this function if it's a FF tree, so as to not impact FFs with this feature at all. Note that FF's partition_nonce only gets updated if the feature flag is enabled which it is not [1] by default.
2. IIUC, the renderer gets the same token [2] and this comment suggests that it wouldn't be guaranteed in the renderer until navigation commit. Since we are not using the renderer's token for this feature at the moment, we should be fine. But if for some path, it is used, then it needs to be post-commit.
Leaving this comment unresolved for navigation owner to confirm that between this point and the commit the document_associated_data_'s token value stays the same for this doc.
[1]https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;drc=899f4dbefcc1c98a2ddf6564409c9cdc651bd235;l=10811
[2]https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;drc=35d26d364efb57d0386b98312ba739f7f65ae97e;l=4357
URLLoaderCompletionStatus(net::ERR_NETWORK_ACCESS_REVOKED));TODO: Different net error, and probably console issues once we're further along.
Done
As noted elsewhere, it seems possible for a fenced frame's document to assert a connection-allowlist, and it doesn't seem like this map would be able to handle that (one would overwrite the other).
That's not a huge issue for this prototype, but it's not clear to me that this is actually the right structure for the kind of thing we're building. Or maybe the map is fine, but we need to do conflict resolution while building it to make sure that the allowlist that will be checked represents the right set of constraints?
Agree that if/when we do allow FFs and this feature to work together, we will need to make sure that a fenced frame only adds one id to this structure.
Resolving for now since FFs have been checked in the new patch to not add allowlists.
// For connection allowlist feature, network_revocation_nonces_ contains thePerhaps wrap all this in ` if (base::FeatureList::IsEnabled(network::features::kConnectionAllowlists)) {`?
Or `CHECK(base::FeatureList::IsEnabled(network::features::kConnectionAllowlists) || allowlisted_urls.empty())`?
Added the check
Nit: Probably worth a comment somewhere (perhaps here and in the mojo file?) that it's intentional that an empty allowlist means all requests should be disallowed. And distinguishing the exception mechanism below, which I'm not familiar with, but doesn't seem like a thing that `connection-allowlist` would take into consideration?
Done
I find this whole comment somewhat confusing, especially now that we're not just dealing with a list of nonces, but a list of pairs of nonces and URLs. I'm not sure it's worth addressing in this CL, but I'd suggest leaving at least a TODO to rename the method now that the control it offers will be substantially more granular: we're not "revoking network access" anymore, we're creating another gate on top of outgoing requests.
Also, framing things at this layer in terms of "nonces" is certainly technically accurate, but it's not terribly explanatory. I wonder if "network access token" or something would have been clarifying?
Added a TODO to rename and also made the comment a bit more clear.
Related details
- Mike West
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mike West
| Code-Review | +1 |
1 comment
//third_party/blink and mojo LGTM. You'll need a //services/network reviewer to sign off on the new function along with the rest of the network-side changes, but it's fine from a mojo perspective. Thanks for following up!
Related details
- Alex Moshchuk
- Shivani Sharma
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shivani Sharma (Gerrit)
Shivani Sharma added 1 comment![]( "Open in Gerrit")
- Alex Moshchuk
- Kenichi Ishibashi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kenichi Ishibashi
| Code-Review | +1 |
2 comments
lgtm, thanks!
auto revoked_nonce_url = mojom::NonceAndAllowlistedUrls::New();
revoked_nonce_url->nonce = revoked_nonce;
std::vector<network::mojom::NonceAndAllowlistedUrlsPtr> nonces_to_urls;
nonces_to_urls.push_back(std::move(revoked_nonce_url));nit: We may consider adding a helper function to create `NonceAndAllowlistedUrls`.
```c++
mojom::NonceAndAllowListedUrlsPtr CreateAllowListedUrls(const std::array<base::UnguessableToken> nonces) { ... }
```
Related details
- Alex Moshchuk
- Shivani Sharma
Alex Moshchuk (Gerrit)
Alex Moshchuk added 8 comments![]( "Open in Gerrit")
Thanks, this is an interesting feature proposal! I've posted some comments and questions below, and also we wanted to discuss this in our weekly CSA sync next Tue, so I might have more feedback then. I realize this is a prototype and not everything might need to be done in this CL, so feel free to push back against any of these if you're planning to eventually get to them. :)
if (GetWeakPtr() && !token_->is_empty()) {Is it really possible for the DUD to go away here? It seems that if that were to happen, we'd crash on line 96 below?
storage_partition->ClearNoncesInNetworkContextAfterDelay({Is this delay big enough to deal with races where a frame might a fetch keepalive request and then destroys itself, setting up a race where the fetch eventually succeeds? I'm not sure whether there are new considerations here compared to fenced frames.
subresource_loader_factories =This path also sends a commit IPC but for error pages. I wouldn't expect error pages to make network requests, but it seems that they do create subresource URLLoaderFactories here. Would this also need to enforce the network restrictions?
void RenderFrameHostImpl::ApplyNetworkRestrictionsIfNeeded(I wonder if you also need to call this when new frames get created without navigations. Otherwise, it seems that a frame could bypass its network restrictions by just creating a blank subframe (which would have a fresh document token) and inject a script into it to do a fetch without ever navigating it. (Same concern for window.open().) Or maybe the renderer reuses the parent's URLLoaderFactory (with the original token) for the child in that case? It'd be good to eventually have a test for this.
// CommitNavigation is sent to the renderer.My understanding is that since this is being sent to the network service before CommitNavigation is sent to the renderer, we are fine with not waiting on a callback. After this the renderer will receive CommitNavigation, process it, send back DidCommitNavigation and only then will use the URLLoaderFactory for fetches. But if there is a chance that the fetch could happen before this is processed by the n/w service, I can add the callback here.
I think the following race might still be possible here:
1. Browser sends CommitNavigation to the renderer
2. Browser sends RevokeNetworkForNonces to the NS
3. Renderer process receives and processes CommitNavigation, sends back DidCommitNavigation, starts loading the document and executing its scripts.
4. Those scripts trigger a network fetch from the renderer process to NS via its subresource URLLoaderFactory
5. The network fetch arrives to the NS process, which hasn't received the nonces yet.
6. RevokeNetworkForNonces arrives in the NS process, too late to take effect.
In other words, I don't see anything that would guarantee that an IPC from browser process to NS would arrive sooner than an IPC from browser to renderer, followed by an IPC from renderer to NS.
So I think we do want to do something here. I'm not sure about waiting for a callback, since this is on a critical path for navigations and could slow things down, though maybe it's ok in the short term if it's just for prototyping and behind your feature flag. Maybe another approach to consider could be for the NS to query the browser process about a newly seen nonce when it sees a fetch from a new frame, to see what kinds of restrictions that nonce should have, if any (and verify that that frame is still active)?
{{document_associated_data_->token().value(),Shivani Sharma1. You're using the `DocumentAssociatedData::token()` for the non-fenced case, and the `FencedFrameProperties::partition_nonce()` otherwise. It's not clear to me how those interact. What if a document in a fenced frame asserts an allowlist?
2. https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/document_associated_data.h;drc=5be0bdcfcde6f70ecafa6ec854c246d58d3fc21e;l=62 suggests that there might be some cases in which the RFH and Document might disagree about the token, which might turn into a bypass? Looking into the lifetimes here seems necessary if you're going to do the revocation prior to committing the navigation. We might also need to look into things like `Link` headers that can cause requests prior to commit (which y'all probably already poked at for FF)?
1. To be safe, adding an early return with a TODO from this function if it's a FF tree, so as to not impact FFs with this feature at all. Note that FF's partition_nonce only gets updated if the feature flag is enabled which it is not [1] by default.
2. IIUC, the renderer gets the same token [2] and this comment suggests that it wouldn't be guaranteed in the renderer until navigation commit. Since we are not using the renderer's token for this feature at the moment, we should be fine. But if for some path, it is used, then it needs to be post-commit.
Leaving this comment unresolved for navigation owner to confirm that between this point and the commit the document_associated_data_'s token value stays the same for this doc.[1]https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;drc=899f4dbefcc1c98a2ddf6564409c9cdc651bd235;l=10811
[2]https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;drc=35d26d364efb57d0386b98312ba739f7f65ae97e;l=4357
Sorry, I haven't had to deal with this token in a while, so had to do a bit of digging around. In general, we seem to restrict access to this token behind [these getters](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.h;l=617-635;drc=1563510976ec6765127e5d1326d37a2b0c0fb140), which warn against accessing it during pending commit, which is exactly the stage where you're using it here. It's worth reading through the [CL description](https://chromium-review.googlesource.com/c/chromium/src/+/3849601) where we introduced this (and [this followup](https://chromium-review.googlesource.com/c/chromium/src/+/3913999)) and thinking whether any of the quirks mentioned there could affect connection allowlists.
I do think that document_associated_data_'s token should stay the same between being constructed for a speculative RFH and the navigation commit (and for that matter, DidCommit as well). So this case should be ok here.
If the RFH was reused for a new cross-document navigation (only the case without RenderDocument, which we still haven't fully shipped, though that's hopefully coming soon), then document_associated_data_'s token is [updated](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;l=15830;drc=1563510976ec6765127e5d1326d37a2b0c0fb140) at DidCommit time. Have you checked how that case works with your feature? It seems the token may not be correct in that case, and you might need to use the one from NavigationRequest.
More generally, I agree with Mike's concern that it seems fragile that the renderer can bypass the protection by finding a way to send a network request out with a token that doesn't match with the one here, which seems easier to do with a token that's per-document instead of per-StoragePartition. E.g., what about requests from workers? What about requests made by scripting same-origin frames that might not have network restrictions? Would the latter suggest that the token granularity should be SiteInstance rather than Document? Or maybe there are arguments for why this is robust maybe based on how the URLLoaderFactories are managed in the renderer, or how network requests in blink would determine the token to pass to NS?
return true;Just curious: have you thought at all what it would take to make this fail closed by default, where there's no network access unless a nonce has been explicitly registered without restrictions?
Related details
- Shivani Sharma