fix(promote): verify named pipe server process - #3042
Open
lhx077 wants to merge 1 commit into
Open
Conversation
pcl-ce-automation
Bot
added
🛠️ 等待审查
Contributor
审阅者指南添加了一个用于 用于验证 promote 进程命名管道服务器的时序图sequenceDiagram
participant PromoteProcess
participant NamedPipeServer
participant KernelInterop
participant Kernel32
PromoteProcess->>NamedPipeServer: NamedPipeClientStream.Connect(10000)
PromoteProcess->>KernelInterop: GetNamedPipeServerProcessId(pipe.SafePipeHandle.DangerousGetHandle())
KernelInterop->>Kernel32: _GetNamedPipeServerProcessId(pipeHandle, out serverProcessId)
Kernel32-->>KernelInterop: serverProcessId
KernelInterop-->>PromoteProcess: serverProcessId
alt serverProcessId == process.Id
PromoteProcess->>NamedPipeServer: [continue start/start-json handling]
else serverProcessId != process.Id
PromoteProcess->>PromoteProcess: Context.Error(...)
PromoteProcess->>NamedPipeServer: pipe.Dispose()
PromoteProcess-->>PromoteProcess: return
end
文件级变更
提示与命令与 Sourcery 交互
自定义你的使用体验访问你的 dashboard 以：
获取帮助Original review guide in EnglishReviewer's GuideAdds a Windows interop wrapper for GetNamedPipeServerProcessId and uses it in the promote helper to verify the named-pipe server process matches the expected parent process, preventing elevation via a spoofed named pipe server while leaving existing promote behavior intact. Sequence diagram for promote process named pipe server verificationsequenceDiagram
participant PromoteProcess
participant NamedPipeServer
participant KernelInterop
participant Kernel32
PromoteProcess->>NamedPipeServer: NamedPipeClientStream.Connect(10000)
PromoteProcess->>KernelInterop: GetNamedPipeServerProcessId(pipe.SafePipeHandle.DangerousGetHandle())
KernelInterop->>Kernel32: _GetNamedPipeServerProcessId(pipeHandle, out serverProcessId)
Kernel32-->>KernelInterop: serverProcessId
KernelInterop-->>PromoteProcess: serverProcessId
alt serverProcessId == process.Id
PromoteProcess->>NamedPipeServer: [continue start/start-json handling]
else serverProcessId != process.Id
PromoteProcess->>PromoteProcess: Context.Error(...)
PromoteProcess->>NamedPipeServer: pipe.Dispose()
PromoteProcess-->>PromoteProcess: return
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并留下了一些总体反馈：
- 在调用
GetNamedPipeServerProcessId时，可以通过重载 P/Invoke 使其接收SafePipeHandle（或者使用pipe.SafePipeHandle.DangerousAddRef/Release），从而避免使用DangerousGetHandle()，这样在本机调用执行期间就能正确遵守安全句柄的 GC/生命周期语义。 - 在
_PerformAsPromoteProcess中，将GetNamedPipeServerProcessId返回的uint强制转换为int时，对于大于int.MaxValue的 PID 可能会发生溢出；建议保持比较逻辑使用uint，或者使用 checked 转换，以避免在高 PID 系统上出现隐蔽的不匹配问题。
供 AI Agent 使用的提示
Please address the comments from this code review:
## Overall Comments
- When calling `GetNamedPipeServerProcessId`, you can avoid `DangerousGetHandle()` by overloading the P/Invoke to take a `SafePipeHandle` (or using `pipe.SafePipeHandle.DangerousAddRef`/`Release`) so the GC/lifetime semantics of the safe handle are respected while the native call is in flight.
- The cast from the `uint` return value of `GetNamedPipeServerProcessId` to `int` in `_PerformAsPromoteProcess` can overflow for PIDs above `int.MaxValue`; consider keeping the comparison in `uint` or using a checked cast to avoid subtle mismatches on high PID systems.
## Individual Comments
### Comment 1
<location path="PCL.Core/App/Essentials/PromoteService.cs" line_range="135" />
<code_context>
var pipeName = _GetPromotePipeName(process.Id);
var pipe = new NamedPipeClientStream(".", pipeName, PipeDirection.InOut);
pipe.Connect(10000);
+ var serverProcessId = (int)KernelInterop.GetNamedPipeServerProcessId(pipe.SafePipeHandle.DangerousGetHandle());
+ if (serverProcessId != process.Id)
+ {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid casting the server PID from uint to int to prevent potential overflow and mismatches on large PIDs.
`GetNamedPipeServerProcessId` returns a `uint` while `Process.Id` is an `int`. Casting the `uint` to `int` risks overflow and an incorrect/negative PID if values exceed `Int32.MaxValue`. Instead, keep the server PID as `uint` and compare against `(uint)process.Id`:
```csharp
var serverProcessId = KernelInterop.GetNamedPipeServerProcessId(pipe.SafePipeHandle.DangerousGetHandle());
if (serverProcessId != (uint)process.Id)
{
// ...
}
```
</issue_to_address>帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- When calling
GetNamedPipeServerProcessId, you can avoidDangerousGetHandle()by overloading the P/Invoke to take aSafePipeHandle(or usingpipe.SafePipeHandle.DangerousAddRef/Release) so the GC/lifetime semantics of the safe handle are respected while the native call is in flight. - The cast from the
uintreturn value ofGetNamedPipeServerProcessIdtointin_PerformAsPromoteProcesscan overflow for PIDs aboveint.MaxValue; consider keeping the comparison inuintor using a checked cast to avoid subtle mismatches on high PID systems.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When calling `GetNamedPipeServerProcessId`, you can avoid `DangerousGetHandle()` by overloading the P/Invoke to take a `SafePipeHandle` (or using `pipe.SafePipeHandle.DangerousAddRef`/`Release`) so the GC/lifetime semantics of the safe handle are respected while the native call is in flight.
- The cast from the `uint` return value of `GetNamedPipeServerProcessId` to `int` in `_PerformAsPromoteProcess` can overflow for PIDs above `int.MaxValue`; consider keeping the comparison in `uint` or using a checked cast to avoid subtle mismatches on high PID systems.
## Individual Comments
### Comment 1
<location path="PCL.Core/App/Essentials/PromoteService.cs" line_range="135" />
<code_context>
var pipeName = _GetPromotePipeName(process.Id);
var pipe = new NamedPipeClientStream(".", pipeName, PipeDirection.InOut);
pipe.Connect(10000);
+ var serverProcessId = (int)KernelInterop.GetNamedPipeServerProcessId(pipe.SafePipeHandle.DangerousGetHandle());
+ if (serverProcessId != process.Id)
+ {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid casting the server PID from uint to int to prevent potential overflow and mismatches on large PIDs.
`GetNamedPipeServerProcessId` returns a `uint` while `Process.Id` is an `int`. Casting the `uint` to `int` risks overflow and an incorrect/negative PID if values exceed `Int32.MaxValue`. Instead, keep the server PID as `uint` and compare against `(uint)process.Id`:
```csharp
var serverProcessId = KernelInterop.GetNamedPipeServerProcessId(pipe.SafePipeHandle.DangerousGetHandle());
if (serverProcessId != (uint)process.Id)
{
// ...
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| var pipeName = _GetPromotePipeName(process.Id); | ||
| var pipe = new NamedPipeClientStream(".", pipeName, PipeDirection.InOut); | ||
| pipe.Connect(10000); | ||
| var serverProcessId = (int)KernelInterop.GetNamedPipeServerProcessId(pipe.SafePipeHandle.DangerousGetHandle()); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): 建议不要将服务器 PID 从 uint 强制转换为 int，以避免在较大的 PID 上发生潜在溢出和不匹配。
GetNamedPipeServerProcessId 返回 uint，而 Process.Id 是 int。当数值超过 Int32.MaxValue 时，将 uint 强制转换为 int 会有溢出风险，并可能得到错误/负数的 PID。相反，建议保持服务器 PID 为 uint，并与 (uint)process.Id 进行比较：
var serverProcessId = KernelInterop.GetNamedPipeServerProcessId(pipe.SafePipeHandle.DangerousGetHandle());
if (serverProcessId != (uint)process.Id)
{
// ...
}Original comment in English
suggestion (bug_risk): Avoid casting the server PID from uint to int to prevent potential overflow and mismatches on large PIDs.
GetNamedPipeServerProcessId returns a uint while Process.Id is an int. Casting the uint to int risks overflow and an incorrect/negative PID if values exceed Int32.MaxValue. Instead, keep the server PID as uint and compare against (uint)process.Id:
var serverProcessId = KernelInterop.GetNamedPipeServerProcessId(pipe.SafePipeHandle.DangerousGetHandle());
if (serverProcessId != (uint)process.Id)
{
// ...
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2fe7e242c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var pipeName = _GetPromotePipeName(process.Id); | ||
| var pipe = new NamedPipeClientStream(".", pipeName, PipeDirection.InOut); | ||
| pipe.Connect(10000); | ||
| var serverProcessId = (int)KernelInterop.GetNamedPipeServerProcessId(pipe.SafePipeHandle.DangerousGetHandle()); |
There was a problem hiding this comment.
When the promoted helper reaches this line, pipe is a NamedPipeClientStream, so its handle was opened as the client end after Connect(). The Win32 GetNamedPipeServerProcessId API expects a named-pipe instance handle created by CreateNamedPipe (the server end), and the wrapper throws on failure; this means every promote launch aborts before Context.Info("已连接，开始通信"), leaving privileged start/start-json operations unusable rather than merely rejecting spoofed servers.
Useful? React with 👍 / 👎.
Member
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
promotehelper could connect to a predictable named pipe owned by an attacker because only the main process executable path was checked.start/start-jsoncommands.Description
KernelInterop.GetNamedPipeServerProcessIdthat callsGetNamedPipeServerProcessIdinPCL.Core/Utils/OS/KernelInterop.cs.PromoteService._PerformAsPromoteProcess, after connecting theNamedPipeClientStream, obtain the server PID viaKernelInterop.GetNamedPipeServerProcessId(pipe.SafePipeHandle.DangerousGetHandle())and compare it to the expectedprocess.Id, closing the pipe and aborting if they differ.Testing
git diff --checkto validate whitespace and basic diff sanity (passed).dotnet --info(failed in this environment because thedotnetSDK/runtime is not installed).dotnet build PCL.Core/PCL.Core.csproj -c Debug(could not run due to missingdotnetin the container).Codex Task
Summary by Sourcery
验证用于特权提升助手（privileged promote helper）的 Windows 命名管道服务器进程，以防止连接到伪造的服务器。
Bug 修复：
增强功能：
GetNamedPipeServerProcessId添加一个KernelInterop包装器，用于获取命名管道的服务器进程 ID，并通过现有的 Win32 错误处理机制抛出错误。Original summary in English
Summary by Sourcery
Verify the Windows named pipe server process for the privileged promote helper to prevent connecting to spoofed servers.
Bug Fixes:
Enhancements: