Elevated Sandbox 3 #7809
Elevated Sandbox 3 #7809
Conversation
![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.
ℹ️ 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".
iceweasel-oai
force-pushed
the
dev/iceweasel/elevated-sandbox-3
branch
from
f8b4ff0 to
a2e9b51
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| if ok == 0 { | ||
| return Err(anyhow::anyhow!("SetInformationJobObject failed")); |
There was a problem hiding this comment.
If SetInformationJobObject fails, the function returns early without closing the job handle and the caller later drops the error with .ok(), so the handle is leaked and the process runs without the intended kill-on-close guard. When the job setup fails (e.g., due to missing privileges), a subsequent crash or timeout can leave the child process orphaned because the job object never enforces kill-on-close.
Useful? React with 👍 / 👎.
| ); | ||
|
|
||
| let policy = parse_policy(&req.policy_json_or_preset).context("parse policy_json_or_preset")?; | ||
| let psid_cap: *mut c_void = unsafe { convert_string_sid_to_sid(&req.cap_sid).unwrap() }; |
There was a problem hiding this comment.
The runner converts the capability SID with convert_string_sid_to_sid(&req.cap_sid).unwrap(). If the request file contains an invalid SID string (corrupted or user-supplied), this panics before any logging or cleanup instead of returning a structured error, so the command runner will crash rather than reporting a bad request.
Useful? React with 👍 / 👎.
etraut-openai
left a comment
etraut-openai
left a comment
There was a problem hiding this comment.
Looks good. I spotted one potential problem that you should look into before merging.
| ) | ||
| }; | ||
| let (proc_info, _si) = match spawn_result { | ||
| Ok(v) => v, |
There was a problem hiding this comment.
We're closing these handles only in the error case. I'm not 100%, but I think we probably want to close them in the success case as well. I think these handles are ref counted by the kernel, so when they're passed to CreateProcessAsUserW, the ref count will be incremented. Since we don't close them here, they will never be closed. You should confirm this.
There was a problem hiding this comment.
great catch! I'll fix this
| }; | ||
|
|
||
| // Optional job kill on close. | ||
| let h_job = unsafe { create_job_kill_on_close().ok() }; |
There was a problem hiding this comment.
Nice use of a job object!
dedicated sandbox command runner exe.