[remoting] Support starting/stopping CRD Linux host as root [chromium/src : main]
Yuwei Huang (Gerrit)
Yuwei Huang added 2 comments![]( "Open in Gerrit")
"current user.")Consider updating the help string to say "current or specified user" to reflect the new `--user-name` behavior and to be consistent with the wording used for `--disable-and-stop`.
args.push_back("--config=" + GetConfigPath().value());If this process is running as root (e.g., from `migrate-host`) and the `--host-config` command line switch wasn't explicitly passed, `GetConfigPath()` will likely default to root's config directory (e.g., `/root/.config/...`).
Passing `--config=/root/...` here means the Python script will try to find a daemon process running with that exact config path, which will fail to match the daemon process started by the actual user (which uses `/home/user/.config/...`). This could cause `Stop()` to silently fail to find the process, and similarly cause `GetState()` to incorrectly report it as `STOPPED`.
Consider either updating `GetConfigPath()` to resolve the home directory of `user_name_` if it's set, or avoid passing `--config=` when `user_name_` is set to let the Python script dynamically resolve the correct user's config path.
Related details
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
"current user.")Consider updating the help string to say "current or specified user" to reflect the new `--user-name` behavior and to be consistent with the wording used for `--disable-and-stop`.
Done
If this process is running as root (e.g., from `migrate-host`) and the `--host-config` command line switch wasn't explicitly passed, `GetConfigPath()` will likely default to root's config directory (e.g., `/root/.config/...`).
Passing `--config=/root/...` here means the Python script will try to find a daemon process running with that exact config path, which will fail to match the daemon process started by the actual user (which uses `/home/user/.config/...`). This could cause `Stop()` to silently fail to find the process, and similarly cause `GetState()` to incorrectly report it as `STOPPED`.
Consider either updating `GetConfigPath()` to resolve the home directory of `user_name_` if it's set, or avoid passing `--config=` when `user_name_` is set to let the Python script dynamically resolve the correct user's config path.
Done
Related details
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 2 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
2 comments
chown(path.value().c_str(), user_info->uid, user_info->gid)) != 0) {Security Vulnerability (Local Privilege Escalation): `path` points to a file or directory inside a user-controlled directory (`~/.config/...`). Because `chown()` and `base::SetPosixFilePermissions` (which uses `chmod()`) follow symlinks, a malicious user could replace the file or directory with a symlink to a sensitive root-owned file (like `/etc/shadow`) in the narrow window between it being created and these functions being called. This TOCTOU (Time-of-Check to Time-of-Use) race condition would result in the root process changing the ownership and permissions of the sensitive file to the attacker, granting them full control.
To fix this, you must avoid operating on paths directly. Instead, open the file or directory using `O_NOFOLLOW` and operate on the file descriptor using `fchmod()` and `fchown()`. For example:
```cpp
base::ScopedFD fd(open(path.value().c_str(), O_RDONLY | O_NOFOLLOW | O_CLOEXEC));
if (!fd.is_valid()) {
PLOG(ERROR) << "Failed to open " << path.value();
return false;
}
if (HANDLE_EINTR(fchmod(fd.get(), mode)) != 0) {
PLOG(ERROR) << "Failed to set permissions for " << path.value();
return false;
} if (HANDLE_EINTR(fchown(fd.get(), user_info->uid, user_info->gid)) != 0) {
PLOG(ERROR) << "Failed to chown " << path.value() << " to " << user_name;
return false;
}
```---
Human feedback: Please add some comment to explain why we need to do this.
if (!SetOwnershipAndPermissions(config_dir, user_name_, 0700)) {AI comment:
If `base::CreateDirectoryAndGetError(config_dir, &error)` needs to create intermediate directories (such as the user's `~/.config` directory), they will be created with `root` ownership. Because `SetOwnershipAndPermissions` only fixes the ownership of the final directory (`chrome-remote-desktop`), the intermediate directories will remain owned by `root`. This could break the user's desktop environment by preventing them from writing to their own `~/.config` directory.
Consider verifying the existence of the parent directory before creation, or recursively fixing ownership if intermediate directories had to be created.
---
Human feedback: Let's resolve the home dir and use `GetPerUserConfigRelativeDir()` directly here, then do `SetOwnershipAndPermissions()` recursively. Please add some comment explaining why we need to do this.
Related details
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 added 4 comments![]( "Open in Gerrit")
#include <fcntl.h>Include `<sys/stat.h>` for `fchmod` and `fstat` to compile correctly.
chown(path.value().c_str(), user_info->uid, user_info->gid)) != 0) {Security Vulnerability (Local Privilege Escalation): `path` points to a file or directory inside a user-controlled directory (`~/.config/...`). Because `chown()` and `base::SetPosixFilePermissions` (which uses `chmod()`) follow symlinks, a malicious user could replace the file or directory with a symlink to a sensitive root-owned file (like `/etc/shadow`) in the narrow window between it being created and these functions being called. This TOCTOU (Time-of-Check to Time-of-Use) race condition would result in the root process changing the ownership and permissions of the sensitive file to the attacker, granting them full control.
To fix this, you must avoid operating on paths directly. Instead, open the file or directory using `O_NOFOLLOW` and operate on the file descriptor using `fchmod()` and `fchown()`. For example:
```cpp
base::ScopedFD fd(open(path.value().c_str(), O_RDONLY | O_NOFOLLOW | O_CLOEXEC));
if (!fd.is_valid()) {
PLOG(ERROR) << "Failed to open " << path.value();
return false;
}if (HANDLE_EINTR(fchmod(fd.get(), mode)) != 0) {
PLOG(ERROR) << "Failed to set permissions for " << path.value();
return false;
}if (HANDLE_EINTR(fchown(fd.get(), user_info->uid, user_info->gid)) != 0) {
PLOG(ERROR) << "Failed to chown " << path.value() << " to " << user_name;
return false;
}
```---
Human feedback: Please add some comment to explain why we need to do this.
Done
open(path.value().c_str(), O_RDONLY | O_NOFOLLOW | O_CLOEXEC));While `O_NOFOLLOW` prevents symlink attacks, it does not prevent hardlink attacks or blocking on FIFOs. Since the user owns the configuration directory, they could replace the configuration file with a hardlink to a sensitive root-owned file (e.g., `/etc/shadow`) or a FIFO just before this code executes.
- If it's a FIFO, `open` will block indefinitely without `O_NONBLOCK`, causing a denial of service.
- If it's a hardlink, `open` will succeed, and `fchown`/`fchmod` will alter the permissions and ownership of the sensitive file, leading to a local privilege escalation.
To prevent this, add `O_NONBLOCK` to the `open` call, and verify with `fstat` that the file is a regular file owned by root (as it was just created by `HostConfigToJsonFile`) with only one link:
```cpp
base::ScopedFD fd(open(path.value().c_str(),
O_RDONLY | O_NOFOLLOW | O_CLOEXEC | O_NONBLOCK));
if (!fd.is_valid()) {
PLOG(ERROR) << "Failed to open " << path.value();
return false;
}
struct stat st;
if (fstat(fd.get(), &st) != 0) {
PLOG(ERROR) << "Failed to stat " << path.value();
return false;
}
// Ensure the file is a regular file, owned by root (since it was just
// created by us), and not a hardlink.
if (!S_ISREG(st.st_mode) || st.st_uid != 0 || st.st_nlink != 1) {
LOG(ERROR) << "Refusing to change ownership of " << path.value()
<< " to prevent hardlink/FIFO attacks.";
return false;
}
```
if (!SetOwnershipAndPermissions(config_dir, user_name_, 0700)) {AI comment:
If `base::CreateDirectoryAndGetError(config_dir, &error)` needs to create intermediate directories (such as the user's `~/.config` directory), they will be created with `root` ownership. Because `SetOwnershipAndPermissions` only fixes the ownership of the final directory (`chrome-remote-desktop`), the intermediate directories will remain owned by `root`. This could break the user's desktop environment by preventing them from writing to their own `~/.config` directory.
Consider verifying the existence of the parent directory before creation, or recursively fixing ownership if intermediate directories had to be created.
---
Human feedback: Let's resolve the home dir and use `GetPerUserConfigRelativeDir()` directly here, then do `SetOwnershipAndPermissions()` recursively. Please add some comment explaining why we need to do this.
Done
Related details
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 Commit-Queue+1![]( "Open in Gerrit")
Voting CQ+1.
| Commit-Queue | +1 |
Related details
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 added 2 comments![]( "Open in Gerrit")
Include `<sys/stat.h>` for `fchmod` and `fstat` to compile correctly.
Done
open(path.value().c_str(), O_RDONLY | O_NOFOLLOW | O_CLOEXEC));While `O_NOFOLLOW` prevents symlink attacks, it does not prevent hardlink attacks or blocking on FIFOs. Since the user owns the configuration directory, they could replace the configuration file with a hardlink to a sensitive root-owned file (e.g., `/etc/shadow`) or a FIFO just before this code executes.
- If it's a FIFO, `open` will block indefinitely without `O_NONBLOCK`, causing a denial of service.
- If it's a hardlink, `open` will succeed, and `fchown`/`fchmod` will alter the permissions and ownership of the sensitive file, leading to a local privilege escalation.
To prevent this, add `O_NONBLOCK` to the `open` call, and verify with `fstat` that the file is a regular file owned by root (as it was just created by `HostConfigToJsonFile`) with only one link:
```cpp
base::ScopedFD fd(open(path.value().c_str(),
O_RDONLY | O_NOFOLLOW | O_CLOEXEC | O_NONBLOCK));
if (!fd.is_valid()) {
PLOG(ERROR) << "Failed to open " << path.value();
return false;
}struct stat st;
if (fstat(fd.get(), &st) != 0) {
PLOG(ERROR) << "Failed to stat " << path.value();
return false;
}// Ensure the file is a regular file, owned by root (since it was just
// created by us), and not a hardlink.
if (!S_ISREG(st.st_mode) || st.st_uid != 0 || st.st_nlink != 1) {
LOG(ERROR) << "Refusing to change ownership of " << path.value()
<< " to prevent hardlink/FIFO attacks.";
return false;
}
```
Done
Related details
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 Commit-Queue+1![]( "Open in Gerrit")
Voting CQ+1 on the refined implementation.
| Commit-Queue | +1 |
Related details
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")
Related details
- Lambros Lambrou
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 Commit-Queue+1![]( "Open in Gerrit")
Voting CQ+1. Simplified Python script by using global CONFIG_DIR directly.
| Commit-Queue | +1 |
Related details
- Lambros Lambrou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lambros Lambrou (Gerrit)
Lambros Lambrou added 1 comment![]( "Open in Gerrit")
This change allows the single-process CRD Linux host to be managed
(started, stopped, status-checked) when the managing process (the WIP
migrate-host binary for migrating between single-process and GDM-managed
hosts) is running as root. This is necessary since the migrate-hostPlease can we avoid doing per-user actions from the privileged process? This generates a lot of AI review comments about file-creation and ownership, where the underlying problem is that a root process is trying to manage some per-user state.
Can we do the user part of this migration from a process started by the user? For example, create a new systemd user service with a start-condition on the migration being incomplete.
Can we do the migration in stages? In an intermediate stage, the user's CRD service could support both single-process and multi-process modes. If there are multiple users, we should support the possibility of some users in single-process mode, and some in multi-process mode. This is much better than trying to synchronize a migration across every user account in one step. Some subset of users on the system might want to keep using CRD with X11.
Otherwise, please can we drop privileges before doing any per-user actions?
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 added 1 comment![]( "Open in Gerrit")
This change allows the single-process CRD Linux host to be managed
(started, stopped, status-checked) when the managing process (the WIP
migrate-host binary for migrating between single-process and GDM-managed
hosts) is running as root. This is necessary since the migrate-hostPlease can we avoid doing per-user actions from the privileged process? This generates a lot of AI review comments about file-creation and ownership, where the underlying problem is that a root process is trying to manage some per-user state.
Can we do the user part of this migration from a process started by the user? For example, create a new systemd user service with a start-condition on the migration being incomplete.
Can we do the migration in stages? In an intermediate stage, the user's CRD service could support both single-process and multi-process modes. If there are multiple users, we should support the possibility of some users in single-process mode, and some in multi-process mode. This is much better than trying to synchronize a migration across every user account in one step. Some subset of users on the system might want to keep using CRD with X11.
Otherwise, please can we drop privileges before doing any per-user actions?
Please can we avoid doing per-user actions from the privileged process? This generates a lot of AI review comments about file-creation and ownership, where the underlying problem is that a root process is trying to manage some per-user state.
It's interesting that you are still seeing AI comments. I thought that I've got it all cleared :)
Can we do the user part of this migration from a process started by the user? For example, create a new systemd user service with a start-condition on the migration being incomplete.
I did wonder if we can do part of the migration as the user. For `multi-process->single-process`, WDYT about giving read access of all files in `/etc/chrome-remote-desktop/` to the target user (with `acl_set_file`?) then relaunching itself as the target user?
Can we do the migration in stages?
We have no plan for automatic migration yet. The `migrate-host` binary is just for switching between the single-process and GDM-managed hosts, since that will be needed in different scenarios (e.g. to support multi-user scenario)
Related details
- Lambros Lambrou
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 added 1 comment![]( "Open in Gerrit")
This change allows the single-process CRD Linux host to be managed
(started, stopped, status-checked) when the managing process (the WIP
migrate-host binary for migrating between single-process and GDM-managed
hosts) is running as root. This is necessary since the migrate-hostYuwei HuangPlease can we avoid doing per-user actions from the privileged process? This generates a lot of AI review comments about file-creation and ownership, where the underlying problem is that a root process is trying to manage some per-user state.
Can we do the user part of this migration from a process started by the user? For example, create a new systemd user service with a start-condition on the migration being incomplete.
Can we do the migration in stages? In an intermediate stage, the user's CRD service could support both single-process and multi-process modes. If there are multiple users, we should support the possibility of some users in single-process mode, and some in multi-process mode. This is much better than trying to synchronize a migration across every user account in one step. Some subset of users on the system might want to keep using CRD with X11.
Otherwise, please can we drop privileges before doing any per-user actions?
Please can we avoid doing per-user actions from the privileged process? This generates a lot of AI review comments about file-creation and ownership, where the underlying problem is that a root process is trying to manage some per-user state.
It's interesting that you are still seeing AI comments. I thought that I've got it all cleared :)
Can we do the user part of this migration from a process started by the user? For example, create a new systemd user service with a start-condition on the migration being incomplete.
I did wonder if we can do part of the migration as the user. For `multi-process->single-process`, WDYT about giving read access of all files in `/etc/chrome-remote-desktop/` to the target user (with `acl_set_file`?) then relaunching itself as the target user?
Can we do the migration in stages?
We have no plan for automatic migration yet. The `migrate-host` binary is just for switching between the single-process and GDM-managed hosts, since that will be needed in different scenarios (e.g. to support multi-user scenario)
Actually, I guess I'll just add a `--get-multi-process-config` flag to print the host config and the pairing entries in JSON. I think this is easier.
Lambros Lambrou (Gerrit)
Lambros Lambrou added 1 comment![]( "Open in Gerrit")
This change allows the single-process CRD Linux host to be managed
(started, stopped, status-checked) when the managing process (the WIP
migrate-host binary for migrating between single-process and GDM-managed
hosts) is running as root. This is necessary since the migrate-hostYuwei HuangPlease can we avoid doing per-user actions from the privileged process? This generates a lot of AI review comments about file-creation and ownership, where the underlying problem is that a root process is trying to manage some per-user state.
Can we do the user part of this migration from a process started by the user? For example, create a new systemd user service with a start-condition on the migration being incomplete.
Can we do the migration in stages? In an intermediate stage, the user's CRD service could support both single-process and multi-process modes. If there are multiple users, we should support the possibility of some users in single-process mode, and some in multi-process mode. This is much better than trying to synchronize a migration across every user account in one step. Some subset of users on the system might want to keep using CRD with X11.
Otherwise, please can we drop privileges before doing any per-user actions?
Please can we avoid doing per-user actions from the privileged process? This generates a lot of AI review comments about file-creation and ownership, where the underlying problem is that a root process is trying to manage some per-user state.
It's interesting that you are still seeing AI comments. I thought that I've got it all cleared :)
Can we do the user part of this migration from a process started by the user? For example, create a new systemd user service with a start-condition on the migration being incomplete.
I did wonder if we can do part of the migration as the user. For `multi-process->single-process`, WDYT about giving read access of all files in `/etc/chrome-remote-desktop/` to the target user (with `acl_set_file`?) then relaunching itself as the target user?
Can we do the migration in stages?
We have no plan for automatic migration yet. The `migrate-host` binary is just for switching between the single-process and GDM-managed hosts, since that will be needed in different scenarios (e.g. to support multi-user scenario)
Can we do the user part of this migration from a process started by the user? For example, create a new systemd user service with a start-condition on the migration being incomplete.
I did wonder if we can do part of the migration as the user. For `multi-process->single-process`, WDYT about giving read access of all files in `/etc/chrome-remote-desktop/` to the target user (with `acl_set_file`?) then relaunching itself as the target user?
This sounds fine, as long as we know which files the user should have access to. I can't remember what the sharing model is supposed to be for `/etc/chrome-remote-desktop/`. We were thinking about putting the pairing-registry in there, or `/var/lib/` and I think we were OK with `/etc/` providing we stick to text or JSON format (otherwise `/var/lib` is preferred for large files or non-text).
We don't really support multi-user setups for CRD, so we could reasonably say: if a machine is configured for system-wide CRD access (via the GDM login screen), then every user account can read the system-wide JSON config. Otherwise, we probably have to implement some kind of ACL and a mechanism for sys-admins to restrict CRD access to specific users.
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 added 1 comment![]( "Open in Gerrit")
This change allows the single-process CRD Linux host to be managed
(started, stopped, status-checked) when the managing process (the WIP
migrate-host binary for migrating between single-process and GDM-managed
hosts) is running as root. This is necessary since the migrate-hostYuwei HuangPlease can we avoid doing per-user actions from the privileged process? This generates a lot of AI review comments about file-creation and ownership, where the underlying problem is that a root process is trying to manage some per-user state.
Can we do the user part of this migration from a process started by the user? For example, create a new systemd user service with a start-condition on the migration being incomplete.
Can we do the migration in stages? In an intermediate stage, the user's CRD service could support both single-process and multi-process modes. If there are multiple users, we should support the possibility of some users in single-process mode, and some in multi-process mode. This is much better than trying to synchronize a migration across every user account in one step. Some subset of users on the system might want to keep using CRD with X11.
Otherwise, please can we drop privileges before doing any per-user actions?
Lambros LambrouPlease can we avoid doing per-user actions from the privileged process? This generates a lot of AI review comments about file-creation and ownership, where the underlying problem is that a root process is trying to manage some per-user state.
It's interesting that you are still seeing AI comments. I thought that I've got it all cleared :)
Can we do the user part of this migration from a process started by the user? For example, create a new systemd user service with a start-condition on the migration being incomplete.
I did wonder if we can do part of the migration as the user. For `multi-process->single-process`, WDYT about giving read access of all files in `/etc/chrome-remote-desktop/` to the target user (with `acl_set_file`?) then relaunching itself as the target user?
Can we do the migration in stages?
We have no plan for automatic migration yet. The `migrate-host` binary is just for switching between the single-process and GDM-managed hosts, since that will be needed in different scenarios (e.g. to support multi-user scenario)
Can we do the user part of this migration from a process started by the user? For example, create a new systemd user service with a start-condition on the migration being incomplete.
I did wonder if we can do part of the migration as the user. For `multi-process->single-process`, WDYT about giving read access of all files in `/etc/chrome-remote-desktop/` to the target user (with `acl_set_file`?) then relaunching itself as the target user?
This sounds fine, as long as we know which files the user should have access to. I can't remember what the sharing model is supposed to be for `/etc/chrome-remote-desktop/`. We were thinking about putting the pairing-registry in there, or `/var/lib/` and I think we were OK with `/etc/` providing we stick to text or JSON format (otherwise `/var/lib` is preferred for large files or non-text).
We don't really support multi-user setups for CRD, so we could reasonably say: if a machine is configured for system-wide CRD access (via the GDM login screen), then every user account can read the system-wide JSON config. Otherwise, we probably have to implement some kind of ACL and a mechanism for sys-admins to restrict CRD access to specific users.
We were thinking about putting the pairing-registry in there
Both the host config file and the pairing entries are stored in `/etc/chrome-remote-desktop`. There are a privileged version and an unprivileged version for host config and pairing entries. The former has credentials and is only readable by root (or network for pairing entries) while the latter doesn't and is readable by anyone. This is why we need to use the correct DaemonController delegate to ensure the correct file structure is created, but I guess we don't need it from reading the source config for migration, since we can always just read the privileged files, assuming we have the permission.
I'm drafting a plan for Gemini.
Related details
- Lambros Lambrou
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)
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |