diff --git a/codex-rs/network-proxy/src/host_policy_tests.rs b/codex-rs/network-proxy/src/host_policy_tests.rs new file mode 100644 index 0000000000..7addfffce8 --- /dev/null +++ b/codex-rs/network-proxy/src/host_policy_tests.rs @@ -0,0 +1,129 @@ +//! Verify DNS authorization ordering and mandatory checks after approval. + +use super::*; +use crate::config::NetworkProxyConfig; +use crate::runtime::network_proxy_state_for_policy; +use pretty_assertions::assert_eq; +use std::cell::Cell; + +#[tokio::test] +async fn unapproved_hosts_do_not_trigger_dns() { + for allowed in [vec![], vec!["allowed.example".to_string()]] { + let mut config = NetworkProxyConfig::default(); + config.set_allowed_domains(allowed); + let state = network_proxy_state_for_policy(config); + let lookups = Cell::new(0); + let decision = state + .host_blocked_with_lookup( + "payload.unapproved.example", + /*port*/ 443, + /*allow_local_binding*/ None, + HostAuthorization::RequireAllowlist, + |_host, _port| { + lookups.set(lookups.get() + 1); + async { Ok(vec!["93.184.215.14:443".parse().unwrap()]) } + }, + ) + .await + .unwrap(); + assert_eq!( + (decision, lookups.get()), + (HostBlockDecision::Blocked(HostBlockReason::NotAllowed), 0), + "rejecting an unapproved hostname must not disclose it to DNS", + ); + } + // Approval permits DNS, but must still reject private resolution. + let state = network_proxy_state_for_policy(NetworkProxyConfig::default()); + let decision = state + .host_blocked_with_lookup( + "approved.example", + /*port*/ 443, + /*allow_local_binding*/ None, + HostAuthorization::Approved, + |_host, _port| async { Ok(vec!["10.0.0.1:443".parse().unwrap()]) }, + ) + .await + .unwrap(); + assert_eq!( + decision, + HostBlockDecision::Blocked(HostBlockReason::NotAllowedLocal) + ); +} + +#[tokio::test] +async fn local_and_remote_approvals_recheck_dns_after_the_decider() -> Result<()> { + use crate::NetworkDecision; + use crate::NetworkDecisionSource; + use crate::NetworkPolicyDecider; + use crate::NetworkPolicyRequest; + use crate::NetworkPolicyRequestArgs; + use crate::NetworkProtocol; + use crate::NetworkProxy; + use crate::network_policy::evaluate_host_policy; + use std::sync::Arc; + use std::sync::atomic::AtomicUsize; + use std::sync::atomic::Ordering; + + for remote in [false, true] { + let calls = Arc::new(AtomicUsize::new(0)); + let decider_calls = Arc::clone(&calls); + let decider: Arc = Arc::new(move |_request| { + decider_calls.fetch_add(1, Ordering::SeqCst); + async { NetworkDecision::Allow } + }); + // Explicit controller false must survive an executor allowing local binding. + let config = NetworkProxyConfig { + allow_local_binding: Some(false), + ..NetworkProxyConfig::default() + }; + let state = Arc::new(network_proxy_state_for_policy(config)); + let proxy = NetworkProxy::builder() + .state(Arc::clone(&state)) + .managed_by_codex(/*managed_by_codex*/ false) + .policy_decider_arc(Arc::clone(&decider)) + .build() + .await?; + let scoped = proxy.for_execution( + "remote", + "execution", + "token".to_string(), + /*environment_policy*/ None, + /*fallback_policy_decider*/ None, + )?; + let remote_decider = scoped + .remote_policy_decider(/*allow_local_binding*/ true) + .unwrap(); + for host in ["does-not-resolve.invalid", "8.8.8.8"] { + let request = NetworkPolicyRequest::new(NetworkPolicyRequestArgs { + protocol: NetworkProtocol::HttpsConnect, + host: host.to_string(), + port: 443, + environment_id: None, + client_addr: None, + method: None, + command: None, + exec_policy_hint: None, + }); + let decision = if remote { + remote_decider.decide(request).await + } else { + evaluate_host_policy(&state, Some(&decider), &request).await? + }; + let expected = if host.ends_with(".invalid") { + NetworkDecision::deny_with_source( + HostBlockReason::NotAllowedLocal.as_str(), + NetworkDecisionSource::BaselinePolicy, + ) + } else { + NetworkDecision::Allow + }; + assert_eq!(decision, expected); + } + assert_eq!( + calls.load(Ordering::SeqCst), + 2, + "domain review precedes DNS" + ); + } + Ok(()) +} diff --git a/codex-rs/network-proxy/src/mitm.rs b/codex-rs/network-proxy/src/mitm.rs index e17e6d462d..60bcc85e34 100644 --- a/codex-rs/network-proxy/src/mitm.rs +++ b/codex-rs/network-proxy/src/mitm.rs @@ -8,6 +8,7 @@ use crate::reasons::REASON_METHOD_NOT_ALLOWED; use crate::reasons::REASON_MITM_HOOK_DENIED; use crate::responses::blocked_text_response; use crate::responses::text_response; +use crate::runtime::HostAuthorization; use crate::runtime::HostBlockDecision; use crate::runtime::HostBlockReason; use crate::state::BlockedRequest; @@ -390,7 +391,12 @@ async fn evaluate_mitm_policy( if matches!( policy .app_state - .host_blocked(&policy.target_host, policy.target_port) + .host_blocked_with_local_binding( + &policy.target_host, + policy.target_port, + /*allow_local_binding*/ None, + HostAuthorization::Approved, + ) .await?, HostBlockDecision::Blocked(HostBlockReason::NotAllowedLocal) ) { diff --git a/codex-rs/network-proxy/src/mitm_tests.rs b/codex-rs/network-proxy/src/mitm_tests.rs index ccbf006bd8..84ceb16b2c 100644 --- a/codex-rs/network-proxy/src/mitm_tests.rs +++ b/codex-rs/network-proxy/src/mitm_tests.rs @@ -154,6 +154,43 @@ async fn mitm_policy_rechecks_local_private_target_after_connect() { assert_eq!(blocked[0].port, Some(443)); } +#[tokio::test] +async fn mitm_policy_rechecks_dns_for_approved_unallowlisted_host() { + let app_state = Arc::new(network_proxy_state_for_policy(NetworkProxyConfig::default())); + let host = "does-not-resolve.invalid"; + // The context represents an approved CONNECT, without a persisted allowlist entry. + let ctx = policy_ctx( + Arc::clone(&app_state), + NetworkMode::Full, + host, + /*target_port*/ 443, + ); + let req = Request::builder() + .method(Method::GET) + .uri("/") + .header(HOST, host) + .body(Body::empty()) + .unwrap(); + let response = mitm_blocking_response(&req, &ctx) + .await + .unwrap() + .expect("approved CONNECT must still validate DNS on inner requests"); + let blocked = app_state.drain_blocked().await.unwrap(); + assert_eq!( + ( + response.status(), + blocked + .iter() + .map(|request| (request.host.as_str(), request.reason.as_str(), request.port)) + .collect::>() + ), + ( + StatusCode::FORBIDDEN, + vec![(host, REASON_NOT_ALLOWED_LOCAL, Some(443))] + ), + ); +} + #[tokio::test] async fn mitm_policy_allows_matching_hooked_write_in_full_mode() { let secret_file = NamedTempFile::new().unwrap(); diff --git a/codex-rs/network-proxy/src/network_policy.rs b/codex-rs/network-proxy/src/network_policy.rs index 1af3d48075..9af47415ac 100644 --- a/codex-rs/network-proxy/src/network_policy.rs +++ b/codex-rs/network-proxy/src/network_policy.rs @@ -1,6 +1,7 @@ use crate::reasons::REASON_POLICY_DENIED; use crate::request_cancellation::NetworkRequestCancellation; use crate::request_disconnect::NetworkRequestDisconnect; +use crate::runtime::HostAuthorization; use crate::runtime::HostBlockDecision; use crate::runtime::HostBlockReason; use crate::state::NetworkProxyState; @@ -371,14 +372,31 @@ pub(crate) async fn evaluate_host_policy( HostBlockDecision::Allowed => (NetworkDecision::Allow, false), HostBlockDecision::Blocked(HostBlockReason::NotAllowed) => { if let Some(decider) = decider { - let mut request = request.clone(); - if request.environment_id.is_none() + let mut attributed_request = request.clone(); + if attributed_request.environment_id.is_none() && let Some(environment_id) = state.environment_id() { - request.environment_id = Some(environment_id.to_string()); + attributed_request.environment_id = Some(environment_id.to_string()); + } + attributed_request.execution_id = execution_id.clone(); + let mut decider_decision = + map_decider_decision(decider.decide(attributed_request).await); + // Approval permits DNS, but cannot bypass private-address or explicit deny policy. + if matches!(decider_decision, NetworkDecision::Allow) + && let HostBlockDecision::Blocked(reason) = state + .host_blocked_with_local_binding( + &request.host, + request.port, + /*allow_local_binding*/ None, + HostAuthorization::Approved, + ) + .await? + { + decider_decision = NetworkDecision::deny_with_source( + reason.as_str(), + NetworkDecisionSource::BaselinePolicy, + ); } - request.execution_id = execution_id.clone(); - let decider_decision = map_decider_decision(decider.decide(request).await); let policy_override = matches!(decider_decision, NetworkDecision::Allow); (decider_decision, policy_override) } else { diff --git a/codex-rs/network-proxy/src/proxy.rs b/codex-rs/network-proxy/src/proxy.rs index 7ca8bb20df..a862bac6d9 100644 --- a/codex-rs/network-proxy/src/proxy.rs +++ b/codex-rs/network-proxy/src/proxy.rs @@ -14,6 +14,7 @@ use crate::network_policy::NetworkDecisionSource; use crate::network_policy::NetworkPolicyDecider; use crate::runtime::BlockedRequestObserver; use crate::runtime::ConfigState; +use crate::runtime::HostAuthorization; use crate::runtime::HostBlockDecision; use crate::runtime::HostBlockReason; use crate::runtime::unix_socket_permissions_supported; @@ -1040,17 +1041,31 @@ impl NetworkProxy { crate::NetworkDecision::deny(crate::reasons::REASON_NOT_ALLOWED) } decision = async { - match state.host_blocked_with_local_binding( + let host_decision = state.host_blocked_with_local_binding( &request.host, request.port, Some(allow_local_binding), - ).await { + HostAuthorization::RequireAllowlist, + ).await; + let host_decision = match host_decision { // Controller approval alone cannot bypass attachment policy. Ok(HostBlockDecision::Allowed) if !environment_policy_applies => { - NetworkDecision::Allow + Ok(HostBlockDecision::Allowed) } Ok(HostBlockDecision::Allowed) | Ok(HostBlockDecision::Blocked(HostBlockReason::NotAllowed)) => { - decider.decide(request).await + let decision = decider.decide(request.clone()).await; + if !matches!(decision, NetworkDecision::Allow) { + return decision; + } + state.host_blocked_with_local_binding( + &request.host, request.port, Some(allow_local_binding), + HostAuthorization::Approved, + ).await } + Ok(HostBlockDecision::Blocked(reason)) => Ok(HostBlockDecision::Blocked(reason)), + Err(err) => Err(err), + }; + match host_decision { + Ok(HostBlockDecision::Allowed) => NetworkDecision::Allow, Ok(HostBlockDecision::Blocked(reason)) => { NetworkDecision::deny_with_source( reason.as_str(), diff --git a/codex-rs/network-proxy/src/runtime.rs b/codex-rs/network-proxy/src/runtime.rs index 10440e00cf..c8d2f6c7a4 100644 --- a/codex-rs/network-proxy/src/runtime.rs +++ b/codex-rs/network-proxy/src/runtime.rs @@ -87,6 +87,12 @@ impl std::fmt::Display for HostBlockReason { } } +#[derive(Clone, Copy, PartialEq, Eq)] +pub(crate) enum HostAuthorization { + RequireAllowlist, + Approved, +} + #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum HostBlockDecision { Allowed, @@ -694,8 +700,13 @@ impl NetworkProxyState { } pub async fn host_blocked(&self, host: &str, port: u16) -> Result { - self.host_blocked_with_local_binding(host, port, /*allow_local_binding*/ None) - .await + self.host_blocked_with_local_binding( + host, + port, + /*allow_local_binding*/ None, + HostAuthorization::RequireAllowlist, + ) + .await } pub(crate) async fn host_blocked_with_local_binding( @@ -703,7 +714,34 @@ impl NetworkProxyState { host: &str, port: u16, allow_local_binding: Option, + authorization: HostAuthorization, ) -> Result { + self.host_blocked_with_lookup( + host, + port, + allow_local_binding, + authorization, + |host, port| async move { + lookup_host((host.as_str(), port)) + .await + .map(Iterator::collect) + }, + ) + .await + } + + async fn host_blocked_with_lookup( + &self, + host: &str, + port: u16, + allow_local_binding: Option, + authorization: HostAuthorization, + lookup: F, + ) -> Result + where + F: FnOnce(String, u16) -> Fut, + Fut: Future>>, + { self.reload_if_needed().await?; let host = match Host::parse(host) { Ok(host) => host, @@ -722,7 +760,6 @@ impl NetworkProxyState { allowed_domains, ) }; - let allowed_domains_empty = allowed_domains.is_none(); let allowed_domains = allowed_domains.unwrap_or_default(); let host_str = host.as_str(); @@ -730,12 +767,13 @@ impl NetworkProxyState { // Decision order matters: // 1) explicit deny always wins // 2) local/private networking is opt-in (defense-in-depth) - // 3) allowlist is enforced when configured + // 3) DNS requires an allowlist match or an approval if globset_matches_host_or_unscoped(&deny_set, host_str) { return Ok(HostBlockDecision::Blocked(HostBlockReason::Denied)); } - let is_allowlisted = globset_matches_host_or_unscoped(&allow_set, host_str); + let is_authorized = authorization == HostAuthorization::Approved + || globset_matches_host_or_unscoped(&allow_set, host_str); if !allow_local_binding { // If the intent is "prevent access to local/internal networks", we must not rely solely // on string checks like `localhost` / `127.0.0.1`. Attackers can use DNS rebinding or @@ -760,23 +798,14 @@ impl NetworkProxyState { if !is_explicit_local_allowlisted(&allowed_domains, &host) { return Ok(HostBlockDecision::Blocked(HostBlockReason::NotAllowedLocal)); } - } else if host_resolves_to_non_public_ip( - host_str, - port, - DNS_LOOKUP_TIMEOUT, - |host, port| async move { - lookup_host((host.as_str(), port)) - .await - .map(Iterator::collect) - }, - ) - .await + } else if is_authorized + && host_resolves_to_non_public_ip(host_str, port, DNS_LOOKUP_TIMEOUT, lookup).await { return Ok(HostBlockDecision::Blocked(HostBlockReason::NotAllowedLocal)); } } - if allowed_domains_empty || !is_allowlisted { + if !is_authorized { Ok(HostBlockDecision::Blocked(HostBlockReason::NotAllowed)) } else { Ok(HostBlockDecision::Allowed) @@ -2388,3 +2417,7 @@ mod tests { assert!(!state.is_unix_socket_allowed(&socket_path).await.unwrap()); } } + +#[cfg(test)] +#[path = "host_policy_tests.rs"] +mod host_policy_tests; diff --git a/codex-rs/sandboxing/src/seatbelt.rs b/codex-rs/sandboxing/src/seatbelt.rs index 665f24f86d..0f8701a167 100644 --- a/codex-rs/sandboxing/src/seatbelt.rs +++ b/codex-rs/sandboxing/src/seatbelt.rs @@ -337,10 +337,6 @@ fn dynamic_network_policy_for_network( policy.push_str("(allow network-inbound (local ip \"localhost:*\"))\n"); policy.push_str("(allow network-outbound (remote ip \"localhost:*\"))\n"); } - if proxy.allow_local_binding && !proxy.ports.is_empty() { - policy.push_str("; allow DNS lookups while application traffic remains proxy-routed\n"); - policy.push_str("(allow network-outbound (remote ip \"*:53\"))\n"); - } for port in &proxy.ports { policy.push_str(&format!( "(allow network-outbound (remote ip \"localhost:{port}\"))\n" @@ -1123,3 +1119,7 @@ mod fcntl_tests; #[cfg(test)] #[path = "seatbelt_tls_tests.rs"] mod tls_tests; + +#[cfg(test)] +#[path = "seatbelt_network_tests.rs"] +mod network_tests; diff --git a/codex-rs/sandboxing/src/seatbelt_network_tests.rs b/codex-rs/sandboxing/src/seatbelt_network_tests.rs new file mode 100644 index 0000000000..6c005cc4ec --- /dev/null +++ b/codex-rs/sandboxing/src/seatbelt_network_tests.rs @@ -0,0 +1,88 @@ +//! Exercise managed network restrictions in an actual Seatbelt child. + +#![cfg(target_os = "macos")] + +use super::CreateSeatbeltCommandArgsParams; +use super::MACOS_PATH_TO_SEATBELT_EXECUTABLE; +use super::create_seatbelt_command_args; +use codex_network_proxy::ManagedNetworkSandboxContext; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::NetworkSandboxPolicy; +use std::net::TcpListener; +use std::net::TcpStream; +use std::net::UdpSocket; +use std::process::Command; +use std::time::Duration; + +#[test] +fn managed_local_binding_does_not_allow_external_dns() { + let workspace = tempfile::tempdir().expect("temporary workspace"); + let proxy = TcpListener::bind("127.0.0.1:0").expect("proxy listener"); + let proxy_address = proxy.local_addr().expect("proxy address"); + let managed_network = ManagedNetworkSandboxContext { + loopback_ports: vec![proxy_address.port()], + allow_local_binding: true, + ..Default::default() + }; + let module = module_path!().split_once("::").expect("test module path").1; + let args = create_seatbelt_command_args(CreateSeatbeltCommandArgsParams { + command: vec![ + std::env::current_exe() + .expect("test executable") + .to_string_lossy() + .into_owned(), + "--exact".into(), + format!("{module}::external_dns_child"), + "--ignored".into(), + "--nocapture".into(), + ], + file_system_sandbox_policy: &FileSystemSandboxPolicy::read_only(), + network_sandbox_policy: NetworkSandboxPolicy::Enabled, + sandbox_policy_cwd: workspace.path(), + enforce_managed_network: true, + managed_network: Some(&managed_network), + environment_id: None, + network: None, + extra_allow_unix_sockets: &[], + }) + .expect("generated Seatbelt arguments"); + let output = Command::new(MACOS_PATH_TO_SEATBELT_EXECUTABLE) + .args(args) + .env("CODEX_TEST_DNS_PROXY_ADDRESS", proxy_address.to_string()) + .output() + .expect("run managed DNS probe"); + let stderr = String::from_utf8_lossy(&output.stderr); + if !output.status.success() + && stderr.contains("sandbox-exec: sandbox_apply: Operation not permitted") + { + eprintln!("skipping DNS regression: nested Seatbelt unavailable"); + return; + } + assert!(output.status.success(), "{output:?}"); + assert!(String::from_utf8_lossy(&output.stdout).contains("DNS probes completed")); +} + +#[test] +#[ignore = "executed by the parent test inside Seatbelt"] +fn external_dns_child() { + let Ok(proxy_address) = std::env::var("CODEX_TEST_DNS_PROXY_ADDRESS") else { + return; + }; + TcpStream::connect(proxy_address).expect("managed proxy remains reachable"); + let socket = UdpSocket::bind("0.0.0.0:0").expect("local binding remains allowed"); + // TEST-NET needs no live DNS service. UDP connect checks the destination + // permission without sending a DNS query or depending on a server reply. + let udp = socket.connect("192.0.2.1:53"); + let tcp = + TcpStream::connect_timeout(&"192.0.2.1:53".parse().unwrap(), Duration::from_millis(100)); + for error in [udp.err(), tcp.err()] { + assert!( + error.as_ref().is_some_and(|error| matches!( + error.raw_os_error(), + Some(libc::EPERM | libc::EACCES) + )), + "external DNS must be denied by Seatbelt, got {error:?}", + ); + } + println!("DNS probes completed"); +} diff --git a/codex-rs/sandboxing/src/seatbelt_tests.rs b/codex-rs/sandboxing/src/seatbelt_tests.rs index 88ae9b97c3..9f80838771 100644 --- a/codex-rs/sandboxing/src/seatbelt_tests.rs +++ b/codex-rs/sandboxing/src/seatbelt_tests.rs @@ -1452,10 +1452,6 @@ fn create_seatbelt_args_allows_local_binding_when_explicitly_enabled() { policy.contains("(allow network-outbound (remote ip \"localhost:*\"))"), "policy should allow loopback outbound when explicitly enabled:\n{policy}" ); - assert!( - policy.contains("(allow network-outbound (remote ip \"*:53\"))"), - "policy should allow DNS egress when local binding is explicitly enabled:\n{policy}" - ); assert!( !policy.contains("\n(allow network-outbound)\n"), "policy should keep proxy-routed behavior without blanket outbound allowance:\n{policy}"