mirror of
https://github.com/openai/codex.git
synced 2026-10-20 17:38:45 +00:00
Run sandbox integrity checks before commands and filesystem operations (#51843)
## What changed Wire existing sandbox integrity checks into local command preparation, exec-server process preparation, and sandboxed filesystem helper and file-open operations. Make execution request preparation asynchronous so callers can await the checks. The checks remain observational: failures and timeouts do not prevent command execution. ## Testing Add regression tests that launch a real command after checker preparation fails or exceeds its timeout, verifying successful output and exit status. GitOrigin-RevId: a82399f8aeb8b08f9965e6121b4c7f460552af3f
This commit is contained in:
@@ -329,7 +329,8 @@ pub async fn process_exec_tool_call(
|
||||
codex_self_exe,
|
||||
windows_sandbox_type,
|
||||
use_legacy_landlock,
|
||||
)?;
|
||||
)
|
||||
.await?;
|
||||
|
||||
// Route through the sandboxing module for a single, unified execution path.
|
||||
crate::sandboxing::execute_env(exec_req, stdout_stream).await
|
||||
@@ -338,7 +339,7 @@ pub async fn process_exec_tool_call(
|
||||
/// Transform a portable exec request into the concrete argv/env that should be
|
||||
/// spawned under the requested sandbox policy.
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub fn build_exec_request(
|
||||
pub async fn build_exec_request(
|
||||
params: ExecParams,
|
||||
permission_profile: &PermissionProfile,
|
||||
sandbox_cwd: &AbsolutePathBuf,
|
||||
@@ -439,7 +440,7 @@ pub fn build_exec_request(
|
||||
} else {
|
||||
Vec::new()
|
||||
};
|
||||
ExecRequest::from_sandbox_exec_request(request, options, windows_sandbox_workspace_roots)
|
||||
ExecRequest::from_sandbox_exec_request(request, options, windows_sandbox_workspace_roots).await
|
||||
}
|
||||
|
||||
pub(crate) async fn execute_exec_request(
|
||||
|
||||
@@ -1,7 +1,13 @@
|
||||
use super::*;
|
||||
use codex_protocol::config_types::WindowsSandboxLevel;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::permissions::FileSystemAccessMode;
|
||||
use codex_protocol::permissions::FileSystemPath;
|
||||
use codex_protocol::permissions::FileSystemSandboxEntry;
|
||||
use codex_protocol::permissions::FileSystemSandboxPolicy;
|
||||
use codex_protocol::sandbox::SandboxOverride;
|
||||
use codex_sandboxing::FileContentsChecker;
|
||||
use codex_sandboxing::SandboxExecRequest;
|
||||
use codex_sandboxing::SandboxType;
|
||||
use core_test_support::PathBufExt;
|
||||
use core_test_support::PathExt;
|
||||
@@ -1197,11 +1203,11 @@ fn process_exec_tool_call_uses_platform_sandbox_for_network_only_restrictions()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_exec_request_projects_workspace_roots_only_for_windows_sandbox() -> Result<()> {
|
||||
#[tokio::test]
|
||||
async fn build_exec_request_projects_workspace_roots_only_for_windows_sandbox() -> Result<()> {
|
||||
let temp_dir = tempfile::TempDir::new()?;
|
||||
let cwd = temp_dir.path().abs();
|
||||
let build_request = |profile: &PermissionProfile, roots: &[PathUri]| {
|
||||
let build_request = async |profile: &PermissionProfile, roots: &[PathUri]| {
|
||||
build_exec_request(
|
||||
ExecParams {
|
||||
command: vec!["echo".to_string(), "ok".to_string()],
|
||||
@@ -1224,6 +1230,7 @@ fn build_exec_request_projects_workspace_roots_only_for_windows_sandbox() -> Res
|
||||
SandboxType::WindowsRestrictedToken,
|
||||
/*use_legacy_landlock*/ false,
|
||||
)
|
||||
.await
|
||||
};
|
||||
let native_roots = vec![cwd.clone(), temp_dir.path().join("additional").abs()];
|
||||
let request = build_request(
|
||||
@@ -1232,7 +1239,8 @@ fn build_exec_request_projects_workspace_roots_only_for_windows_sandbox() -> Res
|
||||
.iter()
|
||||
.map(PathUri::from_abs_path)
|
||||
.collect::<Vec<_>>(),
|
||||
)?;
|
||||
)
|
||||
.await?;
|
||||
assert_eq!(
|
||||
request.windows_sandbox_workspace_roots,
|
||||
if cfg!(windows) {
|
||||
@@ -1250,10 +1258,12 @@ fn build_exec_request_projects_workspace_roots_only_for_windows_sandbox() -> Res
|
||||
.expect("foreign workspace URI");
|
||||
let roots = [PathUri::from_abs_path(&cwd), foreign_root];
|
||||
assert_eq!(
|
||||
build_request(&PermissionProfile::Disabled, &roots)?.windows_sandbox_workspace_roots,
|
||||
build_request(&PermissionProfile::Disabled, &roots)
|
||||
.await?
|
||||
.windows_sandbox_workspace_roots,
|
||||
Vec::new(),
|
||||
);
|
||||
let request = build_request(&PermissionProfile::read_only(), &roots);
|
||||
let request = build_request(&PermissionProfile::read_only(), &roots).await;
|
||||
if cfg!(windows) {
|
||||
let error = request.expect_err("native Windows sandbox must reject foreign roots");
|
||||
assert!(matches!(
|
||||
@@ -1267,6 +1277,99 @@ fn build_exec_request_projects_workspace_roots_only_for_windows_sandbox() -> Res
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(false; "checker_error")]
|
||||
#[test_case(true; "checker_timeout")]
|
||||
fn command_runs_after_integrity_check_failure(time_out: bool) -> anyhow::Result<()> {
|
||||
let directory = tempfile::tempdir()?;
|
||||
let cwd = directory.path().abs();
|
||||
let mut policy = FileSystemSandboxPolicy::read_only();
|
||||
policy.entries.push(FileSystemSandboxEntry::new(
|
||||
FileSystemPath::GlobPattern {
|
||||
pattern: format!("{}/[z-a]", cwd.display()),
|
||||
},
|
||||
FileSystemAccessMode::Deny,
|
||||
));
|
||||
assert!(FileContentsChecker::new(&policy, &cwd).is_err());
|
||||
let permission_profile =
|
||||
PermissionProfile::from_runtime_permissions(&policy, NetworkSandboxPolicy::Restricted);
|
||||
// This boundary receives prepared argv. Exercise a real child without requiring
|
||||
// a native sandbox installation to test the checker's failure isolation.
|
||||
let request = SandboxExecRequest {
|
||||
command: if cfg!(windows) {
|
||||
vec![
|
||||
"cmd.exe".into(),
|
||||
"/D".into(),
|
||||
"/C".into(),
|
||||
"echo integrity-ok".into(),
|
||||
]
|
||||
} else {
|
||||
vec!["/bin/echo".into(), "integrity-ok".into()]
|
||||
},
|
||||
cwd: PathUri::from_abs_path(&cwd),
|
||||
sandbox_policy_cwd: PathUri::from_abs_path(&cwd),
|
||||
env: std::env::vars().collect(),
|
||||
network: None,
|
||||
network_environment_id: None,
|
||||
sandbox: select_process_exec_tool_sandbox_type(
|
||||
&permission_profile,
|
||||
SandboxType::WindowsMxc,
|
||||
/*enforce_managed_network*/ false,
|
||||
),
|
||||
sandbox_override: SandboxOverride::NoOverride,
|
||||
windows_sandbox_level: WindowsSandboxLevel::Disabled,
|
||||
permission_profile,
|
||||
arg0: None,
|
||||
};
|
||||
tokio::runtime::Builder::new_current_thread()
|
||||
.enable_all()
|
||||
.max_blocking_threads(/*val*/ 1)
|
||||
.build()?
|
||||
.block_on(async {
|
||||
let mut prepared = Box::pin(ExecRequest::from_sandbox_exec_request(
|
||||
request,
|
||||
ExecOptions {
|
||||
expiration: ExecExpiration::DefaultTimeout,
|
||||
capture_policy: ExecCapturePolicy::ShellTool,
|
||||
},
|
||||
Vec::new(),
|
||||
));
|
||||
let request = if time_out {
|
||||
tokio::time::pause();
|
||||
// Keep checker work queued past the caller's deadline. Dropping
|
||||
// release unblocks the worker even if an assertion fails.
|
||||
let (release, wait) = std::sync::mpsc::channel::<()>();
|
||||
let (started, ready) = tokio::sync::oneshot::channel();
|
||||
let blocker = tokio::task::spawn_blocking(move || {
|
||||
let _ = started.send(());
|
||||
let _ = wait.recv();
|
||||
});
|
||||
ready.await?;
|
||||
assert!(futures::poll!(&mut prepared).is_pending());
|
||||
tokio::time::advance(Duration::from_secs(/*secs*/ 6)).await;
|
||||
let result = futures::poll!(&mut prepared);
|
||||
drop(release);
|
||||
blocker.await?;
|
||||
tokio::time::resume();
|
||||
let std::task::Poll::Ready(result) = result else {
|
||||
panic!("command preparation must stop waiting for the checker");
|
||||
};
|
||||
result?
|
||||
} else {
|
||||
prepared.await?
|
||||
};
|
||||
let output = crate::sandboxing::execute_env(request, /*stdout_stream*/ None).await?;
|
||||
assert_eq!(
|
||||
(
|
||||
output.exit_code,
|
||||
output.stdout.text.trim(),
|
||||
output.timed_out
|
||||
),
|
||||
(0, "integrity-ok", false),
|
||||
);
|
||||
Ok(())
|
||||
})
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn sandbox_detection_flags_sigsys_exit_code() {
|
||||
|
||||
@@ -114,11 +114,12 @@ impl ExecRequest {
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn from_sandbox_exec_request(
|
||||
pub(crate) async fn from_sandbox_exec_request(
|
||||
request: SandboxExecRequest,
|
||||
options: ExecOptions,
|
||||
windows_sandbox_workspace_roots: Vec<AbsolutePathBuf>,
|
||||
) -> Result<Self, CodexErr> {
|
||||
codex_exec_server::run_integrity_checks(&request).await;
|
||||
let SandboxExecRequest {
|
||||
command,
|
||||
cwd,
|
||||
|
||||
@@ -197,6 +197,7 @@ impl ShellSnapshotSandbox {
|
||||
},
|
||||
workspace_roots,
|
||||
)
|
||||
.await
|
||||
.context("failed to prepare shell snapshot execution")?;
|
||||
// Caller cancellation must leave execution alive long enough to clean up its process group.
|
||||
let output = tokio::spawn(execute_env(request, /*stdout_stream*/ None))
|
||||
|
||||
@@ -147,6 +147,7 @@ async fn explicit_escalation_prepares_exec_without_managed_network() -> anyhow::
|
||||
),
|
||||
/*environment_id*/ None,
|
||||
)
|
||||
.await
|
||||
.expect("prepare exec request");
|
||||
|
||||
assert_eq!(exec_request.cwd, PathUri::from_abs_path(&command_cwd));
|
||||
|
||||
@@ -688,6 +688,7 @@ impl<'a> ToolRuntime<UnifiedExecRequest, UnifiedExecAttempt> for UnifiedExecRunt
|
||||
managed_network,
|
||||
Some(&req.turn_environment.selection.environment_id),
|
||||
)
|
||||
.await
|
||||
.map_err(ToolError::Codex)?;
|
||||
exec_env.exec_server_env_config = req.exec_server_env_config.clone();
|
||||
match zsh_fork::maybe_prepare_unified_exec(req, attempt, ctx, exec_env, zsh_fork_config)
|
||||
|
||||
@@ -717,7 +717,8 @@ impl CoreShellCommandExecutor {
|
||||
env,
|
||||
permission_profile: &self.permission_profile,
|
||||
additional_permissions: None,
|
||||
})?
|
||||
})
|
||||
.await?
|
||||
}
|
||||
EscalationExecution::Permissions(
|
||||
EscalationPermissions::AdditionalPermissionProfile(permission_profile),
|
||||
@@ -729,7 +730,8 @@ impl CoreShellCommandExecutor {
|
||||
env,
|
||||
permission_profile: &self.permission_profile,
|
||||
additional_permissions: Some(permission_profile),
|
||||
})?
|
||||
})
|
||||
.await?
|
||||
}
|
||||
EscalationExecution::Permissions(EscalationPermissions::ResolvedPermissionProfile(
|
||||
permissions,
|
||||
@@ -741,7 +743,8 @@ impl CoreShellCommandExecutor {
|
||||
env,
|
||||
permission_profile: &permissions.permission_profile,
|
||||
additional_permissions: None,
|
||||
})?
|
||||
})
|
||||
.await?
|
||||
}
|
||||
};
|
||||
|
||||
@@ -749,7 +752,7 @@ impl CoreShellCommandExecutor {
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
fn prepare_sandboxed_exec(
|
||||
async fn prepare_sandboxed_exec(
|
||||
&self,
|
||||
params: PrepareSandboxedExecParams<'_>,
|
||||
) -> anyhow::Result<PreparedExec> {
|
||||
@@ -800,7 +803,8 @@ impl CoreShellCommandExecutor {
|
||||
exec_request,
|
||||
options,
|
||||
self.windows_sandbox_workspace_roots.clone(),
|
||||
)?;
|
||||
)
|
||||
.await?;
|
||||
if let Some(network) = exec_request.network.as_ref() {
|
||||
network
|
||||
.apply_to_env_for_optional_environment(
|
||||
|
||||
@@ -487,7 +487,7 @@ impl<'a> SandboxAttempt<'a> {
|
||||
}
|
||||
}
|
||||
|
||||
pub fn env_for(
|
||||
pub async fn env_for(
|
||||
&self,
|
||||
command: SandboxCommand,
|
||||
options: ExecOptions,
|
||||
@@ -517,9 +517,10 @@ impl<'a> SandboxAttempt<'a> {
|
||||
.map(PathUri::to_abs_path)
|
||||
.collect::<std::io::Result<Vec<_>>>()?;
|
||||
crate::sandboxing::ExecRequest::from_sandbox_exec_request(request, options, workspace_roots)
|
||||
.await
|
||||
}
|
||||
|
||||
pub fn env_for_exec_server(
|
||||
pub async fn env_for_exec_server(
|
||||
&self,
|
||||
command: SandboxCommand,
|
||||
options: ExecOptions,
|
||||
@@ -546,11 +547,9 @@ impl<'a> SandboxAttempt<'a> {
|
||||
})
|
||||
.map_err(CodexErr::from)?;
|
||||
request.sandbox_override = self.sandbox_override;
|
||||
let mut exec_request = crate::sandboxing::ExecRequest::from_sandbox_exec_request(
|
||||
request,
|
||||
options,
|
||||
Vec::new(),
|
||||
)?;
|
||||
let mut exec_request =
|
||||
crate::sandboxing::ExecRequest::from_sandbox_exec_request(request, options, Vec::new())
|
||||
.await?;
|
||||
exec_request.exec_server_managed_network = managed_network;
|
||||
if self.sandbox_requested {
|
||||
exec_request.exec_server_sandbox = Some(FileSystemSandboxContext {
|
||||
|
||||
@@ -250,8 +250,8 @@ fn deny_read_preserves_the_sandbox_for_explicit_escalation_and_blocks_policy_byp
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn windows_sandbox_env_preserves_denied_reads_or_rejects_unsupported_backend() {
|
||||
#[tokio::test]
|
||||
async fn windows_sandbox_env_preserves_denied_reads_or_rejects_unsupported_backend() {
|
||||
let temp_dir = tempfile::TempDir::new().expect("create sandbox workspace");
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(
|
||||
dunce::canonicalize(temp_dir.path()).expect("canonicalize sandbox workspace"),
|
||||
@@ -329,6 +329,7 @@ fn windows_sandbox_env_preserves_denied_reads_or_rejects_unsupported_backend() {
|
||||
/*network*/ None,
|
||||
/*environment_id*/ None,
|
||||
)
|
||||
.await
|
||||
.expect("prepare elevated Windows sandbox request");
|
||||
let overrides = request
|
||||
.windows_sandbox_filesystem_overrides
|
||||
@@ -345,6 +346,7 @@ fn windows_sandbox_env_preserves_denied_reads_or_rejects_unsupported_backend() {
|
||||
/*network*/ None,
|
||||
/*environment_id*/ None,
|
||||
)
|
||||
.await
|
||||
.expect_err("restricted-token Windows sandbox cannot enforce deny-read restrictions");
|
||||
assert_eq!(
|
||||
error.to_string(),
|
||||
@@ -352,8 +354,8 @@ fn windows_sandbox_env_preserves_denied_reads_or_rejects_unsupported_backend() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn exec_server_env_keeps_command_native_and_carries_sandbox_context() {
|
||||
#[tokio::test]
|
||||
async fn exec_server_env_keeps_command_native_and_carries_sandbox_context() {
|
||||
let cwd: AbsolutePathBuf = std::env::current_dir()
|
||||
.expect("current dir")
|
||||
.try_into()
|
||||
@@ -400,6 +402,7 @@ fn exec_server_env_keeps_command_native_and_carries_sandbox_context() {
|
||||
};
|
||||
let request = attempt
|
||||
.env_for_exec_server(command(), options())
|
||||
.await
|
||||
.expect("prepare remote exec request");
|
||||
assert!(!attempt.is_escalated());
|
||||
|
||||
@@ -440,6 +443,7 @@ fn exec_server_env_keeps_command_native_and_carries_sandbox_context() {
|
||||
attempt.sandbox_override = SandboxOverride::EscalatedSandboxWithRestrictions;
|
||||
let escalated_request = attempt
|
||||
.env_for_exec_server(command(), options())
|
||||
.await
|
||||
.expect("prepare approved escalation with retained sandbox restrictions");
|
||||
let mut expected_context = request.exec_server_sandbox.expect("sandbox context");
|
||||
expected_context.sandbox_override = SandboxOverride::EscalatedSandboxWithRestrictions;
|
||||
@@ -451,6 +455,7 @@ fn exec_server_env_keeps_command_native_and_carries_sandbox_context() {
|
||||
attempt.sandbox_requested = false;
|
||||
let request = attempt
|
||||
.env_for_exec_server(command(), options())
|
||||
.await
|
||||
.expect("prepare unsandboxed remote exec request");
|
||||
assert!(attempt.is_escalated());
|
||||
|
||||
|
||||
@@ -1268,9 +1268,11 @@ impl UnifiedExecProcessManager {
|
||||
environment: &codex_exec_server::Environment,
|
||||
) -> Result<UnifiedExecProcess, ToolError> {
|
||||
let mut request = if environment.is_remote() || shell_snapshot.is_some() {
|
||||
attempt.env_for_exec_server(command, options)
|
||||
attempt.env_for_exec_server(command, options).await
|
||||
} else {
|
||||
attempt.env_for(command, options, network, environment_id)
|
||||
attempt
|
||||
.env_for(command, options, network, environment_id)
|
||||
.await
|
||||
}
|
||||
.map_err(ToolError::Codex)?;
|
||||
let network_policy_decider = network_proxy_launch
|
||||
|
||||
Reference in New Issue
Block a user