[LNA] Run standard LNA check on WebSocket connections [chromium/src : main]
Hubert Chao (Gerrit)
Hubert Chao voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Chris Thompson
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Thompson (Gerrit)
Chris Thompson voted and added 3 comments![]( "Open in Gerrit")
Votes added by Chris Thompson
| Code-Review | +1 |
3 comments
overalL LGTM with a couple questions
// Currently this function only does LNA checks, so if we're not doing those
// go ahead and return net::OK.
if (!base::FeatureList::IsEnabled(
features::kLocalNetworkAccessChecksWebSockets)) {
return net::OK;
}Do we lose UseCounter calls (via OnWebSocketConnectedToPrivateNetwork() below) if we early return here?
PrivateNetworkAccessChecker checker(
request->url(),
/*target_ip_address_space*/ network::mojom::IPAddressSpace::kUnknown,
request->initiator(),
/*required_ip_address_space*/ network::mojom::IPAddressSpace::kUnknown,
impl_->client_security_state_.get(), impl_->options_);Maybe worth adding a comment here for why both target and required are kUnknown (i.e., there is no way for a site to specify a targetAddressSpace for websockets yet).
(Aside: I was trying to remind myself how we enforce the mixed content exceptions for .local and local IP address literals -- I thought we set the targetAddressSpace somewhere to track that? Or is that only in the spec [1] but not implemented yet?)
Related details
- Hubert Chao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hubert Chao (Gerrit)
Hubert Chao added 2 comments![]( "Open in Gerrit")
// Currently this function only does LNA checks, so if we're not doing those
// go ahead and return net::OK.
if (!base::FeatureList::IsEnabled(
features::kLocalNetworkAccessChecksWebSockets)) {
return net::OK;
}Do we lose UseCounter calls (via OnWebSocketConnectedToPrivateNetwork() below) if we early return here?
Yep; rearranged code to do use counters first.
PrivateNetworkAccessChecker checker(
request->url(),
/*target_ip_address_space*/ network::mojom::IPAddressSpace::kUnknown,
request->initiator(),
/*required_ip_address_space*/ network::mojom::IPAddressSpace::kUnknown,
impl_->client_security_state_.get(), impl_->options_);Maybe worth adding a comment here for why both target and required are kUnknown (i.e., there is no way for a site to specify a targetAddressSpace for websockets yet).
(Aside: I was trying to remind myself how we enforce the mixed content exceptions for .local and local IP address literals -- I thought we set the targetAddressSpace somewhere to track that? Or is that only in the spec [1] but not implemented yet?)
Maybe worth adding a comment here for why both target and required are kUnknown (i.e., there is no way for a site to specify a targetAddressSpace for websockets yet).
Added comment for required.
Question: what exactly is targetAddressSpace used for? I'm looking at this snippet of code in `services/network/private_network_access_checker.cc` and I realize that don't understand what it is doing:
```
if (target_address_space_ != mojom::IPAddressSpace::kUnknown) {
if (resource_address_space == target_address_space_) {
return Result::kAllowedByTargetIpAddressSpace;
}
if (policy == mojom::PrivateNetworkRequestPolicy::kPreflightWarn) {
return Result::kAllowedByPolicyPreflightWarn;
}return Result::kBlockedByTargetIpAddressSpace;
}
```
the comment for `target_ip_address_space` in `services/network/public/mojom/url_request.mojom` doesn't help matters either, as it makes it seem like this is a complication for preflights
(a search for `target_ip_address_space` also makes me think this is preflights-complication)
(Aside: I was trying to remind myself how we enforce the mixed content exceptions for .local and local IP address literals -- I thought we set the targetAddressSpace somewhere to track that? Or is that only in the spec [1] but not implemented yet?)
I didn't touch anything around this, so I'm not sure. Is the worry that:
- we get a local ip address literal/.local address
- so we let the request bypass mixed content checks
- but it actually ends up at a public IP address somehow
- and so it passes the LNA check because the resource_ip_address_space == kPublic?
I don't actually know how we check this, or if we even check this at all.
Related details
- Chris Thompson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Thompson (Gerrit)
Chris Thompson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Chris Thompson
| Code-Review | +1 |
1 comment
Question: what exactly is targetAddressSpace used for?
I'm also not sure what that bit of code is doing, but agreed that it seems to be preflights only. At this point in the code in CheckInternal() the `required_address_space` parameter should what is actually has the value that was plumbed through from the fetch() call if the targetAddressSpace option was set.
Is the worry that: [...]
Yeah the idea is to avoid allowing it to be a bypass of mixed content blocking by enforcing that the resource response was actually from the intended address space.
(I can investigate this part more tomorrow.)
Related details
- Hubert Chao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hubert Chao (Gerrit)
Hubert Chao added 1 comment![]( "Open in Gerrit")
Question: what exactly is targetAddressSpace used for?
I'm also not sure what that bit of code is doing, but agreed that it seems to be preflights only. At this point in the code in CheckInternal() the `required_address_space` parameter should what is actually has the value that was plumbed through from the fetch() call if the targetAddressSpace option was set.
I think I have a working theory (that apparently I thought about while I was sleeping last night): `target_ip_address_space` is what allows the CORS preflight request to pass LNA checks; otherwise the preflight request itself would be blocked.
I'm not 100% sure about this, as I can't figure out where this is set; what I might do later is to try to delete `target_ip_address_space` from `network::ResourceRequest` and see what code falls out.
Is the worry that: [...]
Yeah the idea is to avoid allowing it to be a bypass of mixed content blocking by enforcing that the resource response was actually from the intended address space.
(I can investigate this part more tomorrow.)
- Chris Thompson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Thompson (Gerrit)
Chris Thompson added 1 comment![]( "Open in Gerrit")
Digging into this: I think the only case that is potentially interesting is the `.local` exception, but it is of very limited use as a mixed content bypass because an attacker can't rely on having a .local domain resolve to a public address without already having a foothold on the local network (they don't recurse into public DNS). Filed crbug.com/441900736 to track that.
(The fetch() targetAddressSpace option is already double checked in PrivateNetworkAccessChecker, and private IP literals definitionally always match.)
Related details
- Hubert Chao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hubert Chao (Gerrit)
Hubert Chao added 1 comment![]( "Open in Gerrit")
PrivateNetworkAccessChecker checker(
request->url(),
/*target_ip_address_space*/ network::mojom::IPAddressSpace::kUnknown,
request->initiator(),
/*required_ip_address_space*/ network::mojom::IPAddressSpace::kUnknown,
impl_->client_security_state_.get(), impl_->options_);Digging into this: I think the only case that is potentially interesting is the `.local` exception, but it is of very limited use as a mixed content bypass because an attacker can't rely on having a .local domain resolve to a public address without already having a foothold on the local network (they don't recurse into public DNS). Filed crbug.com/441900736 to track that.
SG.
I added the comment about why `target_ip_address_space` is `kUnknown`, I think next week I'm going to try to start removing the preflight code so we don't get confused again.
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hubert Chao (Gerrit)
Hubert Chao added 1 comment![]( "Open in Gerrit")
+ipc-security-review: network_context.mojom
+ricea: services/network/, content/browser/websockets
+clamy: content/
+nsatragno: device/
Related details
- Adam Rice
- Camille Lamy
- Chromium IPC Reviews
- Nina Satragno
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
WARNING: gwsq was unable to find a reviewer who is not on vacation. As a fallback, gwsq is ignoring vacations and assigning d...@chromium.org.
WARNING: gwsq was unable to find a reviewer who is not on vacation. As a fallback, gwsq is ignoring vacations and assigning d...@chromium.org.
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: cl...@chromium.org, d...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): cl...@chromium.org, d...@chromium.org
Reviewer source(s):
d...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Adam Rice
- Camille Lamy
- Dominic Farolino
- Nina Satragno
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camille Lamy (Gerrit)
Camille Lamy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Camille Lamy
| Code-Review | +1 |
1 comment
- Adam Rice
- Dominic Farolino
- Hubert Chao
- Nina Satragno
Nina Satragno (Gerrit)
Nina Satragno voted and added 1 comment![]( "Open in Gerrit")
Related details
- Adam Rice
- Dominic Farolino
- Hubert Chao
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino voted Code-Review+1![]( "Open in Gerrit")
- Adam Rice
- Hubert Chao
Adam Rice (Gerrit)
Adam Rice voted and added 5 comments![]( "Open in Gerrit")
Votes added by Adam Rice
| Code-Review | +1 |
5 comments
network::mojom::ClientSecurityStatePtr client_security_state_;This can probably be `const` too.
DCHECK_EQ(target_address_space_, mojom::IPAddressSpace::kUnknown) << url;This looks like a cheap comparison, and so should be `CHECK()` according to https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#check_dcheck_and-notreached
network::mojom::ClientSecurityStatePtr client_security_state_;I think this isn't modified after construction and so should be const.
/*target_ip_address_space*/ network::mojom::IPAddressSpace::kUnknown,Nit: the standard format has an `=`, ie. `/*target_ip_address_space=*/`.
Related details
- Hubert Chao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hubert Chao (Gerrit)
Hubert Chao added 4 comments![]( "Open in Gerrit")
network::mojom::ClientSecurityStatePtr client_security_state_;This can probably be `const` too.
Done
DCHECK_EQ(target_address_space_, mojom::IPAddressSpace::kUnknown) << url;This looks like a cheap comparison, and so should be `CHECK()` according to https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#check_dcheck_and-notreached
Done
network::mojom::ClientSecurityStatePtr client_security_state_;I think this isn't modified after construction and so should be const.
Done
/*target_ip_address_space*/ network::mojom::IPAddressSpace::kUnknown,Nit: the standard format has an `=`, ie. `/*target_ip_address_space=*/`.
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hubert Chao (Gerrit)
Hubert Chao voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
8 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: services/network/private_network_access_checker.cc
Insertions: 1, Deletions: 1.
@@ -69,7 +69,7 @@
// No client security state means PNA is implicitly disabled. A policy of
// `kAllow` means PNA is explicitly disabled. In both cases, the target IP
// address space should not be set on the request.
- DCHECK_EQ(target_address_space_, mojom::IPAddressSpace::kUnknown) << url;
+ CHECK_EQ(target_address_space_, mojom::IPAddressSpace::kUnknown) << url;
}
}
```
```
The name of the file: content/browser/websockets/websocket_connector_impl.h
Insertions: 1, Deletions: 1.
@@ -77,7 +77,7 @@
const int frame_id_;
const url::Origin origin_;
const net::IsolationInfo isolation_info_;
- network::mojom::ClientSecurityStatePtr client_security_state_;
+ const network::mojom::ClientSecurityStatePtr client_security_state_;
};
} // namespace content
```
```
The name of the file: services/network/websocket.cc
Insertions: 2, Deletions: 2.
@@ -268,9 +268,9 @@
// checks.
PrivateNetworkAccessChecker checker(
request->url(),
- /*target_ip_address_space*/ network::mojom::IPAddressSpace::kUnknown,
+ /*target_ip_address_space=*/network::mojom::IPAddressSpace::kUnknown,
request->initiator(),
- /*required_ip_address_space*/ network::mojom::IPAddressSpace::kUnknown,
+ /*required_ip_address_space=*/network::mojom::IPAddressSpace::kUnknown,
impl_->client_security_state_.get(), impl_->options_);
PrivateNetworkAccessCheckResult check_result = checker.Check(info);
```
```
The name of the file: services/network/websocket.h
Insertions: 1, Deletions: 1.
@@ -232,7 +232,7 @@
// The web origin to use for the WebSocket.
const url::Origin origin_;
- network::mojom::ClientSecurityStatePtr client_security_state_;
+ const network::mojom::ClientSecurityStatePtr client_security_state_;
// For 3rd-party cookie permission checking.
net::SiteForCookies site_for_cookies_;
```
Change information
[LNA] Run standard LNA check on WebSocket connections
Change WebSocket::WebSocketEventHandler::OnURLRequestConnected to run a
proper LNA check (as opposed to the previous CL's bad check of if the
address space is kLocal or kLoopback).
This requires threading the client security state through from all of
the various places websockets can be created all the way through the
network context into services/network/websocket.cc
Change the basic test in chrome/browser/net/websocket_browsertest.cc to
ensure that this works. Future CLs will add more tests with websockets
being created in various workers (dedicated, shared, service).
- M chrome/browser/net/websocket_browsertest.cc
- A chrome/test/data/websocket/connect_to_as_public_address.html
- A chrome/test/data/websocket/connect_to_as_public_address.html.mock-http-headers
- M content/browser/renderer_host/render_frame_host_impl.cc
- M content/browser/service_worker/service_worker_host.cc
- M content/browser/websockets/websocket_connector_impl.cc
- M content/browser/websockets/websocket_connector_impl.h
- M content/browser/worker_host/dedicated_worker_host.cc
- M content/browser/worker_host/shared_worker_host.cc
- M device/fido/cable/fido_tunnel_device.cc
- M device/fido/cable/v2_authenticator.cc
- M device/fido/cable/v2_test_util.cc
- M device/fido/enclave/enclave_websocket_client.cc
- M services/network/network_context.cc
- M services/network/network_context.h
- M services/network/private_network_access_checker.cc
- M services/network/private_network_access_checker.h
- M services/network/public/mojom/network_context.mojom
- M services/network/test/test_network_context.h
- M services/network/websocket.cc
- M services/network/websocket.h
- M services/network/websocket_factory.cc
- M services/network/websocket_factory.h
Code-Review: +1 by Nina Satragno, +1 by Chris Thompson, +1 by Adam Rice, +1 by Dominic Farolino, +1 by Camille Lamy
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |