Skip to content

fix(policy): classify advisory private-IP notes with the canonical is_internal_ip (#1777) #1824

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
latenighthackathon wants to merge 1 commit into
base: main
Choose a base branch
from
+83 −29
Open

fix(policy): classify advisory private-IP notes with the canonical is_internal_ip (#1777) #1824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 19 additions & 13 deletions crates/openshell-core/src/net.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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]
Expand Down
42 changes: 33 additions & 9 deletions crates/openshell-sandbox/src/mechanistic_mapper.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -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::<IpAddr>().is_ok_and(is_internal_ip);
if resolves_internal || host == "localhost" {
notes.push(format!(
"Destination '{host}' appears to be an internal/private address."
));
Expand Down Expand Up @@ -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(&[]);
Expand Down
38 changes: 31 additions & 7 deletions crates/openshell-server/src/grpc/policy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -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::<IpAddr>().is_ok_and(is_internal_ip);
if resolves_internal || host == "localhost" {
notes.push(format!(
"Destination '{host}' appears to be an internal/private address."
));
Expand Down Expand Up @@ -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 {
Expand Down
Loading