[remoting] Fix PID-reuse TOCTOU in IsTrustedMojoEndpoint [chromium/src : main]
Yuwei Huang (Gerrit)
Yuwei Huang added 4 comments![]( "Open in Gerrit")
base::Process process;Like `audit_token` and `credentials`, consider wrapping `process` in `#if BUILDFLAG(IS_WIN)` since it's only populated and used on Windows.
bool should_open_caller_process = false;Since `should_open_caller_process` is only implemented and relevant on Windows, consider wrapping it in `#if BUILDFLAG(IS_WIN)` (similar to `require_same_peer_user` for Linux). This prevents confusion and misuse on other platforms.
OnError();If the client process terminates immediately after connecting, `OpenWithAccess` will fail. Calling `OnError()` here introduces a 3-second delay (`kRetryConnectionTimeout`) before the server accepts new connections. A malicious local user could exploit this to perform a DoS attack on the IPC server by continuously connecting and terminating.
Instead of `OnError()`, you should just drop the invalid connection and listen for the next one:
```cpp
ResetConnectionObjects();
Connect();
```
(Note: You might want to consider doing the same for the `GetNamedPipeClientProcessId` failure above.)
CHECK(caller.process.IsValid());If another IPC server uses this security checker but doesn't set `should_open_caller_process = true` in its `EndpointOptions`, this `CHECK` will crash the entire host process upon any connection. To prevent a misconfiguration from becoming a Denial-of-Service vector, consider logging an error and returning `false` instead:
```cpp
if (!caller.process.IsValid()) {
LOG(ERROR) << "caller.process is invalid. Was should_open_caller_process set to true?";
return false;
}
```
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 6 comments![]( "Open in Gerrit")
Like `audit_token` and `credentials`, consider wrapping `process` in `#if BUILDFLAG(IS_WIN)` since it's only populated and used on Windows.
Done
Since `should_open_caller_process` is only implemented and relevant on Windows, consider wrapping it in `#if BUILDFLAG(IS_WIN)` (similar to `require_same_peer_user` for Linux). This prevents confusion and misuse on other platforms.
Done
#if BUILDFLAG(IS_WIN)This file is specific to Windows (`_win.cc`), so `BUILDFLAG(IS_WIN)` is guaranteed to be true here. You can safely remove the `#if` guards.
If the client process terminates immediately after connecting, `OpenWithAccess` will fail. Calling `OnError()` here introduces a 3-second delay (`kRetryConnectionTimeout`) before the server accepts new connections. A malicious local user could exploit this to perform a DoS attack on the IPC server by continuously connecting and terminating.
Instead of `OnError()`, you should just drop the invalid connection and listen for the next one:
```cpp
ResetConnectionObjects();
Connect();
```
(Note: You might want to consider doing the same for the `GetNamedPipeClientProcessId` failure above.)
Done
// TODO: yuweih - see if it's possible to move away from PID-based securitySince this CL resolves the PID-reuse issue for Windows by using the pinned process handle, consider updating this TODO to reflect that this is now only a concern for non-Windows platforms (or remove it if Windows was the primary concern).
If another IPC server uses this security checker but doesn't set `should_open_caller_process = true` in its `EndpointOptions`, this `CHECK` will crash the entire host process upon any connection. To prevent a misconfiguration from becoming a Denial-of-Service vector, consider logging an error and returning `false` instead:
```cpp
if (!caller.process.IsValid()) {
LOG(ERROR) << "caller.process is invalid. Was should_open_caller_process set to true?";
return false;
}
```
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 and added 2 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
2 comments
This file is specific to Windows (`_win.cc`), so `BUILDFLAG(IS_WIN)` is guaranteed to be true here. You can safely remove the `#if` guards.
Done
// TODO: yuweih - see if it's possible to move away from PID-based securitySince this CL resolves the PID-reuse issue for Windows by using the pinned process handle, consider updating this TODO to reflect that this is now only a concern for non-Windows platforms (or remove it if Windows was the primary concern).
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. |