github/codex
4
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-19 16:17:49 +00:00
Code Issues Packages Projects Releases Wiki Activity

Require hostname authorization before proxy DNS lookups (#51650)

Browse Source 
## Why

Resolving unapproved hostnames discloses them to DNS even when the proxy ultimately rejects the request. Allowing local binding in a managed macOS sandbox also granted direct outbound access to port `53`.

## What changed

- Require an allowlist match or policy approval before resolving a hostname for address checks.
- Recheck baseline restrictions after local and remote policy approval so approval cannot bypass explicit denies or local/private address restrictions.
- Preserve DNS checks for inner HTTPS requests after an approved CONNECT, including hosts without a persisted allowlist entry.
- Remove the Seatbelt allowance for external DNS while retaining local binding and loopback proxy access.

## Testing

Add regression tests for zero DNS lookups on unapproved hosts, private-address rejection after approval, local and remote approval ordering, and DNS checks after approved CONNECT requests. Add a macOS Seatbelt child test that verifies external UDP and TCP port `53` access is denied while local binding and proxy connectivity remain available.

GitOrigin-RevId: 490790462c51928e831dd8375ac098bc3a257bac
This commit is contained in:
jif
2026-10-07 11:10:20 +00:00
committed by copyberry
parent a513012869
commit 37eaae6eeb
9 changed files with 357 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/network-proxy/src/host_policy_tests.rs
+129
View File
@@ -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<dyn NetworkPolicyDecider> = 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(())
}
codex-rs/network-proxy/src/mitm.rs
+7 -1
View File
@@ -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)
) {
codex-rs/network-proxy/src/mitm_tests.rs
+37
View File
@@ -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::<Vec<_>>()
),
(
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();
codex-rs/network-proxy/src/network_policy.rs
+23 -5
View File
@@ -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 {
codex-rs/network-proxy/src/proxy.rs
+19 -4
View File
@@ -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(),
codex-rs/network-proxy/src/runtime.rs
+50 -17
View File
@@ -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<HostBlockDecision> {
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<bool>,
authorization: HostAuthorization,
) -> Result<HostBlockDecision> {
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<F, Fut>(
&self,
host: &str,
port: u16,
allow_local_binding: Option<bool>,
authorization: HostAuthorization,
lookup: F,
) -> Result<HostBlockDecision>
where
F: FnOnce(String, u16) -> Fut,
Fut: Future<Output = std::io::Result<Vec<SocketAddr>>>,
{
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;
codex-rs/sandboxing/src/seatbelt.rs
+4 -4
View File
@@ -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;
codex-rs/sandboxing/src/seatbelt_network_tests.rs
+88
View File
@@ -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");
}
codex-rs/sandboxing/src/seatbelt_tests.rs
-4
View File
@@ -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}"