fix: run apply_patch calls through the sandbox by bolinfest · Pull Request #1705 · openai/codex

bolinfest · 2025-07-28T07:04:56Z

Building on the work of #1702, this changes how a shell call to apply_patch is handled.

Previously, a shell call to apply_patch was always handled in-process, never leveraging a sandbox. To determine whether the apply_patch operation could be auto-approved, the is_write_patch_constrained_to_writable_paths() function would check if all the paths listed in the paths were writable. If so, the agent would apply the changes listed in the patch.

Unfortunately, this approach afforded a loophole: symlinks!

For a soft link, we could fix this issue by tracing the link and checking whether the target is in the set of writable paths, however...
...For a hard link, things are not as simple. We can run stat FILE to see if the number of links is greater than 1, but then we would have to do something potentially expensive like find . -inum <inode_number> to find the other paths for FILE. Further, even if this worked, this approach runs the risk of a TOCTOU race condition, so it is not robust.

The solution, implemented in this PR, is to take the virtual execution of the apply_patch CLI into an actual execution using codex --codex-run-as-apply-patch PATCH, which we can run under the sandbox the user specified, just like any other shell call.

This, of course, assumes that the sandbox prevents writing through symlinks as a mechanism to write to folders that are not in the writable set configured by the sandbox. I verified this by testing the following on both Mac and Linux:

#!/usr/bin/env bash
set -euo pipefail

# Can running a command in SANDBOX_DIR write a file in EXPLOIT_DIR?

# Codex is run in SANDBOX_DIR, so writes should be constrianed to this directory.
SANDBOX_DIR=$(mktemp -d -p "$HOME" sandboxtesttemp.XXXXXX)
# EXPLOIT_DIR is outside of SANDBOX_DIR, so let's see if we can write to it.
EXPLOIT_DIR=$(mktemp -d -p "$HOME" sandboxtesttemp.XXXXXX)

echo "SANDBOX_DIR: $SANDBOX_DIR"
echo "EXPLOIT_DIR: $EXPLOIT_DIR"

cleanup() {
  # Only remove if it looks sane and still exists
  [[ -n "${SANDBOX_DIR:-}" && -d "$SANDBOX_DIR" ]] && rm -rf -- "$SANDBOX_DIR"
  [[ -n "${EXPLOIT_DIR:-}" && -d "$EXPLOIT_DIR" ]] && rm -rf -- "$EXPLOIT_DIR"
}

trap cleanup EXIT

echo "I am the original content" > "${EXPLOIT_DIR}/original.txt"

# Drop the -s to test hard links.
ln -s "${EXPLOIT_DIR}/original.txt" "${SANDBOX_DIR}/link-to-original.txt"

cat "${SANDBOX_DIR}/link-to-original.txt"

if [[ "$(uname)" == "Linux" ]]; then
    SANDBOX_SUBCOMMAND=landlock
else
    SANDBOX_SUBCOMMAND=seatbelt
fi

# Attempt the exploit
cd "${SANDBOX_DIR}"

codex debug "${SANDBOX_SUBCOMMAND}" bash -lc "echo pwned > ./link-to-original.txt" || true

cat "${EXPLOIT_DIR}/original.txt"

Admittedly, this change merits a proper integration test, but I think I will have to do that in a follow-up PR.

Apparently `std::env::args()` will panic during iteration if any argument to the process is not valid Unicode: https://doc.rust-lang.org/std/env/fn.args.html Let's avoid the risk and just go with `std::env::args_os()`. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/1698). * #1705 * #1703 * #1702 * __->__ #1698 * #1697

This introduces some special behavior to the CLIs that are using the `codex-arg0` crate where if `arg1` is `--codex-run-as-apply-patch`, then it will run as if `apply_patch arg2` were invoked. This is important because it means we can do things like: ``` SANDBOX_TYPE=landlock # or seatbelt for macOS codex debug "${SANDBOX_TYPE}" -- codex --codex-run-as-apply-patch PATCH ``` which gives us a way to run `apply_patch` while ensuring it adheres to the sandbox the user specified. While it would be nice to use the `arg0` trick like we are currently doing for `codex-linux-sandbox`, there is no way to specify the `arg0` for the underlying command when running under `/usr/bin/sandbox-exec`, so it will not work for us in this case. Admittedly, we could have also supported this via a custom environment variable (e.g., `CODEX_ARG0`), but since environment variables are inherited by child processes, that seemed like a potentially leakier abstraction. This change, as well as our existing reliance on checking `arg0`, place additional requirements on those who include `codex-core`. Its `README.md` has been updated to reflect this. While we could have just added an `apply-patch` subcommand to the `codex` multitool CLI, that would not be sufficient for the standalone `codex-exec` CLI, which is something that we distribute as part of our GitHub releases for those who know they will not be using the TUI and therefore prefer to use a slightly smaller executable: https://github.com/openai/codex/releases/tag/rust-v0.10.0 To that end, this PR adds an integration test to ensure that the `--codex-run-as-apply-patch` option works with the standalone `codex-exec` CLI. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/1702). * #1705 * #1703 * __->__ #1702 * #1698 * #1697

