[codex] Fix elevated Windows sandbox named-pipe access#20270
Conversation
iceweasel-oai
changed the title
- openai/codex#20270: thread token user SID into restricting-SID list for elevated Windows sandbox to fix Ninja named-pipe ACL access (merge-after-nits) - BerriAI/litellm#26823: drop sensitive locals (client_messages, dynamic_callback_params, _response_headers) from re-raised error messages to prevent PII/secret leakage into exception payloads (merge-after-nits) - BerriAI/litellm#26819: route team-callback + org-member mutation endpoints through _verify_team_access/_verify_org_access closing IDOR cluster, with 4xx-honest exception handling and comprehensive regression suite (merge-as-is)
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
| match &policy { | ||
| SandboxPolicy::ReadOnly { .. } => { | ||
| create_readonly_token_with_caps_from(base.raw(), &cap_psid_ptrs) | ||
| create_readonly_token_with_caps_and_user_from(base.raw(), &cap_psid_ptrs) |
There was a problem hiding this comment.
Non-blocking, and I do not think this has security impact, but one small side effect worth calling out: this is a little broader than the named-pipe case. For example, an object that already grants CodexSandboxOffline access, such as a file owned by that sandbox account under its own profile, will now also satisfy the restricted-SID pass. If we ever wanted to keep read-only narrower, the follow-up would be to leave ReadOnly on the old helper and only opt it into this path once we have a concrete read-only repro that needs the broader behavior too. I think the current tradeoff is still reasonable because this is the dedicated sandbox account, not the real signed-in user.
There was a problem hiding this comment.
@viyatb-oai yep, it is broader for sure. I addressed this in the PR with
this does not affect file-writes for the sandbox because the sandbox users themselves do not receive any additional permissions over what the capability SIDs already have. In fact we don't even explicitly grant the sandbox user ACLs anywhere.
viyatb-oai
left a comment
viyatb-oai
left a comment
There was a problem hiding this comment.
lgtm with a non blocking comment
2 participants
Summary
Why
Windows named pipes created by tools like Ninja use the platform's default named-pipe ACL when no explicit security descriptor is provided. In the elevated sandbox, the pipe owner has access, but the write-restricted token can still fail its restricted-SID access check because the sandbox user SID was not in the restricting SID set. That causes child processes to exit successfully while Ninja never receives the expected pipe completion/close behavior and hangs.
Including the elevated sandbox user's SID in the restricting SID list lets the restricted check succeed for these owner-scoped pipe objects without broadening the unelevated sandbox to the real signed-in user.
Impact
Validation
cargo build -p codex-windows-sandbox --quietninja.exeminimal repro exits normally on host and in the elevated sandbox