From 39919cc832ff983ba1013c6db705cb8d7b917649 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Wed, 2 Sep 2026 21:37:19 -0500 Subject: [PATCH] appview: stop webhooks reaching private addresses The URL check accepted http:// while telling the user "must be https", and guarded no addresses at all. POST /api/webhooks with http://127.0.0.1:9/hook returned 200 and created the webhook, so both scheduled deliveries and the synchronous Test button would dial arbitrary destinations from the appview host, on demand, for any authenticated user. Loopback, link-local (including the cloud metadata endpoint at 169.254.169.254) and RFC1918 were all reachable. Enforces https, and refuses non-public destinations. The load-bearing half is the dial-time check, not the creation-time one. An attacker controls their own DNS, so a hostname that resolves publicly when the webhook is created can resolve to loopback when it is delivered, and a creation-time check cannot see a redirect either. The guard is therefore a net.Dialer Control hook on the delivery client, which inspects the resolved address on every connection attempt. Transport.Proxy is explicitly nil: honouring HTTP(S)_PROXY would route around the Control hook and hand the bypass straight back. Redirects are re-validated per hop and capped at 3. The creation-time check stays so the user gets an immediate, comprehensible error instead of a silent delivery failure later. IPv4-mapped IPv6 is unmapped before every check, so ::ffff:127.0.0.1 and friends hit the IPv4 rules. Ranges with no net.IP helper are listed explicitly: CGNAT, NAT64, ::/96, TEST-NET and reserved space. Both outbound paths are covered, since the scheduled dispatcher and the Test button both funnel through attemptDelivery. The dispatcher's other client is deliberately left unguarded: it fetches quota stats from holds, which legitimately live on private addresses, and those URLs are not user-supplied. Note this removes the ability to point a webhook at a localhost receiver in local development. There is deliberately no environment-variable escape hatch, since a security toggle read from the environment is the same bypass wearing a nicer coat. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9 --- pkg/appview/handlers/webhook_url_test.go | 125 ++++++ pkg/appview/handlers/webhooks.go | 10 +- pkg/appview/webhooks/dispatch.go | 64 +++- .../webhooks/dispatch_entitlement_test.go | 5 + pkg/appview/webhooks/dispatch_quota_test.go | 4 + pkg/appview/webhooks/ssrf.go | 236 ++++++++++++ pkg/appview/webhooks/ssrf_test.go | 358 ++++++++++++++++++ 7 files changed, 790 insertions(+), 12 deletions(-) create mode 100644 pkg/appview/handlers/webhook_url_test.go create mode 100644 pkg/appview/webhooks/ssrf.go create mode 100644 pkg/appview/webhooks/ssrf_test.go diff --git a/pkg/appview/handlers/webhook_url_test.go b/pkg/appview/handlers/webhook_url_test.go new file mode 100644 index 0000000..006b7b6 --- /dev/null +++ b/pkg/appview/handlers/webhook_url_test.go @@ -0,0 +1,125 @@ +package handlers + +import ( + "html/template" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "atcr.io/pkg/appview/db" + "atcr.io/pkg/appview/middleware" +) + +// Finding 35: the creation-time check said "must be https" but accepted +// http://, so POST /api/webhooks with http://127.0.0.1:9/hook returned 200 and +// created a webhook the appview would then dial on demand. + +// webhookTestTemplates provides the two templates the webhook handlers render. +// The bodies only need to be identifiable, not faithful. +func webhookTestTemplates(t *testing.T) *template.Template { + t.Helper() + tmpl := template.Must(template.New("alert").Parse(`ALERT:{{ .Message }}`)) + template.Must(tmpl.New("webhooks_list").Parse( + `LIST:{{ range .Webhooks }}{{ .URL }};{{ end }}`)) + return tmpl +} + +func postWebhook(t *testing.T, h *AddWebhookHandler, webhookURL string) string { + t.Helper() + form := url.Values{} + form.Set("url", webhookURL) + form.Set("trigger_push", "on") + + req := httptest.NewRequest("POST", "/api/webhooks", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req = middleware.WithUser(req, &db.User{DID: "did:plc:webhooktester", Handle: "wh.test"}) + + rr := httptest.NewRecorder() + h.ServeHTTP(rr, req) + return rr.Body.String() +} + +func TestAddWebhook_RejectsNonHTTPSAndPrivateDestinations(t *testing.T) { + database := setupTestDB(t) + defer database.Close() + + h := &AddWebhookHandler{BaseUIHandler: BaseUIHandler{ + Templates: webhookTestTemplates(t), + DB: database, + ReadOnlyDB: database, + }} + + tests := []struct { + name string + url string + wantMsg string + }{ + {"plain http loopback", "http://127.0.0.1:9/hook", "must be https"}, + {"plain http public host", "http://example.com/hook", "must be https"}, + {"no scheme", "example.com/hook", "must be https"}, + {"file scheme", "file:///etc/passwd", "must be https"}, + {"https loopback", "https://127.0.0.1:9/hook", "private or loopback"}, + {"https loopback v6", "https://[::1]/hook", "private or loopback"}, + {"https rfc1918", "https://10.0.0.1/hook", "private or loopback"}, + {"https link-local metadata", "https://169.254.169.254/latest/meta-data/", "private or loopback"}, + {"https ipv4-mapped loopback", "https://[::ffff:127.0.0.1]/hook", "private or loopback"}, + {"https localhost name", "https://localhost:9/hook", "private or loopback"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + body := postWebhook(t, h, tt.url) + if !strings.HasPrefix(body, "ALERT:") { + t.Fatalf("expected an error alert, got %q", body) + } + if !strings.Contains(body, tt.wantMsg) { + t.Errorf("error message %q does not mention %q", body, tt.wantMsg) + } + }) + } + + // Nothing above may have been stored. + stored, err := db.ListWebhooks(database, "did:plc:webhooktester") + if err != nil { + t.Fatalf("list webhooks: %v", err) + } + if len(stored) != 0 { + t.Fatalf("rejected URLs were persisted anyway: %+v", stored) + } +} + +func TestAddWebhook_AcceptsPublicHTTPS(t *testing.T) { + database := setupTestDB(t) + defer database.Close() + + h := &AddWebhookHandler{BaseUIHandler: BaseUIHandler{ + Templates: webhookTestTemplates(t), + DB: database, + ReadOnlyDB: database, + }} + + // The webhooks table has a FK on the owning user. + if err := db.InsertUserIfNotExists(database, &db.User{DID: "did:plc:webhooktester", Handle: "wh.test"}); err != nil { + t.Fatalf("seed user: %v", err) + } + + const good = "https://hooks.example.com/services/abc" + body := postWebhook(t, h, good) + if !strings.HasPrefix(body, "LIST:") { + t.Fatalf("expected the webhook list to render, got %q", body) + } + // ListWebhooks masks the path for display, so match on the host prefix. + const displayPrefix = "https://hooks.example.com/" + if !strings.Contains(body, displayPrefix) { + t.Errorf("rendered list %q does not contain the new webhook", body) + } + + stored, err := db.ListWebhooks(database, "did:plc:webhooktester") + if err != nil { + t.Fatalf("list webhooks: %v", err) + } + if len(stored) != 1 || !strings.HasPrefix(stored[0].URL, displayPrefix) { + t.Fatalf("expected the public https webhook to be stored, got %+v", stored) + } +} diff --git a/pkg/appview/handlers/webhooks.go b/pkg/appview/handlers/webhooks.go index 1b1a402..98ae55e 100644 --- a/pkg/appview/handlers/webhooks.go +++ b/pkg/appview/handlers/webhooks.go @@ -81,9 +81,13 @@ func (h *AddWebhookHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - // Validate URL scheme - if !strings.HasPrefix(webhookURL, "https://") && !strings.HasPrefix(webhookURL, "http://") { - h.renderWebhookError(w, "Invalid webhook URL: must be https") + // Validate the destination: https only, and no non-public address. This is + // the friendly half of the SSRF guard; the half that actually holds is the + // dial-time check on the delivery client (pkg/appview/webhooks/ssrf.go), + // since an attacker controlling DNS can make a hostname that looks public + // here resolve to loopback at delivery. + if err := webhooks.ValidateWebhookURL(webhookURL); err != nil { + h.renderWebhookError(w, "Invalid webhook URL: "+err.Error()) return } diff --git a/pkg/appview/webhooks/dispatch.go b/pkg/appview/webhooks/dispatch.go index 5714bab..61c6598 100644 --- a/pkg/appview/webhooks/dispatch.go +++ b/pkg/appview/webhooks/dispatch.go @@ -31,20 +31,33 @@ type WebhookLimiter func(userDID string) (maxWebhooks int, allTriggers bool) // It reads webhooks from the appview DB and delivers payloads // with Discord/Slack formatting and HMAC signing. type Dispatcher struct { - db db.DBTX - meta atproto.AppviewMetadata + db db.DBTX + meta atproto.AppviewMetadata + // httpClient talks to internal services (hold quota stats). Holds may + // legitimately live on a private address, so it is deliberately NOT the + // SSRF-guarded client. httpClient *http.Client - limits WebhookLimiter + // deliveryClient is the only client that ever dials a user-supplied webhook + // URL. Its dialer refuses non-public addresses at connect time, which is + // what stops DNS rebinding and redirect chains from reaching loopback, + // RFC1918 or the cloud metadata endpoint. + deliveryClient *http.Client + // validateURL is the destination policy applied before every attempt. Nil + // means ValidateWebhookURL. Only tests replace it, so that they can deliver + // to their own loopback httptest servers. + validateURL func(string) error + limits WebhookLimiter } // NewDispatcher creates a new webhook dispatcher. limits may be nil (treated as // unlimited), but production wires it to the billing manager's GetWebhookLimits. func NewDispatcher(database db.DBTX, meta atproto.AppviewMetadata, limits WebhookLimiter) *Dispatcher { return &Dispatcher{ - db: database, - meta: meta, - httpClient: http.DefaultClient, - limits: limits, + db: database, + meta: meta, + httpClient: http.DefaultClient, + deliveryClient: NewSafeClient(deliveryTimeout), + limits: limits, } } @@ -361,6 +374,13 @@ func (d *Dispatcher) DeliverTest(ctx context.Context, webhookID, userDID, userHa // deliverWithRetry attempts to deliver a webhook with exponential backoff func (d *Dispatcher) deliverWithRetry(webhookURL, secret string, payload []byte) { + // A destination that fails validation will never become valid, so don't + // hold a goroutine open through the backoff schedule for it. + if err := d.validateDestination(webhookURL); err != nil { + slog.Warn("Refusing webhook delivery to disallowed URL", "url", maskURL(webhookURL), "error", err) + return + } + delays := []time.Duration{0, 30 * time.Second, 2 * time.Minute, 8 * time.Minute} for attempt, delay := range delays { if attempt > 0 { @@ -373,9 +393,30 @@ func (d *Dispatcher) deliverWithRetry(webhookURL, secret string, payload []byte) slog.Warn("Webhook delivery failed after retries", "url", maskURL(webhookURL)) } +// validateDestination applies the destination policy. Tests override +// validateURL so they can deliver to their own loopback httptest servers; +// production always gets ValidateWebhookURL. +func (d *Dispatcher) validateDestination(webhookURL string) error { + if d.validateURL != nil { + return d.validateURL(webhookURL) + } + return ValidateWebhookURL(webhookURL) +} + +// deliveryTimeout bounds a single webhook POST, including redirects. +const deliveryTimeout = 10 * time.Second + // attemptDelivery sends a single webhook HTTP POST func (d *Dispatcher) attemptDelivery(webhookURL, secret string, payload []byte) bool { - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + // Re-validate on every attempt, not just at creation. Rows predating the + // guard (or written by any future path that skips the handler) are refused + // here rather than dialed. + if err := d.validateDestination(webhookURL); err != nil { + slog.Warn("Refusing webhook delivery to disallowed URL", "url", maskURL(webhookURL), "error", err) + return false + } + + ctx, cancel := context.WithTimeout(context.Background(), deliveryTimeout) defer cancel() // Reformat payload for platform-specific webhook APIs @@ -404,7 +445,12 @@ func (d *Dispatcher) attemptDelivery(webhookURL, secret string, payload []byte) req.Header.Set("X-Webhook-Signature-256", "sha256="+sig) } - client := &http.Client{Timeout: 10 * time.Second} + client := d.deliveryClient + if client == nil { + // Zero-value Dispatchers (tests) must not fall back to an unguarded + // client. + client = NewSafeClient(deliveryTimeout) + } resp, err := client.Do(req) if err != nil { slog.Warn("Webhook delivery attempt failed", "url", maskURL(webhookURL), "error", err) diff --git a/pkg/appview/webhooks/dispatch_entitlement_test.go b/pkg/appview/webhooks/dispatch_entitlement_test.go index 3c84299..e1b4743 100644 --- a/pkg/appview/webhooks/dispatch_entitlement_test.go +++ b/pkg/appview/webhooks/dispatch_entitlement_test.go @@ -62,6 +62,10 @@ func TestDispatchForScan_EntitlementGate(t *testing.T) { // Free tier: max 1 webhook, no paid triggers. The cap keeps only the oldest // webhook, and scan:all (paid) is masked out — leaving just scan:first. free := NewDispatcher(conn, meta, func(string) (int, bool) { return 1, false }) + // The delivery guard refuses http and loopback addresses; the receiver here + // is an httptest server on 127.0.0.1, so relax it for this test. The guard + // itself is covered in ssrf_test.go. + free.allowLoopbackDeliveryForTest() free.DispatchForScan(context.Background(), scanForUser(userDID), nil, "se.test", "latest", "https://hold") if !receiver.waitFor(1, 2*time.Second) { t.Fatalf("free tier: expected 1 delivery (scan:first on oldest hook), got %d", receiver.count()) @@ -75,6 +79,7 @@ func TestDispatchForScan_EntitlementGate(t *testing.T) { // scan:first AND scan:all = 4 deliveries (regression guard that the gate // doesn't over-suppress). entitled := NewDispatcher(conn, meta, func(string) (int, bool) { return -1, true }) + entitled.allowLoopbackDeliveryForTest() entitled.DispatchForScan(context.Background(), scanForUser(userDID), nil, "se.test", "latest", "https://hold") if !receiver.waitFor(1+4, 2*time.Second) { t.Fatalf("entitled: expected 4 more deliveries (2 hooks x scan:first+scan:all), total got %d", receiver.count()) diff --git a/pkg/appview/webhooks/dispatch_quota_test.go b/pkg/appview/webhooks/dispatch_quota_test.go index 4a6d950..67ac41b 100644 --- a/pkg/appview/webhooks/dispatch_quota_test.go +++ b/pkg/appview/webhooks/dispatch_quota_test.go @@ -137,6 +137,9 @@ func TestDispatchForQuotaEdgeTriggered(t *testing.T) { } d := NewDispatcher(conn, atproto.AppviewMetadata{ClientShortName: "ATCR", BaseURL: "https://atcr.test"}, nil) + // The receiver is an httptest server on loopback, which the delivery guard + // would otherwise refuse. See ssrf_test.go for the guard's own coverage. + d.allowLoopbackDeliveryForTest() event := storage.QuotaWebhookEvent{ UserDID: userDID, @@ -253,6 +256,7 @@ func TestDispatchForQuotaUnlimitedHold(t *testing.T) { } d := NewDispatcher(conn, atproto.AppviewMetadata{}, nil) + d.allowLoopbackDeliveryForTest() d.DispatchForQuota(context.Background(), storage.QuotaWebhookEvent{ UserDID: userDID, HoldDID: holdSrv.URL, HoldEndpoint: holdSrv.URL, }) diff --git a/pkg/appview/webhooks/ssrf.go b/pkg/appview/webhooks/ssrf.go new file mode 100644 index 0000000..bd6b6b4 --- /dev/null +++ b/pkg/appview/webhooks/ssrf.go @@ -0,0 +1,236 @@ +package webhooks + +import ( + "errors" + "fmt" + "net" + "net/http" + "net/url" + "strings" + "syscall" + "time" +) + +// Webhook URLs are attacker-supplied: any authenticated user can register one +// and then make the appview dial it on demand with the Test button. Without a +// destination guard that is a server-side request forgery primitive against +// everything the appview host can reach, including the cloud metadata endpoint +// at 169.254.169.254 and anything bound to loopback. +// +// The guard has two layers: +// +// 1. ValidateWebhookURL, applied at creation time and again before every +// delivery attempt. It enforces https and rejects URLs that name a +// non-public IP literal. Its real job is giving the user an immediate, +// comprehensible error instead of a mysterious delivery failure later. +// +// 2. A net.Dialer Control hook on the delivery HTTP client, which inspects the +// *resolved* address on every connection attempt. This is the layer that +// actually holds: the attacker controls DNS for their own hostname, so a +// name that resolves public at creation can resolve to 127.0.0.1 at +// delivery (DNS rebinding), and creation-time validation cannot see a +// redirect target at all. +// +// Both the scheduled dispatcher and the synchronous Test button deliver through +// attemptDelivery, which uses the guarded client, so neither path can dial a +// private address. + +// User-facing validation failures. The messages are written to be shown +// verbatim in the settings UI after an "Invalid webhook URL: " prefix. +var ( + // ErrMalformedURL means the string did not parse as an absolute URL. + ErrMalformedURL = errors.New("could not be parsed as a URL") + + // ErrSchemeNotHTTPS means the URL used something other than https. + // Webhook payloads carry repository names and HMAC signatures, so plaintext + // http is refused outright rather than merely discouraged. + ErrSchemeNotHTTPS = errors.New("must be https") + + // ErrPrivateAddress means the URL resolved to, or literally named, an + // address that is not publicly routable. + ErrPrivateAddress = errors.New("cannot point at a private or loopback address") +) + +// extraBlocked covers ranges that have no net.IP helper. Everything with a +// stdlib predicate (loopback, RFC1918 + IPv6 ULA, link-local, multicast, +// unspecified, broadcast) is handled by blockedReason instead of being +// re-derived here. +var extraBlocked = []struct { + net *net.IPNet + reason string +}{ + {mustCIDR("100.64.0.0/10"), "carrier-grade NAT address"}, + {mustCIDR("192.0.0.0/24"), "IETF protocol assignment address"}, + {mustCIDR("198.18.0.0/15"), "benchmarking address"}, + {mustCIDR("240.0.0.0/4"), "reserved address"}, + // IPv4-compatible IPv6 (::a.b.c.d). To4 does not unmap these, and + // IsLoopback on ::127.0.0.1 is false, so they need an explicit range. + {mustCIDR("::/96"), "IPv4-compatible IPv6 address"}, + {mustCIDR("64:ff9b::/96"), "NAT64 address"}, + {mustCIDR("64:ff9b:1::/48"), "local-use NAT64 address"}, + {mustCIDR("100::/64"), "discard-only address"}, + {mustCIDR("2001:db8::/32"), "documentation address"}, +} + +func mustCIDR(s string) *net.IPNet { + _, n, err := net.ParseCIDR(s) + if err != nil { + panic("webhooks: bad CIDR " + s + ": " + err.Error()) + } + return n +} + +// blockedReason reports why an address must not be dialed, or "" if it is a +// public destination. IPv4-mapped IPv6 forms (::ffff:127.0.0.1) are unmapped +// first, so every IPv4 rule below applies to them too. +func blockedReason(ip net.IP) string { + if ip == nil { + return "not a valid IP address" + } + if v4 := ip.To4(); v4 != nil { + ip = v4 + } + + switch { + case ip.IsUnspecified(): + return "unspecified address" + case ip.IsLoopback(): + // 127.0.0.0/8 and ::1 + return "loopback address" + case ip.IsPrivate(): + // 10/8, 172.16/12, 192.168/16 and IPv6 unique-local fc00::/7 + return "private address" + case ip.IsLinkLocalUnicast(): + // 169.254/16 (covers the 169.254.169.254 metadata endpoint) and fe80::/10 + return "link-local address" + case ip.IsInterfaceLocalMulticast(), ip.IsLinkLocalMulticast(), ip.IsMulticast(): + return "multicast address" + case !ip.IsGlobalUnicast(): + // Catches the IPv4 broadcast address and anything else the stdlib does + // not consider globally routable. + return "non-global address" + } + + for _, b := range extraBlocked { + if b.net.Contains(ip) { + return b.reason + } + } + return "" +} + +// ValidateWebhookURL checks a user-supplied webhook URL at rest: https scheme, +// a host, and no non-public IP literal. It does not resolve DNS. A hostname +// that resolves to a private address is caught at dial time by the Control hook +// instead, which is the only check an attacker who controls DNS cannot dodge. +func ValidateWebhookURL(raw string) error { + u, err := url.Parse(strings.TrimSpace(raw)) + if err != nil { + return ErrMalformedURL + } + if !strings.EqualFold(u.Scheme, "https") { + return ErrSchemeNotHTTPS + } + return validateParsedURL(u) +} + +// validateParsedURL is the scheme + host check applied both at creation and to +// every redirect hop. +func validateParsedURL(u *url.URL) error { + if !strings.EqualFold(u.Scheme, "https") { + return ErrSchemeNotHTTPS + } + host := u.Hostname() + if host == "" { + return ErrMalformedURL + } + if ip := net.ParseIP(host); ip != nil { + if reason := blockedReason(ip); reason != "" { + return fmt.Errorf("%w (%s is a %s)", ErrPrivateAddress, host, reason) + } + return nil + } + // Names are resolved at dial time, but reject the obvious ones up front so + // the user gets a real message instead of a delivery that just never works. + lower := strings.ToLower(host) + if lower == "localhost" || strings.HasSuffix(lower, ".localhost") { + return fmt.Errorf("%w (%s is a loopback name)", ErrPrivateAddress, host) + } + return nil +} + +// checkDialAddr inspects a resolved "host:port" address just before the socket +// is connected. This runs for every attempt, every redirect hop, and every +// address the resolver returns, which is what makes DNS rebinding ineffective. +func checkDialAddr(network, address string) error { + switch network { + case "tcp", "tcp4", "tcp6": + default: + return fmt.Errorf("%w (network %q is not allowed)", ErrPrivateAddress, network) + } + + host, _, err := net.SplitHostPort(address) + if err != nil { + host = address + } + ip := net.ParseIP(host) + if ip == nil { + // Control receives an already-resolved literal. Anything else is + // unexpected, so refuse rather than guess. + return fmt.Errorf("%w (%q did not resolve to an IP)", ErrPrivateAddress, address) + } + if reason := blockedReason(ip); reason != "" { + return fmt.Errorf("%w (%s is a %s)", ErrPrivateAddress, ip, reason) + } + return nil +} + +// safeDialControl is the net.Dialer Control hook. The signature is fixed by +// net.Dialer. +func safeDialControl(network, address string, _ syscall.RawConn) error { + return checkDialAddr(network, address) +} + +// maxRedirects bounds how far a webhook endpoint can bounce us. Each hop is +// re-validated, and the dial guard applies to every hop regardless. +const maxRedirects = 3 + +// NewSafeClient builds the HTTP client used for delivering webhook payloads to +// user-supplied URLs. It must not be reused for talking to holds or any other +// internal service, which may legitimately live on a private address. +func NewSafeClient(timeout time.Duration) *http.Client { + dialer := &net.Dialer{ + Timeout: 5 * time.Second, + KeepAlive: 30 * time.Second, + Control: safeDialControl, + } + transport := &http.Transport{ + // Deliberately no Proxy: honoring HTTP(S)_PROXY would route the request + // through a proxy that the Control hook cannot see past, handing the + // bypass straight back. + Proxy: nil, + DialContext: dialer.DialContext, + ForceAttemptHTTP2: true, + MaxIdleConns: 10, + IdleConnTimeout: 30 * time.Second, + TLSHandshakeTimeout: 10 * time.Second, + ResponseHeaderTimeout: timeout, + ExpectContinueTimeout: 1 * time.Second, + } + return &http.Client{ + Timeout: timeout, + Transport: transport, + CheckRedirect: func(req *http.Request, via []*http.Request) error { + if len(via) >= maxRedirects { + return fmt.Errorf("stopped after %d redirects", maxRedirects) + } + // A public https endpoint that 302s to http://169.254.169.254/ is + // refused here on scheme alone; the dial guard would refuse the + // address anyway. + if err := validateParsedURL(req.URL); err != nil { + return fmt.Errorf("refusing webhook redirect to %s: %w", req.URL.Redacted(), err) + } + return nil + }, + } +} diff --git a/pkg/appview/webhooks/ssrf_test.go b/pkg/appview/webhooks/ssrf_test.go new file mode 100644 index 0000000..c365958 --- /dev/null +++ b/pkg/appview/webhooks/ssrf_test.go @@ -0,0 +1,358 @@ +package webhooks + +import ( + "context" + "errors" + "net" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "atcr.io/pkg/appview/db" + "atcr.io/pkg/atproto" +) + +// Finding 35: the creation-time check claimed "must be https" while accepting +// http, and nothing anywhere looked at the destination address. Both the +// scheduled dispatcher and the synchronous Test button would dial whatever the +// user typed, including 127.0.0.1 and 169.254.169.254. + +func TestValidateWebhookURL_Scheme(t *testing.T) { + tests := []struct { + name string + url string + wantErr error + }{ + {"plain http is refused", "http://example.com/hook", ErrSchemeNotHTTPS}, + {"http to a public host is still refused", "http://93.184.216.34/hook", ErrSchemeNotHTTPS}, + {"uppercase scheme is still http", "HTTP://example.com/hook", ErrSchemeNotHTTPS}, + {"no scheme", "example.com/hook", ErrSchemeNotHTTPS}, + {"file scheme", "file:///etc/passwd", ErrSchemeNotHTTPS}, + {"gopher scheme", "gopher://example.com/", ErrSchemeNotHTTPS}, + {"https with no host", "https://", ErrMalformedURL}, + + {"public https host", "https://example.com/hook", nil}, + {"public https host with port and path", "https://hooks.slack.com:443/services/T/B/x", nil}, + {"public https IP literal", "https://93.184.216.34/hook", nil}, + {"public https IPv6 literal", "https://[2606:2800:220:1:248:1893:25c8:1946]/hook", nil}, + {"uppercase HTTPS is accepted", "HTTPS://example.com/hook", nil}, + {"leading and trailing space tolerated", " https://example.com/hook ", nil}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateWebhookURL(tt.url) + if tt.wantErr == nil { + if err != nil { + t.Fatalf("ValidateWebhookURL(%q) = %v, want nil", tt.url, err) + } + return + } + if !errors.Is(err, tt.wantErr) { + t.Fatalf("ValidateWebhookURL(%q) = %v, want %v", tt.url, err, tt.wantErr) + } + }) + } +} + +func TestValidateWebhookURL_PrivateLiterals(t *testing.T) { + urls := []string{ + "https://127.0.0.1/hook", + "https://127.1.2.3:8443/hook", + "https://[::1]/hook", + "https://10.0.0.5/hook", + "https://172.16.0.1/hook", + "https://192.168.1.1/hook", + "https://169.254.169.254/latest/meta-data/", + "https://[fe80::1]/hook", + "https://[fd00::1]/hook", + "https://[::ffff:127.0.0.1]/hook", + "https://[::ffff:169.254.169.254]/hook", + "https://0.0.0.0/hook", + "https://localhost/hook", + "https://api.localhost/hook", + } + for _, u := range urls { + t.Run(u, func(t *testing.T) { + err := ValidateWebhookURL(u) + if !errors.Is(err, ErrPrivateAddress) { + t.Fatalf("ValidateWebhookURL(%q) = %v, want ErrPrivateAddress", u, err) + } + }) + } +} + +// blockedReason is the single decision the dial guard makes. Table-driven so a +// range that stops being blocked has to come past this test. +func TestBlockedReason(t *testing.T) { + tests := []struct { + ip string + blocked bool + }{ + // Loopback + {"127.0.0.1", true}, + {"127.255.255.254", true}, + {"::1", true}, + {"::ffff:127.0.0.1", true}, // IPv4-mapped IPv6 + {"::127.0.0.1", true}, // IPv4-compatible IPv6 + + // RFC1918 + {"10.0.0.1", true}, + {"10.255.255.255", true}, + {"172.16.0.1", true}, + {"172.31.255.255", true}, + {"192.168.0.1", true}, + {"::ffff:10.0.0.1", true}, + {"::ffff:192.168.1.1", true}, + {"::ffff:172.20.0.1", true}, + + // Link-local, including the cloud metadata endpoint + {"169.254.169.254", true}, + {"169.254.0.1", true}, + {"::ffff:169.254.169.254", true}, + {"fe80::1", true}, + + // IPv6 unique-local + {"fc00::1", true}, + {"fd12:3456:789a::1", true}, + + // Unspecified / multicast / broadcast + {"0.0.0.0", true}, + {"::", true}, + {"224.0.0.1", true}, + {"239.255.255.250", true}, + {"ff02::1", true}, + {"255.255.255.255", true}, + + // Ranges with no stdlib helper + {"100.64.0.1", true}, // CGNAT + {"192.0.0.1", true}, // IETF protocol assignments + {"198.18.0.1", true}, // benchmarking + {"240.0.0.1", true}, // reserved + {"64:ff9b::1", true}, // NAT64 + {"100::1", true}, // discard-only + {"2001:db8::1", true}, // documentation + + // Public destinations must keep working + {"93.184.216.34", false}, + {"1.1.1.1", false}, + {"8.8.8.8", false}, + {"172.32.0.1", false}, // just outside 172.16/12 + {"172.15.0.1", false}, // just below 172.16/12 + {"100.63.255.255", false}, // just below CGNAT + {"100.128.0.1", false}, // just above CGNAT + {"169.253.0.1", false}, // just below link-local + {"2606:2800:220:1:248:1893:25c8:1946", false}, + {"::ffff:93.184.216.34", false}, + } + + for _, tt := range tests { + t.Run(tt.ip, func(t *testing.T) { + ip := net.ParseIP(tt.ip) + if ip == nil { + t.Fatalf("test bug: %q is not a valid IP", tt.ip) + } + reason := blockedReason(ip) + if tt.blocked && reason == "" { + t.Errorf("blockedReason(%s) allowed a non-public address", tt.ip) + } + if !tt.blocked && reason != "" { + t.Errorf("blockedReason(%s) blocked a public address: %s", tt.ip, reason) + } + }) + } +} + +func TestBlockedReason_NilIP(t *testing.T) { + if blockedReason(nil) == "" { + t.Error("blockedReason(nil) must not report the address as allowed") + } +} + +// checkDialAddr is what the Control hook runs on the resolved address, so it is +// the check DNS rebinding has to get past. +func TestCheckDialAddr(t *testing.T) { + tests := []struct { + name string + network string + address string + blocked bool + }{ + {"loopback v4", "tcp", "127.0.0.1:80", true}, + {"loopback v6", "tcp", "[::1]:443", true}, + {"rfc1918", "tcp4", "10.1.2.3:443", true}, + {"link-local metadata", "tcp4", "169.254.169.254:80", true}, + {"ipv4-mapped loopback", "tcp6", "[::ffff:127.0.0.1]:443", true}, + {"ipv4-mapped rfc1918", "tcp6", "[::ffff:192.168.0.9]:443", true}, + {"unix socket network", "unix", "/var/run/docker.sock", true}, + {"unresolved name", "tcp", "example.com:443", true}, + {"public v4", "tcp", "93.184.216.34:443", false}, + {"public v6", "tcp6", "[2606:2800:220:1:248:1893:25c8:1946]:443", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := checkDialAddr(tt.network, tt.address) + if tt.blocked && err == nil { + t.Errorf("checkDialAddr(%q, %q) allowed the connection", tt.network, tt.address) + } + if !tt.blocked && err != nil { + t.Errorf("checkDialAddr(%q, %q) = %v, want nil", tt.network, tt.address, err) + } + }) + } +} + +// End to end: the guarded client must refuse a loopback server even when the +// URL is handed to it directly, which is what the Test button effectively does. +func TestSafeClient_RefusesLoopbackServer(t *testing.T) { + reached := false + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + client := NewSafeClient(5 * time.Second) + resp, err := client.Get(srv.URL) //nolint:noctx // short-lived guard check + if err == nil { + resp.Body.Close() + t.Fatal("guarded client connected to a loopback server") + } + if reached { + t.Fatal("request reached the loopback server despite the dial guard") + } + if !strings.Contains(err.Error(), ErrPrivateAddress.Error()) { + t.Errorf("error %v does not mention the private-address refusal", err) + } +} + +// A public endpoint that redirects to the metadata address must not be +// followed. The redirect target is checked before the hop is made, and the dial +// guard would refuse the address anyway. +func TestSafeClient_RefusesRedirectToPrivate(t *testing.T) { + err := redirectCheck(t, "http://169.254.169.254/latest/meta-data/") + if err == nil { + t.Fatal("CheckRedirect followed a redirect to the metadata endpoint") + } + if !errors.Is(err, ErrSchemeNotHTTPS) && !errors.Is(err, ErrPrivateAddress) { + t.Errorf("unexpected refusal reason: %v", err) + } + + err = redirectCheck(t, "https://127.0.0.1/hook") + if !errors.Is(err, ErrPrivateAddress) { + t.Errorf("https redirect to loopback: got %v, want ErrPrivateAddress", err) + } + + if err := redirectCheck(t, "https://example.org/next"); err != nil { + t.Errorf("redirect to a public https URL was refused: %v", err) + } +} + +// redirectCheck exercises the client's CheckRedirect hook directly; dialing a +// real redirect chain would need network access. +func redirectCheck(t *testing.T, target string) error { + t.Helper() + client := NewSafeClient(5 * time.Second) + req, err := http.NewRequest(http.MethodGet, target, nil) //nolint:noctx // not sent + if err != nil { + t.Fatalf("building request: %v", err) + } + via, err := http.NewRequest(http.MethodPost, "https://example.com/hook", nil) //nolint:noctx // not sent + if err != nil { + t.Fatalf("building via request: %v", err) + } + return client.CheckRedirect(req, []*http.Request{via}) +} + +func TestSafeClient_StopsRedirectLoop(t *testing.T) { + client := NewSafeClient(5 * time.Second) + req, err := http.NewRequest(http.MethodGet, "https://example.com/hook", nil) //nolint:noctx // not sent + if err != nil { + t.Fatalf("building request: %v", err) + } + via := make([]*http.Request, maxRedirects) + for i := range via { + via[i] = req + } + if err := client.CheckRedirect(req, via); err == nil { + t.Errorf("CheckRedirect allowed hop %d, want a stop after %d", len(via)+1, maxRedirects) + } +} + +// The delivery path re-validates every attempt, so a row written before the +// guard (or by any path that skips the handler) is refused instead of dialed. +func TestAttemptDelivery_RefusesDisallowedURLs(t *testing.T) { + reached := false + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + d := &Dispatcher{deliveryClient: NewSafeClient(5 * time.Second)} + + for _, u := range []string{srv.URL, "http://example.com/hook", "https://127.0.0.1:9/hook"} { + if d.attemptDelivery(u, "", []byte(`{}`)) { + t.Errorf("attemptDelivery(%q) reported success", u) + } + } + if reached { + t.Fatal("a delivery attempt reached the loopback server") + } +} + +// allowLoopbackDeliveryForTest relaxes the destination guard so a test can +// deliver to its own httptest server. It exists only in the test build: there +// is no way to reach it from production code. +func (d *Dispatcher) allowLoopbackDeliveryForTest() { + d.validateURL = func(string) error { return nil } + d.deliveryClient = &http.Client{Timeout: deliveryTimeout} +} + +// The Test button is the more dangerous of the two delivery paths: it is +// immediate and attacker-timed. DeliverTest must go through the same guard as +// the scheduled dispatcher. +func TestDeliverTest_RefusesLoopbackWebhook(t *testing.T) { + conn, err := db.InitDB(t.TempDir()+"/test.db", db.LibsqlConfig{}) + if err != nil { + t.Fatalf("init db: %v", err) + } + defer conn.Close() + + const userDID = "did:plc:ssrftester" + if err := db.UpsertUser(conn, &db.User{ + DID: userDID, Handle: "ssrf.test", PDSEndpoint: "https://pds", LastSeen: time.Now(), + }); err != nil { + t.Fatalf("upsert user: %v", err) + } + + reached := false + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + // A row created before the guard existed, pointing at loopback over http. + if err := db.InsertWebhook(conn, &db.Webhook{ + ID: "wh-loopback", UserDID: userDID, URL: srv.URL, + Triggers: PackTriggers(TriggerFirst, 0), CreatedAt: time.Now().UTC(), + }); err != nil { + t.Fatalf("insert webhook: %v", err) + } + + d := NewDispatcher(conn, atproto.AppviewMetadata{ClientShortName: "ATCR"}, nil) + ok, err := d.DeliverTest(context.Background(), "wh-loopback", userDID, "ssrf.test") + if err != nil { + t.Fatalf("DeliverTest returned an error: %v", err) + } + if ok { + t.Error("DeliverTest reported success for a loopback destination") + } + if reached { + t.Error("the Test button reached a loopback server") + } +}