From 8c4f9330a71d633adf8ff5be5766cd7eff8f653e Mon Sep 17 00:00:00 2001 From: Jeremy Hatfield Date: Mon, 5 Oct 2026 16:47:05 +0000 Subject: [PATCH 1/3] refactor: consolidate outbound address checks Route address checks through one shared policy type. --- .../internal/network/address_policy_test.go | 105 ++++++++++++++++++ backend/internal/network/safeclient.go | 68 +++++++++--- .../network/safeclient_policy_test.go | 12 +- backend/internal/security/url_validator.go | 89 ++++++--------- .../security/url_validator_policy_test.go | 73 ++++++++++++ .../internal/security/url_validator_test.go | 88 --------------- .../internal/services/remotestorage/ssrf.go | 6 +- .../services/remotestorage/ssrf_test.go | 27 +++++ backend/internal/utils/url_testing.go | 20 ++-- .../internal/utils/url_testing_policy_test.go | 49 ++++++++ backend/pkg/safehttp/safehttp.go | 6 +- backend/pkg/safehttp/safehttp_parity_test.go | 62 +++++++++++ 12 files changed, 420 insertions(+), 185 deletions(-) create mode 100644 backend/internal/network/address_policy_test.go create mode 100644 backend/internal/security/url_validator_policy_test.go create mode 100644 backend/internal/utils/url_testing_policy_test.go create mode 100644 backend/pkg/safehttp/safehttp_parity_test.go diff --git a/backend/internal/network/address_policy_test.go b/backend/internal/network/address_policy_test.go new file mode 100644 index 000000000..1470925c4 --- /dev/null +++ b/backend/internal/network/address_policy_test.go @@ -0,0 +1,105 @@ +package network + +import ( + "net" + "testing" +) + +func TestAddressPolicy_Blocked(t *testing.T) { + t.Parallel() + tests := []struct { + name string + ip string + policy AddressPolicy + want bool + }{ + {"nil", "", AddressPolicy{}, true}, + {"public", "8.8.8.8", AddressPolicy{}, false}, + {"loopback zero value", "127.0.0.1", AddressPolicy{}, true}, + {"loopback allowed", "::1", AddressPolicy{AllowLocalhost: true}, false}, + {"rfc1918 zero value", "10.0.0.1", AddressPolicy{}, true}, + {"rfc1918 allowed", "::ffff:10.0.0.1", AddressPolicy{AllowRFC1918: true}, false}, + {"link-local never opened", "169.254.169.254", AddressPolicy{AllowLocalhost: true, AllowRFC1918: true, AllowCGNAT: true}, true}, + {"shared space zero value", "100.64.0.1", AddressPolicy{}, true}, + {"shared space upper edge", "100.127.255.255", AddressPolicy{}, true}, + {"below shared space", "100.63.255.255", AddressPolicy{}, false}, + {"above shared space", "100.128.0.0", AddressPolicy{}, false}, + {"shared space mapped", "::ffff:100.64.0.1", AddressPolicy{}, true}, + {"shared space allowed", "100.64.0.1", AddressPolicy{AllowCGNAT: true}, false}, + {"metadata alias with allowance", "100.100.100.200", AddressPolicy{AllowCGNAT: true}, true}, + {"metadata alias mapped with allowance", "::ffff:100.100.100.200", AddressPolicy{AllowCGNAT: true, AllowRFC1918: true}, true}, + {"special-purpose v4", "192.0.0.1", AddressPolicy{}, true}, + {"special-purpose v4 neighbour", "192.0.1.1", AddressPolicy{}, false}, + {"benchmark range", "198.18.0.1", AddressPolicy{}, true}, + {"benchmark range upper", "198.19.255.255", AddressPolicy{}, true}, + {"benchmark range neighbour", "198.20.0.1", AddressPolicy{}, false}, + {"translation v6", "64:ff9b::808:808", AddressPolicy{}, true}, + {"translation v6 local", "64:ff9b:1::1", AddressPolicy{}, true}, + {"tunnel v6", "2002:c000:204::1", AddressPolicy{}, true}, + {"embedded v4 v6", "::1.2.3.4", AddressPolicy{}, true}, + {"tunnel v6 prefix", "2001:0:4136:e378:8000:63bf:3fff:fdd2", AddressPolicy{}, true}, + {"translation with every allowance", "64:ff9b::1", AddressPolicy{AllowLocalhost: true, AllowRFC1918: true, AllowCGNAT: true}, true}, + {"staging flag lifts translation block", "2002::1", AddressPolicy{AllowTransition: true}, false}, + {"staging flag keeps loopback blocked", "127.0.0.1", AddressPolicy{AllowTransition: true}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + var ip net.IP + if tt.ip != "" { + ip = net.ParseIP(tt.ip) + } + if got := tt.policy.Blocked(ip); got != tt.want { + t.Errorf("Blocked(%q) = %v, want %v", tt.ip, got, tt.want) + } + }) + } +} + +// TestClientOptionsPolicy_Precedence covers the combined legacy and new +// options: allow wins over block, and the default is unchanged. +func TestClientOptionsPolicy_Precedence(t *testing.T) { + t.Parallel() + cgnat := []struct { + blockCGNAT, allowCGNAT, wantAllowed bool + }{ + {false, false, true}, + {true, false, false}, + {false, true, true}, + {true, true, true}, + } + transition := []struct { + block, wantAllowed bool + }{ + {false, true}, + {true, false}, + } + for _, c := range cgnat { + for _, tr := range transition { + opts := ClientOptions{BlockCGNAT: c.blockCGNAT, AllowCGNAT: c.allowCGNAT, BlockTransitionRanges: tr.block} + p := opts.policy() + if got := !p.Blocked(net.ParseIP("100.64.0.1")); got != c.wantAllowed { + t.Errorf("%+v: shared space allowed = %v, want %v", opts, got, c.wantAllowed) + } + for _, ip := range []string{"198.18.0.1", "2002::1"} { + if got := !p.Blocked(net.ParseIP(ip)); got != tr.wantAllowed { + t.Errorf("%+v: %s allowed = %v, want %v", opts, ip, got, tr.wantAllowed) + } + } + for _, ip := range []string{"100.100.100.200", "::ffff:100.100.100.200"} { + if !p.Blocked(net.ParseIP(ip)) { + t.Errorf("%+v: %s must be blocked", opts, ip) + } + } + } + } +} + +func TestAllowOverlayOption_SetsField(t *testing.T) { + t.Parallel() + cfg := defaultOptions() + WithAllowCGNAT()(&cfg) + if !cfg.AllowCGNAT { + t.Fatal("WithAllowCGNAT did not set AllowCGNAT") + } +} diff --git a/backend/internal/network/safeclient.go b/backend/internal/network/safeclient.go index 2b5e2b250..7a2df762a 100644 --- a/backend/internal/network/safeclient.go +++ b/backend/internal/network/safeclient.go @@ -150,6 +150,10 @@ func initRFC1918Blocks() { // - Reserved ranges: 0.0.0.0/8, 240.0.0.0/4, 255.255.255.255/32 // - IPv6 unique local addresses: fc00::/7 // +// It does NOT cover the shared-address space (other than the one metadata alias) +// or the transition and special-purpose ranges. Use AddressPolicy.Blocked, the +// single policy entry point, to apply those rules. +// // IPv4-mapped IPv6 addresses (::ffff:x.x.x.x) are correctly handled by extracting // the IPv4 portion and validating it. // @@ -278,6 +282,9 @@ type ClientOptions struct { // cloud metadata (169.254.x.x), and reserved — remain blocked regardless. AllowRFC1918 bool + // AllowCGNAT permits the shared address space used by overlay networks. + AllowCGNAT bool + // BlockCGNAT rejects the carrier-grade NAT range (100.64.0.0/10). Off by // default so existing callers are unchanged. BlockCGNAT bool @@ -360,6 +367,14 @@ func WithAllowRFC1918() Option { } } +// WithAllowCGNAT permits destinations in the shared address space used by +// overlay networks. The cloud metadata alias inside it stays blocked. +func WithAllowCGNAT() Option { + return func(opts *ClientOptions) { + opts.AllowCGNAT = true + } +} + // WithBlockCGNAT rejects destinations in the carrier-grade NAT range // (100.64.0.0/10) in addition to the ranges blocked by IsPrivateIP. func WithBlockCGNAT() Option { @@ -376,35 +391,60 @@ func WithBlockTransitionRanges() Option { } } -// blockedByPolicy is the single address-policy predicate shared by the dialer -// (both its validation and selection passes) and the redirect check. It reports -// whether ip must NOT be connected to under opts. +// AddressPolicy is the single outbound address policy. The zero value is the +// strictest policy: only public addresses are allowed. Every opt-in is explicit. +type AddressPolicy struct { + // AllowLocalhost permits loopback addresses. + AllowLocalhost bool + // AllowRFC1918 permits the three RFC 1918 private ranges. + AllowRFC1918 bool + // AllowCGNAT permits the shared address space used by overlay networks. + // The cloud metadata alias inside it stays blocked. + AllowCGNAT bool + // AllowTransition is a temporary staging field: when true, transition + // ranges are not rejected up front. + AllowTransition bool +} + +// Blocked reports whether ip must NOT be connected to under the policy. It is +// shared by every dial-time, redirect and URL pre-validation check. // // The always-on rules (IsPrivateIP, which includes loopback and link-local) can // only be relaxed by the explicit AllowLocalhost / AllowRFC1918 branches, and // those branches only match loopback and RFC 1918 addresses, so no other // restricted address is reachable through them. -func blockedByPolicy(ip net.IP, opts *ClientOptions) bool { +func (p AddressPolicy) Blocked(ip net.IP) bool { if ip == nil { return true } // Transition ranges are matched on the raw address first: IPv6-only forms - // do not survive the To4 normalisation below. - if opts.BlockTransitionRanges && IsTransitionRange(ip) { + // do not survive the To4 normalisation in IsPrivateIP. + if !p.AllowTransition && IsTransitionRange(ip) { return true } - if opts.AllowLocalhost && ip.IsLoopback() { + if p.AllowLocalhost && ip.IsLoopback() { return false } - if opts.AllowRFC1918 && IsRFC1918(ip) { + if p.AllowRFC1918 && IsRFC1918(ip) { return false } - if opts.BlockCGNAT && IsCGNAT(ip) { + if IsCGNAT(ip) && !p.AllowCGNAT { return true } return IsPrivateIP(ip) } +// policy derives the address policy from the client options. While the legacy +// Block* options coexist with AllowCGNAT, an explicit allow wins over a block. +func (o *ClientOptions) policy() AddressPolicy { + return AddressPolicy{ + AllowLocalhost: o.AllowLocalhost, + AllowRFC1918: o.AllowRFC1918, + AllowCGNAT: !o.BlockCGNAT || o.AllowCGNAT, + AllowTransition: !o.BlockTransitionRanges, + } +} + // WithKeepAlive enables connection pooling (HTTP keep-alives) on the SSRF-safe // client. Without this option the client's transport is byte-for-byte identical // to today: keep-alives disabled, a single idle connection, IdleConnTimeout @@ -441,6 +481,7 @@ func WithKeepAlive(maxIdle, perHost int, idleTimeout time.Duration) Option { // This approach defeats Time-of-Check to Time-of-Use (TOCTOU) attacks where // DNS could return different IPs between validation and connection. func safeDialer(opts *ClientOptions) func(ctx context.Context, network, addr string) (net.Conn, error) { + policy := opts.policy() return func(ctx context.Context, network, addr string) (net.Conn, error) { // Parse host:port from address host, port, err := net.SplitHostPort(addr) @@ -470,7 +511,7 @@ func safeDialer(opts *ClientOptions) func(ctx context.Context, network, addr str // This prevents attackers from using DNS load balancing to mix private/public IPs. // The error deliberately omits the resolved address. for _, ip := range ips { - if blockedByPolicy(ip.IP, opts) { + if policy.Blocked(ip.IP) { return nil, fmt.Errorf("connection to private IP blocked for host %s: %w", host, ErrBlockedAddress) } } @@ -479,7 +520,7 @@ func safeDialer(opts *ClientOptions) func(ctx context.Context, network, addr str // blocked address can never be chosen even if the loops drift apart. var selectedIP net.IP for _, ip := range ips { - if !blockedByPolicy(ip.IP, opts) { + if !policy.Blocked(ip.IP) { selectedIP = ip.IP break } @@ -496,7 +537,7 @@ func safeDialer(opts *ClientOptions) func(ctx context.Context, network, addr str } // validateRedirectTarget checks if a redirect URL is safe to follow. -// It applies the same address policy as the dialer (blockedByPolicy), so the +// It applies the same address policy as the dialer (AddressPolicy.Blocked), so the // AllowLocalhost, AllowRFC1918 and range-blocking options behave identically. func validateRedirectTarget(req *http.Request, opts *ClientOptions) error { host := req.URL.Hostname() @@ -516,13 +557,14 @@ func validateRedirectTarget(req *http.Request, opts *ClientOptions) error { ctx, cancel := context.WithTimeout(context.Background(), opts.DialTimeout) defer cancel() + policy := opts.policy() ips, err := lookupIPAddr(ctx, host) if err != nil { return fmt.Errorf("DNS resolution failed for redirect target %s: %w", host, err) } for _, ip := range ips { - if blockedByPolicy(ip.IP, opts) { + if policy.Blocked(ip.IP) { return fmt.Errorf("redirect to private IP blocked for host %s: %w", host, ErrBlockedAddress) } } diff --git a/backend/internal/network/safeclient_policy_test.go b/backend/internal/network/safeclient_policy_test.go index 06712ef17..a7ca47471 100644 --- a/backend/internal/network/safeclient_policy_test.go +++ b/backend/internal/network/safeclient_policy_test.go @@ -92,7 +92,7 @@ func TestIsTransitionRange(t *testing.T) { } } -func TestBlockedByPolicy(t *testing.T) { +func TestClientOptionsPolicy(t *testing.T) { t.Parallel() tests := []struct { name string @@ -125,8 +125,8 @@ func TestBlockedByPolicy(t *testing.T) { ip = net.ParseIP(tt.ip) } opts := tt.opts - if got := blockedByPolicy(ip, &opts); got != tt.want { - t.Errorf("blockedByPolicy(%q) = %v, want %v", tt.ip, got, tt.want) + if got := opts.policy().Blocked(ip); got != tt.want { + t.Errorf("Blocked(%q) = %v, want %v", tt.ip, got, tt.want) } }) } @@ -352,7 +352,7 @@ func TestIsPrivateIP_CGNATMetadataAliasAddress(t *testing.T) { } } -func TestBlockedByPolicy_CGNATMetadataAliasNeverReachableThroughAllowBranches(t *testing.T) { +func TestClientOptionsPolicy_MetadataAliasNeverReachableThroughAllowBranches(t *testing.T) { t.Parallel() combos := []ClientOptions{ {}, @@ -366,14 +366,14 @@ func TestBlockedByPolicy_CGNATMetadataAliasNeverReachableThroughAllowBranches(t for _, ipStr := range []string{"100.100.100.200", "::ffff:100.100.100.200"} { for i, opts := range combos { o := opts - if !blockedByPolicy(net.ParseIP(ipStr), &o) { + if !o.policy().Blocked(net.ParseIP(ipStr)) { t.Errorf("combo %d: %s must be blocked", i, ipStr) } } } // Neighbouring CGNAT addresses stay reachable when CGNAT is not blocked. o := ClientOptions{AllowRFC1918: true} - if blockedByPolicy(net.ParseIP("100.100.100.199"), &o) { + if o.policy().Blocked(net.ParseIP("100.100.100.199")) { t.Error("100.100.100.199 must not be blocked when CGNAT blocking is off") } } diff --git a/backend/internal/security/url_validator.go b/backend/internal/security/url_validator.go index 4e740b94f..949f9c5f2 100644 --- a/backend/internal/security/url_validator.go +++ b/backend/internal/security/url_validator.go @@ -128,6 +128,10 @@ type ValidationConfig struct { // monitors. Link-local (169.254.x.x), loopback, cloud metadata, and all other // restricted ranges remain blocked regardless of this flag. AllowRFC1918 bool + + // AllowCGNAT permits the shared address space used by overlay networks. + // The cloud metadata alias inside it stays blocked. + AllowCGNAT bool } // ValidationOption allows customizing validation behavior. @@ -162,6 +166,12 @@ func WithAllowRFC1918() ValidationOption { return func(c *ValidationConfig) { c.AllowRFC1918 = true } } +// WithAllowCGNAT permits the shared address space used by overlay networks. +// The cloud metadata alias inside it stays blocked. +func WithAllowCGNAT() ValidationOption { + return func(c *ValidationConfig) { c.AllowCGNAT = true } +} + // ValidateExternalURL validates a URL for external HTTP requests with comprehensive SSRF protection. // This function provides defense-in-depth against Server-Side Request Forgery attacks by: // 1. Validating URL format and scheme @@ -195,6 +205,7 @@ func ValidateExternalURL(rawURL string, options ...ValidationOption) (string, er MaxRedirects: 0, Timeout: 3 * time.Second, BlockPrivateIPs: true, + AllowCGNAT: true, } // Apply custom options @@ -280,52 +291,19 @@ func ValidateExternalURL(rawURL string, options ...ValidationOption) (string, er return "", fmt.Errorf("no ip addresses resolved for hostname: %s", host) } - // Phase 4: Private IP Blocking - // Check ALL resolved IPs against private/reserved ranges + // Phase 4: Address Policy + // Check ALL resolved IPs against the shared address policy. AllowLocalhost + // is intentionally not part of it: it only short-circuits the literal + // localhost hosts in Phase 2. if config.BlockPrivateIPs { + policy := network.AddressPolicy{ + AllowRFC1918: config.AllowRFC1918, + AllowCGNAT: config.AllowCGNAT, + AllowTransition: true, + } for _, ip := range ips { - // ENHANCEMENT: IPv4-mapped IPv6 Detection - // Prevent bypass via ::ffff:192.168.1.1 format - if ip.To4() != nil && ip.To16() != nil && isIPv4MappedIPv6(ip) { - // Extract the IPv4 address from the mapped format - ipv4 := ip.To4() - // Allow RFC 1918 IPv4-mapped IPv6 only when the caller has explicitly opted in. - if config.AllowRFC1918 && network.IsRFC1918(ipv4) { - continue - } - if network.IsPrivateIP(ipv4) { - // Cloud metadata endpoint must produce the specific error even - // when the address arrives as an IPv4-mapped IPv6 value. - if ipv4.String() == "169.254.169.254" { - return "", blockedAddressError("access to cloud metadata endpoints is blocked for security (detected: %s)", sanitizeIPForError(ipv4.String())) - } - return "", blockedAddressError("connection to private ip addresses is blocked for security (detected: %s)", sanitizeIPForError(ipv4.String())) - } - } - - // Allow RFC 1918 addresses only when the caller has explicitly opted in - // (e.g., admin-configured uptime monitors targeting internal hosts). - // Link-local (169.254.x.x), loopback, cloud metadata, and all other - // restricted ranges remain blocked regardless of this flag. - if config.AllowRFC1918 && network.IsRFC1918(ip) { - continue - } - - // Check if IP is in private/reserved ranges using centralized network.IsPrivateIP - // This includes: - // - RFC 1918 private networks (10.x, 172.16.x, 192.168.x) - // - Loopback (127.x.x.x, ::1) - // - Link-local (169.254.x.x, fe80::) including cloud metadata - // - Reserved ranges (0.x.x.x, 240.x.x.x, 255.255.255.255) - // - IPv6 unique local (fc00::) - if network.IsPrivateIP(ip) { - // ENHANCEMENT: Sanitize Error Messages - // Don't leak internal IPs in error messages to external users - sanitizedIP := sanitizeIPForError(ip.String()) - if ip.String() == "169.254.169.254" { - return "", blockedAddressError("access to cloud metadata endpoints is blocked for security (detected: %s)", sanitizedIP) - } - return "", blockedAddressError("connection to private ip addresses is blocked for security (detected: %s)", sanitizedIP) + if policy.Blocked(ip) { + return "", blockedIPError(ip) } } } @@ -336,21 +314,18 @@ func ValidateExternalURL(rawURL string, options ...ValidationOption) (string, er return normalized, nil } -// isIPv4MappedIPv6 detects IPv4-mapped IPv6 addresses (::ffff:192.168.1.1). -// This prevents SSRF bypass via IPv6 notation of private IPv4 addresses. -func isIPv4MappedIPv6(ip net.IP) bool { - // IPv4-mapped IPv6 addresses have the form ::ffff:a.b.c.d - // In binary: 80 bits of zeros, 16 bits of ones, 32 bits of IPv4 - if len(ip) != net.IPv6len { - return false +// blockedIPError builds the rejection error for an address the policy refused. +// The cloud metadata endpoint, including its IPv4-mapped form, gets a specific +// message; the address shown is always sanitized. +func blockedIPError(ip net.IP) error { + if ip4 := ip.To4(); ip4 != nil { + ip = ip4 } - // Check for ::ffff: prefix (10 zero bytes, 2 0xff bytes) - for i := 0; i < 10; i++ { - if ip[i] != 0 { - return false - } + sanitized := sanitizeIPForError(ip.String()) + if ip.String() == "169.254.169.254" { + return blockedAddressError("access to cloud metadata endpoints is blocked for security (detected: %s)", sanitized) } - return ip[10] == 0xff && ip[11] == 0xff + return blockedAddressError("connection to private ip addresses is blocked for security (detected: %s)", sanitized) } // parsePort safely parses a port string to an integer. diff --git a/backend/internal/security/url_validator_policy_test.go b/backend/internal/security/url_validator_policy_test.go new file mode 100644 index 000000000..014c30553 --- /dev/null +++ b/backend/internal/security/url_validator_policy_test.go @@ -0,0 +1,73 @@ +package security + +import ( + "errors" + "strings" + "testing" + + "github.com/Wikid82/charon/backend/internal/network" +) + +func TestValidateExternalURL_MetadataMessageChosenAfterPolicy(t *testing.T) { + t.Parallel() + for _, raw := range []string{"http://169.254.169.254", "http://[::ffff:169.254.169.254]"} { + _, err := ValidateExternalURL(raw, WithAllowHTTP(), WithAllowRFC1918()) + if err == nil { + t.Fatalf("%s accepted", raw) + } + if !errors.Is(err, network.ErrBlockedAddress) { + t.Errorf("%s: sentinel missing: %v", raw, err) + } + if !strings.Contains(err.Error(), "cloud metadata endpoints") { + t.Errorf("%s: unexpected message %q", raw, err.Error()) + } + if !strings.Contains(err.Error(), "169.x.x.x") { + t.Errorf("%s: address not sanitized: %q", raw, err.Error()) + } + } + _, err := ValidateExternalURL("http://10.1.2.3", WithAllowHTTP()) + if err == nil || strings.Contains(err.Error(), "cloud metadata") || !strings.Contains(err.Error(), "private ip addresses") { + t.Errorf("expected generic private-address message, got %v", err) + } +} + +func TestValidateExternalURL_AllowLocalhostPinned(t *testing.T) { + t.Parallel() + for _, raw := range []string{"http://localhost:8080", "http://127.0.0.1:8080", "http://[::1]:8080"} { + if _, err := ValidateExternalURL(raw, WithAllowHTTP(), WithAllowLocalhost()); err != nil { + t.Errorf("%s rejected: %v", raw, err) + } + } + // Only the exact literal hosts are exempt: other loopback forms and names + // that resolve to loopback stay rejected. + for _, raw := range []string{"http://127.0.0.2:8080", "http://localhost.:8080"} { + _, err := ValidateExternalURL(raw, WithAllowHTTP(), WithAllowLocalhost()) + if err == nil { + t.Errorf("%s accepted", raw) + } else if !errors.Is(err, network.ErrBlockedAddress) { + t.Errorf("%s: sentinel missing: %v", raw, err) + } + } +} + +func TestValidateExternalURL_OverlayAllowanceOption(t *testing.T) { + t.Parallel() + cfg := &ValidationConfig{} + WithAllowCGNAT()(cfg) + if !cfg.AllowCGNAT { + t.Fatal("WithAllowCGNAT did not set AllowCGNAT") + } + + // Staging default: the shared address space is accepted without the option. + for _, opts := range [][]ValidationOption{{WithAllowHTTP()}, {WithAllowHTTP(), WithAllowCGNAT()}} { + if _, err := ValidateExternalURL("http://100.64.0.1", opts...); err != nil { + t.Errorf("shared-space literal rejected: %v", err) + } + } + // The metadata alias is rejected in every combination. + for _, opts := range [][]ValidationOption{{WithAllowHTTP()}, {WithAllowHTTP(), WithAllowCGNAT()}} { + if _, err := ValidateExternalURL("http://100.100.100.200", opts...); err == nil { + t.Error("metadata alias accepted") + } + } +} diff --git a/backend/internal/security/url_validator_test.go b/backend/internal/security/url_validator_test.go index 3b3c207f1..f5b0178ab 100644 --- a/backend/internal/security/url_validator_test.go +++ b/backend/internal/security/url_validator_test.go @@ -660,55 +660,6 @@ func TestIsPrivateIP_IPv6Comprehensive(t *testing.T) { } } -// TestIPv4MappedIPv6Detection tests detection of IPv4-mapped IPv6 addresses. -// ENHANCEMENT: Required by Supervisor review for SSRF bypass prevention -func TestIPv4MappedIPv6Detection(t *testing.T) { - t.Parallel() - tests := []struct { - name string - ip string - expected bool - }{ - // IPv4-mapped IPv6 addresses (::ffff:x.x.x.x) - {"IPv4-mapped loopback", "::ffff:127.0.0.1", true}, - {"IPv4-mapped private 10.x", "::ffff:10.0.0.1", true}, - {"IPv4-mapped private 192.168", "::ffff:192.168.1.1", true}, - {"IPv4-mapped metadata", "::ffff:169.254.169.254", true}, - {"IPv4-mapped public", "::ffff:8.8.8.8", true}, - - // Regular IPv6 addresses (not mapped) - {"Regular IPv6 loopback", "::1", false}, - {"Regular IPv6 link-local", "fe80::1", false}, - {"Regular IPv6 public", "2001:4860:4860::8888", false}, - } - - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - ip := net.ParseIP(tt.ip) - if ip == nil { - t.Fatalf("Failed to parse IP: %s", tt.ip) - } - - result := isIPv4MappedIPv6(ip) - if result != tt.expected { - t.Errorf("isIPv4MappedIPv6(%s) = %v, want %v", tt.ip, result, tt.expected) - } - }) - } -} - -// TestValidateExternalURL_IPv4MappedIPv6Blocking tests blocking of private IPs via IPv6 mapping. -// ENHANCEMENT: Critical security test per Supervisor review -func TestValidateExternalURL_IPv4MappedIPv6Blocking(t *testing.T) { - t.Parallel() - // NOTE: These tests will fail DNS resolution since we can't actually - // set up DNS records to return IPv4-mapped IPv6 addresses - // The isIPv4MappedIPv6 function itself is tested above - t.Skip("DNS resolution of IPv4-mapped IPv6 not testable without custom DNS server") -} - // TestValidateExternalURL_HostnameValidation tests enhanced hostname validation. // ENHANCEMENT: Tests RFC 1035 compliance and suspicious pattern detection func TestValidateExternalURL_HostnameValidation(t *testing.T) { @@ -1016,45 +967,6 @@ func TestValidateExternalURL_EdgeCases(t *testing.T) { } } -// TestIsIPv4MappedIPv6_EdgeCases tests IPv4-mapped IPv6 detection edge cases. -// ENHANCEMENT: Additional edge cases for SSRF bypass prevention -func TestIsIPv4MappedIPv6_EdgeCases(t *testing.T) { - t.Parallel() - tests := []struct { - name string - ip string - expected bool - }{ - // Standard IPv4-mapped format - {"Standard mapped", "::ffff:192.168.1.1", true}, - {"Mapped public IP", "::ffff:8.8.8.8", true}, - - // Edge cases - Note: net.ParseIP returns 16-byte representation for IPv4 - // So we need to check the raw parsing behavior - {"Pure IPv6 2001:db8", "2001:db8::1", false}, - {"IPv6 loopback", "::1", false}, - - // Boundary checks - {"All zeros except prefix", "::ffff:0.0.0.0", true}, - {"All ones", "::ffff:255.255.255.255", true}, - } - - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - ip := net.ParseIP(tt.ip) - if ip == nil { - t.Fatalf("Failed to parse IP: %s", tt.ip) - } - result := isIPv4MappedIPv6(ip) - if result != tt.expected { - t.Errorf("isIPv4MappedIPv6(%s) = %v, want %v", tt.ip, result, tt.expected) - } - }) - } -} - // PR-3: WithAllowRFC1918 validation option tests func TestValidateExternalURL_WithAllowRFC1918_Permits10x(t *testing.T) { diff --git a/backend/internal/services/remotestorage/ssrf.go b/backend/internal/services/remotestorage/ssrf.go index ffa96c09f..155c31490 100644 --- a/backend/internal/services/remotestorage/ssrf.go +++ b/backend/internal/services/remotestorage/ssrf.go @@ -74,10 +74,8 @@ func ValidateHostSSRF(host string) error { // validateIPSSRF applies the RFC1918-allowed SSRF policy to a single // resolved IP. func validateIPSSRF(ip net.IP) error { - if network.IsRFC1918(ip) { - return nil - } - if network.IsPrivateIP(ip) { + policy := network.AddressPolicy{AllowRFC1918: true, AllowCGNAT: true, AllowTransition: true} + if policy.Blocked(ip) { return fmt.Errorf("connection to disallowed address blocked: %s", ip) } return nil diff --git a/backend/internal/services/remotestorage/ssrf_test.go b/backend/internal/services/remotestorage/ssrf_test.go index 5dbfac1ac..10f74190c 100644 --- a/backend/internal/services/remotestorage/ssrf_test.go +++ b/backend/internal/services/remotestorage/ssrf_test.go @@ -154,3 +154,30 @@ func TestWithPermissiveSSRFForTesting_SwapsAndRestores(t *testing.T) { assert.Error(t, ssrfValidateHost("127.0.0.1"), "restore must reinstate the production default") assert.Error(t, ssrfValidateDialAddress(net.ParseIP("127.0.0.1")), "restore must reinstate the production default") } + +func TestRemoteStoragePolicy_Outcomes(t *testing.T) { + t.Parallel() + tests := []struct { + ip string + wantErr bool + }{ + {"8.8.8.8", false}, + {"10.0.0.5", false}, + {"192.168.1.10", false}, + {"100.64.0.1", false}, + {"127.0.0.1", true}, + {"::1", true}, + {"169.254.169.254", true}, + {"100.100.100.200", true}, + {"fd00::1", true}, + } + for _, tt := range tests { + err := validateIPSSRF(net.ParseIP(tt.ip)) + if (err != nil) != tt.wantErr { + t.Errorf("validateIPSSRF(%s) err = %v, wantErr %v", tt.ip, err, tt.wantErr) + } + } + if err := validateIPSSRF(nil); err == nil { + t.Error("nil address accepted") + } +} diff --git a/backend/internal/utils/url_testing.go b/backend/internal/utils/url_testing.go index 5894ba463..358d59303 100644 --- a/backend/internal/utils/url_testing.go +++ b/backend/internal/utils/url_testing.go @@ -22,12 +22,11 @@ func resolveAllowedIP(ctx context.Context, host string, allowLocalhost bool) (ne return nil, fmt.Errorf("missing hostname") } + policy := network.AddressPolicy{AllowLocalhost: allowLocalhost, AllowCGNAT: true, AllowTransition: true} + // Fast-path: IP literal. if ip := net.ParseIP(host); ip != nil { - if allowLocalhost && ip.IsLoopback() { - return ip, nil - } - if network.IsPrivateIP(ip) { + if policy.Blocked(ip) { return nil, fmt.Errorf("access to private IP addresses is blocked (resolved to %s)", ip) } return ip, nil @@ -43,13 +42,7 @@ func resolveAllowedIP(ctx context.Context, host string, allowLocalhost bool) (ne var selected net.IP for _, ip := range ips { - if allowLocalhost && ip.IP.IsLoopback() { - if selected == nil { - selected = ip.IP - } - continue - } - if network.IsPrivateIP(ip.IP) { + if policy.Blocked(ip.IP) { return nil, fmt.Errorf("access to private IP addresses is blocked (resolved to %s)", ip.IP) } if selected == nil { @@ -67,6 +60,7 @@ func resolveAllowedIP(ctx context.Context, host string, allowLocalhost bool) (ne // This prevents DNS rebinding attacks by validating the IP just before connecting. // Returns a DialContext function suitable for use in http.Transport. func ssrfSafeDialer() func(ctx context.Context, network, addr string) (net.Conn, error) { + policy := network.AddressPolicy{AllowCGNAT: true, AllowTransition: true} return func(ctx context.Context, netw, addr string) (net.Conn, error) { // Parse host and port from address host, port, err := net.SplitHostPort(addr) @@ -85,9 +79,9 @@ func ssrfSafeDialer() func(ctx context.Context, network, addr string) (net.Conn, } // Validate ALL resolved IPs - if any are private, reject immediately - // Using centralized network.IsPrivateIP for consistent SSRF protection + // Using the shared network.AddressPolicy for consistent protection for _, ip := range ips { - if network.IsPrivateIP(ip.IP) { + if policy.Blocked(ip.IP) { return nil, fmt.Errorf("access to private IP addresses is blocked (resolved to %s)", ip.IP) } } diff --git a/backend/internal/utils/url_testing_policy_test.go b/backend/internal/utils/url_testing_policy_test.go new file mode 100644 index 000000000..94afbc79b --- /dev/null +++ b/backend/internal/utils/url_testing_policy_test.go @@ -0,0 +1,49 @@ +package utils + +import ( + "context" + "testing" +) + +func TestResolveAllowedIP_PolicyOutcomes(t *testing.T) { + t.Parallel() + tests := []struct { + name string + host string + allowLocalhost bool + wantErr bool + }{ + {"public literal", "8.8.8.8", false, false}, + {"loopback literal", "127.0.0.1", false, true}, + {"loopback literal allowed", "127.0.0.1", true, false}, + {"loopback v6 allowed", "::1", true, false}, + {"private literal", "10.0.0.1", true, true}, + {"link-local literal", "169.254.169.254", true, true}, + {"metadata alias", "100.100.100.200", false, true}, + {"shared space literal", "100.64.0.1", false, false}, + {"special-purpose literal", "198.18.0.1", false, false}, + {"loopback name blocked", "localhost", false, true}, + {"loopback name allowed", "localhost", true, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + _, err := resolveAllowedIP(context.Background(), tt.host, tt.allowLocalhost) + if (err != nil) != tt.wantErr { + t.Errorf("resolveAllowedIP(%q, %v) err = %v, wantErr %v", tt.host, tt.allowLocalhost, err, tt.wantErr) + } + }) + } +} + +func TestOutboundDialer_RejectsLoopbackAlways(t *testing.T) { + t.Parallel() + dial := ssrfSafeDialer() + for _, addr := range []string{"127.0.0.1:80", "[::1]:80", "10.0.0.1:80", "100.100.100.200:80"} { + conn, err := dial(context.Background(), "tcp", addr) + if err == nil { + _ = conn.Close() + t.Errorf("dial %s succeeded", addr) + } + } +} diff --git a/backend/pkg/safehttp/safehttp.go b/backend/pkg/safehttp/safehttp.go index e0cddb88d..faa1140ef 100644 --- a/backend/pkg/safehttp/safehttp.go +++ b/backend/pkg/safehttp/safehttp.go @@ -141,10 +141,8 @@ func checkLiteralHost(host string, p Policy) error { return nil } - blocked := network.IsTransitionRange(ip) || - (network.IsCGNAT(ip) && !p.allowCGNAT) || - (network.IsPrivateIP(ip) && (!p.allowRFC1918 || !network.IsRFC1918(ip))) - if blocked { + policy := network.AddressPolicy{AllowRFC1918: p.allowRFC1918, AllowCGNAT: p.allowCGNAT, AllowTransition: false} + if policy.Blocked(ip) { return fmt.Errorf("url host is not allowed: %w", ErrBlockedAddress) } return nil diff --git a/backend/pkg/safehttp/safehttp_parity_test.go b/backend/pkg/safehttp/safehttp_parity_test.go new file mode 100644 index 000000000..89d6c2a0b --- /dev/null +++ b/backend/pkg/safehttp/safehttp_parity_test.go @@ -0,0 +1,62 @@ +package safehttp + +import ( + "errors" + "testing" + "time" +) + +// TestClientPolicyOutcomes pins which literal destinations each policy lets the +// client dial. A refused destination fails with ErrBlockedAddress; an allowed +// one fails later (nothing listens there), never with the sentinel. +func TestClientPolicyOutcomes(t *testing.T) { + tests := []struct { + name string + policy Policy + host string + wantBlocked bool + }{ + {"shared space under strict policy", PublicHTTPSOnly(), "100.64.0.1", true}, + {"shared space under private policy", PrivateNetworkOK(), "100.64.0.1", false}, + {"metadata alias under private policy", PrivateNetworkOK(), "100.100.100.200", true}, + {"benchmark range under strict policy", PublicHTTPSOnly(), "198.18.0.1", true}, + {"benchmark range under private policy", PrivateNetworkOK(), "198.18.0.1", true}, + {"tunnel range under private policy", PrivateNetworkOK(), "[2002::1]", true}, + {"private range under strict policy", PublicHTTPSOnly(), "10.0.0.1", true}, + {"private range under private policy", PrivateNetworkOK(), "10.255.255.1", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client := NewClient(tt.policy, 300*time.Millisecond) + resp, err := client.Get("http://" + tt.host + ":9/") + if err == nil { + _ = resp.Body.Close() + t.Fatal("unexpected success") + } + if got := errors.Is(err, ErrBlockedAddress); got != tt.wantBlocked { + t.Errorf("blocked = %v, want %v (err: %v)", got, tt.wantBlocked, err) + } + }) + } +} + +func TestValidateURL_LiteralOutcomesPerPolicy(t *testing.T) { + t.Parallel() + tests := []struct { + policy Policy + raw string + wantErr bool + }{ + {PublicHTTPSOnly(), "https://100.64.0.1/", true}, + {PrivateNetworkOK(), "http://100.64.0.1/", false}, + {PrivateNetworkOK(), "http://100.100.100.200/", true}, + {PrivateNetworkOK(), "http://198.18.0.1/", true}, + {PrivateNetworkOK(), "http://[2002::1]/", true}, + } + for _, tt := range tests { + _, err := ValidateURL(tt.raw, tt.policy) + if (err != nil) != tt.wantErr { + t.Errorf("ValidateURL(%q) err = %v, wantErr %v", tt.raw, err, tt.wantErr) + } + } +} From 2d17278102d78d745f7c88eb6ab3245b92bb7efa Mon Sep 17 00:00:00 2001 From: Jeremy Hatfield Date: Mon, 5 Oct 2026 16:53:54 +0000 Subject: [PATCH 2/3] refactor: declare overlay-friendly outbound destinations Declare destination allowances explicitly at each call site. --- .../enhanced_security_notification_service.go | 2 + .../services/notify_client_adapter.go | 3 +- .../services/outbound_policy_wiring_test.go | 82 +++++++++++++++++++ .../internal/services/remotestorage/ssrf.go | 11 ++- .../services/security_notification_service.go | 2 + backend/internal/services/uptime_check.go | 2 + backend/pkg/safehttp/safehttp.go | 6 ++ 7 files changed, 105 insertions(+), 3 deletions(-) create mode 100644 backend/internal/services/outbound_policy_wiring_test.go diff --git a/backend/internal/services/enhanced_security_notification_service.go b/backend/internal/services/enhanced_security_notification_service.go index 9f47014c4..e445b95b1 100644 --- a/backend/internal/services/enhanced_security_notification_service.go +++ b/backend/internal/services/enhanced_security_notification_service.go @@ -578,6 +578,7 @@ func (s *EnhancedSecurityNotificationService) sendWebhook(ctx context.Context, w validatedURL, err := security.ValidateExternalURL(webhookURL, security.WithAllowHTTP(), // Allow HTTP for backwards compatibility security.WithAllowLocalhost(), // Allow localhost for testing + security.WithAllowCGNAT(), ) if err != nil { return fmt.Errorf("ssrf validation failed: %w", err) @@ -600,6 +601,7 @@ func (s *EnhancedSecurityNotificationService) sendWebhook(ctx context.Context, w client := network.NewSafeHTTPClient( network.WithTimeout(10*time.Second), network.WithAllowLocalhost(), // Allow localhost for testing + network.WithAllowCGNAT(), ) resp, err := client.Do(req) if err != nil { diff --git a/backend/internal/services/notify_client_adapter.go b/backend/internal/services/notify_client_adapter.go index 5dc5fc2d7..e0a3b0a36 100644 --- a/backend/internal/services/notify_client_adapter.go +++ b/backend/internal/services/notify_client_adapter.go @@ -88,6 +88,7 @@ func notifyClientFactory(allowHTTP bool, maxRedirects int) *http.Client { opts := []network.Option{ network.WithTimeout(notifyClientTimeout), network.WithMaxRedirects(maxRedirects), + network.WithAllowCGNAT(), } if allowHTTP { opts = append(opts, network.WithAllowLocalhost()) @@ -99,7 +100,7 @@ func notifyClientFactory(allowHTTP bool, maxRedirects int) *http.Client { // existing SSRF-safe URL validation (internal/security) — Seam 2 of the // extraction spec (§3.2). func notifyURLValidator(rawURL string, allowHTTP bool) (string, error) { - var opts []security.ValidationOption + opts := []security.ValidationOption{security.WithAllowCGNAT()} if allowHTTP { opts = append(opts, security.WithAllowHTTP(), security.WithAllowLocalhost()) } diff --git a/backend/internal/services/outbound_policy_wiring_test.go b/backend/internal/services/outbound_policy_wiring_test.go new file mode 100644 index 000000000..257068032 --- /dev/null +++ b/backend/internal/services/outbound_policy_wiring_test.go @@ -0,0 +1,82 @@ +package services + +import ( + "context" + "errors" + "net/http" + "testing" + "time" + + "github.com/Wikid82/charon/backend/internal/models" + "github.com/Wikid82/charon/backend/internal/network" +) + +// dialPolicyProbe dials addr through the client's transport and reports whether +// the address policy refused it. An allowed address fails later (nothing +// listens there) without the policy sentinel. +func dialPolicyProbe(t *testing.T, client *http.Client, addr string) bool { + t.Helper() + transport, ok := client.Transport.(*http.Transport) + if !ok { + t.Fatalf("unexpected transport type %T", client.Transport) + } + ctx, cancel := context.WithTimeout(context.Background(), 300*time.Millisecond) + defer cancel() + conn, err := transport.DialContext(ctx, "tcp", addr) + if err == nil { + _ = conn.Close() + return false + } + return errors.Is(err, network.ErrBlockedAddress) +} + +func TestOutboundClients_OverlayDestinations(t *testing.T) { + clients := map[string]*http.Client{ + "uptime": newUptimeChecker(&UptimeService{}).httpClient, + "notify": notifyClientFactory(true, 0), + } + for name, client := range clients { + if dialPolicyProbe(t, client, "100.64.0.1:9") { + t.Errorf("%s: overlay address refused", name) + } + if !dialPolicyProbe(t, client, "100.100.100.200:9") { + t.Errorf("%s: metadata alias allowed", name) + } + if !dialPolicyProbe(t, client, "169.254.169.254:9") { + t.Errorf("%s: link-local allowed", name) + } + } +} + +func TestNotifyURLValidator_OverlayDestinations(t *testing.T) { + t.Parallel() + if _, err := notifyURLValidator("http://100.64.0.1/hook", true); err != nil { + t.Errorf("overlay address rejected: %v", err) + } + if _, err := notifyURLValidator("http://100.100.100.200/hook", true); err == nil { + t.Error("metadata alias accepted") + } +} + +func TestSecurityWebhooks_OverlayDestinations(t *testing.T) { + event := models.SecurityEvent{EventType: "waf_block", Severity: "high", Message: "probe"} + senders := map[string]func(ctx context.Context, url string) error{ + "legacy": func(ctx context.Context, url string) error { + return (&SecurityNotificationService{}).sendWebhook(ctx, url, event) + }, + "enhanced": func(ctx context.Context, url string) error { + return (&EnhancedSecurityNotificationService{}).sendWebhook(ctx, url, event) + }, + } + for name, send := range senders { + ctx, cancel := context.WithTimeout(context.Background(), 400*time.Millisecond) + err := send(ctx, "http://100.64.0.1:9/hook") + cancel() + if err != nil && errors.Is(err, network.ErrBlockedAddress) { + t.Errorf("%s: overlay address refused: %v", name, err) + } + if err := send(context.Background(), "http://100.100.100.200:9/hook"); err == nil { + t.Errorf("%s: metadata alias accepted", name) + } + } +} diff --git a/backend/internal/services/remotestorage/ssrf.go b/backend/internal/services/remotestorage/ssrf.go index 155c31490..fe95613f0 100644 --- a/backend/internal/services/remotestorage/ssrf.go +++ b/backend/internal/services/remotestorage/ssrf.go @@ -71,11 +71,18 @@ func ValidateHostSSRF(host string) error { return nil } +// overlayFriendlyPolicy is the address policy for remote storage targets: a NAS +// on the operator's LAN or on an overlay network is a legitimate destination, +// while loopback, link-local and every other reserved range stay blocked. +// It is hoisted into one package var so the save-time check and the dial-time +// check share a single documented policy. +// AllowTransition is a temporary staging field. +var overlayFriendlyPolicy = network.AddressPolicy{AllowRFC1918: true, AllowCGNAT: true, AllowTransition: true} + // validateIPSSRF applies the RFC1918-allowed SSRF policy to a single // resolved IP. func validateIPSSRF(ip net.IP) error { - policy := network.AddressPolicy{AllowRFC1918: true, AllowCGNAT: true, AllowTransition: true} - if policy.Blocked(ip) { + if overlayFriendlyPolicy.Blocked(ip) { return fmt.Errorf("connection to disallowed address blocked: %s", ip) } return nil diff --git a/backend/internal/services/security_notification_service.go b/backend/internal/services/security_notification_service.go index ea1b23960..692e6dc9a 100644 --- a/backend/internal/services/security_notification_service.go +++ b/backend/internal/services/security_notification_service.go @@ -103,6 +103,7 @@ func (s *SecurityNotificationService) sendWebhook(ctx context.Context, webhookUR validatedURL, err := security.ValidateExternalURL(webhookURL, security.WithAllowLocalhost(), // Allow localhost for testing security.WithAllowHTTP(), // Some webhooks use HTTP + security.WithAllowCGNAT(), ) if err != nil { // Log SSRF attempt with high severity @@ -133,6 +134,7 @@ func (s *SecurityNotificationService) sendWebhook(ctx context.Context, webhookUR client := network.NewSafeHTTPClient( network.WithTimeout(10*time.Second), network.WithAllowLocalhost(), // Allow localhost for testing + network.WithAllowCGNAT(), ) resp, err := client.Do(req) if err != nil { diff --git a/backend/internal/services/uptime_check.go b/backend/internal/services/uptime_check.go index bfe40b9bd..058033790 100644 --- a/backend/internal/services/uptime_check.go +++ b/backend/internal/services/uptime_check.go @@ -78,6 +78,7 @@ func newUptimeChecker(svc *UptimeService) *uptimeChecker { network.WithMaxRedirects(0), network.WithAllowLocalhost(), network.WithAllowRFC1918(), + network.WithAllowCGNAT(), network.WithKeepAlive(100, 4, 30*time.Second), ) return &uptimeChecker{ @@ -112,6 +113,7 @@ func (c *uptimeChecker) probe(ctx context.Context, monitor models.UptimeMonitor) // hosts. Link-local (169.254.x.x), cloud metadata, and all other // restricted ranges remain blocked at both validation layers. security.WithAllowRFC1918(), + security.WithAllowCGNAT(), ) if err != nil { msg = fmt.Sprintf("security validation failed: %s", err.Error()) diff --git a/backend/pkg/safehttp/safehttp.go b/backend/pkg/safehttp/safehttp.go index faa1140ef..20e347a1e 100644 --- a/backend/pkg/safehttp/safehttp.go +++ b/backend/pkg/safehttp/safehttp.go @@ -53,6 +53,9 @@ func (p Policy) clientOptions(timeout time.Duration) []network.Option { if p.allowRFC1918 { opts = append(opts, network.WithAllowRFC1918()) } + if p.allowCGNAT { + opts = append(opts, network.WithAllowCGNAT()) + } if !p.allowCGNAT { opts = append(opts, network.WithBlockCGNAT()) } @@ -205,6 +208,9 @@ func ValidateURL(raw string, p Policy) (*url.URL, error) { if p.allowRFC1918 { opts = append(opts, security.WithAllowRFC1918()) } + if p.allowCGNAT { + opts = append(opts, security.WithAllowCGNAT()) + } // The returned normalised string is discarded on purpose: requests are built // from the *url.URL parsed above, never from a re-parsed string. if _, err := security.ValidateExternalURL(raw, opts...); err != nil { From 2dc3815cdb67263a12cf387ad8db71d1311d760d Mon Sep 17 00:00:00 2001 From: Jeremy Hatfield Date: Mon, 5 Oct 2026 17:27:16 +0000 Subject: [PATCH 3/3] fix(security): harden outbound destination validation Apply a single shared address policy, secure by default, to outbound destination checks. --- .../api/handlers/settings_handler_test.go | 24 ++++++ .../internal/crowdsec/hub_sync_policy_test.go | 29 +++++++ .../internal/network/address_policy_test.go | 75 ++++++++++--------- backend/internal/network/safeclient.go | 50 +++---------- .../network/safeclient_policy_test.go | 61 ++++++++------- backend/internal/security/url_validator.go | 8 +- .../security/url_validator_policy_test.go | 41 ++++++++-- .../services/outbound_policy_wiring_test.go | 11 +++ .../internal/services/remotestorage/ssrf.go | 3 +- .../services/remotestorage/ssrf_test.go | 29 +++++++ .../internal/services/update_service_test.go | 13 ++++ backend/internal/utils/url_testing.go | 5 +- .../internal/utils/url_testing_policy_test.go | 12 ++- backend/pkg/safehttp/safehttp.go | 12 +-- 14 files changed, 245 insertions(+), 128 deletions(-) create mode 100644 backend/internal/crowdsec/hub_sync_policy_test.go diff --git a/backend/internal/api/handlers/settings_handler_test.go b/backend/internal/api/handlers/settings_handler_test.go index 23c272cbb..ffff45bf2 100644 --- a/backend/internal/api/handlers/settings_handler_test.go +++ b/backend/internal/api/handlers/settings_handler_test.go @@ -1830,6 +1830,30 @@ func TestSettingsHandler_TestPublicURL_IPv6LocalhostBlocked(t *testing.T) { // IPv6 loopback should be blocked } +func TestSettingsHandler_TestPublicURL_ReservedRangesBlocked(t *testing.T) { + handler, _ := setupSettingsHandlerWithMail(t) + + router := newAdminRouter() + router.Use(func(c *gin.Context) { + c.Set("role", "admin") + c.Next() + }) + router.POST("/settings/test-url", handler.TestPublicURL) + + for _, target := range []string{"http://100.64.0.1", "http://198.18.0.1", "http://[2002::1]", "http://[64:ff9b::808:808]"} { + jsonBody, _ := json.Marshal(map[string]string{"url": target}) + req, _ := http.NewRequest("POST", "/settings/test-url", bytes.NewBuffer(jsonBody)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + assert.Equal(t, http.StatusOK, w.Code, target) + var resp map[string]any + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp), target) + assert.False(t, resp["reachable"].(bool), target) + } +} + // TestUpdateSetting_EmptyValueIsAccepted guards the PR-1 fix: Value must NOT carry // binding:"required". Gin treats "" as missing for string fields and returns 400 if // the tag is present. Re-adding the tag would silently regress the CrowdSec enable diff --git a/backend/internal/crowdsec/hub_sync_policy_test.go b/backend/internal/crowdsec/hub_sync_policy_test.go new file mode 100644 index 000000000..7c4a4e91d --- /dev/null +++ b/backend/internal/crowdsec/hub_sync_policy_test.go @@ -0,0 +1,29 @@ +package crowdsec + +import ( + "context" + "errors" + "net/http" + "testing" + "time" + + "github.com/Wikid82/charon/backend/internal/network" +) + +func TestNewHubHTTPClient_RejectsReservedDestinations(t *testing.T) { + transport, ok := newHubHTTPClient(time.Second).Transport.(*http.Transport) + if !ok { + t.Fatal("unexpected transport type") + } + for _, addr := range []string{"100.64.0.1:9", "198.18.0.1:9", "[2002::1]:9", "10.0.0.1:9"} { + conn, err := transport.DialContext(context.Background(), "tcp", addr) + if err == nil { + _ = conn.Close() + t.Errorf("dial %s succeeded", addr) + continue + } + if !errors.Is(err, network.ErrBlockedAddress) { + t.Errorf("dial %s: expected policy rejection, got %v", addr, err) + } + } +} diff --git a/backend/internal/network/address_policy_test.go b/backend/internal/network/address_policy_test.go index 1470925c4..16a93c515 100644 --- a/backend/internal/network/address_policy_test.go +++ b/backend/internal/network/address_policy_test.go @@ -1,8 +1,11 @@ package network import ( + "context" + "errors" "net" "testing" + "time" ) func TestAddressPolicy_Blocked(t *testing.T) { @@ -39,8 +42,6 @@ func TestAddressPolicy_Blocked(t *testing.T) { {"embedded v4 v6", "::1.2.3.4", AddressPolicy{}, true}, {"tunnel v6 prefix", "2001:0:4136:e378:8000:63bf:3fff:fdd2", AddressPolicy{}, true}, {"translation with every allowance", "64:ff9b::1", AddressPolicy{AllowLocalhost: true, AllowRFC1918: true, AllowCGNAT: true}, true}, - {"staging flag lifts translation block", "2002::1", AddressPolicy{AllowTransition: true}, false}, - {"staging flag keeps loopback blocked", "127.0.0.1", AddressPolicy{AllowTransition: true}, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -56,41 +57,29 @@ func TestAddressPolicy_Blocked(t *testing.T) { } } -// TestClientOptionsPolicy_Precedence covers the combined legacy and new -// options: allow wins over block, and the default is unchanged. -func TestClientOptionsPolicy_Precedence(t *testing.T) { +// TestClientOptionsPolicy_EndState covers the policy derived from client options. +func TestClientOptionsPolicy_EndState(t *testing.T) { t.Parallel() - cgnat := []struct { - blockCGNAT, allowCGNAT, wantAllowed bool - }{ - {false, false, true}, - {true, false, false}, - {false, true, true}, - {true, true, true}, - } - transition := []struct { - block, wantAllowed bool + tests := []struct { + name string + opts ClientOptions + ip string + want bool }{ - {false, true}, - {true, false}, + {"shared space default", ClientOptions{}, "100.64.0.1", true}, + {"shared space allowed", ClientOptions{AllowCGNAT: true}, "100.64.0.1", false}, + {"shared space with other allowances", ClientOptions{AllowLocalhost: true, AllowRFC1918: true}, "100.64.0.1", true}, + {"metadata alias with allowance", ClientOptions{AllowCGNAT: true}, "100.100.100.200", true}, + {"mapped metadata alias with allowance", ClientOptions{AllowCGNAT: true}, "::ffff:100.100.100.200", true}, + {"benchmark range default", ClientOptions{}, "198.18.0.1", true}, + {"benchmark range with every allowance", ClientOptions{AllowLocalhost: true, AllowRFC1918: true, AllowCGNAT: true}, "198.19.255.255", true}, + {"tunnel range with every allowance", ClientOptions{AllowLocalhost: true, AllowRFC1918: true, AllowCGNAT: true}, "2002::1", true}, + {"public with every allowance", ClientOptions{AllowLocalhost: true, AllowRFC1918: true, AllowCGNAT: true}, "8.8.8.8", false}, } - for _, c := range cgnat { - for _, tr := range transition { - opts := ClientOptions{BlockCGNAT: c.blockCGNAT, AllowCGNAT: c.allowCGNAT, BlockTransitionRanges: tr.block} - p := opts.policy() - if got := !p.Blocked(net.ParseIP("100.64.0.1")); got != c.wantAllowed { - t.Errorf("%+v: shared space allowed = %v, want %v", opts, got, c.wantAllowed) - } - for _, ip := range []string{"198.18.0.1", "2002::1"} { - if got := !p.Blocked(net.ParseIP(ip)); got != tr.wantAllowed { - t.Errorf("%+v: %s allowed = %v, want %v", opts, ip, got, tr.wantAllowed) - } - } - for _, ip := range []string{"100.100.100.200", "::ffff:100.100.100.200"} { - if !p.Blocked(net.ParseIP(ip)) { - t.Errorf("%+v: %s must be blocked", opts, ip) - } - } + for _, tt := range tests { + opts := tt.opts + if got := opts.policy().Blocked(net.ParseIP(tt.ip)); got != tt.want { + t.Errorf("%s: Blocked(%s) = %v, want %v", tt.name, tt.ip, got, tt.want) } } } @@ -103,3 +92,21 @@ func TestAllowOverlayOption_SetsField(t *testing.T) { t.Fatal("WithAllowCGNAT did not set AllowCGNAT") } } + +func TestNewSafeHTTPClient_OverlayAllowanceWiredToDialer(t *testing.T) { + withResolver(t, map[string][]string{"overlay.example": {"100.64.0.9"}}) + + blocked := safeDialer(&ClientOptions{DialTimeout: time.Second}) + if _, err := blocked(context.Background(), "tcp", "overlay.example:9"); !errors.Is(err, ErrBlockedAddress) { + t.Fatalf("default dialer: expected ErrBlockedAddress, got %v", err) + } + + allowed := safeDialer(&ClientOptions{DialTimeout: 200 * time.Millisecond, AllowCGNAT: true}) + conn, err := allowed(context.Background(), "tcp", "overlay.example:9") + if conn != nil { + _ = conn.Close() + } + if errors.Is(err, ErrBlockedAddress) { + t.Fatalf("allowing dialer still refused the address: %v", err) + } +} diff --git a/backend/internal/network/safeclient.go b/backend/internal/network/safeclient.go index 7a2df762a..8c0059bd9 100644 --- a/backend/internal/network/safeclient.go +++ b/backend/internal/network/safeclient.go @@ -82,21 +82,20 @@ var privateCIDRs = []string{ "fe80::/10", // Cloud instance metadata alias, inside the CGNAT range (100.64.0.0/10). - // Blocked globally; the rest of the CGNAT range is only blocked by WithBlockCGNAT. + // Blocked even when the rest of the range is allowed. "100.100.100.200/32", } // cgnatCIDRs is the shared-address space used by carrier-grade NAT and many -// overlay networks. It is NOT part of IsPrivateIP; callers opt in to blocking -// it with WithBlockCGNAT. +// overlay networks. It is NOT part of IsPrivateIP; AddressPolicy blocks it unless +// AllowCGNAT is set. var cgnatCIDRs = []string{ "100.64.0.0/10", } // transitionCIDRs lists IPv4/IPv6 transition, translation and special-purpose // ranges that have no legitimate use as an outbound HTTP destination. They are -// NOT part of IsPrivateIP; callers opt in to blocking them with -// WithBlockTransitionRanges. +// NOT part of IsPrivateIP; AddressPolicy always blocks them. var transitionCIDRs = []string{ "192.0.0.0/24", // IETF protocol assignments "198.18.0.0/15", // Benchmarking @@ -285,14 +284,6 @@ type ClientOptions struct { // AllowCGNAT permits the shared address space used by overlay networks. AllowCGNAT bool - // BlockCGNAT rejects the carrier-grade NAT range (100.64.0.0/10). Off by - // default so existing callers are unchanged. - BlockCGNAT bool - - // BlockTransitionRanges rejects IPv4/IPv6 transition and special-purpose - // ranges (see IsTransitionRange). Off by default. - BlockTransitionRanges bool - // keepAlive, when true, enables HTTP connection pooling on the SSRF-safe // client. When false (the default) the client keeps its historical // behaviour byte-for-byte: DisableKeepAlives=true, MaxIdleConns=1, @@ -375,22 +366,6 @@ func WithAllowCGNAT() Option { } } -// WithBlockCGNAT rejects destinations in the carrier-grade NAT range -// (100.64.0.0/10) in addition to the ranges blocked by IsPrivateIP. -func WithBlockCGNAT() Option { - return func(opts *ClientOptions) { - opts.BlockCGNAT = true - } -} - -// WithBlockTransitionRanges rejects destinations in IPv4/IPv6 transition and -// special-purpose ranges (see IsTransitionRange). -func WithBlockTransitionRanges() Option { - return func(opts *ClientOptions) { - opts.BlockTransitionRanges = true - } -} - // AddressPolicy is the single outbound address policy. The zero value is the // strictest policy: only public addresses are allowed. Every opt-in is explicit. type AddressPolicy struct { @@ -401,9 +376,6 @@ type AddressPolicy struct { // AllowCGNAT permits the shared address space used by overlay networks. // The cloud metadata alias inside it stays blocked. AllowCGNAT bool - // AllowTransition is a temporary staging field: when true, transition - // ranges are not rejected up front. - AllowTransition bool } // Blocked reports whether ip must NOT be connected to under the policy. It is @@ -419,7 +391,7 @@ func (p AddressPolicy) Blocked(ip net.IP) bool { } // Transition ranges are matched on the raw address first: IPv6-only forms // do not survive the To4 normalisation in IsPrivateIP. - if !p.AllowTransition && IsTransitionRange(ip) { + if IsTransitionRange(ip) { return true } if p.AllowLocalhost && ip.IsLoopback() { @@ -434,14 +406,12 @@ func (p AddressPolicy) Blocked(ip net.IP) bool { return IsPrivateIP(ip) } -// policy derives the address policy from the client options. While the legacy -// Block* options coexist with AllowCGNAT, an explicit allow wins over a block. +// policy derives the address policy from the client options. func (o *ClientOptions) policy() AddressPolicy { return AddressPolicy{ - AllowLocalhost: o.AllowLocalhost, - AllowRFC1918: o.AllowRFC1918, - AllowCGNAT: !o.BlockCGNAT || o.AllowCGNAT, - AllowTransition: !o.BlockTransitionRanges, + AllowLocalhost: o.AllowLocalhost, + AllowRFC1918: o.AllowRFC1918, + AllowCGNAT: o.AllowCGNAT, } } @@ -583,7 +553,7 @@ func validateRedirectTarget(req *http.Request, opts *ClientOptions) error { // - 10 second timeout // - No redirects (returns http.ErrUseLastResponse) // - Keep-alives disabled -// - Private IPs blocked +// - Private, shared-address and reserved ranges blocked // // Use functional options to customize behavior: // diff --git a/backend/internal/network/safeclient_policy_test.go b/backend/internal/network/safeclient_policy_test.go index a7ca47471..b8418b034 100644 --- a/backend/internal/network/safeclient_policy_test.go +++ b/backend/internal/network/safeclient_policy_test.go @@ -108,14 +108,13 @@ func TestClientOptionsPolicy(t *testing.T) { {"rfc1918 default", "10.0.0.5", ClientOptions{}, true}, {"rfc1918 allowed", "10.0.0.5", ClientOptions{AllowRFC1918: true}, false}, {"link-local never allowed by rfc1918", "169.254.169.254", ClientOptions{AllowRFC1918: true, AllowLocalhost: true}, true}, - {"cgnat default allowed through", "100.64.0.1", ClientOptions{}, false}, - {"cgnat blocked by option", "100.64.0.1", ClientOptions{BlockCGNAT: true}, true}, - {"cgnat blocked even with rfc1918 allowed", "100.64.0.1", ClientOptions{BlockCGNAT: true, AllowRFC1918: true}, true}, - {"transition default allowed through", "2002::1", ClientOptions{}, false}, - {"transition blocked by option", "2002::1", ClientOptions{BlockTransitionRanges: true}, true}, - {"transition blocked with every allowance", "64:ff9b::1", ClientOptions{BlockTransitionRanges: true, AllowLocalhost: true, AllowRFC1918: true}, true}, - {"teredo blocked by option", "2001:0:4136:e378:8000:63bf:3fff:fdd2", ClientOptions{BlockTransitionRanges: true}, true}, - {"loopback v6 with transition option and allowance", "::1", ClientOptions{BlockTransitionRanges: true, AllowLocalhost: true}, false}, + {"shared space default blocked", "100.64.0.1", ClientOptions{}, true}, + {"shared space allowed by option", "100.64.0.1", ClientOptions{AllowCGNAT: true}, false}, + {"shared space blocked even with rfc1918 allowed", "100.64.0.1", ClientOptions{AllowRFC1918: true}, true}, + {"transition blocked by default", "2002::1", ClientOptions{}, true}, + {"transition blocked with every allowance", "64:ff9b::1", ClientOptions{AllowLocalhost: true, AllowRFC1918: true, AllowCGNAT: true}, true}, + {"tunnel range blocked", "2001:0:4136:e378:8000:63bf:3fff:fdd2", ClientOptions{}, true}, + {"loopback v6 with allowance", "::1", ClientOptions{AllowLocalhost: true}, false}, {"unspecified blocked", "0.0.0.0", ClientOptions{AllowLocalhost: true, AllowRFC1918: true}, true}, } for _, tt := range tests { @@ -187,7 +186,7 @@ func TestSafeDialer_RejectsBlockedAnswerInEitherPosition(t *testing.T) { withResolver(t, map[string][]string{ "blocked-first.example": {"10.0.0.5", "127.0.0.1"}, "blocked-last.example": {"127.0.0.1", "10.0.0.5"}, - "cgnat-mixed.example": {"127.0.0.1", "100.64.0.1"}, + "shared-mixed.example": {"127.0.0.1", "100.64.0.1"}, "trans-mixed.example": {"2002::1", "127.0.0.1"}, "meta-alias.example": {"::ffff:169.254.169.254", "127.0.0.1"}, "clean.example": {"127.0.0.1"}, @@ -202,9 +201,10 @@ func TestSafeDialer_RejectsBlockedAnswerInEitherPosition(t *testing.T) { {"rfc1918 blocked first", "blocked-first.example", ClientOptions{AllowLocalhost: true}, false}, {"rfc1918 blocked last", "blocked-last.example", ClientOptions{AllowLocalhost: true}, false}, {"rfc1918 allowed last", "blocked-last.example", ClientOptions{AllowLocalhost: true, AllowRFC1918: true}, true}, - {"cgnat blocked by option", "cgnat-mixed.example", ClientOptions{AllowLocalhost: true, BlockCGNAT: true}, false}, - {"cgnat not blocked without option", "cgnat-mixed.example", ClientOptions{AllowLocalhost: true}, true}, - {"transition blocked by option", "trans-mixed.example", ClientOptions{AllowLocalhost: true, BlockTransitionRanges: true}, false}, + {"shared space blocked by default", "shared-mixed.example", ClientOptions{AllowLocalhost: true}, false}, + {"shared space allowed by option", "shared-mixed.example", ClientOptions{AllowLocalhost: true, AllowCGNAT: true}, true}, + {"transition blocked by default", "trans-mixed.example", ClientOptions{AllowLocalhost: true}, false}, + {"transition blocked with every allowance", "trans-mixed.example", ClientOptions{AllowLocalhost: true, AllowRFC1918: true, AllowCGNAT: true}, false}, {"mapped link-local blocked despite allowances", "meta-alias.example", ClientOptions{AllowLocalhost: true, AllowRFC1918: true}, false}, {"clean answer", "clean.example", ClientOptions{AllowLocalhost: true}, true}, } @@ -235,11 +235,11 @@ func TestSafeDialer_RejectsBlockedAnswerInEitherPosition(t *testing.T) { func TestValidateRedirectTarget_SharedPolicy(t *testing.T) { withResolver(t, map[string][]string{ - "lan.example": {"192.168.1.10"}, - "overlay.example": {"100.64.0.9"}, - "nat64.example": {"64:ff9b::1"}, - "public.example": {"8.8.8.8"}, - "meta.example": {"169.254.169.254"}, + "lan.example": {"192.168.1.10"}, + "overlay.example": {"100.64.0.9"}, + "translation.example": {"64:ff9b::1"}, + "public.example": {"8.8.8.8"}, + "meta.example": {"169.254.169.254"}, }) tests := []struct { @@ -250,9 +250,9 @@ func TestValidateRedirectTarget_SharedPolicy(t *testing.T) { }{ {"rfc1918 default", "lan.example", ClientOptions{}, true}, {"rfc1918 allowed", "lan.example", ClientOptions{AllowRFC1918: true}, false}, - {"cgnat blocked", "overlay.example", ClientOptions{AllowRFC1918: true, BlockCGNAT: true}, true}, - {"cgnat default", "overlay.example", ClientOptions{}, false}, - {"transition blocked", "nat64.example", ClientOptions{BlockTransitionRanges: true}, true}, + {"shared space blocked", "overlay.example", ClientOptions{AllowRFC1918: true}, true}, + {"shared space allowed by option", "overlay.example", ClientOptions{AllowCGNAT: true}, false}, + {"transition blocked", "translation.example", ClientOptions{}, true}, {"public", "public.example", ClientOptions{}, false}, {"link-local with allowances", "meta.example", ClientOptions{AllowRFC1918: true, AllowLocalhost: true}, true}, } @@ -297,7 +297,7 @@ func TestValidateRedirectTarget_ResolutionFailure(t *testing.T) { } } -func TestNewSafeHTTPClient_BlockOptionsWired(t *testing.T) { +func TestNewSafeHTTPClient_DefaultsBlockReservedRanges(t *testing.T) { var hits atomic.Int32 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { hits.Add(1) @@ -306,13 +306,13 @@ func TestNewSafeHTTPClient_BlockOptionsWired(t *testing.T) { _, port, _ := net.SplitHostPort(srv.Listener.Addr().String()) withResolver(t, map[string][]string{ - "overlay.example": {"100.64.0.9"}, - "nat64.example": {"64:ff9b::1"}, + "overlay.example": {"100.64.0.9"}, + "translation.example": {"64:ff9b::1"}, }) - for _, host := range []string{"overlay.example", "nat64.example"} { + for _, host := range []string{"overlay.example", "translation.example"} { client := NewSafeHTTPClient( - WithAllowRFC1918(), WithBlockCGNAT(), WithBlockTransitionRanges(), + WithAllowRFC1918(), WithTimeout(2*time.Second), ) resp, err := client.Get("http://" + net.JoinHostPort(host, port) + "/") @@ -359,9 +359,8 @@ func TestClientOptionsPolicy_MetadataAliasNeverReachableThroughAllowBranches(t * {AllowLocalhost: true}, {AllowRFC1918: true}, {AllowLocalhost: true, AllowRFC1918: true}, - // CGNAT-allowed policy: the range is not blocked by option. - {AllowRFC1918: true, BlockTransitionRanges: true}, - {AllowLocalhost: true, AllowRFC1918: true, BlockTransitionRanges: true, BlockCGNAT: true}, + {AllowCGNAT: true}, + {AllowLocalhost: true, AllowRFC1918: true, AllowCGNAT: true}, } for _, ipStr := range []string{"100.100.100.200", "::ffff:100.100.100.200"} { for i, opts := range combos { @@ -371,10 +370,10 @@ func TestClientOptionsPolicy_MetadataAliasNeverReachableThroughAllowBranches(t * } } } - // Neighbouring CGNAT addresses stay reachable when CGNAT is not blocked. - o := ClientOptions{AllowRFC1918: true} + // Neighbouring addresses stay reachable once the shared space is allowed. + o := ClientOptions{AllowRFC1918: true, AllowCGNAT: true} if o.policy().Blocked(net.ParseIP("100.100.100.199")) { - t.Error("100.100.100.199 must not be blocked when CGNAT blocking is off") + t.Error("100.100.100.199 must not be blocked when the range is allowed") } } diff --git a/backend/internal/security/url_validator.go b/backend/internal/security/url_validator.go index 949f9c5f2..697d5ffaa 100644 --- a/backend/internal/security/url_validator.go +++ b/backend/internal/security/url_validator.go @@ -187,6 +187,8 @@ func WithAllowCGNAT() ValidationOption { // - Link-local addresses (169.254.0.0/16, fe80::/10) including cloud metadata endpoints // - Reserved IP ranges (0.0.0.0/8, 240.0.0.0/4, 255.255.255.255/32) // - IPv6 unique local addresses (fc00::/7) +// - Transition, translation and other special-purpose ranges +// - The shared overlay address space, unless AllowCGNAT is set // // Example usage: // @@ -205,7 +207,6 @@ func ValidateExternalURL(rawURL string, options ...ValidationOption) (string, er MaxRedirects: 0, Timeout: 3 * time.Second, BlockPrivateIPs: true, - AllowCGNAT: true, } // Apply custom options @@ -297,9 +298,8 @@ func ValidateExternalURL(rawURL string, options ...ValidationOption) (string, er // localhost hosts in Phase 2. if config.BlockPrivateIPs { policy := network.AddressPolicy{ - AllowRFC1918: config.AllowRFC1918, - AllowCGNAT: config.AllowCGNAT, - AllowTransition: true, + AllowRFC1918: config.AllowRFC1918, + AllowCGNAT: config.AllowCGNAT, } for _, ip := range ips { if policy.Blocked(ip) { diff --git a/backend/internal/security/url_validator_policy_test.go b/backend/internal/security/url_validator_policy_test.go index 014c30553..036a381f2 100644 --- a/backend/internal/security/url_validator_policy_test.go +++ b/backend/internal/security/url_validator_policy_test.go @@ -58,11 +58,11 @@ func TestValidateExternalURL_OverlayAllowanceOption(t *testing.T) { t.Fatal("WithAllowCGNAT did not set AllowCGNAT") } - // Staging default: the shared address space is accepted without the option. - for _, opts := range [][]ValidationOption{{WithAllowHTTP()}, {WithAllowHTTP(), WithAllowCGNAT()}} { - if _, err := ValidateExternalURL("http://100.64.0.1", opts...); err != nil { - t.Errorf("shared-space literal rejected: %v", err) - } + if _, err := ValidateExternalURL("http://100.64.0.1", WithAllowHTTP()); !errors.Is(err, network.ErrBlockedAddress) { + t.Errorf("shared-space literal accepted by default: %v", err) + } + if _, err := ValidateExternalURL("http://100.64.0.1", WithAllowHTTP(), WithAllowCGNAT()); err != nil { + t.Errorf("shared-space literal rejected with the allowance: %v", err) } // The metadata alias is rejected in every combination. for _, opts := range [][]ValidationOption{{WithAllowHTTP()}, {WithAllowHTTP(), WithAllowCGNAT()}} { @@ -71,3 +71,34 @@ func TestValidateExternalURL_OverlayAllowanceOption(t *testing.T) { } } } + +func TestValidateExternalURL_ReservedRangesRejectedByDefault(t *testing.T) { + t.Parallel() + hosts := []string{ + "192.0.0.1", "198.18.0.1", "198.19.255.255", + "[64:ff9b::808:808]", "[64:ff9b:1::1]", "[2002:c000:204::1]", "[::1.2.3.4]", + "[2001:0:4136:e378:8000:63bf:3fff:fdd2]", "[::ffff:100.64.0.1]", "100.127.255.255", + } + allowances := [][]ValidationOption{ + {WithAllowHTTP()}, + {WithAllowHTTP(), WithAllowRFC1918(), WithAllowCGNAT(), WithAllowLocalhost()}, + } + for _, h := range hosts { + for i, opts := range allowances { + if h == "[::ffff:100.64.0.1]" || h == "100.127.255.255" { + if i == 1 { + continue // shared space is intentionally allowed with the opt-in + } + } + _, err := ValidateExternalURL("http://"+h+":8080", opts...) + if !errors.Is(err, network.ErrBlockedAddress) { + t.Errorf("%s (allowance set %d): expected policy rejection, got %v", h, i, err) + } + } + } + for _, h := range []string{"100.63.255.255", "100.128.0.0", "192.0.1.1", "198.20.0.1"} { + if _, err := ValidateExternalURL("http://"+h+":8080", WithAllowHTTP()); err != nil { + t.Errorf("%s rejected: %v", h, err) + } + } +} diff --git a/backend/internal/services/outbound_policy_wiring_test.go b/backend/internal/services/outbound_policy_wiring_test.go index 257068032..352d45413 100644 --- a/backend/internal/services/outbound_policy_wiring_test.go +++ b/backend/internal/services/outbound_policy_wiring_test.go @@ -45,6 +45,11 @@ func TestOutboundClients_OverlayDestinations(t *testing.T) { if !dialPolicyProbe(t, client, "169.254.169.254:9") { t.Errorf("%s: link-local allowed", name) } + for _, addr := range []string{"198.18.0.1:9", "192.0.0.1:9", "[2002::1]:9", "[64:ff9b::808:808]:9"} { + if !dialPolicyProbe(t, client, addr) { + t.Errorf("%s: %s allowed", name, addr) + } + } } } @@ -56,6 +61,9 @@ func TestNotifyURLValidator_OverlayDestinations(t *testing.T) { if _, err := notifyURLValidator("http://100.100.100.200/hook", true); err == nil { t.Error("metadata alias accepted") } + if _, err := notifyURLValidator("http://198.18.0.1/hook", true); err == nil { + t.Error("benchmark-range address accepted") + } } func TestSecurityWebhooks_OverlayDestinations(t *testing.T) { @@ -78,5 +86,8 @@ func TestSecurityWebhooks_OverlayDestinations(t *testing.T) { if err := send(context.Background(), "http://100.100.100.200:9/hook"); err == nil { t.Errorf("%s: metadata alias accepted", name) } + if err := send(context.Background(), "http://198.18.0.1:9/hook"); err == nil { + t.Errorf("%s: benchmark-range address accepted", name) + } } } diff --git a/backend/internal/services/remotestorage/ssrf.go b/backend/internal/services/remotestorage/ssrf.go index fe95613f0..28f1dd688 100644 --- a/backend/internal/services/remotestorage/ssrf.go +++ b/backend/internal/services/remotestorage/ssrf.go @@ -76,8 +76,7 @@ func ValidateHostSSRF(host string) error { // while loopback, link-local and every other reserved range stay blocked. // It is hoisted into one package var so the save-time check and the dial-time // check share a single documented policy. -// AllowTransition is a temporary staging field. -var overlayFriendlyPolicy = network.AddressPolicy{AllowRFC1918: true, AllowCGNAT: true, AllowTransition: true} +var overlayFriendlyPolicy = network.AddressPolicy{AllowRFC1918: true, AllowCGNAT: true} // validateIPSSRF applies the RFC1918-allowed SSRF policy to a single // resolved IP. diff --git a/backend/internal/services/remotestorage/ssrf_test.go b/backend/internal/services/remotestorage/ssrf_test.go index 10f74190c..b6d4bd51a 100644 --- a/backend/internal/services/remotestorage/ssrf_test.go +++ b/backend/internal/services/remotestorage/ssrf_test.go @@ -3,6 +3,7 @@ package remotestorage import ( "context" "net" + "strings" "sync" "testing" "time" @@ -181,3 +182,31 @@ func TestRemoteStoragePolicy_Outcomes(t *testing.T) { t.Error("nil address accepted") } } + +func TestRemoteStoragePolicy_RejectsReservedRanges(t *testing.T) { + t.Parallel() + for _, ip := range []string{ + "192.0.0.1", "198.18.0.1", "198.19.255.255", "64:ff9b::808:808", "64:ff9b:1::1", + "2002:c000:204::1", "::1.2.3.4", "2001:0:4136:e378:8000:63bf:3fff:fdd2", + } { + if err := validateIPSSRF(net.ParseIP(ip)); err == nil { + t.Errorf("validateIPSSRF(%s) accepted", ip) + } + } +} + +func TestSafeDialer_ControlHookRejectsReservedLiteral(t *testing.T) { + t.Parallel() + d := safeDialer(200 * time.Millisecond) + for _, addr := range []string{"198.18.0.1:9", "[2002::1]:9"} { + conn, err := d.DialContext(context.Background(), "tcp", addr) + if err == nil { + _ = conn.Close() + t.Errorf("dial %s succeeded", addr) + continue + } + if !strings.Contains(err.Error(), "disallowed address blocked") { + t.Errorf("dial %s: expected policy rejection, got %v", addr, err) + } + } +} diff --git a/backend/internal/services/update_service_test.go b/backend/internal/services/update_service_test.go index 570856621..b9052ec82 100644 --- a/backend/internal/services/update_service_test.go +++ b/backend/internal/services/update_service_test.go @@ -2,11 +2,13 @@ package services import ( "encoding/json" + "errors" "net/http" "net/http/httptest" "testing" "time" + "github.com/Wikid82/charon/backend/internal/network" "github.com/stretchr/testify/assert" ) @@ -158,3 +160,14 @@ func TestUpdateService_SetAPIURL_GitHubValidation(t *testing.T) { }) } } + +func TestUpdateService_CheckForUpdates_RejectsReservedDestinations(t *testing.T) { + for _, target := range []string{"http://100.64.0.1:9/", "http://198.18.0.1:9/", "http://[2002::1]:9/"} { + svc := NewUpdateService() + svc.apiURL = target + _, err := svc.CheckForUpdates() + if err == nil || !errors.Is(err, network.ErrBlockedAddress) { + t.Errorf("%s: expected policy rejection, got %v", target, err) + } + } +} diff --git a/backend/internal/utils/url_testing.go b/backend/internal/utils/url_testing.go index 358d59303..3425c60b2 100644 --- a/backend/internal/utils/url_testing.go +++ b/backend/internal/utils/url_testing.go @@ -22,7 +22,7 @@ func resolveAllowedIP(ctx context.Context, host string, allowLocalhost bool) (ne return nil, fmt.Errorf("missing hostname") } - policy := network.AddressPolicy{AllowLocalhost: allowLocalhost, AllowCGNAT: true, AllowTransition: true} + policy := network.AddressPolicy{AllowLocalhost: allowLocalhost} // Fast-path: IP literal. if ip := net.ParseIP(host); ip != nil { @@ -60,7 +60,8 @@ func resolveAllowedIP(ctx context.Context, host string, allowLocalhost bool) (ne // This prevents DNS rebinding attacks by validating the IP just before connecting. // Returns a DialContext function suitable for use in http.Transport. func ssrfSafeDialer() func(ctx context.Context, network, addr string) (net.Conn, error) { - policy := network.AddressPolicy{AllowCGNAT: true, AllowTransition: true} + // Zero-value policy: only public addresses, loopback included in the block. + policy := network.AddressPolicy{} return func(ctx context.Context, netw, addr string) (net.Conn, error) { // Parse host and port from address host, port, err := net.SplitHostPort(addr) diff --git a/backend/internal/utils/url_testing_policy_test.go b/backend/internal/utils/url_testing_policy_test.go index 94afbc79b..6e2a9e5fb 100644 --- a/backend/internal/utils/url_testing_policy_test.go +++ b/backend/internal/utils/url_testing_policy_test.go @@ -20,8 +20,16 @@ func TestResolveAllowedIP_PolicyOutcomes(t *testing.T) { {"private literal", "10.0.0.1", true, true}, {"link-local literal", "169.254.169.254", true, true}, {"metadata alias", "100.100.100.200", false, true}, - {"shared space literal", "100.64.0.1", false, false}, - {"special-purpose literal", "198.18.0.1", false, false}, + {"shared space literal", "100.64.0.1", false, true}, + {"shared space literal allowed loopback only", "100.64.0.1", true, true}, + {"special-purpose literal", "198.18.0.1", false, true}, + {"special-purpose v4 literal", "192.0.0.1", false, true}, + {"translation literal", "64:ff9b::808:808", false, true}, + {"translation local-use literal", "64:ff9b:1::1", false, true}, + {"tunnel literal", "2002:c000:204::1", false, true}, + {"embedded v4 literal", "::1.2.3.4", false, true}, + {"tunnel v6 literal", "2001:0:4136:e378:8000:63bf:3fff:fdd2", false, true}, + {"mapped shared space literal", "::ffff:100.64.0.1", false, true}, {"loopback name blocked", "localhost", false, true}, {"loopback name allowed", "localhost", true, false}, } diff --git a/backend/pkg/safehttp/safehttp.go b/backend/pkg/safehttp/safehttp.go index 20e347a1e..efdb6e977 100644 --- a/backend/pkg/safehttp/safehttp.go +++ b/backend/pkg/safehttp/safehttp.go @@ -49,16 +49,13 @@ func PrivateNetworkOK() Policy { // clientOptions maps the policy onto the internal client options. func (p Policy) clientOptions(timeout time.Duration) []network.Option { - opts := []network.Option{network.WithTimeout(timeout), network.WithBlockTransitionRanges()} + opts := []network.Option{network.WithTimeout(timeout)} if p.allowRFC1918 { opts = append(opts, network.WithAllowRFC1918()) } if p.allowCGNAT { opts = append(opts, network.WithAllowCGNAT()) } - if !p.allowCGNAT { - opts = append(opts, network.WithBlockCGNAT()) - } return opts } @@ -144,7 +141,7 @@ func checkLiteralHost(host string, p Policy) error { return nil } - policy := network.AddressPolicy{AllowRFC1918: p.allowRFC1918, AllowCGNAT: p.allowCGNAT, AllowTransition: false} + policy := network.AddressPolicy{AllowRFC1918: p.allowRFC1918, AllowCGNAT: p.allowCGNAT} if policy.Blocked(ip) { return fmt.Errorf("url host is not allowed: %w", ErrBlockedAddress) } @@ -192,9 +189,8 @@ func isNumericSegment(seg string) bool { // ValidateURL runs ValidateURLSyntax and then a DNS-based address check. // // It is an early-error convenience only: the dialer inside NewClient re-checks -// every address at connect time and is the authoritative control. In particular -// the early check does not apply the carrier-grade NAT or transition-range -// rules to resolved hostnames. +// every address at connect time and is the authoritative control. The early +// check applies the same address policy to every resolved address. func ValidateURL(raw string, p Policy) (*url.URL, error) { u, err := ValidateURLSyntax(raw, p) if err != nil {