mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
fix(network-proxy): normalize network proxy host matching (#19995)
## Why The proxy matches allow and deny rules against normalized host strings. Scoped IPv6 literals can arrive in equivalent forms, such as `fd00::1%eth0`, `[fd00::1%eth0]`, or `[fd00::1%25eth0]`. Policy should canonicalize those spellings without erasing scope granularity: an unscoped rule like `fd00::1` should still cover scoped requests for that address, while a scoped rule like `fd00::1%eth0` should remain exact to that scope. ## What changed - preserve IPv6 scope IDs during host normalization and canonicalize `%25scope` to `%scope` - match policy against the exact normalized host plus the unscoped IP base for scoped literals - keep local-address explicit allow checks aligned with the same scoped/unscoped semantics - add focused coverage for scoped IPv6 normalization, scoped allow rules, and scoped deny rules in `network-proxy` ## Security impact A request cannot bypass a broad deny rule by adding an IPv6 scope suffix. At the same time, scoped policy remains precise: `deny=fd00::1%eth0` affects that scoped spelling without collapsing `fd00::1%eth1` onto the same key, and `allow=fe80::1%eth0` does not implicitly allow other scopes. ## Verification - `just fmt` - `cargo test -p codex-network-proxy` - `just fix -p codex-network-proxy` - `git diff --check` --------- Co-authored-by: Codex <noreply@openai.com> Co-authored-by: evawong-oai <evawong@openai.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
3291463ff1
commit
2dbde94aa9
@@ -1034,7 +1034,10 @@ mod tests {
|
||||
#[tokio::test]
|
||||
async fn http_connect_accept_allows_allowlisted_host_in_full_mode() {
|
||||
let policy = {
|
||||
let mut policy = NetworkProxySettings::default();
|
||||
let mut policy = NetworkProxySettings {
|
||||
allow_local_binding: true,
|
||||
..NetworkProxySettings::default()
|
||||
};
|
||||
policy.set_allowed_domains(vec!["example.com".to_string()]);
|
||||
policy
|
||||
};
|
||||
|
||||
@@ -32,7 +32,7 @@ impl Host {
|
||||
/// Returns true if the host is a loopback hostname or IP literal.
|
||||
pub fn is_loopback_host(host: &Host) -> bool {
|
||||
let host = host.as_str();
|
||||
let host = host.split_once('%').map(|(ip, _)| ip).unwrap_or(host);
|
||||
let host = unscoped_ip_literal(host).unwrap_or(host);
|
||||
if host == "localhost" {
|
||||
return true;
|
||||
}
|
||||
@@ -103,24 +103,48 @@ pub fn normalize_host(host: &str) -> String {
|
||||
if host.starts_with('[')
|
||||
&& let Some(end) = host.find(']')
|
||||
{
|
||||
return normalize_dns_host(&host[1..end]);
|
||||
return normalize_dns_host_or_ip_literal(&host[1..end]);
|
||||
}
|
||||
|
||||
// The proxy stack should typically hand us a host without a port, but be
|
||||
// defensive and strip `:port` when there is exactly one `:`.
|
||||
if host.bytes().filter(|b| *b == b':').count() == 1 {
|
||||
let host = host.split(':').next().unwrap_or_default();
|
||||
return normalize_dns_host(host);
|
||||
return normalize_dns_host_or_ip_literal(host);
|
||||
}
|
||||
|
||||
// Avoid mangling unbracketed IPv6 literals, but strip trailing dots so fully qualified domain
|
||||
// names are treated the same as their dotless variants.
|
||||
normalize_dns_host(host)
|
||||
normalize_dns_host_or_ip_literal(host)
|
||||
}
|
||||
|
||||
fn normalize_dns_host(host: &str) -> String {
|
||||
fn normalize_dns_host_or_ip_literal(host: &str) -> String {
|
||||
let host = host.to_ascii_lowercase();
|
||||
host.trim_end_matches('.').to_string()
|
||||
let host = host.trim_end_matches('.');
|
||||
if let Some(ip) = normalize_ip_literal(host) {
|
||||
return ip;
|
||||
}
|
||||
host.to_string()
|
||||
}
|
||||
|
||||
pub(crate) fn unscoped_ip_literal(host: &str) -> Option<&str> {
|
||||
let (ip, _) = host.split_once('%')?;
|
||||
ip.parse::<IpAddr>().ok()?;
|
||||
Some(ip)
|
||||
}
|
||||
|
||||
fn normalize_ip_literal(host: &str) -> Option<String> {
|
||||
if host.parse::<IpAddr>().is_ok() {
|
||||
return Some(host.to_string());
|
||||
}
|
||||
for delimiter in ["%25", "%"] {
|
||||
if let Some((ip, scope)) = host.split_once(delimiter)
|
||||
&& ip.parse::<IpAddr>().is_ok()
|
||||
{
|
||||
return Some(format!("{ip}%{scope}"));
|
||||
}
|
||||
}
|
||||
None
|
||||
}
|
||||
|
||||
fn normalize_pattern(pattern: &str) -> String {
|
||||
@@ -392,6 +416,15 @@ mod tests {
|
||||
assert_eq!(true, set.is_match("::1"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn compile_globset_preserves_scoped_ipv6_literals() {
|
||||
let set = compile_denylist_globset(&["[fe80::1%25lo0]".to_string()]).unwrap();
|
||||
|
||||
assert_eq!(true, set.is_match("fe80::1%lo0"));
|
||||
assert_eq!(false, set.is_match("fe80::1%lo1"));
|
||||
assert_eq!(false, set.is_match("fe80::1"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn is_loopback_host_handles_localhost_variants() {
|
||||
assert!(is_loopback_host(&Host::parse("localhost").unwrap()));
|
||||
@@ -462,4 +495,11 @@ mod tests {
|
||||
assert_eq!(normalize_host("[::1]"), "::1");
|
||||
assert_eq!(normalize_host("[::1]:443"), "::1");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn normalize_host_preserves_ipv6_scope_ids() {
|
||||
assert_eq!(normalize_host("fe80::1%lo0"), "fe80::1%lo0");
|
||||
assert_eq!(normalize_host("[fe80::1%lo0]"), "fe80::1%lo0");
|
||||
assert_eq!(normalize_host("[fe80::1%25lo0]"), "fe80::1%lo0");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -790,9 +790,16 @@ mod tests {
|
||||
|
||||
#[tokio::test]
|
||||
async fn managed_proxy_builder_uses_loopback_ports() {
|
||||
let http_listener = StdTcpListener::bind(SocketAddr::from(([127, 0, 0, 1], 0))).unwrap();
|
||||
let http_addr = http_listener.local_addr().unwrap();
|
||||
let socks_listener = StdTcpListener::bind(SocketAddr::from(([127, 0, 0, 1], 0))).unwrap();
|
||||
let socks_addr = socks_listener.local_addr().unwrap();
|
||||
drop(http_listener);
|
||||
drop(socks_listener);
|
||||
|
||||
let state = Arc::new(network_proxy_state_for_policy(NetworkProxySettings {
|
||||
proxy_url: "http://127.0.0.1:43128".to_string(),
|
||||
socks_url: "http://127.0.0.1:48081".to_string(),
|
||||
proxy_url: format!("http://{http_addr}"),
|
||||
socks_url: format!("http://{socks_addr}"),
|
||||
..NetworkProxySettings::default()
|
||||
}));
|
||||
let proxy = match NetworkProxy::builder().state(state).build().await {
|
||||
@@ -812,14 +819,8 @@ mod tests {
|
||||
assert!(proxy.socks_addr.ip().is_loopback());
|
||||
#[cfg(target_os = "windows")]
|
||||
{
|
||||
assert_eq!(
|
||||
proxy.http_addr,
|
||||
"127.0.0.1:43128".parse::<SocketAddr>().unwrap()
|
||||
);
|
||||
assert_eq!(
|
||||
proxy.socks_addr,
|
||||
"127.0.0.1:48081".parse::<SocketAddr>().unwrap()
|
||||
);
|
||||
assert_eq!(proxy.http_addr, http_addr);
|
||||
assert_eq!(proxy.socks_addr, socks_addr);
|
||||
}
|
||||
#[cfg(not(target_os = "windows"))]
|
||||
{
|
||||
|
||||
@@ -7,6 +7,7 @@ use crate::policy::Host;
|
||||
use crate::policy::is_loopback_host;
|
||||
use crate::policy::is_non_public_ip;
|
||||
use crate::policy::normalize_host;
|
||||
use crate::policy::unscoped_ip_literal;
|
||||
use crate::reasons::REASON_DENIED;
|
||||
use crate::reasons::REASON_NOT_ALLOWED;
|
||||
use crate::reasons::REASON_NOT_ALLOWED_LOCAL;
|
||||
@@ -371,11 +372,11 @@ impl NetworkProxyState {
|
||||
// 1) explicit deny always wins
|
||||
// 2) local/private networking is opt-in (defense-in-depth)
|
||||
// 3) allowlist is enforced when configured
|
||||
if deny_set.is_match(host_str) {
|
||||
if globset_matches_host_or_unscoped(&deny_set, host_str) {
|
||||
return Ok(HostBlockDecision::Blocked(HostBlockReason::Denied));
|
||||
}
|
||||
|
||||
let is_allowlisted = allow_set.is_match(host_str);
|
||||
let is_allowlisted = 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
|
||||
@@ -386,10 +387,7 @@ impl NetworkProxyState {
|
||||
// allowlisted; hostnames that resolve to local/private IPs are blocked even if
|
||||
// allowlisted.
|
||||
let local_literal = {
|
||||
let host_no_scope = host_str
|
||||
.split_once('%')
|
||||
.map(|(ip, _)| ip)
|
||||
.unwrap_or(host_str);
|
||||
let host_no_scope = unscoped_ip_literal(host_str).unwrap_or(host_str);
|
||||
if is_loopback_host(&host) {
|
||||
true
|
||||
} else if let Ok(ip) = host_no_scope.parse::<IpAddr>() {
|
||||
@@ -797,8 +795,13 @@ fn log_domain_list_changes(list_name: &str, previous: &[String], next: &[String]
|
||||
}
|
||||
}
|
||||
|
||||
fn globset_matches_host_or_unscoped(set: &GlobSet, host: &str) -> bool {
|
||||
set.is_match(host) || unscoped_ip_literal(host).is_some_and(|ip| set.is_match(ip))
|
||||
}
|
||||
|
||||
fn is_explicit_local_allowlisted(allowed_domains: &[String], host: &Host) -> bool {
|
||||
let normalized_host = host.as_str();
|
||||
let unscoped_host = unscoped_ip_literal(normalized_host);
|
||||
allowed_domains.iter().any(|pattern| {
|
||||
let pattern = pattern.trim();
|
||||
if pattern == "*" || pattern.starts_with("*.") || pattern.starts_with("**.") {
|
||||
@@ -807,7 +810,9 @@ fn is_explicit_local_allowlisted(allowed_domains: &[String], host: &Host) -> boo
|
||||
if pattern.contains('*') || pattern.contains('?') {
|
||||
return false;
|
||||
}
|
||||
normalize_host(pattern) == normalized_host
|
||||
let normalized_pattern = normalize_host(pattern);
|
||||
normalized_pattern == normalized_host
|
||||
|| unscoped_host.is_some_and(|ip| normalized_pattern == ip)
|
||||
})
|
||||
}
|
||||
|
||||
@@ -1247,7 +1252,7 @@ mod tests {
|
||||
|
||||
#[tokio::test]
|
||||
async fn host_blocked_allows_scoped_ipv6_literal_when_explicitly_allowlisted() {
|
||||
let state = network_proxy_state_for_policy(network_settings(&["fe80::1%lo0"], &[]));
|
||||
let state = network_proxy_state_for_policy(network_settings(&["fe80::1"], &[]));
|
||||
|
||||
assert_eq!(
|
||||
state
|
||||
@@ -1258,6 +1263,68 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn host_blocked_requires_exact_scoped_ipv6_allowlist_match() {
|
||||
let state = network_proxy_state_for_policy(NetworkProxySettings {
|
||||
allow_local_binding: true,
|
||||
..network_settings(&["fe80::1%eth0"], &[])
|
||||
});
|
||||
|
||||
assert_eq!(
|
||||
state
|
||||
.host_blocked("fe80::1%eth0", /*port*/ 80)
|
||||
.await
|
||||
.unwrap(),
|
||||
HostBlockDecision::Allowed
|
||||
);
|
||||
assert_eq!(
|
||||
state
|
||||
.host_blocked("fe80::1%eth1", /*port*/ 80)
|
||||
.await
|
||||
.unwrap(),
|
||||
HostBlockDecision::Blocked(HostBlockReason::NotAllowed)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn host_blocked_denies_scoped_ipv6_literal_before_local_binding() {
|
||||
let state = network_proxy_state_for_policy(NetworkProxySettings {
|
||||
allow_local_binding: true,
|
||||
..network_settings(&["*"], &["fd00::1"])
|
||||
});
|
||||
|
||||
for host in ["fd00::1%eth0", "[fd00::1%eth0]", "[fd00::1%25eth0]"] {
|
||||
assert_eq!(
|
||||
state.host_blocked(host, /*port*/ 80).await.unwrap(),
|
||||
HostBlockDecision::Blocked(HostBlockReason::Denied),
|
||||
"host should be denied after normalization: {host}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn host_blocked_requires_exact_scoped_ipv6_denylist_match() {
|
||||
let state = network_proxy_state_for_policy(NetworkProxySettings {
|
||||
allow_local_binding: true,
|
||||
..network_settings(&["*"], &["fd00::1%eth0"])
|
||||
});
|
||||
|
||||
assert_eq!(
|
||||
state
|
||||
.host_blocked("fd00::1%eth0", /*port*/ 80)
|
||||
.await
|
||||
.unwrap(),
|
||||
HostBlockDecision::Blocked(HostBlockReason::Denied)
|
||||
);
|
||||
assert_eq!(
|
||||
state
|
||||
.host_blocked("fd00::1%eth1", /*port*/ 80)
|
||||
.await
|
||||
.unwrap(),
|
||||
HostBlockDecision::Allowed
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn host_blocked_rejects_private_ip_literals_when_local_binding_disabled() {
|
||||
let state = network_proxy_state_for_policy(network_settings(&["example.com"], &[]));
|
||||
|
||||
Reference in New Issue
Block a user