mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
feat(sandbox): add Windows deny-read parity (#18202)
## Why The split filesystem policy stack already supports exact and glob `access = none` read restrictions on macOS and Linux. Windows still needed subprocess handling for those deny-read policies without claiming enforcement from a backend that cannot provide it. ## Key finding The unelevated restricted-token backend cannot safely enforce deny-read overlays. Its `WRITE_RESTRICTED` token model is authoritative for write checks, not read denials, so this PR intentionally fails that backend closed when deny-read overrides are present instead of claiming unsupported enforcement. ## What changed This PR adds the Windows deny-read enforcement layer and makes the backend split explicit: - Resolves Windows deny-read filesystem policy entries into concrete ACL targets. - Preserves exact missing paths so they can be materialized and denied before an enforceable sandboxed process starts. - Snapshot-expands existing glob matches into ACL targets for Windows subprocess enforcement. - Honors `glob_scan_max_depth` when expanding Windows deny-read globs. - Plans both the configured lexical path and the canonical target for existing paths so reparse-point aliases are covered. - Threads deny-read overrides through the elevated/logon-user Windows sandbox backend and unified exec. - Applies elevated deny-read ACLs synchronously before command launch rather than delegating them to the background read-grant helper. - Reconciles persistent deny-read ACEs per sandbox principal so policy changes do not leave stale deny-read ACLs behind. - Fails closed on the unelevated restricted-token backend when deny-read overrides are present, because its `WRITE_RESTRICTED` token model is not authoritative for read denials. ## Landed prerequisites These prerequisite PRs are already on `main`: 1. #15979 `feat(permissions): add glob deny-read policy support` 2. #18096 `feat(sandbox): add glob deny-read platform enforcement` 3. #17740 `feat(config): support managed deny-read requirements` This PR targets `main` directly and contains only the Windows deny-read enforcement layer. ## Implementation notes - Exact deny-read paths remain enforceable on the elevated path even when they do not exist yet: Windows materializes the missing path before applying the deny ACE, so the sandboxed command cannot create and read it during the same run. - Existing exact deny paths are preserved lexically until the ACL planner, which then adds the canonical target as a second ACL target when needed. That keeps both the configured alias and the resolved object covered. - Windows ACLs do not consume Codex glob syntax directly, so glob deny-read entries are expanded to the concrete matches that exist before process launch. - Glob traversal deduplicates directory visits within each pattern walk to avoid cycles, without collapsing distinct lexical roots that happen to resolve to the same target. - Persistent deny-read ACL state is keyed by sandbox principal SID, so cleanup only removes ACEs owned by the same backend principal. - Deny-read ACEs are fail-closed on the elevated path: setup aborts if mandatory deny-read ACL application fails. - Unelevated restricted-token sessions reject deny-read overrides early instead of running with a silently unenforceable read policy. ## Verification - `cargo test -p codex-core windows_restricted_token_rejects_unreadable_split_carveouts` - `just fmt` - `just fix -p codex-core` - `just fix -p codex-windows-sandbox` - GitHub Actions rerun is in progress on the pushed head. --------- Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
1 parent
c9e46ed639
commit
46f30d0282
24 files changed
+1548
-176
No files matched your search
@@ -5,6 +5,8 @@ use crate::allow::AllowDenyPaths;
|
||||
use crate::allow::compute_allow_paths;
|
||||
use crate::cap::load_or_create_cap_sids;
|
||||
use crate::cap::workspace_cap_sid_for_cwd;
|
||||
use crate::deny_read_acl::apply_deny_read_acls;
|
||||
use crate::deny_read_state::sync_persistent_deny_read_acls;
|
||||
use crate::env::apply_no_network_to_env;
|
||||
use crate::env::ensure_non_interactive_pager;
|
||||
use crate::env::inherit_path_env;
|
||||
@@ -25,6 +27,7 @@ use crate::token::get_logon_sid_bytes;
|
||||
use crate::workspace_acl::is_command_cwd_root;
|
||||
use crate::workspace_acl::protect_workspace_agents_dir;
|
||||
use crate::workspace_acl::protect_workspace_codex_dir;
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
use std::collections::HashMap;
|
||||
use std::ffi::c_void;
|
||||
@@ -211,20 +214,34 @@ pub(crate) fn allow_null_device_for_workspace_write(is_workspace_write: bool) {
|
||||
}
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub(crate) fn apply_legacy_session_acl_rules(
|
||||
policy: &SandboxPolicy,
|
||||
sandbox_policy_cwd: &Path,
|
||||
codex_home: &Path,
|
||||
current_dir: &Path,
|
||||
env_map: &HashMap<String, String>,
|
||||
psid_generic: &LocalSid,
|
||||
psid_workspace: Option<&LocalSid>,
|
||||
cap_sid_str: &str,
|
||||
additional_deny_read_paths: &[PathBuf],
|
||||
additional_deny_write_paths: &[PathBuf],
|
||||
persist_aces: bool,
|
||||
) -> Vec<PathBuf> {
|
||||
let AllowDenyPaths { allow, deny } =
|
||||
) -> Result<Vec<PathBuf>> {
|
||||
let AllowDenyPaths { allow, mut deny } =
|
||||
compute_allow_paths(policy, sandbox_policy_cwd, current_dir, env_map);
|
||||
let mut guards: Vec<PathBuf> = Vec::new();
|
||||
let canonical_cwd = canonicalize_path(current_dir);
|
||||
unsafe {
|
||||
for path in additional_deny_write_paths {
|
||||
// Explicit carveouts must exist before the command starts so the
|
||||
// sandbox cannot create them under a writable parent first.
|
||||
if !path.exists() {
|
||||
std::fs::create_dir_all(path)
|
||||
.with_context(|| format!("create deny-write path {}", path.display()))?;
|
||||
}
|
||||
deny.insert(path.clone());
|
||||
}
|
||||
for p in &allow {
|
||||
let psid = if matches!(policy, SandboxPolicy::WorkspaceWrite { .. })
|
||||
&& is_command_cwd_root(p, &canonical_cwd)
|
||||
@@ -245,6 +262,19 @@ pub(crate) fn apply_legacy_session_acl_rules(
|
||||
guards.push(p.clone());
|
||||
}
|
||||
}
|
||||
let applied_deny_read_paths = if persist_aces {
|
||||
sync_persistent_deny_read_acls(
|
||||
codex_home,
|
||||
cap_sid_str,
|
||||
additional_deny_read_paths,
|
||||
psid_generic.as_ptr(),
|
||||
)?
|
||||
} else {
|
||||
apply_deny_read_acls(additional_deny_read_paths, psid_generic.as_ptr())?
|
||||
};
|
||||
if !persist_aces {
|
||||
guards.extend(applied_deny_read_paths);
|
||||
}
|
||||
allow_null_device(psid_generic.as_ptr());
|
||||
if let Some(psid_workspace) = psid_workspace {
|
||||
allow_null_device(psid_workspace.as_ptr());
|
||||
@@ -254,9 +284,10 @@ pub(crate) fn apply_legacy_session_acl_rules(
|
||||
}
|
||||
}
|
||||
}
|
||||
guards
|
||||
Ok(guards)
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub(crate) fn prepare_elevated_spawn_context(
|
||||
policy_json_or_preset: &str,
|
||||
sandbox_policy_cwd: &Path,
|
||||
@@ -264,6 +295,11 @@ pub(crate) fn prepare_elevated_spawn_context(
|
||||
cwd: &Path,
|
||||
env_map: &mut HashMap<String, String>,
|
||||
command: &[String],
|
||||
read_roots_override: Option<&[PathBuf]>,
|
||||
read_roots_include_platform_defaults: bool,
|
||||
write_roots_override: Option<&[PathBuf]>,
|
||||
deny_read_paths_override: &[PathBuf],
|
||||
deny_write_paths_override: &[PathBuf],
|
||||
) -> Result<ElevatedSpawnContext> {
|
||||
let common = prepare_spawn_context_common(
|
||||
policy_json_or_preset,
|
||||
@@ -283,7 +319,7 @@ pub(crate) fn prepare_elevated_spawn_context(
|
||||
);
|
||||
let write_roots: Vec<PathBuf> = allow.into_iter().collect();
|
||||
let deny_write_paths: Vec<PathBuf> = deny.into_iter().collect();
|
||||
let write_roots_override = if common.is_workspace_write {
|
||||
let computed_write_roots_override = if common.is_workspace_write {
|
||||
Some(write_roots.as_slice())
|
||||
} else {
|
||||
None
|
||||
@@ -294,10 +330,15 @@ pub(crate) fn prepare_elevated_spawn_context(
|
||||
cwd,
|
||||
env_map,
|
||||
codex_home,
|
||||
/*read_roots_override*/ None,
|
||||
/*read_roots_include_platform_defaults*/ false,
|
||||
write_roots_override,
|
||||
&deny_write_paths,
|
||||
read_roots_override,
|
||||
read_roots_include_platform_defaults,
|
||||
write_roots_override.or(computed_write_roots_override),
|
||||
deny_read_paths_override,
|
||||
if deny_write_paths_override.is_empty() {
|
||||
&deny_write_paths
|
||||
} else {
|
||||
deny_write_paths_override
|
||||
},
|
||||
/*proxy_enforced*/ false,
|
||||
)?;
|
||||
let caps = load_or_create_cap_sids(codex_home)?;
|
||||
|
||||
Reference in new issue
Block a user