remoting Linux multi-process: Bind session services via DesktopSessionConnectionEvents [chromium/src : main]
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
1 comment
Related details
- Joe Downing
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang added 1 comment![]( "Open in Gerrit")
pending_receiver<ChromotingSessionServices> receiver);Note that this is the `Session` services instead of the `Host` services ([chromoting_host_services.mojom](https://source.chromium.org/chromium/chromium/src/+/main:remoting/host/mojom/chromoting_host_services.mojom;l=1;drc=f479adcb5631c7050b0dcd65267466f7cf03d15b)). Binding `ChromotingHostServices` into `ClientSession` doesn't make much sense, since some processes keep `ChromotingHostServices` bound all the time and expect `ChromotingSessionServices` to only be available when the CRD client is connected.
Joe Downing (Gerrit)
Joe Downing voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joe Downing
| Code-Review | +1 |
1 comment
receivers_.Add(this, std::move(receiver), std::move(connection_info));*AI suggestion* You should verify that `connection_info->credentials` is valid (on Linux, it should contain the peer's UID) before adding it to the receiver set. If the credentials are not available, the binding should likely be rejected.
*Joedow* IIRC you can't reliably get the peer_pid on Linux so you use IPC security which requires that the peer binary exists in the same directory as the current process. Is that correct?
Related details
- Yuwei Huang
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. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
2 comments
receivers_.Add(this, std::move(receiver), std::move(connection_info));*AI suggestion* You should verify that `connection_info->credentials` is valid (on Linux, it should contain the peer's UID) before adding it to the receiver set. If the credentials are not available, the binding should likely be rejected.
*Joedow* IIRC you can't reliably get the peer_pid on Linux so you use IPC security which requires that the peer binary exists in the same directory as the current process. Is that correct?
It is interesting that when I ran the AI check, it made no suggestions and claimed that the CL looked good. Maybe it's a good idea to run the check multiple times even if the CL hasn't changed :)
*AI suggestion* You should verify that `connection_info->credentials` is valid (on Linux, it should contain the peer's UID) before adding it to the receiver set. If the credentials are not available, the binding should likely be rejected.
I'm not very sure what AI meant by verifying `connection_info->credentials`. We can't validate `uid` this early since it is supposed to be (and is) checked in `BindSessionServices`. It sounds like it is asking for a null check. `connection_info` could be null even though the underlying IPC server always provides it, but I've added a null check anyway. `connection_info->credentials` is not nullable since it's just a struct with int fields in the storage of `ConnectionInfo`.
I've also added some comment to explain the verification logic.
*Joedow* IIRC you can't reliably get the peer_pid on Linux so you use IPC security which requires that the peer binary exists in the same directory as the current process. Is that correct?
You *can* reliably get the `peer_pid` on Linux. The problem is any PID-based security checks are susceptible of PID reuse attacks. `IsTrustedMojoEndpoint()` does the binary path check you just mentioned, but it's not safe enough because it looks up the binary path from the PID (and I got bugs filed by AI because of that 🙂). One reason I changed the server to pass `connection_info` is that the socket `credentials` gives you the peer's UID, which is not spoofable. You could technically lookup a process' UID with its PID, but it will have the same PID reuse problem.
Related details
- Chromium IPC Reviews
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: ff...@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): ff...@chromium.org
Reviewer source(s):
ff...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Fred Shih
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih voted Code-Review+1![]( "Open in Gerrit")
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +2 |
1 comment
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
remoting Linux multi-process: Bind session services via DesktopSessionConnectionEvents
This is for routing the ChromotingSessionServices to the correct
ClientSession in case of multi-user. The old code path was
DaemonProcess->RemotingHostControl->ChromotingHost->ClientSession. In
order to make the old path work, I'd need to plump client_id through the
pipeline. I gave it a try in https://crrev.com/c/7713292, which worked,
but the code is awkward for Windows and single-process hosts.
This CL does this differently. The daemon process will instead find the
desktop session associated with the calling process, then pass the
receiver together with the terminal_id to the
IpcDesktopEnvironmentFactory via DesktopSessionConnectionEvents. This is
a cleaner approach since it doesn't affect the single-process code path,
and it is directly targeting the desktop session instead of `client_id`,
which isn't really used much in the mojo IDL.
- M remoting/host/chromoting_host.cc
- M remoting/host/chromoting_host.h
- M remoting/host/chromoting_host_services_server.cc
- M remoting/host/chromoting_host_services_server.h
- M remoting/host/chromoting_host_services_server_unittest.cc
- M remoting/host/client_session.cc
- M remoting/host/client_session.h
- M remoting/host/client_session_events.h
- M remoting/host/daemon_process_linux.cc
- M remoting/host/daemon_process_win.cc
- M remoting/host/desktop_session_proxy.cc
- M remoting/host/desktop_session_proxy.h
- M remoting/host/host_mock_objects.h
- M remoting/host/ipc_desktop_environment.cc
- M remoting/host/ipc_desktop_environment.h
- M remoting/host/linux/desktop_session_factory_linux.cc
- M remoting/host/linux/desktop_session_factory_linux.h
- M remoting/host/linux/passwd_utils.h
- M remoting/host/mac/agent_process_broker.cc
- M remoting/host/mac/agent_process_broker.h
- M remoting/host/mojom/remoting_host.mojom
- M remoting/host/remoting_me2me_host.cc
Code-Review: +1 by Fred Shih, +1 by Joe Downing
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |