fix: allow restricted filesystem profiles to read helper executables (#15114)
## Summary This PR fixes restricted filesystem permission profiles so Codex's runtime-managed helper executables remain readable without requiring explicit user configuration. - add implicit readable roots for the configured `zsh` helper path and the main execve wrapper - allowlist the shared `$CODEX_HOME/tmp/arg0` root when the execve wrapper lives there, so session-specific helper paths keep working - dedupe injected paths and avoid adding duplicate read entries to the sandbox policy - add regression coverage for restricted read mode with helper executable overrides ## Testing before this change: got this error when executing a shell command via zsh fork: ``` "sandbox error: sandbox denied exec error, exit code: 127, stdout: , stderr: /etc/zprofile:11: operation not permitted: /usr/libexec/path_helper\nzsh:1: operation not permitted: .codex/skills/proxy-a/scripts/fetch_example.sh\n" ``` saw this change went away after this change, meaning the readable roots and injected correctly.
This commit is contained in:
parent
10a936d127
commit
9eef2e91fc
4 changed files with 220 additions and 27 deletions
|
|
@ -98,6 +98,7 @@ use std::path::Path;
|
|||
use std::path::PathBuf;
|
||||
|
||||
use crate::config::permissions::compile_permission_profile;
|
||||
use crate::config::permissions::get_readable_roots_required_for_codex_runtime;
|
||||
use crate::config::permissions::network_proxy_config_from_profile_network;
|
||||
use crate::config::profile::ConfigProfile;
|
||||
use codex_network_proxy::NetworkProxyConfig;
|
||||
|
|
@ -1918,29 +1919,6 @@ fn resolve_permission_config_syntax(
|
|||
})
|
||||
}
|
||||
|
||||
fn add_additional_file_system_writes(
|
||||
file_system_sandbox_policy: &mut FileSystemSandboxPolicy,
|
||||
additional_writable_roots: &[AbsolutePathBuf],
|
||||
) {
|
||||
for path in additional_writable_roots {
|
||||
let exists = file_system_sandbox_policy.entries.iter().any(|entry| {
|
||||
matches!(
|
||||
&entry.path,
|
||||
codex_protocol::permissions::FileSystemPath::Path { path: existing }
|
||||
if existing == path && entry.access == codex_protocol::permissions::FileSystemAccessMode::Write
|
||||
)
|
||||
});
|
||||
if !exists {
|
||||
file_system_sandbox_policy.entries.push(
|
||||
codex_protocol::permissions::FileSystemSandboxEntry {
|
||||
path: codex_protocol::permissions::FileSystemPath::Path { path: path.clone() },
|
||||
access: codex_protocol::permissions::FileSystemAccessMode::Write,
|
||||
},
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Optional overrides for user configuration (e.g., from CLI flags).
|
||||
#[derive(Default, Debug, Clone)]
|
||||
pub struct ConfigOverrides {
|
||||
|
|
@ -2309,10 +2287,8 @@ impl Config {
|
|||
let mut sandbox_policy = file_system_sandbox_policy
|
||||
.to_legacy_sandbox_policy(network_sandbox_policy, &resolved_cwd)?;
|
||||
if matches!(sandbox_policy, SandboxPolicy::WorkspaceWrite { .. }) {
|
||||
add_additional_file_system_writes(
|
||||
&mut file_system_sandbox_policy,
|
||||
&additional_writable_roots,
|
||||
);
|
||||
file_system_sandbox_policy = file_system_sandbox_policy
|
||||
.with_additional_writable_roots(&resolved_cwd, &additional_writable_roots);
|
||||
sandbox_policy = file_system_sandbox_policy
|
||||
.to_legacy_sandbox_policy(network_sandbox_policy, &resolved_cwd)?;
|
||||
}
|
||||
|
|
@ -2663,6 +2639,11 @@ impl Config {
|
|||
} else {
|
||||
network.enabled().then_some(network)
|
||||
};
|
||||
let helper_readable_roots = get_readable_roots_required_for_codex_runtime(
|
||||
&codex_home,
|
||||
zsh_path.as_ref(),
|
||||
main_execve_wrapper_exe.as_ref(),
|
||||
);
|
||||
let effective_sandbox_policy = constrained_sandbox_policy.value.get().clone();
|
||||
let effective_file_system_sandbox_policy =
|
||||
if effective_sandbox_policy == original_sandbox_policy {
|
||||
|
|
@ -2673,6 +2654,8 @@ impl Config {
|
|||
&resolved_cwd,
|
||||
)
|
||||
};
|
||||
let effective_file_system_sandbox_policy = effective_file_system_sandbox_policy
|
||||
.with_additional_readable_roots(&resolved_cwd, &helper_readable_roots);
|
||||
let effective_network_sandbox_policy =
|
||||
if effective_sandbox_policy == original_sandbox_policy {
|
||||
network_sandbox_policy
|
||||
|
|
|
|||
|
|
@ -190,6 +190,37 @@ pub(crate) fn compile_permission_profile(
|
|||
))
|
||||
}
|
||||
|
||||
/// Returns a list of paths that must be readable by shell tools in order
|
||||
/// for Codex to function. These should always be added to the
|
||||
/// `FileSystemSandboxPolicy` for a thread.
|
||||
pub(crate) fn get_readable_roots_required_for_codex_runtime(
|
||||
codex_home: &Path,
|
||||
zsh_path: Option<&PathBuf>,
|
||||
main_execve_wrapper_exe: Option<&PathBuf>,
|
||||
) -> Vec<AbsolutePathBuf> {
|
||||
let arg0_root = AbsolutePathBuf::from_absolute_path(codex_home.join("tmp").join("arg0")).ok();
|
||||
let zsh_path = zsh_path.and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok());
|
||||
let execve_wrapper_root = main_execve_wrapper_exe.and_then(|path| {
|
||||
let path = AbsolutePathBuf::from_absolute_path(path).ok()?;
|
||||
if let Some(arg0_root) = arg0_root.as_ref()
|
||||
&& path.as_path().starts_with(arg0_root.as_path())
|
||||
{
|
||||
path.parent()
|
||||
} else {
|
||||
Some(path)
|
||||
}
|
||||
});
|
||||
|
||||
let mut readable_roots = Vec::new();
|
||||
if let Some(zsh_path) = zsh_path {
|
||||
readable_roots.push(zsh_path);
|
||||
}
|
||||
if let Some(execve_wrapper_root) = execve_wrapper_root {
|
||||
readable_roots.push(execve_wrapper_root);
|
||||
}
|
||||
readable_roots
|
||||
}
|
||||
|
||||
fn compile_network_sandbox_policy(network: Option<&NetworkToml>) -> NetworkSandboxPolicy {
|
||||
let Some(network) = network else {
|
||||
return NetworkSandboxPolicy::Restricted;
|
||||
|
|
|
|||
|
|
@ -1,5 +1,11 @@
|
|||
use super::*;
|
||||
use crate::config::Config;
|
||||
use crate::config::ConfigOverrides;
|
||||
use crate::config::ConfigToml;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::BTreeMap;
|
||||
use tempfile::TempDir;
|
||||
|
||||
#[test]
|
||||
fn normalize_absolute_path_for_platform_simplifies_windows_verbatim_paths() {
|
||||
|
|
@ -7,3 +13,66 @@ fn normalize_absolute_path_for_platform_simplifies_windows_verbatim_paths() {
|
|||
normalize_absolute_path_for_platform(r"\\?\D:\c\x\worktrees\2508\swift-base", true);
|
||||
assert_eq!(parsed, PathBuf::from(r"D:\c\x\worktrees\2508\swift-base"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn restricted_read_implicitly_allows_helper_executables() -> std::io::Result<()> {
|
||||
let temp_dir = TempDir::new()?;
|
||||
let cwd = temp_dir.path().join("workspace");
|
||||
let codex_home = temp_dir.path().join(".codex");
|
||||
let zsh_path = temp_dir.path().join("runtime").join("zsh");
|
||||
let arg0_root = codex_home.join("tmp").join("arg0");
|
||||
let allowed_arg0_dir = arg0_root.join("codex-arg0-session");
|
||||
let sibling_arg0_dir = arg0_root.join("codex-arg0-other-session");
|
||||
let execve_wrapper = allowed_arg0_dir.join("codex-execve-wrapper");
|
||||
std::fs::create_dir_all(&cwd)?;
|
||||
std::fs::create_dir_all(zsh_path.parent().expect("zsh path should have parent"))?;
|
||||
std::fs::create_dir_all(&allowed_arg0_dir)?;
|
||||
std::fs::create_dir_all(&sibling_arg0_dir)?;
|
||||
std::fs::write(&zsh_path, "")?;
|
||||
std::fs::write(&execve_wrapper, "")?;
|
||||
|
||||
let config = Config::load_from_base_config_with_overrides(
|
||||
ConfigToml {
|
||||
default_permissions: Some("workspace".to_string()),
|
||||
permissions: Some(PermissionsToml {
|
||||
entries: BTreeMap::from([(
|
||||
"workspace".to_string(),
|
||||
PermissionProfileToml {
|
||||
filesystem: Some(FilesystemPermissionsToml {
|
||||
entries: BTreeMap::new(),
|
||||
}),
|
||||
network: None,
|
||||
},
|
||||
)]),
|
||||
}),
|
||||
..Default::default()
|
||||
},
|
||||
ConfigOverrides {
|
||||
cwd: Some(cwd.clone()),
|
||||
zsh_path: Some(zsh_path.clone()),
|
||||
main_execve_wrapper_exe: Some(execve_wrapper),
|
||||
..Default::default()
|
||||
},
|
||||
codex_home,
|
||||
)?;
|
||||
|
||||
let expected_zsh = AbsolutePathBuf::try_from(zsh_path)?;
|
||||
let expected_allowed_arg0_dir = AbsolutePathBuf::try_from(allowed_arg0_dir)?;
|
||||
let expected_sibling_arg0_dir = AbsolutePathBuf::try_from(sibling_arg0_dir)?;
|
||||
let policy = &config.permissions.file_system_sandbox_policy;
|
||||
|
||||
assert!(
|
||||
policy.can_read_path_with_cwd(expected_zsh.as_path(), &cwd),
|
||||
"expected zsh helper path to be readable, policy: {policy:?}"
|
||||
);
|
||||
assert!(
|
||||
policy.can_read_path_with_cwd(expected_allowed_arg0_dir.as_path(), &cwd),
|
||||
"expected active arg0 helper dir to be readable, policy: {policy:?}"
|
||||
);
|
||||
assert!(
|
||||
!policy.can_read_path_with_cwd(expected_sibling_arg0_dir.as_path(), &cwd),
|
||||
"expected sibling arg0 helper dir to remain unreadable, policy: {policy:?}"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
|
|
|||
|
|
@ -347,6 +347,48 @@ impl FileSystemSandboxPolicy {
|
|||
self.resolve_access_with_cwd(path, cwd).can_write()
|
||||
}
|
||||
|
||||
pub fn with_additional_readable_roots(
|
||||
mut self,
|
||||
cwd: &Path,
|
||||
additional_readable_roots: &[AbsolutePathBuf],
|
||||
) -> Self {
|
||||
if self.has_full_disk_read_access() {
|
||||
return self;
|
||||
}
|
||||
|
||||
for path in additional_readable_roots {
|
||||
if self.can_read_path_with_cwd(path.as_path(), cwd) {
|
||||
continue;
|
||||
}
|
||||
|
||||
self.entries.push(FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Path { path: path.clone() },
|
||||
access: FileSystemAccessMode::Read,
|
||||
});
|
||||
}
|
||||
|
||||
self
|
||||
}
|
||||
|
||||
pub fn with_additional_writable_roots(
|
||||
mut self,
|
||||
cwd: &Path,
|
||||
additional_writable_roots: &[AbsolutePathBuf],
|
||||
) -> Self {
|
||||
for path in additional_writable_roots {
|
||||
if self.can_write_path_with_cwd(path.as_path(), cwd) {
|
||||
continue;
|
||||
}
|
||||
|
||||
self.entries.push(FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Path { path: path.clone() },
|
||||
access: FileSystemAccessMode::Write,
|
||||
});
|
||||
}
|
||||
|
||||
self
|
||||
}
|
||||
|
||||
pub fn needs_direct_runtime_enforcement(
|
||||
&self,
|
||||
network_policy: NetworkSandboxPolicy,
|
||||
|
|
@ -1782,6 +1824,74 @@ mod tests {
|
|||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn with_additional_readable_roots_skips_existing_effective_access() {
|
||||
let cwd = TempDir::new().expect("tempdir");
|
||||
let cwd_root = AbsolutePathBuf::from_absolute_path(cwd.path()).expect("absolute cwd");
|
||||
let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Special {
|
||||
value: FileSystemSpecialPath::CurrentWorkingDirectory,
|
||||
},
|
||||
access: FileSystemAccessMode::Read,
|
||||
}]);
|
||||
|
||||
let actual = policy
|
||||
.clone()
|
||||
.with_additional_readable_roots(cwd.path(), std::slice::from_ref(&cwd_root));
|
||||
|
||||
assert_eq!(actual, policy);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn with_additional_writable_roots_skips_existing_effective_access() {
|
||||
let cwd = TempDir::new().expect("tempdir");
|
||||
let cwd_root = AbsolutePathBuf::from_absolute_path(cwd.path()).expect("absolute cwd");
|
||||
let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Special {
|
||||
value: FileSystemSpecialPath::CurrentWorkingDirectory,
|
||||
},
|
||||
access: FileSystemAccessMode::Write,
|
||||
}]);
|
||||
|
||||
let actual = policy
|
||||
.clone()
|
||||
.with_additional_writable_roots(cwd.path(), std::slice::from_ref(&cwd_root));
|
||||
|
||||
assert_eq!(actual, policy);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn with_additional_writable_roots_adds_new_root() {
|
||||
let temp_dir = TempDir::new().expect("tempdir");
|
||||
let cwd = temp_dir.path().join("workspace");
|
||||
let extra = AbsolutePathBuf::from_absolute_path(temp_dir.path().join("extra"))
|
||||
.expect("resolve extra root");
|
||||
let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Special {
|
||||
value: FileSystemSpecialPath::CurrentWorkingDirectory,
|
||||
},
|
||||
access: FileSystemAccessMode::Write,
|
||||
}]);
|
||||
|
||||
let actual = policy.with_additional_writable_roots(&cwd, std::slice::from_ref(&extra));
|
||||
|
||||
assert_eq!(
|
||||
actual,
|
||||
FileSystemSandboxPolicy::restricted(vec![
|
||||
FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Special {
|
||||
value: FileSystemSpecialPath::CurrentWorkingDirectory,
|
||||
},
|
||||
access: FileSystemAccessMode::Write,
|
||||
},
|
||||
FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Path { path: extra },
|
||||
access: FileSystemAccessMode::Write,
|
||||
},
|
||||
])
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn file_system_access_mode_orders_by_conflict_precedence() {
|
||||
assert!(FileSystemAccessMode::Write > FileSystemAccessMode::Read);
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue