Yuwei Huang (Gerrit)

unread,

Apr 22, 2026, 4:14:43 AM (2 days ago) Apr 22

to android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org, droger+w...@chromium.org

Yuwei Huang added 2 comments

File components/named_mojo_ipc_server/named_mojo_server_endpoint_connector_win.cc

Line 184, Patchset 12 (Latest): info->process = base::Process::OpenWithAccess(

Yuwei Huang . unresolved

For IWYU (Include What You Use), consider explicitly including `"base/process/process.h"` in this file since `base::Process` is being directly used here.

Line 187, Patchset 12 (Latest): LOG(ERROR) << "Failed to open peer process " << info->pid;

Yuwei Huang . unresolved

Consider using `PLOG(ERROR)` instead of `LOG(ERROR)` here. Since `base::Process::OpenWithAccess` uses `OpenProcess` under the hood on Windows, `PLOG` will log the system error (e.g. `ERROR_ACCESS_DENIED`), which can be very helpful for debugging.

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yuwei Huang (Gerrit)

unread,

Apr 22, 2026, 4:31:22 AM (2 days ago) Apr 22

to Joe Downing, Noah Rose Ledesma, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org, droger+w...@chromium.org

Attention needed from Joe Downing

Yuwei Huang voted and added 3 comments

Votes added by Yuwei Huang

Auto-Submit

+1

3 comments

Patchset-level comments

File-level comment, Patchset 13 (Latest):

Yuwei Huang . resolved

PTAL thanks!

File components/named_mojo_ipc_server/named_mojo_server_endpoint_connector_win.cc

Line 184, Patchset 12: info->process = base::Process::OpenWithAccess(

Yuwei Huang . resolved

For IWYU (Include What You Use), consider explicitly including `"base/process/process.h"` in this file since `base::Process` is being directly used here.

Yuwei Huang

Done

Line 187, Patchset 12: LOG(ERROR) << "Failed to open peer process " << info->pid;

Yuwei Huang . resolved

Consider using `PLOG(ERROR)` instead of `LOG(ERROR)` here. Since `base::Process::OpenWithAccess` uses `OpenProcess` under the hood on Windows, `PLOG` will log the system error (e.g. `ERROR_ACCESS_DENIED`), which can be very helpful for debugging.

Yuwei Huang

Done

Open in Gerrit

Related details

Attention is currently required from:

Joe Downing

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Joe Downing (Gerrit)

unread,

Apr 22, 2026, 1:28:45 PM (2 days ago) Apr 22

to Yuwei Huang, Noah Rose Ledesma, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org, droger+w...@chromium.org

Attention needed from Yuwei Huang

Joe Downing added 3 comments

Patchset-level comments

File-level comment, Patchset 13 (Latest):

Joe Downing . resolved

In general this looks OK but I would like to adjust some of the names.

File components/named_mojo_ipc_server/connection_info.h

Line 40, Patchset 13 (Latest): base::Process process;

Joe Downing . unresolved

Can we call this `peer_process`?

File components/named_mojo_ipc_server/endpoint_options.h

Line 78, Patchset 13 (Latest): bool should_open_caller_process = false;

Joe Downing . unresolved

I would prefer that this is more authoritative, something like 'include_peer_process_info' or 'include_peer_info'. The current wording does not imply that anything will be provided, just that it will open the process.

Open in Gerrit

Related details

Attention is currently required from:

Yuwei Huang

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yuwei Huang (Gerrit)

unread,

Apr 22, 2026, 2:47:48 PM (2 days ago) Apr 22

to Joe Downing, Noah Rose Ledesma, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org, droger+w...@chromium.org

Attention needed from Joe Downing

Yuwei Huang added 1 comment

File components/named_mojo_ipc_server/connection_info.h

Line 40, Patchset 13 (Latest): base::Process process;

Joe Downing . resolved

Can we call this `peer_process`?

Yuwei Huang

I'm following the existing naming scheme of other fields. Maybe the struct itself should be renamed `PeerConnectionInfo`, but that would be a large refactoring.

Open in Gerrit

Related details

Attention is currently required from:

Joe Downing

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yuwei Huang (Gerrit)

unread,

Apr 22, 2026, 3:10:27 PM (2 days ago) Apr 22

to Joe Downing, Noah Rose Ledesma, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org, droger+w...@chromium.org

Attention needed from Joe Downing

Yuwei Huang voted and added 2 comments

Votes added by Yuwei Huang

Auto-Submit	+1
Commit-Queue	+1

2 comments

Patchset-level comments

File-level comment, Patchset 15 (Latest):

Yuwei Huang . resolved

PTAL thanks!

File components/named_mojo_ipc_server/endpoint_options.h

Line 78, Patchset 13: bool should_open_caller_process = false;

Joe Downing . resolved

I would prefer that this is more authoritative, something like 'include_peer_process_info' or 'include_peer_info'. The current wording does not imply that anything will be provided, just that it will open the process.

Yuwei Huang

Done

Open in Gerrit

Related details

Attention is currently required from:

Joe Downing

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Joe Downing (Gerrit)

unread,

Apr 22, 2026, 3:13:38 PM (2 days ago) Apr 22

to Yuwei Huang, Noah Rose Ledesma, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org, droger+w...@chromium.org

Attention needed from Yuwei Huang

Joe Downing voted and added 2 comments

Votes added by Joe Downing

Code-Review

+1

2 comments

File components/named_mojo_ipc_server/connection_info.h

Line 36, Patchset 15 (Latest): // The process of the peer. Only valid if |should_open_caller_process| is true

Joe Downing . unresolved

include_peer_process_info

File remoting/host/mojo_caller_security_checker.cc

Line 80, Patchset 15 (Latest): // checks for Linux, which might be susceptible of PID reuse attacks.

Joe Downing . unresolved

nit: to

Open in Gerrit

Related details

Attention is currently required from:

Yuwei Huang

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yuwei Huang (Gerrit)

unread,

Apr 22, 2026, 5:10:35 PM (2 days ago) Apr 22

to Joe Downing, Noah Rose Ledesma, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org, droger+w...@chromium.org

Yuwei Huang voted and added 3 comments

Votes added by Yuwei Huang

Auto-Submit	+1
Commit-Queue	+2

3 comments

Patchset-level comments

File-level comment, Patchset 16 (Latest):

Yuwei Huang . resolved

Thanks!

File components/named_mojo_ipc_server/connection_info.h

Line 36, Patchset 15: // The process of the peer. Only valid if |should_open_caller_process| is true

Joe Downing . resolved

include_peer_process_info

Yuwei Huang

Done

File remoting/host/mojo_caller_security_checker.cc

Line 80, Patchset 15: // checks for Linux, which might be susceptible of PID reuse attacks.

Joe Downing . resolved

nit: to

Yuwei Huang

Done

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Apr 22, 2026, 6:29:58 PM (2 days ago) Apr 22

to Yuwei Huang, Joe Downing, Noah Rose Ledesma, android-bu...@system.gserviceaccount.com, chromium...@chromium.org, chromotin...@chromium.org, droger+w...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

15 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: remoting/host/mojo_caller_security_checker.cc
Insertions: 1, Deletions: 1.

@@ -77,7 +77,7 @@
   caller_process_image_path = GetProcessImagePath(caller.process);
 #else
   // TODO: yuweih - see if it's possible to move away from PID-based security
-  // checks for Linux, which might be susceptible of PID reuse attacks.
+  // checks for Linux, which might be susceptible to PID reuse attacks.
   caller_process_image_path = GetProcessImagePath(caller.pid);
 #endif
 
```
```
The name of the file: components/named_mojo_ipc_server/connection_info.h
Insertions: 1, Deletions: 1.

@@ -33,7 +33,7 @@
 #elif BUILDFLAG(IS_LINUX)
   ucred credentials{};
 #elif BUILDFLAG(IS_WIN)
-  // The process of the peer. Only valid if |should_open_caller_process| is true
+  // The process of the peer. Only valid if `include_peer_process_info` is true
   // in EndpointOptions.
   base::Process process;
 #endif
```

Change information

Commit message:

Improve security of IsTrustedMojoEndpoint

Add a new should_open_caller_process endpoint option, which makes the
endpoint connector open a process handle and set it in ConnectionInfo.
IsTrustedMojoEndpoint then uses it to validate the process image path.
This minimizes the window of potential PID-reuse attacks.

Bug: 496193452

Change-Id: Ie9624c8e279baf1aed8ff2ba53ef98f717332ff5

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7782841

Commit-Queue: Yuwei Huang <yuw...@chromium.org>

Reviewed-by: Joe Downing <joe...@chromium.org>

Auto-Submit: Yuwei Huang <yuw...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1619137}

Files:

M components/named_mojo_ipc_server/connection_info.h
M components/named_mojo_ipc_server/endpoint_options.h
M components/named_mojo_ipc_server/named_mojo_server_endpoint_connector_win.cc
M remoting/host/base/process_util.cc
M remoting/host/base/process_util.h
M remoting/host/chromoting_host_services_server.cc
M remoting/host/mojo_caller_security_checker.cc

Change size: M

Delta: 7 files changed, 58 insertions(+), 11 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Joe Downing

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

Improve security of IsTrustedMojoEndpoint [chromium/src : main]

Yuwei Huang (Gerrit)

Yuwei Huang added 2 comments

Related details

Yuwei Huang (Gerrit)

Yuwei Huang voted and added 3 comments

Votes added by Yuwei Huang

3 comments

Related details

Joe Downing (Gerrit)

Joe Downing added 3 comments

Related details

Yuwei Huang (Gerrit)

Yuwei Huang added 1 comment

Related details

Yuwei Huang (Gerrit)

Yuwei Huang voted and added 2 comments

Votes added by Yuwei Huang

2 comments

Related details

Joe Downing (Gerrit)

Joe Downing voted and added 2 comments

Votes added by Joe Downing

2 comments

Related details

Yuwei Huang (Gerrit)

Yuwei Huang voted and added 3 comments

Votes added by Yuwei Huang

3 comments

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information