mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Include legacy deny paths in elevated Windows sandbox setup (#17365)
## Summary This updates the Windows elevated sandbox setup/refresh path to include the legacy `compute_allow_paths(...).deny` protected children in the same deny-write payload pipe added for split filesystem carveouts. Concretely, elevated setup and elevated refresh now both build deny-write payload paths from: - explicit split-policy deny-write paths, preserving missing paths so setup can materialize them before applying ACLs - legacy `compute_allow_paths(...).deny`, which includes existing `.git`, `.codex`, and `.agents` children under writable roots This lets the elevated backend protect `.git` consistently with the unelevated/restricted-token path, and removes the old janky hard-coded `.codex` / `.agents` elevated setup helpers in favor of the shared payload path. ## Root Cause The landed split-carveout PR threaded a `deny_write_paths` pipe through elevated setup/refresh, but the legacy workspace-write deny set from `compute_allow_paths(...).deny` was not included in that payload. As a result, elevated workspace-write did not apply the intended deny-write ACLs for existing protected children like `<cwd>/.git`. ## Notes The legacy protected children still only enter the deny set if they already exist, because `compute_allow_paths` filters `.git`, `.codex`, and `.agents` with `exists()`. Missing explicit split-policy deny paths are preserved separately because setup intentionally materializes those before applying ACLs. ## Validation - `cargo fmt --check -p codex-windows-sandbox` - `cargo test -p codex-windows-sandbox` - `cargo build -p codex-cli -p codex-windows-sandbox --bins` - Elevated `codex exec` smoke with `windows.sandbox='elevated'`: fresh git repo, attempted append to `.git/config`, observed `Access is denied`, marker not written, Deny ACE present on `.git` - Unelevated `codex exec` smoke with `windows.sandbox='unelevated'`: fresh git repo, attempted append to `.git/config`, observed `Access is denied`, marker not written, Deny ACE present on `.git`
This commit is contained in:
@@ -191,10 +191,6 @@ pub use winutil::string_from_sid_bytes;
|
|||||||
pub use winutil::to_wide;
|
pub use winutil::to_wide;
|
||||||
#[cfg(target_os = "windows")]
|
#[cfg(target_os = "windows")]
|
||||||
pub use workspace_acl::is_command_cwd_root;
|
pub use workspace_acl::is_command_cwd_root;
|
||||||
#[cfg(target_os = "windows")]
|
|
||||||
pub use workspace_acl::protect_workspace_agents_dir;
|
|
||||||
#[cfg(target_os = "windows")]
|
|
||||||
pub use workspace_acl::protect_workspace_codex_dir;
|
|
||||||
|
|
||||||
#[cfg(not(target_os = "windows"))]
|
#[cfg(not(target_os = "windows"))]
|
||||||
pub use stub::CaptureResult;
|
pub use stub::CaptureResult;
|
||||||
@@ -228,8 +224,6 @@ mod windows_impl {
|
|||||||
use super::token::convert_string_sid_to_sid;
|
use super::token::convert_string_sid_to_sid;
|
||||||
use super::token::create_workspace_write_token_with_caps_from;
|
use super::token::create_workspace_write_token_with_caps_from;
|
||||||
use super::workspace_acl::is_command_cwd_root;
|
use super::workspace_acl::is_command_cwd_root;
|
||||||
use super::workspace_acl::protect_workspace_agents_dir;
|
|
||||||
use super::workspace_acl::protect_workspace_codex_dir;
|
|
||||||
use anyhow::Result;
|
use anyhow::Result;
|
||||||
use std::collections::HashMap;
|
use std::collections::HashMap;
|
||||||
use std::ffi::c_void;
|
use std::ffi::c_void;
|
||||||
@@ -441,8 +435,6 @@ mod windows_impl {
|
|||||||
allow_null_device(psid_generic);
|
allow_null_device(psid_generic);
|
||||||
if let Some(psid) = psid_workspace {
|
if let Some(psid) = psid_workspace {
|
||||||
allow_null_device(psid);
|
allow_null_device(psid);
|
||||||
let _ = protect_workspace_codex_dir(¤t_dir, psid);
|
|
||||||
let _ = protect_workspace_agents_dir(¤t_dir, psid);
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -625,8 +617,6 @@ mod windows_impl {
|
|||||||
}
|
}
|
||||||
allow_null_device(psid_generic);
|
allow_null_device(psid_generic);
|
||||||
allow_null_device(psid_workspace);
|
allow_null_device(psid_workspace);
|
||||||
let _ = protect_workspace_codex_dir(¤t_dir, psid_workspace);
|
|
||||||
let _ = protect_workspace_agents_dir(¤t_dir, psid_workspace);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
Ok(())
|
Ok(())
|
||||||
|
|||||||
@@ -22,8 +22,6 @@ use codex_windows_sandbox::is_command_cwd_root;
|
|||||||
use codex_windows_sandbox::load_or_create_cap_sids;
|
use codex_windows_sandbox::load_or_create_cap_sids;
|
||||||
use codex_windows_sandbox::log_note;
|
use codex_windows_sandbox::log_note;
|
||||||
use codex_windows_sandbox::path_mask_allows;
|
use codex_windows_sandbox::path_mask_allows;
|
||||||
use codex_windows_sandbox::protect_workspace_agents_dir;
|
|
||||||
use codex_windows_sandbox::protect_workspace_codex_dir;
|
|
||||||
use codex_windows_sandbox::sandbox_bin_dir;
|
use codex_windows_sandbox::sandbox_bin_dir;
|
||||||
use codex_windows_sandbox::sandbox_dir;
|
use codex_windows_sandbox::sandbox_dir;
|
||||||
use codex_windows_sandbox::sandbox_secrets_dir;
|
use codex_windows_sandbox::sandbox_secrets_dir;
|
||||||
@@ -767,17 +765,15 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<(
|
|||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
// These are explicit read-only-under-a-writable-root carveouts from the transformed
|
// These are deny-write carveouts, not deny-read paths. They may come from explicit
|
||||||
// sandbox policy; they are not deny-read paths.
|
// read-only-under-a-writable-root carveouts in the transformed sandbox policy, or from
|
||||||
|
// legacy protected children such as `.git`, `.codex`, and `.agents`.
|
||||||
//
|
//
|
||||||
// They are also not optional workspace sentinels such as `.codex` or `.agents`: those
|
// Deny ACEs attach to filesystem objects; if an explicit policy carveout does not exist
|
||||||
// are protected best-effort below and still skip missing directories so we do not leave
|
// during setup, the sandbox could otherwise create it later under a writable parent and
|
||||||
// empty protection artifacts behind in a workspace.
|
// bypass the carveout. Materialize missing carveouts as directories so the deny-write ACL
|
||||||
//
|
// is present before the command starts. Legacy protected children are filtered before
|
||||||
// Deny ACEs attach to filesystem objects; if a policy carveout does not exist during
|
// payload creation, so this should not create sentinel directories in a workspace.
|
||||||
// setup, the sandbox could otherwise create it later under a writable parent and
|
|
||||||
// bypass the carveout. Materialize missing carveouts as directories so the deny-write
|
|
||||||
// ACL is present before the command starts.
|
|
||||||
if !path.exists() {
|
if !path.exists() {
|
||||||
std::fs::create_dir_all(path)
|
std::fs::create_dir_all(path)
|
||||||
.with_context(|| format!("failed to create deny-write path {}", path.display()))?;
|
.with_context(|| format!("failed to create deny-write path {}", path.display()))?;
|
||||||
@@ -880,54 +876,6 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Protect the current workspace's `.codex` and `.agents` directories from tampering
|
|
||||||
// (write/delete) by using a workspace-specific capability SID. If a directory doesn't exist
|
|
||||||
// yet, skip it (it will be picked up on the next refresh).
|
|
||||||
match unsafe { protect_workspace_codex_dir(&payload.command_cwd, workspace_psid) } {
|
|
||||||
Ok(true) => {
|
|
||||||
let cwd_codex = payload.command_cwd.join(".codex");
|
|
||||||
log_line(
|
|
||||||
log,
|
|
||||||
&format!(
|
|
||||||
"applied deny ACE to protect workspace .codex {}",
|
|
||||||
cwd_codex.display()
|
|
||||||
),
|
|
||||||
)?;
|
|
||||||
}
|
|
||||||
Ok(false) => {}
|
|
||||||
Err(err) => {
|
|
||||||
let cwd_codex = payload.command_cwd.join(".codex");
|
|
||||||
refresh_errors.push(format!("deny ACE failed on {}: {err}", cwd_codex.display()));
|
|
||||||
log_line(
|
|
||||||
log,
|
|
||||||
&format!("deny ACE failed on {}: {err}", cwd_codex.display()),
|
|
||||||
)?;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
match unsafe { protect_workspace_agents_dir(&payload.command_cwd, workspace_psid) } {
|
|
||||||
Ok(true) => {
|
|
||||||
let cwd_agents = payload.command_cwd.join(".agents");
|
|
||||||
log_line(
|
|
||||||
log,
|
|
||||||
&format!(
|
|
||||||
"applied deny ACE to protect workspace .agents {}",
|
|
||||||
cwd_agents.display()
|
|
||||||
),
|
|
||||||
)?;
|
|
||||||
}
|
|
||||||
Ok(false) => {}
|
|
||||||
Err(err) => {
|
|
||||||
let cwd_agents = payload.command_cwd.join(".agents");
|
|
||||||
refresh_errors.push(format!(
|
|
||||||
"deny ACE failed on {}: {err}",
|
|
||||||
cwd_agents.display()
|
|
||||||
));
|
|
||||||
log_line(
|
|
||||||
log,
|
|
||||||
&format!("deny ACE failed on {}: {err}", cwd_agents.display()),
|
|
||||||
)?;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
unsafe {
|
unsafe {
|
||||||
if !sandbox_group_psid.is_null() {
|
if !sandbox_group_psid.is_null() {
|
||||||
LocalFree(sandbox_group_psid as HLOCAL);
|
LocalFree(sandbox_group_psid as HLOCAL);
|
||||||
|
|||||||
@@ -163,6 +163,7 @@ fn run_setup_refresh_inner(
|
|||||||
return Ok(());
|
return Ok(());
|
||||||
}
|
}
|
||||||
let (read_roots, write_roots) = build_payload_roots(&request, &overrides);
|
let (read_roots, write_roots) = build_payload_roots(&request, &overrides);
|
||||||
|
let deny_write_paths = build_payload_deny_write_paths(&request, overrides.deny_write_paths);
|
||||||
let network_identity =
|
let network_identity =
|
||||||
SandboxNetworkIdentity::from_policy(request.policy, request.proxy_enforced);
|
SandboxNetworkIdentity::from_policy(request.policy, request.proxy_enforced);
|
||||||
let offline_proxy_settings = offline_proxy_settings_from_env(request.env_map, network_identity);
|
let offline_proxy_settings = offline_proxy_settings_from_env(request.env_map, network_identity);
|
||||||
@@ -174,7 +175,7 @@ fn run_setup_refresh_inner(
|
|||||||
command_cwd: request.command_cwd.to_path_buf(),
|
command_cwd: request.command_cwd.to_path_buf(),
|
||||||
read_roots,
|
read_roots,
|
||||||
write_roots,
|
write_roots,
|
||||||
deny_write_paths: overrides.deny_write_paths.unwrap_or_default(),
|
deny_write_paths,
|
||||||
proxy_ports: offline_proxy_settings.proxy_ports,
|
proxy_ports: offline_proxy_settings.proxy_ports,
|
||||||
allow_local_binding: offline_proxy_settings.allow_local_binding,
|
allow_local_binding: offline_proxy_settings.allow_local_binding,
|
||||||
real_user: std::env::var("USERNAME").unwrap_or_else(|_| "Administrators".to_string()),
|
real_user: std::env::var("USERNAME").unwrap_or_else(|_| "Administrators".to_string()),
|
||||||
@@ -735,6 +736,7 @@ pub fn run_elevated_setup(
|
|||||||
)
|
)
|
||||||
})?;
|
})?;
|
||||||
let (read_roots, write_roots) = build_payload_roots(&request, &overrides);
|
let (read_roots, write_roots) = build_payload_roots(&request, &overrides);
|
||||||
|
let deny_write_paths = build_payload_deny_write_paths(&request, overrides.deny_write_paths);
|
||||||
let network_identity =
|
let network_identity =
|
||||||
SandboxNetworkIdentity::from_policy(request.policy, request.proxy_enforced);
|
SandboxNetworkIdentity::from_policy(request.policy, request.proxy_enforced);
|
||||||
let offline_proxy_settings = offline_proxy_settings_from_env(request.env_map, network_identity);
|
let offline_proxy_settings = offline_proxy_settings_from_env(request.env_map, network_identity);
|
||||||
@@ -746,7 +748,7 @@ pub fn run_elevated_setup(
|
|||||||
command_cwd: request.command_cwd.to_path_buf(),
|
command_cwd: request.command_cwd.to_path_buf(),
|
||||||
read_roots,
|
read_roots,
|
||||||
write_roots,
|
write_roots,
|
||||||
deny_write_paths: overrides.deny_write_paths.unwrap_or_default(),
|
deny_write_paths,
|
||||||
proxy_ports: offline_proxy_settings.proxy_ports,
|
proxy_ports: offline_proxy_settings.proxy_ports,
|
||||||
allow_local_binding: offline_proxy_settings.allow_local_binding,
|
allow_local_binding: offline_proxy_settings.allow_local_binding,
|
||||||
real_user: std::env::var("USERNAME").unwrap_or_else(|_| "Administrators".to_string()),
|
real_user: std::env::var("USERNAME").unwrap_or_else(|_| "Administrators".to_string()),
|
||||||
@@ -797,6 +799,31 @@ fn build_payload_roots(
|
|||||||
(read_roots, write_roots)
|
(read_roots, write_roots)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn build_payload_deny_write_paths(
|
||||||
|
request: &SandboxSetupRequest<'_>,
|
||||||
|
explicit_deny_write_paths: Option<Vec<PathBuf>>,
|
||||||
|
) -> Vec<PathBuf> {
|
||||||
|
let allow_deny_paths: AllowDenyPaths = compute_allow_paths(
|
||||||
|
request.policy,
|
||||||
|
request.policy_cwd,
|
||||||
|
request.command_cwd,
|
||||||
|
request.env_map,
|
||||||
|
);
|
||||||
|
let mut deny_write_paths: Vec<PathBuf> = explicit_deny_write_paths
|
||||||
|
.unwrap_or_default()
|
||||||
|
.into_iter()
|
||||||
|
.map(|path| {
|
||||||
|
if path.exists() {
|
||||||
|
dunce::canonicalize(&path).unwrap_or(path)
|
||||||
|
} else {
|
||||||
|
path
|
||||||
|
}
|
||||||
|
})
|
||||||
|
.collect();
|
||||||
|
deny_write_paths.extend(allow_deny_paths.deny);
|
||||||
|
deny_write_paths
|
||||||
|
}
|
||||||
|
|
||||||
fn filter_sensitive_write_roots(mut roots: Vec<PathBuf>, codex_home: &Path) -> Vec<PathBuf> {
|
fn filter_sensitive_write_roots(mut roots: Vec<PathBuf>, codex_home: &Path) -> Vec<PathBuf> {
|
||||||
// Never grant capability write access to CODEX_HOME or anything under CODEX_HOME/.sandbox,
|
// Never grant capability write access to CODEX_HOME or anything under CODEX_HOME/.sandbox,
|
||||||
// CODEX_HOME/.sandbox-bin, or CODEX_HOME/.sandbox-secrets. These locations contain sandbox
|
// CODEX_HOME/.sandbox-bin, or CODEX_HOME/.sandbox-secrets. These locations contain sandbox
|
||||||
@@ -1267,6 +1294,54 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn payload_deny_write_paths_merge_explicit_and_protected_children() {
|
||||||
|
let tmp = TempDir::new().expect("tempdir");
|
||||||
|
let codex_home = tmp.path().join("codex-home");
|
||||||
|
let command_cwd = tmp.path().join("workspace");
|
||||||
|
let extra_write_root = tmp.path().join("extra-write-root");
|
||||||
|
let command_git = command_cwd.join(".git");
|
||||||
|
let extra_codex = extra_write_root.join(".codex");
|
||||||
|
let explicit_deny = tmp.path().join("explicit-deny");
|
||||||
|
fs::create_dir_all(&command_git).expect("create command .git");
|
||||||
|
fs::create_dir_all(&extra_codex).expect("create extra .codex");
|
||||||
|
let policy = SandboxPolicy::WorkspaceWrite {
|
||||||
|
writable_roots: vec![
|
||||||
|
AbsolutePathBuf::from_absolute_path(&extra_write_root)
|
||||||
|
.expect("absolute writable root"),
|
||||||
|
],
|
||||||
|
read_only_access: ReadOnlyAccess::Restricted {
|
||||||
|
include_platform_defaults: false,
|
||||||
|
readable_roots: Vec::new(),
|
||||||
|
},
|
||||||
|
network_access: false,
|
||||||
|
exclude_tmpdir_env_var: true,
|
||||||
|
exclude_slash_tmp: true,
|
||||||
|
};
|
||||||
|
let request = super::SandboxSetupRequest {
|
||||||
|
policy: &policy,
|
||||||
|
policy_cwd: &command_cwd,
|
||||||
|
command_cwd: &command_cwd,
|
||||||
|
env_map: &HashMap::new(),
|
||||||
|
codex_home: &codex_home,
|
||||||
|
proxy_enforced: false,
|
||||||
|
};
|
||||||
|
|
||||||
|
let deny_write_paths =
|
||||||
|
super::build_payload_deny_write_paths(&request, Some(vec![explicit_deny.clone()]));
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
[
|
||||||
|
dunce::canonicalize(&command_git).expect("canonical command .git"),
|
||||||
|
dunce::canonicalize(&extra_codex).expect("canonical extra .codex"),
|
||||||
|
explicit_deny,
|
||||||
|
]
|
||||||
|
.into_iter()
|
||||||
|
.collect::<HashSet<PathBuf>>(),
|
||||||
|
deny_write_paths.into_iter().collect()
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn full_read_roots_preserve_legacy_platform_defaults() {
|
fn full_read_roots_preserve_legacy_platform_defaults() {
|
||||||
let tmp = TempDir::new().expect("tempdir");
|
let tmp = TempDir::new().expect("tempdir");
|
||||||
|
|||||||
@@ -1,30 +1,6 @@
|
|||||||
use crate::acl::add_deny_write_ace;
|
|
||||||
use crate::path_normalization::canonicalize_path;
|
use crate::path_normalization::canonicalize_path;
|
||||||
use anyhow::Result;
|
|
||||||
use std::ffi::c_void;
|
|
||||||
use std::path::Path;
|
use std::path::Path;
|
||||||
|
|
||||||
pub fn is_command_cwd_root(root: &Path, canonical_command_cwd: &Path) -> bool {
|
pub fn is_command_cwd_root(root: &Path, canonical_command_cwd: &Path) -> bool {
|
||||||
canonicalize_path(root) == canonical_command_cwd
|
canonicalize_path(root) == canonical_command_cwd
|
||||||
}
|
}
|
||||||
|
|
||||||
/// # Safety
|
|
||||||
/// Caller must ensure `psid` is a valid SID pointer.
|
|
||||||
pub unsafe fn protect_workspace_codex_dir(cwd: &Path, psid: *mut c_void) -> Result<bool> {
|
|
||||||
protect_workspace_subdir(cwd, psid, ".codex")
|
|
||||||
}
|
|
||||||
|
|
||||||
/// # Safety
|
|
||||||
/// Caller must ensure `psid` is a valid SID pointer.
|
|
||||||
pub unsafe fn protect_workspace_agents_dir(cwd: &Path, psid: *mut c_void) -> Result<bool> {
|
|
||||||
protect_workspace_subdir(cwd, psid, ".agents")
|
|
||||||
}
|
|
||||||
|
|
||||||
unsafe fn protect_workspace_subdir(cwd: &Path, psid: *mut c_void, subdir: &str) -> Result<bool> {
|
|
||||||
let path = cwd.join(subdir);
|
|
||||||
if path.is_dir() {
|
|
||||||
add_deny_write_ace(&path, psid)
|
|
||||||
} else {
|
|
||||||
Ok(false)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|||||||
Reference in New Issue
Block a user