diff --git a/internal/delivery/export_test.go b/internal/delivery/export_test.go index 9dd2531..98ecd4c 100644 --- a/internal/delivery/export_test.go +++ b/internal/delivery/export_test.go @@ -40,11 +40,6 @@ const ( ExportPendingSweepMinAge = pendingSweepMinAge ) -// ExportIsBlockedIP exposes isBlockedIP for testing. -func ExportIsBlockedIP(ip net.IP) bool { - return isBlockedIP(ip) -} - // NewTestGuard builds an SSRF Guard from an explicit egress // allowlist, without going through config. Passing no prefixes // yields the default guard, which blocks every private/reserved @@ -70,6 +65,11 @@ func ExportBlockedNetworks() []*net.IPNet { return blockedNetworks } +// ExportBlockedPublicNetworks exposes blockedPublicNetworks. +func ExportBlockedPublicNetworks() []*net.IPNet { + return blockedPublicNetworks +} + // ExportIsForwardableHeader exposes isForwardableHeader. func ExportIsForwardableHeader(name string) bool { return isForwardableHeader(name) diff --git a/internal/delivery/ssrf.go b/internal/delivery/ssrf.go index 547c1e6..9f5533e 100644 --- a/internal/delivery/ssrf.go +++ b/internal/delivery/ssrf.go @@ -25,8 +25,16 @@ var ( errNoIPs = errors.New( "hostname resolved to no IP addresses", ) - errBlockedIP = errors.New( - "blocked private, reserved or cloud metadata address", + // ErrBlockedPrivateOrReservedIP reports an address in the + // default blocklist's private and reserved ranges, + // blockedNetworks. + ErrBlockedPrivateOrReservedIP = errors.New( + "blocked private or reserved address", + ) + // errBlockedPublicMetadata reports a public address on the + // default blocklist, one in blockedPublicNetworks. + errBlockedPublicMetadata = errors.New( + "blocked cloud metadata address", ) errBlockedMetadata = errors.New( "blocked link-local or cloud instance metadata " + @@ -37,22 +45,32 @@ var ( ) ) -// blockedNetworks is the default blocklist: the private and -// reserved IP ranges, plus the public cloud metadata addresses, -// that are blocked to prevent SSRF attacks. An operator can -// permit specific blocks out of this set with -// ALLOWED_EGRESS_CIDRS; see Guard. +// blockedNetworks and blockedPublicNetworks together are the +// default blocklist: the private and reserved IP ranges, plus +// the public cloud metadata addresses, that are blocked to +// prevent SSRF attacks. An operator can permit specific blocks +// out of this set with ALLOWED_EGRESS_CIDRS; see Guard. // -// A public address belongs on the default blocklist only if it -// hands credentials, user data or bootstrap material to whatever -// can reach it, without the caller presenting anything. A -// provider's other public addresses are not refused, since -// reaching them can be legitimate and no list of them could be -// complete. +// blockedNetworks holds the private and reserved IP ranges. // //nolint:gochecknoglobals // package-level network list is appropriate here var blockedNetworks []*net.IPNet +// blockedPublicNetworks holds the default blocklist's public +// addresses, kept apart from blockedNetworks so that they are +// refused as cloud metadata addresses, never as private or +// reserved ones. +// +// A public address belongs on the default blocklist only if it +// hands credentials, user data or bootstrap material to whatever +// can reach it, without the caller presenting anything; it goes +// in this list. A provider's other public addresses are not +// refused, since reaching them can be legitimate and no list of +// them could be complete. +// +//nolint:gochecknoglobals // package-level network list is appropriate here +var blockedPublicNetworks []*net.IPNet + // alwaysBlockedNetworks are the ranges no configuration can // open: the link-local blocks and the cloud instance metadata // endpoints that live outside them. Reaching one is credential @@ -88,8 +106,8 @@ var blockedNetworks []*net.IPNet // when it clears both halves. Nothing in this list can be // reopened, so putting a public address here leaves the operator // no escape hatch at all — the condition ALLOWED_EGRESS_CIDRS -// exists to remove. Default-block it in blockedNetworks instead, -// which an allowlist can override. +// exists to remove. Default-block it in blockedPublicNetworks +// instead, which an allowlist can override. // // This is a criterion, not an enumeration of every metadata // address in existence. @@ -130,6 +148,9 @@ func init() { "::1/128", "fc00::/7", "fe80::/10", + }) + + blockedPublicNetworks = mustParseCIDRs([]string{ // Azure WireServer, a public address that serves VM credentials. "168.63.129.16/32", }) @@ -225,13 +246,6 @@ func matchesAny(networks []*net.IPNet, ip net.IP) bool { return false } -// isBlockedIP checks whether an IP address falls within -// the default blocklist, before any operator allowlist is -// considered. -func isBlockedIP(ip net.IP) bool { - return matchesAny(blockedNetworks, ip) -} - // Guard makes every SSRF decision in the process. // // It holds the operator's ALLOWED_EGRESS_CIDRS allowlist and @@ -332,7 +346,8 @@ func (g *Guard) allows(ip net.IP) bool { // consulted, so no configured CIDR reaches link-local or a // cloud metadata endpoint at a non-public address. // 2. The allowlist is consulted next, so a listed private -// network becomes reachable. +// network, or a listed public address on the default +// blocklist, becomes reachable. // 3. Everything else keeps the default blocklist's answer. func (g *Guard) checkIP(ip net.IP) error { if matchesAny(alwaysBlockedNetworks, ip) { @@ -345,9 +360,15 @@ func (g *Guard) checkIP(ip net.IP) error { return nil } - if isBlockedIP(ip) { + if matchesAny(blockedNetworks, ip) { return fmt.Errorf( - "target IP %s: %w", ip, errBlockedIP, + "target IP %s: %w", ip, ErrBlockedPrivateOrReservedIP, + ) + } + + if matchesAny(blockedPublicNetworks, ip) { + return fmt.Errorf( + "target IP %s: %w", ip, errBlockedPublicMetadata, ) } diff --git a/internal/delivery/ssrf_allowlist_test.go b/internal/delivery/ssrf_allowlist_test.go index 314865c..8c3dbfc 100644 --- a/internal/delivery/ssrf_allowlist_test.go +++ b/internal/delivery/ssrf_allowlist_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "net/netip" "net/url" + "slices" "testing" "time" @@ -23,6 +24,10 @@ const ( metadataIP = "169.254.169.254" metadataURL = "http://" + metadataIP + "/latest/meta-data/" + // linkLocalIPv4 is the IPv4 link-local block, which holds + // metadataIP. + linkLocalIPv4 = "169.254.0.0/16" + // loopbackHookURL is a target on this host: blocked by // default, reachable only once an operator allowlists // loopback. @@ -237,7 +242,7 @@ func linkLocalRefusedCases() []metadataAlwaysRefusedCase { }, { name: "whole link-local block", - allow: "169.254.0.0/16", + allow: linkLocalIPv4, target: metadataURL, }, { @@ -412,6 +417,9 @@ func TestGuardAllowlist_AzureWireServerReopenable(t *testing.T) { "WireServer must be refused by the default blocklist, "+ "which an allowlist can override", ) + require.NotErrorIs(t, err, delivery.ErrBlockedPrivateOrReservedIP, + "WireServer is public, not private or reserved", + ) assertDialRefused(t, defaultGuard, target) @@ -496,7 +504,7 @@ func TestAlwaysBlockedNetworks_PinnedSet(t *testing.T) { want := []string{ // IPv4 link-local: the 169.254.169.254 metadata // service on AWS, Azure and others. - "169.254.0.0/16", + linkLocalIPv4, // IPv6 link-local. "fe80::/10", // AWS IPv6 IMDS, inside the ULA space an operator may @@ -526,6 +534,76 @@ func TestAlwaysBlockedNetworks_PinnedSet(t *testing.T) { assert.Equal(t, want, got) } +// TestDefaultBlocklist_PinnedSet pins the default blocklist, its +// private and reserved ranges and its public addresses together, +// and how ALLOWED_EGRESS_CIDRS opens each entry: listing an entry +// opens it unless the unconditional set also holds it. +func TestDefaultBlocklist_PinnedSet(t *testing.T) { + t.Parallel() + + tests := []struct { + cidr string + reopenable bool + }{ + {"127.0.0.0/8", true}, + {"10.0.0.0/8", true}, + {"172.16.0.0/12", true}, + {"192.168.0.0/16", true}, + {linkLocalIPv4, false}, + {"0.0.0.0/8", true}, + {"100.64.0.0/10", true}, + {"192.0.0.0/24", true}, + {"192.0.2.0/24", true}, + {"198.18.0.0/15", true}, + {"198.51.100.0/24", true}, + {"203.0.113.0/24", true}, + {"224.0.0.0/4", true}, + {"240.0.0.0/4", true}, + {"::1/128", true}, + {"fc00::/7", true}, + {"fe80::/10", false}, + {"168.63.129.16/32", true}, + } + + want := make([]string, 0, len(tests)) + for _, tt := range tests { + want = append(want, tt.cidr) + } + + nets := slices.Concat( + delivery.ExportBlockedNetworks(), + delivery.ExportBlockedPublicNetworks(), + ) + + got := make([]string, 0, len(nets)) + for _, n := range nets { + got = append(got, n.String()) + } + + assert.ElementsMatch(t, want, got) + + for _, tt := range tests { + t.Run(tt.cidr, func(t *testing.T) { + t.Parallel() + + prefix := netip.MustParsePrefix(tt.cidr) + ip := net.IP(prefix.Addr().AsSlice()) + + require.Error(t, + delivery.NewTestGuard().ExportCheckIP(ip), + "the default guard must refuse %s", ip, + ) + + err := delivery.NewTestGuard(prefix).ExportCheckIP(ip) + if tt.reopenable { + assert.NoError(t, err, "listing %s must open it", tt.cidr) + } else { + assert.Error(t, err, "listing %s must not open it", tt.cidr) + } + }) + } +} + // requireLoopback fails the test unless rawURL's host is a // loopback address, so the allowlist test cannot silently stop // exercising a blocked range. diff --git a/internal/delivery/ssrf_test.go b/internal/delivery/ssrf_test.go index 14454e9..4e79988 100644 --- a/internal/delivery/ssrf_test.go +++ b/internal/delivery/ssrf_test.go @@ -10,7 +10,7 @@ import ( "sneak.berlin/go/webhooker/internal/delivery" ) -func TestIsBlockedIP_PrivateRanges(t *testing.T) { +func TestGuardCheckIP_PrivateRanges(t *testing.T) { t.Parallel() tests := []struct { @@ -56,12 +56,14 @@ func TestIsBlockedIP_PrivateRanges(t *testing.T) { "failed to parse IP %s", tt.ip, ) + refused := delivery.NewTestGuard().ExportCheckIP(ip) != nil + assert.Equal(t, tt.blocked, - delivery.ExportIsBlockedIP(ip), - "isBlockedIP(%s) = %v, want %v", + refused, + "default guard refuses %s = %v, want %v", tt.ip, - delivery.ExportIsBlockedIP(ip), + refused, tt.blocked, ) }) diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index 9147c1f..20f4a4c 100644 --- a/internal/handlers/source_management.go +++ b/internal/handlers/source_management.go @@ -1577,11 +1577,22 @@ func (h *Handlers) validateTargetURL( "url", delivery.MaskURL(targetURL), "error", err, ) - http.Error( - w, - "Invalid target URL: "+err.Error(), - http.StatusBadRequest, - ) + + msg := "Invalid target URL: " + err.Error() + + // Only a private or reserved address's refusal says how + // to allow it. Metadata refusals never do: link-local and + // the other unconditional metadata addresses cannot be + // opened, and the default blocklist's public addresses, + // which listing does open, hand out credentials. + if errors.Is(err, delivery.ErrBlockedPrivateOrReservedIP) { + msg += ". Private and reserved addresses are refused " + + "by default; the server's ALLOWED_EGRESS_CIDRS " + + "setting allows named networks (see \"Allowing " + + "egress to your own network\" in the README)." + } + + http.Error(w, msg, http.StatusBadRequest) return err } diff --git a/internal/handlers/target_private_refusal_test.go b/internal/handlers/target_private_refusal_test.go new file mode 100644 index 0000000..2d80731 --- /dev/null +++ b/internal/handlers/target_private_refusal_test.go @@ -0,0 +1,116 @@ +package handlers_test + +import ( + "net/http" + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/database" +) + +// privateRefusalHint is the sentence that tells an operator a private +// destination is refused on purpose, and how to allow one. +const privateRefusalHint = "Private and reserved addresses are " + + "refused by default; the server's ALLOWED_EGRESS_CIDRS setting " + + "allows named networks (see \"Allowing egress to your own " + + "network\" in the README)." + +// TestTargetRefusal_PrivateDestinationSaysHowToAllowIt covers both +// target types that take a URL, on add and on edit. +func TestTargetRefusal_PrivateDestinationSaysHowToAllowIt( + t *testing.T, +) { + t.Parallel() + + env := setupSourceTest(t) + + targetTypes := []database.TargetType{ + database.TargetTypeHTTP, + database.TargetTypeSlack, + } + + for _, targetType := range targetTypes { + t.Run(string(targetType), func(t *testing.T) { + t.Parallel() + + webhook := seedWebhookWithRetention(t, env.db, 30) + targetsPath := "/source/" + webhook.ID + "/targets" + + form := url.Values{} + form.Set("name", "private") + form.Set("type", string(targetType)) + form.Set("url", editBlockedURL) + + added := serveTarget( + env, http.MethodPost, targetsPath, form, + ) + assert.Equal(t, http.StatusBadRequest, added.Code) + assert.Contains( + t, added.Body.String(), privateRefusalHint, + ) + + form.Set("url", editOriginalURL) + + created := serveTarget( + env, http.MethodPost, targetsPath, form, + ) + require.Equal( + t, http.StatusSeeOther, created.Code, + created.Body.String(), + ) + + targets := targetsForWebhook(t, env.db, webhook.ID) + require.Len(t, targets, 1) + + form.Set("url", editBlockedURL) + + edited := submitTargetEdit( + env, webhook.ID, targets[0].ID, form, + ) + assert.Equal(t, http.StatusBadRequest, edited.Code) + assert.Contains( + t, edited.Body.String(), privateRefusalHint, + ) + }) + } +} + +// TestTargetRefusal_MetadataDestinationDoesNotSayHowToAllowIt: no +// setting opens a link-local address, and Azure's WireServer hands out +// VM credentials, so neither refusal points at the setting. +func TestTargetRefusal_MetadataDestinationDoesNotSayHowToAllowIt( + t *testing.T, +) { + t.Parallel() + + env := setupSourceTest(t) + + metadataURLs := map[string]string{ + "link-local": "http://169.254.169.254/latest/meta-data/", + "wireserver": "http://168.63.129.16/?comp=versions", + } + + for name, metadataURL := range metadataURLs { + t.Run(name, func(t *testing.T) { + t.Parallel() + + webhook := seedWebhookWithRetention(t, env.db, 30) + + form := url.Values{} + form.Set("name", "metadata") + form.Set("type", string(database.TargetTypeHTTP)) + form.Set("url", metadataURL) + + w := serveTarget( + env, http.MethodPost, + "/source/"+webhook.ID+"/targets", form, + ) + assert.Equal(t, http.StatusBadRequest, w.Code) + assert.NotContains( + t, w.Body.String(), privateRefusalHint, + ) + }) + } +}