fix: Other local users can no longer connect to the Unity Editor's uloop channel on Windows - #1322
Merged
Merged
Conversation
The named pipe server was created without an explicit ACL, so it inherited a default security descriptor that lets other local users open the pipe. Because any connected client can invoke execute-dynamic-code (arbitrary C# inside the Editor process), this let any local user on a shared or RDP host reach the deterministically named pipe and gain code execution as the Editor owner. Pass an explicit PipeSecurity that grants FullControl only to the current user's SID, denying every other local principal at the transport boundary.
…s PipeSecurity The previous commit (47ec948) restricted the Windows IPC pipe with the NamedPipeServerStream(..., PipeSecurity) overload. Verification on a real Windows editor (2022.3.62f3, NET_4_6 Mono) showed that approach is broken twice over: - WindowsIdentity.GetCurrent().User throws NotImplementedException, so every AcceptClient call failed and the swallowed exception made the accept loop spin at full CPU with no pipe ever created; uloop was completely unreachable. - Even with a valid SID, Mono silently ignores the PipeSecurity argument: the created pipe kept the default DACL (Everyone/Anonymous read), so the ACL was never applied. Create the pipe through CreateNamedPipeW with an explicit owner-only security descriptor (D:P(A;;FA;;;<current user SID>)) and wrap the native handle in a managed stream. The SID comes from the process token via GetTokenInformation because the managed identity APIs are unimplemented. The handle is deliberately non-overlapped: Mono hangs WaitForConnection on a wrapped overlapped handle, and the accept loop already waits on a dedicated worker thread. Closing a non-overlapped handle does not cancel a pending synchronous ConnectNamedPipe wait, so Stop now wakes a pending accept with a loopback connection before disposing, and AcceptClient discards a wait that ended after stop/cancellation instead of treating the wake client as a session. Verified on Windows 11: uloop compile/get-logs succeed across domain reloads, the live pipe DACL contains exactly one ACE for the owning user, and sequential CLI commands keep working. PipeStream.GetAccessControl was also observed to crash the editor natively on this runtime, so the DACL check must stay external (e.g. GetSecurityInfo from another process).
Contributor
|
Warning Review limit reached
More reviews will be available in 18 minutes and 16 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds Windows named pipe security restrictions by introducing an owner-only SDDL-protected pipe factory, integrates it into the bridge transport listener with graceful accept-loop shutdown, replaces the public OnError event with structured logging across the server, and adds recovery failure console visibility. ChangesNamed Pipe Security and Error Reporting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…bscribed OnError event UnityCliLoopBridgeServer raised every failure through an OnError event that has no subscriber anywhere in the codebase, so accept-loop errors, client session crashes, and disposal failures all vanished silently. This is why the Mono PipeSecurity regression (fixed in the previous commit) could spin the accept loop at full CPU for half an hour without a single log line. - Remove the dead OnError event and log each former call site directly. - Accept-loop failures now exit the loop and hand off to the existing bounded-backoff recovery path instead of retrying in a tight loop, and log to the Unity console: VibeLogger alone is not enough because all of its methods are [Conditional(ULOOP_DEBUG)] and compile out of end-user installs. - Terminal recovery failure also logs to the console; previously it ended in an unobserved task exception plus a debug-only log, leaving an unreachable server completely silent for end users. - Teardown-noise sites (client disconnect/dispose errors, thread aborts outside domain reload) log through VibeLogger at warning level. Verified with uloop compile (0 errors / 0 warnings) and the server-related EditMode suites (13 passed / 0 failed) against a live Windows editor.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
… path Address review feedback on the Windows named pipe ACL change: - Add PIPE_REJECT_REMOTE_CLIENTS to the native CreateNamedPipeW open mode. The managed NamedPipeServerStream sets this by default but the native path does not, so without it the execute-code channel would accept a remote principal over the network. Defense in depth on top of the owner-only DACL; local loopback (including the Stop() wake connection) is unaffected. - WakePendingAccept previously swallowed every exception, hiding failures in the only path that unblocks a pending synchronous accept. Log the failure at debug level: the common case (no accept pending) is harmless, but if a stuck accept ever causes a shutdown hang this is the only breadcrumb pointing at it. Verified with uloop compile (0/0) and the factory/listener/shutdown EditMode suites (7 passed / 0 failed) on a live Windows editor.
Merged
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
1 participant
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.
Summary
PipeSecurity) turned out to be silently broken on Unity's Mono runtime; this PR replaces it with an implementation that verifiably applies the ACL on a real Windows editor.uloopcommand without leaving a trace.User Impact
execute-dynamic-code, i.e. run arbitrary C# with the Editor owner's privileges.uloopcommand hung and the Editor burned a full CPU core — with no log output whatsoever, because all server errors were raised through an event nobody subscribes to.uloopworks normally for the Editor's owner, the pipe's DACL contains exactly one ACE granting access to that user only, and if the server ever fails to accept connections the Unity console says so and the server restarts itself with bounded backoff.Changes
CreateNamedPipeW) with an explicit owner-only security descriptor and wrap the handle in a managed stream, because Unity's Mono ignores thePipeSecurityctor argument andWindowsIdentity.GetCurrent().UserthrowsNotImplementedException.OpenProcessToken/GetTokenInformation).WaitForConnectionon a wrapped overlapped handle. Since closing a non-overlapped handle does not cancel a pending synchronous wait,Stop()now wakes a pending accept with a loopback connection before disposing, and the accept loop discards that wake connection.OnErrorevent (zero subscribers) and log each former call site directly. Genuine failures (accept loop, client session crashes, terminal recovery failure) go to the Unity console —VibeLoggeralone compiles out of end-user installs ([Conditional(ULOOP_DEBUG)]). Teardown noise stays at debug-only warning level.Verification
uloop compile(0 errors / 0 warnings),uloop get-logs, and repeated sequential commands against a live Windows 11 / Unity 2022.3.62f3 editor, including recovery across domain reloads.GetSecurityInfo:O:<owner> D:P(A;;FA;;;<owner SID>)— no Everyone/Anonymous/Administrators/SYSTEM ACEs.uloop run-tests(EditMode): server-related suites 13 passed / 0 failed (new factory tests + listener contract + server shutdown + heartbeat tests).