#1703) This is a straight refactor, moving apply-patch-related code from `codex.rs` and into the new `apply_patch.rs` file. The only "logical" change is inlining `#[allow(clippy::unwrap_used)]` instead of declaring `#![allow(clippy::unwrap_used)]` at the top of the file (which is currently the case in `codex.rs`). --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/1703). * #1705 * __->__ #1703 * #1702 * #1698 * #1697

bolinfest · 2025-07-29T00:36:23Z

/recheck

bolinfest · 2025-07-29T00:36:50Z

/cla recheck

pakrym-oai · 2025-07-29T01:20:36Z

+            let path_to_codex = std::env::current_exe()
+                .ok()
+                .map(|p| p.to_string_lossy().to_string());
+            let Some(path_to_codex) = path_to_codex else {


Should we even be returning to the model in this case?

I mean, it is possible the user has a non-UTF8 path to codex, so we would get it in our server logs to find out? I think that's better than just panic'ing.

pakrym-oai · 2025-07-29T01:21:30Z

+                };
            };
+
+            let params = ExecParams {


Can we abstract this out of codex.rs? to some generic apply path function that does exec internally?

codex.rs is getting huge

here, the goal is to generate (params, safety, command_for_display) so that everything else is the same as a normal exec, though

I can look at refactoring this large function in a follow-up though

bolinfest changed the base branch from main to pr1703 July 28, 2025 07:05

bolinfest force-pushed the pr1703 branch from 4bb8685 to 2d190ae Compare July 28, 2025 15:34

bolinfest force-pushed the pr1705 branch from 68076e6 to 99608d5 Compare July 28, 2025 15:34

bolinfest force-pushed the pr1705 branch from 99608d5 to 73a21d0 Compare July 28, 2025 15:53

bolinfest force-pushed the pr1703 branch from 2d190ae to 1b1d4b6 Compare July 28, 2025 15:53

bolinfest force-pushed the pr1705 branch from 73a21d0 to 787b7a7 Compare July 28, 2025 16:27

Base automatically changed from pr1703 to main July 28, 2025 16:51

bolinfest force-pushed the pr1705 branch 6 times, most recently from be2a1c3 to a41375c Compare July 29, 2025 00:18

bolinfest marked this pull request as ready for review July 29, 2025 00:35

bolinfest force-pushed the pr1705 branch from a41375c to 53f2bbc Compare July 29, 2025 00:37

bolinfest requested review from khai-oai and pakrym-oai July 29, 2025 00:38

pakrym-oai reviewed Jul 29, 2025

View reviewed changes

khai-oai approved these changes Jul 29, 2025

View reviewed changes

pakrym-oai approved these changes Jul 30, 2025

View reviewed changes

fix: run apply_patch calls through the sandbox

355cded

bolinfest force-pushed the pr1705 branch from 53f2bbc to 355cded Compare July 30, 2025 23:39

bolinfest merged commit 221ebfc into main Jul 30, 2025
22 checks passed

bolinfest deleted the pr1705 branch July 30, 2025 23:45

github-actions Bot locked and limited conversation to collaborators Jul 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: run apply_patch calls through the sandbox#1705

fix: run apply_patch calls through the sandbox#1705
bolinfest merged 1 commit into
mainfrom
pr1705

bolinfest commented Jul 28, 2025 •

edited

Loading

Uh oh!

bolinfest commented Jul 29, 2025

Uh oh!

bolinfest commented Jul 29, 2025

Uh oh!

pakrym-oai Jul 29, 2025

Uh oh!

bolinfest Jul 30, 2025

Uh oh!

pakrym-oai Jul 29, 2025

Uh oh!

pakrym-oai Jul 29, 2025

Uh oh!

bolinfest Jul 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

bolinfest commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bolinfest commented Jul 29, 2025

Uh oh!

bolinfest commented Jul 29, 2025

Uh oh!

pakrym-oai Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

bolinfest Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

pakrym-oai Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

pakrym-oai Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

bolinfest Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bolinfest commented Jul 28, 2025 •

edited

Loading