diff --git a/internal/gateway/methods/bitrix_portals.go b/internal/gateway/methods/bitrix_portals.go index 8175dd41..3adf6a54 100644 --- a/internal/gateway/methods/bitrix_portals.go +++ b/internal/gateway/methods/bitrix_portals.go @@ -368,6 +368,11 @@ func portalRowToView(row store.BitrixPortalData) bitrixPortalView { return v } +// lookupHost is the DNS resolver used by validateSelfHostedDomain. +// Replaced in tests to avoid real network calls and to exercise multi-IP +// SSRF bypass scenarios. +var lookupHost = net.LookupHost + // validateSelfHostedDomain checks a self-hosted Bitrix24 domain for SSRF // risks and invalid port ranges. Cloud domains (*.bitrix24.*, *.bitrix.info) // are Bitrix-operated and implicitly trusted — this function is only called @@ -376,8 +381,7 @@ func portalRowToView(row store.BitrixPortalData) bitrixPortalView { // Policy: // - Rejects literal private/loopback/metadata IPs (127.x, 10.x, 192.168.x, // 169.254.x, ::1, fc00::, etc.) -// - Rejects hostnames that resolve to blocked IPs (defense against -// internal service names like "postgres", "redis", "metadata") +// - Rejects hostnames where ANY resolved IP is blocked (not just the first) // - Rejects .localhost and .local TLDs (commonly used for local dev) // - Validates port range 1-65535 when a port is specified func validateSelfHostedDomain(domain string) error { @@ -411,20 +415,24 @@ func validateSelfHostedDomain(domain string) error { return nil } - // For hostnames, resolve and check the first returned IP. - addrs, err := net.LookupHost(host) + // For hostnames, resolve and check ALL returned IPs. DNS result ordering + // is not a security boundary — a resolver may return a public IP first + // followed by a private one (e.g. split-horizon, fallback addresses). + addrs, err := lookupHost(host) if err != nil { return fmt.Errorf("cannot resolve hostname %q", host) } if len(addrs) == 0 { return fmt.Errorf("hostname %q resolved to no addresses", host) } - ip := net.ParseIP(addrs[0]) - if ip == nil { - return fmt.Errorf("resolved address %q is not a valid IP", addrs[0]) - } - if security.IsBlocked(ip) { - return fmt.Errorf("hostname %q resolved to blocked IP %s (loopback/private/metadata)", host, ip) + for _, addr := range addrs { + ip := net.ParseIP(addr) + if ip == nil { + return fmt.Errorf("resolved address %q is not a valid IP", addr) + } + if security.IsBlocked(ip) { + return fmt.Errorf("hostname %q resolved to blocked IP %s (loopback/private/metadata)", host, ip) + } } return nil } diff --git a/internal/gateway/methods/bitrix_portals_test.go b/internal/gateway/methods/bitrix_portals_test.go index a5074ad3..c5cfc76a 100644 --- a/internal/gateway/methods/bitrix_portals_test.go +++ b/internal/gateway/methods/bitrix_portals_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "strings" "sync" "testing" @@ -694,19 +695,56 @@ func TestValidateSelfHostedDomain_PortRange(t *testing.T) { } } -func TestValidateSelfHostedDomain_ValidPublicDomain(t *testing.T) { - // Public domains that resolve to public IPs should pass. - // (Note: this does a real DNS lookup — if the test environment has no - // internet, this will fail. That's acceptable; the SSRF logic is still - // tested via the blocked cases above which use literal IPs.) - valid := []string{ - "google.com", - "example.com:443", +func TestValidateSelfHostedDomain_StubbedDNS(t *testing.T) { + // Save and restore the real resolver. + orig := lookupHost + defer func() { lookupHost = orig }() + + tests := []struct { + name string + host string + addrs []string + dnsErr error + wantErr bool + }{ + {"single public IP", "public.example.com", []string{"8.8.8.8"}, nil, false}, + {"public IP with port", "public.example.com", []string{"93.184.216.34"}, nil, false}, + {"single private IP", "internal.example.com", []string{"10.0.0.1"}, nil, true}, + {"DNS resolution failure", "unresolvable.example.com", nil, fmt.Errorf("no such host"), true}, + {"no addresses returned", "empty.example.com", []string{}, nil, true}, + {"invalid IP string", "bad.example.com", []string{"not-an-ip"}, nil, true}, + // Regression: multi-IP SSRF bypass — public IP first, private IP second. + // DNS ordering is not a security boundary; must reject if ANY IP is blocked. + {"multi-ip public-then-private", "evil.example.com", []string{"8.8.8.8", "10.0.0.1"}, nil, true}, + {"multi-ip private-then-public", "evil2.example.com", []string{"192.168.1.1", "8.8.4.4"}, nil, true}, + {"multi-ip all public", "safe.example.com", []string{"8.8.8.8", "8.8.4.4"}, nil, false}, + {"multi-ip metadata IP", "meta.example.com", []string{"8.8.8.8", "169.254.169.254"}, nil, true}, + {"multi-ip IPv6 loopback", "ipv6evil.example.com", []string{"2001:db8::1", "::1"}, nil, true}, } - for _, d := range valid { - if err := validateSelfHostedDomain(d); err != nil { - t.Errorf("should accept %q, got error: %v", d, err) - } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + lookupHost = func(host string) ([]string, error) { + if host == tc.host { + return tc.addrs, tc.dnsErr + } + return nil, fmt.Errorf("unexpected host: %s", host) + } + + domain := tc.host + // Add port for the "public IP with port" case. + if tc.name == "public IP with port" { + domain = tc.host + ":443" + } + + err := validateSelfHostedDomain(domain) + if tc.wantErr && err == nil { + t.Errorf("expected error for %q, got nil", tc.name) + } + if !tc.wantErr && err != nil { + t.Errorf("expected no error for %q, got: %v", tc.name, err) + } + }) } }