diff --git a/crates/openshell-core/src/net.rs b/crates/openshell-core/src/net.rs index 06e6096ee..59bfdfc5d 100644 --- a/crates/openshell-core/src/net.rs +++ b/crates/openshell-core/src/net.rs @@ -191,31 +191,33 @@ pub fn is_internal_ip(ip: IpAddr) -> bool { /// IPv4 internal address check covering RFC 1918, CGNAT (RFC 6598), and other /// special-use ranges that should never be reachable from sandbox egress. fn is_internal_v4(v4: Ipv4Addr) -> bool { - if v4.is_loopback() || v4.is_private() || v4.is_link_local() || v4.is_unspecified() { + // Prefer the stable `Ipv4Addr` predicates. `is_documentation()` (RFC 5737) + // covers all three TEST-NET ranges, including 192.0.2.0/24 (TEST-NET-1) which + // the previous manual checks missed. + if v4.is_loopback() + || v4.is_private() + || v4.is_link_local() + || v4.is_unspecified() + || v4.is_documentation() + || v4.is_broadcast() + { return true; } let octets = v4.octets(); - // 100.64.0.0/10 — CGNAT / shared address space (RFC 6598). Commonly used by - // cloud VPC peering, Tailscale, and similar overlay networks. + // The ranges below have no stable std predicate yet, so keep them manual. + // 100.64.0.0/10 — CGNAT / shared address space (RFC 6598; `is_shared` is + // unstable). Commonly used by cloud VPC peering, Tailscale, and similar overlays. if octets[0] == 100 && (octets[1] & 0xC0) == 64 { return true; } - // 192.0.0.0/24 — IETF protocol assignments (RFC 6890) + // 192.0.0.0/24 — IETF protocol assignments (RFC 6890; no stable predicate). if octets[0] == 192 && octets[1] == 0 && octets[2] == 0 { return true; } - // 198.18.0.0/15 — benchmarking (RFC 2544) + // 198.18.0.0/15 — benchmarking (RFC 2544; `is_benchmarking` is unstable). if octets[0] == 198 && (octets[1] & 0xFE) == 18 { return true; } - // 198.51.100.0/24 — TEST-NET-2 (RFC 5737) - if octets[0] == 198 && octets[1] == 51 && octets[2] == 100 { - return true; - } - // 203.0.113.0/24 — TEST-NET-3 (RFC 5737) - if octets[0] == 203 && octets[1] == 0 && octets[2] == 113 { - return true; - } false } @@ -567,10 +569,14 @@ mod tests { // 198.18.0.0/15 — benchmarking assert!(is_internal_ip(IpAddr::V4(Ipv4Addr::new(198, 18, 0, 1)))); assert!(is_internal_ip(IpAddr::V4(Ipv4Addr::new(198, 19, 255, 255)))); + // 192.0.2.0/24 — TEST-NET-1 (RFC 5737), now covered via is_documentation() + assert!(is_internal_ip(IpAddr::V4(Ipv4Addr::new(192, 0, 2, 1)))); // 198.51.100.0/24 — TEST-NET-2 assert!(is_internal_ip(IpAddr::V4(Ipv4Addr::new(198, 51, 100, 1)))); // 203.0.113.0/24 — TEST-NET-3 assert!(is_internal_ip(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 1)))); + // 255.255.255.255 — limited broadcast (RFC 919), via is_broadcast() + assert!(is_internal_ip(IpAddr::V4(Ipv4Addr::BROADCAST))); } #[test] diff --git a/crates/openshell-sandbox/src/mechanistic_mapper.rs b/crates/openshell-sandbox/src/mechanistic_mapper.rs index 89261f303..ba7c51de9 100644 --- a/crates/openshell-sandbox/src/mechanistic_mapper.rs +++ b/crates/openshell-sandbox/src/mechanistic_mapper.rs @@ -12,7 +12,7 @@ //! The LLM-powered `PolicyAdvisor` (issue #205) wraps and enriches these //! mechanistic proposals with context-aware rationale and smarter grouping. -use openshell_core::net::{is_always_blocked_ip, is_known_metadata_hostname}; +use openshell_core::net::{is_always_blocked_ip, is_internal_ip, is_known_metadata_hostname}; use openshell_core::proto::{ DenialSummary, L7Allow, L7Rule, NetworkBinary, NetworkEndpoint, NetworkPolicyRule, PolicyChunk, }; @@ -293,14 +293,15 @@ fn generate_security_notes(host: &str, port: u16, is_ssrf: bool) -> String { ); } - // Check for private/reserved IP patterns in the host. - if host.starts_with("10.") - || host.starts_with("172.") - || host.starts_with("192.168.") - || host == "localhost" - || host.starts_with("127.") - || host.starts_with("169.254.") - { + // Flag destinations that are an internal/private address. Parse the host as + // an IP literal and defer to the canonical RFC-accurate classifier + // (openshell-core net::is_internal_ip) rather than naive string prefixes: + // `starts_with("172.")` wrongly matched 172.0-15 / 172.32-255 (RFC 1918 is + // only 172.16.0.0/12) and missed CGNAT (100.64.0.0/10), IPv6 ULA, etc. The + // "localhost" hostname is not an IP literal, so it is checked separately. + // See #1777. + let resolves_internal = host.parse::().is_ok_and(is_internal_ip); + if resolves_internal || host == "localhost" { notes.push(format!( "Destination '{host}' appears to be an internal/private address." )); @@ -470,6 +471,29 @@ mod tests { assert!(notes.contains("SSRF")); } + #[test] + fn test_security_notes_internal_ip_uses_canonical_classifier() { + // RFC 1918 is 172.16.0.0/12 only: the old starts_with("172.") prefix + // wrongly flagged 172.15/172.32 and missed CGNAT (100.64.0.0/10). #1777. + assert!(generate_security_notes("172.16.0.1", 80, false).contains("internal/private")); + assert!(!generate_security_notes("172.15.0.1", 80, false).contains("internal/private")); + assert!(!generate_security_notes("172.32.0.1", 80, false).contains("internal/private")); + assert!(generate_security_notes("100.64.0.1", 80, false).contains("internal/private")); + assert!(generate_security_notes("10.0.0.1", 80, false).contains("internal/private")); + assert!(generate_security_notes("192.168.1.1", 80, false).contains("internal/private")); + assert!(generate_security_notes("127.0.0.1", 80, false).contains("internal/private")); + assert!(generate_security_notes("localhost", 80, false).contains("internal/private")); + assert!(!generate_security_notes("8.8.8.8", 80, false).contains("internal/private")); + // Hostnames that merely start with a private-range prefix must NOT be + // flagged: classification parses an IP literal, not a string prefix. #1824. + assert!(!generate_security_notes("10.example.com", 80, false).contains("internal/private")); + assert!( + !generate_security_notes("172.example.com", 80, false).contains("internal/private") + ); + // IPv6 ULA (fc00::/7, RFC 4193) is internal/private. + assert!(generate_security_notes("fd00::1", 80, false).contains("internal/private")); + } + #[test] fn test_generate_proposals_empty() { let proposals = generate_proposals(&[]); diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 380671f10..d13ed7676 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -14,6 +14,7 @@ use crate::ServerState; use crate::auth::principal::Principal; use crate::persistence::{DraftChunkRecord, ObjectId, ObjectName, ObjectType, PolicyRecord, Store}; use crate::policy_store::PolicyStoreExt; +use openshell_core::net::is_internal_ip; use openshell_core::proto::policy_merge_operation; use openshell_core::proto::setting_value; use openshell_core::proto::{ @@ -3171,13 +3172,15 @@ fn policy_record_to_revision(record: &PolicyRecord, include_policy: bool) -> San fn generate_security_notes(host: &str, port: u16) -> String { let mut notes = Vec::new(); - if host.starts_with("10.") - || host.starts_with("172.") - || host.starts_with("192.168.") - || host == "localhost" - || host.starts_with("127.") - || host.starts_with("169.254.") - { + // Flag destinations that are an internal/private address. Parse the host as + // an IP literal and defer to the canonical RFC-accurate classifier + // (openshell-core net::is_internal_ip) rather than naive string prefixes: + // `starts_with("172.")` wrongly matched 172.0-15 / 172.32-255 (RFC 1918 is + // only 172.16.0.0/12) and missed CGNAT (100.64.0.0/10), IPv6 ULA, etc. The + // "localhost" hostname is not an IP literal, so it is checked separately. + // See #1777. + let resolves_internal = host.parse::().is_ok_and(is_internal_ip); + if resolves_internal || host == "localhost" { notes.push(format!( "Destination '{host}' appears to be an internal/private address." )); @@ -3893,6 +3896,27 @@ mod tests { request } + #[test] + fn security_notes_use_canonical_internal_ip_classifier() { + // RFC 1918 is 172.16.0.0/12 only: the old starts_with("172.") prefix + // wrongly flagged 172.15/172.32 and missed CGNAT (100.64.0.0/10). #1777. + assert!(generate_security_notes("172.16.0.1", 80).contains("internal/private")); + assert!(!generate_security_notes("172.15.0.1", 80).contains("internal/private")); + assert!(!generate_security_notes("172.32.0.1", 80).contains("internal/private")); + assert!(generate_security_notes("100.64.0.1", 80).contains("internal/private")); + assert!(generate_security_notes("10.0.0.1", 80).contains("internal/private")); + assert!(generate_security_notes("192.168.1.1", 80).contains("internal/private")); + assert!(generate_security_notes("127.0.0.1", 80).contains("internal/private")); + assert!(generate_security_notes("localhost", 80).contains("internal/private")); + assert!(!generate_security_notes("8.8.8.8", 80).contains("internal/private")); + // Hostnames that merely start with a private-range prefix must NOT be + // flagged: classification parses an IP literal, not a string prefix. #1824. + assert!(!generate_security_notes("10.example.com", 80).contains("internal/private")); + assert!(!generate_security_notes("172.example.com", 80).contains("internal/private")); + // IPv6 ULA (fc00::/7, RFC 4193) is internal/private. + assert!(generate_security_notes("fd00::1", 80).contains("internal/private")); + } + #[test] fn sandbox_caller_update_validation_allows_sandbox_policy_sync() { let req = UpdateConfigRequest {