fix(core): enforce explicit deny-read paths

This commit is contained in:
viyatb-oai
2026-03-27 00:56:08 -07:00
parent 47a9e2e084
commit 57df260e4c
15 changed files with 408 additions and 8 deletions
+6
View File
@@ -1158,6 +1158,12 @@ impl SessionConfiguration {
let mut sandbox_policy_changed = false;
if let Some(sandbox_policy) = updates.sandbox_policy.clone() {
next_configuration.sandbox_policy.set(sandbox_policy)?;
next_configuration.file_system_sandbox_policy =
FileSystemSandboxPolicy::from_legacy_sandbox_policy_preserving_read_denies(
next_configuration.sandbox_policy.get(),
&next_configuration.cwd,
&next_configuration.file_system_sandbox_policy,
);
next_configuration.network_sandbox_policy =
NetworkSandboxPolicy::from(next_configuration.sandbox_policy.get());
sandbox_policy_changed = true;
+6 -4
View File
@@ -296,10 +296,12 @@ impl ExecPolicyManager {
}
}
Decision::Allow => ExecApprovalRequirement::Skip {
// Bypass sandbox if execpolicy allows the command
bypass_sandbox: evaluation.matched_rules.iter().any(|rule_match| {
is_policy_match(rule_match) && rule_match.decision() == Decision::Allow
}),
// Keep sandboxing in place when the filesystem policy
// contains explicit deny-read carveouts.
bypass_sandbox: !file_system_sandbox_policy.has_denied_read_restrictions()
&& evaluation.matched_rules.iter().any(|rule_match| {
is_policy_match(rule_match) && rule_match.decision() == Decision::Allow
}),
proposed_execpolicy_amendment: if auto_amendment_allowed {
try_derive_execpolicy_amendment_for_allow_rules(&evaluation.matched_rules)
} else {
+31
View File
@@ -1645,6 +1645,37 @@ async fn dangerous_command_forbidden_in_external_sandbox_when_policy_matches() {
.await;
}
#[tokio::test]
async fn allow_rule_does_not_bypass_sandbox_when_deny_read_paths_exist() {
let policy_src = Some(r#"prefix_rule(pattern=["cat"], decision="allow")"#.to_string());
let command = vec_str(&["cat", "~/.gitconfig"]);
let deny_path = AbsolutePathBuf::try_from("/tmp/secret-config").expect("absolute path");
let file_system_sandbox_policy =
codex_protocol::permissions::FileSystemSandboxPolicy::restricted(vec![
FileSystemSandboxEntry {
path: FileSystemPath::Path { path: deny_path },
access: FileSystemAccessMode::None,
},
]);
assert_exec_approval_requirement_for_command(
ExecApprovalRequirementScenario {
policy_src,
command,
approval_policy: AskForApproval::OnRequest,
sandbox_policy: SandboxPolicy::new_workspace_write_policy(),
file_system_sandbox_policy,
sandbox_permissions: SandboxPermissions::UseDefault,
prefix_rule: None,
},
ExecApprovalRequirement::Skip {
bypass_sandbox: false,
proposed_execpolicy_amendment: None,
},
)
.await;
}
struct ExecApprovalRequirementScenario {
/// Source for the Starlark `.rules` file.
policy_src: Option<String>,
+174
View File
@@ -0,0 +1,174 @@
use std::path::Path;
use std::path::PathBuf;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_utils_absolute_path::AbsolutePathBuf;
use crate::function_tool::FunctionCallError;
const DENY_READ_POLICY_MESSAGE: &str =
"access denied: reading this path is blocked by filesystem deny_read policy";
pub(crate) fn ensure_read_allowed(
path: &Path,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
cwd: &Path,
) -> Result<(), FunctionCallError> {
if is_read_denied(path, file_system_sandbox_policy, cwd) {
return Err(FunctionCallError::RespondToModel(format!(
"{DENY_READ_POLICY_MESSAGE}: `{}`",
path.display()
)));
}
Ok(())
}
pub(crate) fn ensure_search_root_does_not_overlap_deny_read(
search_root: &Path,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
cwd: &Path,
) -> Result<(), FunctionCallError> {
if overlaps_deny_read(search_root, file_system_sandbox_policy, cwd) {
return Err(FunctionCallError::RespondToModel(format!(
"access denied: grep_files path `{}` overlaps a filesystem deny_read path; narrow the search path",
search_root.display()
)));
}
Ok(())
}
pub(crate) fn is_read_denied(
path: &Path,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
cwd: &Path,
) -> bool {
let denied_paths = file_system_sandbox_policy.get_unreadable_roots_with_cwd(cwd);
let path_candidates = normalized_and_canonical_candidates(path);
denied_paths.iter().any(|denied| {
let denied_candidates = normalized_and_canonical_candidates(denied.as_path());
path_candidates.iter().any(|candidate| {
denied_candidates.iter().any(|denied_candidate| {
candidate == denied_candidate || candidate.starts_with(denied_candidate)
})
})
})
}
pub(crate) fn overlaps_deny_read(
path: &Path,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
cwd: &Path,
) -> bool {
let denied_paths = file_system_sandbox_policy.get_unreadable_roots_with_cwd(cwd);
let path_candidates = normalized_and_canonical_candidates(path);
denied_paths.iter().any(|denied| {
let denied_candidates = normalized_and_canonical_candidates(denied.as_path());
path_candidates.iter().any(|candidate| {
denied_candidates.iter().any(|denied_candidate| {
candidate.starts_with(denied_candidate) || denied_candidate.starts_with(candidate)
})
})
})
}
fn normalized_and_canonical_candidates(path: &Path) -> Vec<PathBuf> {
let mut candidates = Vec::new();
if let Ok(normalized) = AbsolutePathBuf::from_absolute_path(path) {
push_unique(&mut candidates, normalized.to_path_buf());
} else {
push_unique(&mut candidates, path.to_path_buf());
}
if let Ok(canonical) = path.canonicalize()
&& let Ok(canonical_absolute) = AbsolutePathBuf::from_absolute_path(canonical)
{
push_unique(&mut candidates, canonical_absolute.to_path_buf());
}
candidates
}
fn push_unique(candidates: &mut Vec<PathBuf>, candidate: PathBuf) {
if !candidates.iter().any(|existing| existing == &candidate) {
candidates.push(candidate);
}
}
#[cfg(test)]
mod tests {
use codex_protocol::permissions::FileSystemAccessMode;
use codex_protocol::permissions::FileSystemPath;
use codex_protocol::permissions::FileSystemSandboxEntry;
use pretty_assertions::assert_eq;
use tempfile::tempdir;
use super::is_read_denied;
use super::overlaps_deny_read;
use super::*;
fn deny_policy(path: &std::path::Path) -> FileSystemSandboxPolicy {
FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
path: FileSystemPath::Path {
path: AbsolutePathBuf::try_from(path).expect("absolute deny path"),
},
access: FileSystemAccessMode::None,
}])
}
#[test]
fn exact_path_and_descendants_are_denied() {
let temp = tempdir().expect("temp dir");
let denied_dir = temp.path().join("denied");
let nested = denied_dir.join("nested.txt");
std::fs::create_dir_all(&denied_dir).expect("create denied dir");
std::fs::write(&nested, "secret").expect("write secret");
let policy = deny_policy(&denied_dir);
assert_eq!(is_read_denied(&denied_dir, &policy, temp.path()), true);
assert_eq!(is_read_denied(&nested, &policy, temp.path()), true);
assert_eq!(
is_read_denied(&temp.path().join("other.txt"), &policy, temp.path()),
false
);
}
#[cfg(unix)]
#[test]
fn canonical_target_matches_denied_symlink_alias() {
use std::os::unix::fs::symlink;
let temp = tempdir().expect("temp dir");
let real_dir = temp.path().join("real");
let alias_dir = temp.path().join("alias");
std::fs::create_dir_all(&real_dir).expect("create real dir");
symlink(&real_dir, &alias_dir).expect("symlink alias");
let secret = real_dir.join("secret.txt");
std::fs::write(&secret, "secret").expect("write secret");
let alias_secret = alias_dir.join("secret.txt");
let policy = deny_policy(&real_dir);
assert_eq!(is_read_denied(&alias_secret, &policy, temp.path()), true);
}
#[test]
fn overlap_detects_parent_and_child_relationships() {
let temp = tempdir().expect("temp dir");
let denied = temp.path().join("private");
let search_root = temp.path();
let nested_search_root = denied.join("nested");
std::fs::create_dir_all(&nested_search_root).expect("create nested");
let policy = deny_policy(&denied);
assert_eq!(overlaps_deny_read(search_root, &policy, temp.path()), true);
assert_eq!(
overlaps_deny_read(&nested_search_root, &policy, temp.path()),
true
);
assert_eq!(
overlaps_deny_read(&temp.path().join("public"), &policy, temp.path()),
false
);
}
}
+1
View File
@@ -39,6 +39,7 @@ pub mod exec_env;
mod exec_policy;
pub mod external_agent_config;
pub mod file_watcher;
mod filesystem_deny_read;
mod flags;
#[cfg(test)]
mod git_info_tests;
+33 -4
View File
@@ -9,6 +9,8 @@ use codex_utils_string::take_bytes_at_char_boundary;
use serde::Deserialize;
use tokio::fs;
use crate::filesystem_deny_read::ensure_read_allowed;
use crate::filesystem_deny_read::is_read_denied;
use crate::function_tool::FunctionCallError;
use crate::tools::context::FunctionToolOutput;
use crate::tools::context::ToolInvocation;
@@ -54,7 +56,7 @@ impl ToolHandler for ListDirHandler {
}
async fn handle(&self, invocation: ToolInvocation) -> Result<Self::Output, FunctionCallError> {
let ToolInvocation { payload, .. } = invocation;
let ToolInvocation { payload, turn, .. } = invocation;
let arguments = match payload {
ToolPayload::Function { arguments } => arguments,
@@ -98,8 +100,16 @@ impl ToolHandler for ListDirHandler {
"dir_path must be an absolute path".to_string(),
));
}
ensure_read_allowed(&path, &turn.file_system_sandbox_policy, &turn.cwd)?;
let entries = list_dir_slice(&path, offset, limit, depth).await?;
let entries = list_dir_slice_with_policy(
&path,
offset,
limit,
depth,
Some((&turn.file_system_sandbox_policy, &turn.cwd)),
)
.await?;
let mut output = Vec::with_capacity(entries.len() + 1);
output.push(format!("Absolute path: {}", path.display()));
output.extend(entries);
@@ -107,14 +117,25 @@ impl ToolHandler for ListDirHandler {
}
}
#[cfg(test)]
async fn list_dir_slice(
path: &Path,
offset: usize,
limit: usize,
depth: usize,
) -> Result<Vec<String>, FunctionCallError> {
list_dir_slice_with_policy(path, offset, limit, depth, None).await
}
async fn list_dir_slice_with_policy(
path: &Path,
offset: usize,
limit: usize,
depth: usize,
sandbox_policy: Option<(&codex_protocol::permissions::FileSystemSandboxPolicy, &Path)>,
) -> Result<Vec<String>, FunctionCallError> {
let mut entries = Vec::new();
collect_entries(path, Path::new(""), depth, &mut entries).await?;
collect_entries(path, Path::new(""), depth, sandbox_policy, &mut entries).await?;
if entries.is_empty() {
return Ok(Vec::new());
@@ -150,6 +171,7 @@ async fn collect_entries(
dir_path: &Path,
relative_prefix: &Path,
depth: usize,
sandbox_policy: Option<(&codex_protocol::permissions::FileSystemSandboxPolicy, &Path)>,
entries: &mut Vec<DirEntry>,
) -> Result<(), FunctionCallError> {
let mut queue = VecDeque::new();
@@ -165,6 +187,13 @@ async fn collect_entries(
while let Some(entry) = read_dir.next_entry().await.map_err(|err| {
FunctionCallError::RespondToModel(format!("failed to read directory: {err}"))
})? {
let entry_path = entry.path();
if let Some((policy, cwd)) = sandbox_policy
&& is_read_denied(&entry_path, policy, cwd)
{
continue;
}
let file_type = entry.file_type().await.map_err(|err| {
FunctionCallError::RespondToModel(format!("failed to inspect entry: {err}"))
})?;
@@ -181,7 +210,7 @@ async fn collect_entries(
let sort_key = format_entry_name(&relative_path);
let kind = DirEntryKind::from(&file_type);
dir_entries.push((
entry.path(),
entry_path,
relative_path,
kind,
DirEntry {
@@ -1,4 +1,8 @@
use super::*;
use codex_protocol::permissions::FileSystemAccessMode;
use codex_protocol::permissions::FileSystemPath;
use codex_protocol::permissions::FileSystemSandboxEntry;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use pretty_assertions::assert_eq;
use tempfile::tempdir;
@@ -239,3 +243,53 @@ async fn truncation_respects_sorted_order() -> anyhow::Result<()> {
Ok(())
}
#[tokio::test]
async fn hides_denied_entries_and_prunes_denied_subtrees() {
let temp = tempdir().expect("create tempdir");
let dir_path = temp.path();
let visible_dir = dir_path.join("visible");
let denied_dir = dir_path.join("private");
tokio::fs::create_dir(&visible_dir)
.await
.expect("create visible dir");
tokio::fs::create_dir(&denied_dir)
.await
.expect("create denied dir");
tokio::fs::write(visible_dir.join("ok.txt"), b"ok")
.await
.expect("write visible file");
tokio::fs::write(denied_dir.join("secret.txt"), b"secret")
.await
.expect("write denied file");
tokio::fs::write(dir_path.join("top_secret.txt"), b"secret")
.await
.expect("write denied top-level file");
let policy = FileSystemSandboxPolicy::restricted(vec![
FileSystemSandboxEntry {
path: FileSystemPath::Path {
path: denied_dir.try_into().expect("absolute denied dir"),
},
access: FileSystemAccessMode::None,
},
FileSystemSandboxEntry {
path: FileSystemPath::Path {
path: dir_path
.join("top_secret.txt")
.try_into()
.expect("absolute denied file"),
},
access: FileSystemAccessMode::None,
},
]);
let entries = list_dir_slice_with_policy(dir_path, 1, 20, 3, Some((&policy, dir_path)))
.await
.expect("list directory");
assert_eq!(
entries,
vec!["visible/".to_string(), " ok.txt".to_string(),]
);
}
+15
View File
@@ -174,6 +174,21 @@ pub(super) fn implicit_granted_permissions(
}
}
pub(super) fn reject_explicit_escalation_if_deny_read_present(
sandbox_permissions: SandboxPermissions,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
) -> Result<(), FunctionCallError> {
if sandbox_permissions.requires_escalated_permissions()
&& file_system_sandbox_policy.has_denied_read_restrictions()
{
return Err(FunctionCallError::RespondToModel(
"filesystem deny_read policy is enforced; reject command — you cannot ask for escalated permissions because managed read restrictions must remain sandboxed".to_string(),
));
}
Ok(())
}
pub(super) async fn apply_granted_turn_permissions(
session: &Session,
sandbox_permissions: SandboxPermissions,
@@ -27,6 +27,7 @@ use crate::tools::handlers::implicit_granted_permissions;
use crate::tools::handlers::normalize_and_validate_additional_permissions;
use crate::tools::handlers::parse_arguments;
use crate::tools::handlers::parse_arguments_with_base_path;
use crate::tools::handlers::reject_explicit_escalation_if_deny_read_present;
use crate::tools::handlers::resolve_workdir_base_path;
use crate::tools::orchestrator::ToolOrchestrator;
use crate::tools::registry::PostToolUsePayload;
@@ -459,6 +460,10 @@ impl ShellHandler {
"approval policy is {approval_policy:?}; reject command — you should not ask for escalated permissions if the approval policy is {approval_policy:?}"
)));
}
reject_explicit_escalation_if_deny_read_present(
effective_additional_permissions.sandbox_permissions,
&turn.file_system_sandbox_policy,
)?;
// Intercept apply_patch if present.
if let Some(output) = intercept_apply_patch(
@@ -16,6 +16,7 @@ use crate::tools::handlers::implicit_granted_permissions;
use crate::tools::handlers::normalize_and_validate_additional_permissions;
use crate::tools::handlers::parse_arguments;
use crate::tools::handlers::parse_arguments_with_base_path;
use crate::tools::handlers::reject_explicit_escalation_if_deny_read_present;
use crate::tools::handlers::resolve_workdir_base_path;
use crate::tools::registry::PostToolUsePayload;
use crate::tools::registry::PreToolUsePayload;
@@ -246,6 +247,10 @@ impl ToolHandler for UnifiedExecHandler {
"approval policy is {approval_policy:?}; reject command — you cannot ask for escalated permissions if the approval policy is {approval_policy:?}"
)));
}
reject_explicit_escalation_if_deny_read_present(
effective_additional_permissions.sandbox_permissions,
&context.turn.file_system_sandbox_policy,
)?;
let workdir = workdir.filter(|value| !value.is_empty());
@@ -10,6 +10,7 @@ use codex_utils_image::PromptImageMode;
use codex_utils_image::load_for_prompt_bytes;
use serde::Deserialize;
use crate::filesystem_deny_read::ensure_read_allowed;
use crate::function_tool::FunctionCallError;
use crate::original_image_detail::can_request_original_image_detail;
use crate::protocol::EventMsg;
@@ -93,6 +94,7 @@ impl ToolHandler for ViewImageHandler {
AbsolutePathBuf::try_from(turn.resolve_path(Some(args.path))).map_err(|error| {
FunctionCallError::RespondToModel(format!("unable to resolve image path: {error}"))
})?;
ensure_read_allowed(abs_path.as_path(), &turn.file_system_sandbox_policy, &turn.cwd)?;
let metadata = turn
.environment
+9
View File
@@ -241,6 +241,15 @@ impl ToolOrchestrator {
network_policy_decision,
})));
}
if turn_ctx
.file_system_sandbox_policy
.has_denied_read_restrictions()
{
return Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied {
output,
network_policy_decision,
})));
}
if !tool.escalate_on_failure() {
return Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied {
output,
+9
View File
@@ -8,6 +8,7 @@ use std::path::Path;
use codex_core::error::CodexErr;
use codex_core::error::Result;
use codex_core::error::SandboxErr;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_protocol::protocol::NetworkSandboxPolicy;
use codex_protocol::protocol::SandboxPolicy;
use codex_utils_absolute_path::AbsolutePathBuf;
@@ -41,6 +42,7 @@ use seccompiler::apply_filter;
/// Filesystem restrictions are intentionally handled by bubblewrap.
pub(crate) fn apply_sandbox_policy_to_current_thread(
sandbox_policy: &SandboxPolicy,
file_system_sandbox_policy: &FileSystemSandboxPolicy,
network_sandbox_policy: NetworkSandboxPolicy,
cwd: &Path,
apply_landlock_fs: bool,
@@ -67,6 +69,13 @@ pub(crate) fn apply_sandbox_policy_to_current_thread(
install_network_seccomp_filter_on_current_thread(mode)?;
}
if apply_landlock_fs && file_system_sandbox_policy.has_denied_read_restrictions() {
return Err(CodexErr::UnsupportedOperation(
"Filesystem deny_read restrictions are not supported by the legacy Linux Landlock filesystem backend."
.to_string(),
));
}
if apply_landlock_fs && !sandbox_policy.has_full_disk_write_access() {
if !sandbox_policy.has_full_disk_read_access() {
return Err(CodexErr::UnsupportedOperation(
@@ -149,6 +149,7 @@ pub fn run_main() -> ! {
let proxy_routing_active = allow_network_for_proxy;
if let Err(e) = apply_sandbox_policy_to_current_thread(
&sandbox_policy,
&file_system_sandbox_policy,
network_sandbox_policy,
&sandbox_policy_cwd,
/*apply_landlock_fs*/ false,
@@ -163,6 +164,7 @@ pub fn run_main() -> ! {
if file_system_sandbox_policy.has_full_disk_write_access() && !allow_network_for_proxy {
if let Err(e) = apply_sandbox_policy_to_current_thread(
&sandbox_policy,
&file_system_sandbox_policy,
network_sandbox_policy,
&sandbox_policy_cwd,
/*apply_landlock_fs*/ false,
@@ -210,6 +212,7 @@ pub fn run_main() -> ! {
// Legacy path: Landlock enforcement only, when bwrap sandboxing is not enabled.
if let Err(e) = apply_sandbox_policy_to_current_thread(
&sandbox_policy,
&file_system_sandbox_policy,
network_sandbox_policy,
&sandbox_policy_cwd,
/*apply_landlock_fs*/ true,
+55
View File
@@ -197,6 +197,33 @@ impl FileSystemSandboxPolicy {
.any(|entry| entry.access == FileSystemAccessMode::None)
}
pub fn has_denied_read_restrictions(&self) -> bool {
self.has_explicit_deny_entries()
}
pub fn from_legacy_sandbox_policy_preserving_read_denies(
sandbox_policy: &SandboxPolicy,
cwd: &Path,
existing: &Self,
) -> Self {
let mut rebuilt = Self::from_legacy_sandbox_policy(sandbox_policy, cwd);
if !matches!(rebuilt.kind, FileSystemSandboxKind::Restricted) {
return rebuilt;
}
for deny_entry in existing
.entries
.iter()
.filter(|entry| entry.access == FileSystemAccessMode::None)
{
if !rebuilt.entries.iter().any(|entry| entry == deny_entry) {
rebuilt.entries.push(deny_entry.clone());
}
}
rebuilt
}
/// Returns true when a restricted policy contains any entry that really
/// reduces a broader `:root = write` grant.
///
@@ -2121,4 +2148,32 @@ mod tests {
assert!(FileSystemAccessMode::Write > FileSystemAccessMode::Read);
assert!(FileSystemAccessMode::None > FileSystemAccessMode::Write);
}
#[test]
fn legacy_bridge_preserves_explicit_deny_entries() {
let denied = AbsolutePathBuf::try_from("/tmp/private").expect("absolute path");
let existing = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
path: FileSystemPath::Path {
path: denied.clone(),
},
access: FileSystemAccessMode::None,
}]);
let rebuilt = FileSystemSandboxPolicy::from_legacy_sandbox_policy_preserving_read_denies(
&SandboxPolicy::new_workspace_write_policy(),
Path::new("/tmp/workspace"),
&existing,
);
assert!(
rebuilt.entries.iter().any(|entry| {
entry.path
== FileSystemPath::Path {
path: denied.clone(),
}
&& entry.access == FileSystemAccessMode::None
}),
"expected explicit deny entry to be preserved"
);
}
}