From 070a32717fbbaa722518b0a7530628568664dc9a Mon Sep 17 00:00:00 2001 From: Vassiliy Yegorov Date: Sat, 4 Jul 2026 13:48:22 +0700 Subject: [PATCH] =?UTF-8?q?fix(sec):=20webhook=20SSRF-guard=20=D1=87=D0=B5?= =?UTF-8?q?=D1=80=D0=B5=D0=B7=20Dialer.Control=20(=D0=B7=D0=B0=D0=BA=D1=80?= =?UTF-8?q?=D1=8B=D1=82=D0=B8=D0=B5=20DNS-rebinding=20TOCTOU)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- internal/notify/dispatch.go | 5 +- internal/notify/notify_test.go | 71 +++++++++++++++++++++++++++++ internal/notify/webhook.go | 83 ++++++++++++++++++++++++++++++---- 3 files changed, 148 insertions(+), 11 deletions(-) diff --git a/internal/notify/dispatch.go b/internal/notify/dispatch.go index 3d2329e..c8d76db 100644 --- a/internal/notify/dispatch.go +++ b/internal/notify/dispatch.go @@ -39,7 +39,10 @@ func NewDispatcher(store ChannelStore, cipher Decryptor) *Dispatcher { cipher: cipher, byType: map[string]Notifier{ "telegram": &Telegram{BaseURL: "https://api.telegram.org", HTTP: &http.Client{Timeout: 15 * time.Second}}, - "webhook": &Webhook{HTTP: &http.Client{Timeout: 15 * time.Second}}, + "webhook": &Webhook{HTTP: &http.Client{ + Timeout: 15 * time.Second, + Transport: newWebhookTransport(false), + }}, }, } } diff --git a/internal/notify/notify_test.go b/internal/notify/notify_test.go index 329bac4..9c1126d 100644 --- a/internal/notify/notify_test.go +++ b/internal/notify/notify_test.go @@ -165,6 +165,77 @@ func TestIsAllowedURL(t *testing.T) { } } +func TestDialControlBlocksActualConnectingAddress(t *testing.T) { + cases := []struct { + name string + address string + blocked bool + }{ + {"loopback v4", "127.0.0.1:80", true}, + {"loopback v6", "[::1]:80", true}, + {"metadata link-local", "169.254.169.254:80", true}, + {"private class a", "10.0.0.1:80", true}, + {"private class b", "172.16.0.1:80", true}, + {"private class c", "192.168.1.1:80", true}, + {"unspecified", "0.0.0.0:80", true}, + {"multicast", "224.0.0.1:80", true}, + {"public ip", "93.184.216.34:443", false}, + } + control := dialControl(false) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := control("tcp", tc.address, nil) + if tc.blocked && err == nil { + t.Fatalf("expected %q to be blocked", tc.address) + } + if !tc.blocked && err != nil { + t.Fatalf("expected %q to be allowed, got: %v", tc.address, err) + } + }) + } +} + +func TestDialControlAllowsEverythingWhenAllowPrivate(t *testing.T) { + control := dialControl(true) + if err := control("tcp", "127.0.0.1:80", nil); err != nil { + t.Fatalf("expected allowPrivate to skip the dial guard, got: %v", err) + } +} + +// TestWebhookControlBlocksConnectionEvenWhenPreCheckPasses simulates the +// DNS-rebinding TOCTOU: allowPrivate=true skips the pre-request isAllowedURL +// check (standing in for a rebinding attacker answering a public IP to that +// lookup), but the Transport's Control func — wired independently of +// Webhook.allowPrivate — still inspects the literal address the dialer +// connects to and must still reject it. If Control did not exist, this +// request would reach the httptest handler; it must not. +func TestWebhookControlBlocksConnectionEvenWhenPreCheckPasses(t *testing.T) { + var handlerCalled bool + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + handlerCalled = true + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + wh := &Webhook{ + HTTP: &http.Client{Transport: newWebhookTransport(false)}, + allowPrivate: true, // pre-check bypassed on purpose; Control is not + } + ev := Event{Project: "proj", Domain: "example.com", Status: "drift", Summary: "x", At: time.Now()} + cfg, _ := json.Marshal(map[string]string{"url": srv.URL}) + + err := wh.Send(context.Background(), cfg, "", ev) + if err == nil { + t.Fatal("expected error: Control should have blocked the loopback connection") + } + if !strings.Contains(err.Error(), "destination not allowed") { + t.Fatalf("unexpected error: %v", err) + } + if handlerCalled { + t.Fatal("Control should have rejected the dial before the handler ran") + } +} + // --- Dispatcher --- type mockChannelStore struct { diff --git a/internal/notify/webhook.go b/internal/notify/webhook.go index 1f86e23..d15a5ea 100644 --- a/internal/notify/webhook.go +++ b/internal/notify/webhook.go @@ -9,6 +9,8 @@ import ( "net" "net/http" "net/url" + "syscall" + "time" ) // Webhook delivers notifications as a JSON POST of the Event to a @@ -16,17 +18,30 @@ import ( // unused (reserved for future request signing) and is never logged. // // The destination URL is project-controlled (any project owner can set it), -// so it is treated as untrusted input: isAllowedURL blocks requests to -// loopback/private/link-local/unspecified addresses to prevent SSRF against -// internal services and cloud metadata endpoints (e.g. 169.254.169.254). -// Redirects are not followed, since a redirect response could otherwise be -// used to bypass the destination check. +// so it is treated as untrusted input. Two layers guard against SSRF: +// +// 1. isAllowedURL is a pre-request fast-fail check on the URL's scheme and +// (resolved) hostname. +// 2. HTTP's Transport, when built via newWebhookTransport, wires a +// net.Dialer.Control that re-checks the actual "ip:port" being dialed for +// every connection net/http opens — including the DNS resolution +// http.Client.Do performs internally, independent of (1). +// +// Layer (2) is the source of truth: DNS answers are attacker-influenceable +// (an attacker with authoritative DNS and a low TTL can answer a public IP to +// a pre-request lookup and a private/loopback IP to the actual connection — +// DNS rebinding). Relying on (1) alone leaves that TOCTOU window open; (2) +// closes it because it inspects the address the connection is actually made +// to, not a name. Redirects are not followed, since a redirect response +// could otherwise be used to bypass the destination checks. type Webhook struct { HTTP *http.Client - // allowPrivate disables the SSRF guard. It exists only so tests can - // exercise Send happy-paths against httptest servers, which listen on - // loopback. Production Dispatchers (NewDispatcher) must never set this. + // allowPrivate disables the isAllowedURL pre-check. It exists only so + // tests can exercise Send happy-paths against httptest servers, which + // listen on loopback. Production Dispatchers (NewDispatcher) must never + // set this; they also wire a Transport whose Control func enforces the + // same guard at dial time regardless of this flag. allowPrivate bool } @@ -61,14 +76,18 @@ func isAllowedURL(rawurl string) error { } for _, ip := range ips { - if isDisallowedIP(ip) { + if isBlockedIP(ip) { return errors.New("webhook: destination not allowed") } } return nil } -func isDisallowedIP(ip net.IP) bool { +// isBlockedIP reports whether ip must never be connected to: loopback, +// private (RFC1918 etc.), link-local, unspecified, or multicast. Used both +// by isAllowedURL's pre-request check and by dialControl's per-connection +// check. +func isBlockedIP(ip net.IP) bool { return ip.IsLoopback() || ip.IsPrivate() || ip.IsLinkLocalUnicast() || @@ -77,6 +96,50 @@ func isDisallowedIP(ip net.IP) bool { ip.IsMulticast() } +// dialControl returns a net.Dialer.Control function enforcing the SSRF guard +// on the literal address ("ip:port") that net/http is about to connect to. +// It runs after any DNS resolution net/http performs internally — including +// resolution done independently of, and possibly later than, isAllowedURL's +// own lookup — so it sees the real connecting IP and closes the DNS-rebinding +// TOCTOU window described on Webhook. +// +// allowPrivate disables the check entirely; it exists so tests can dial +// httptest servers, which listen on loopback. +func dialControl(allowPrivate bool) func(network, address string, c syscall.RawConn) error { + return func(network, address string, c syscall.RawConn) error { + if allowPrivate { + return nil + } + host, _, err := net.SplitHostPort(address) + if err != nil { + return errors.New("webhook: destination not allowed") + } + ip := net.ParseIP(host) + if ip == nil { + return errors.New("webhook: destination not allowed") + } + if isBlockedIP(ip) { + return errors.New("webhook: destination not allowed") + } + return nil + } +} + +// newWebhookTransport builds an http.Transport whose dialer enforces the +// SSRF guard on the actual address being connected to, for every connection +// it opens (see dialControl). This is the guard of record; isAllowedURL is +// only a fast pre-request rejection layered in front of it. +func newWebhookTransport(allowPrivate bool) *http.Transport { + dialer := &net.Dialer{ + Timeout: 30 * time.Second, + KeepAlive: 30 * time.Second, + Control: dialControl(allowPrivate), + } + t := http.DefaultTransport.(*http.Transport).Clone() + t.DialContext = dialer.DialContext + return t +} + func (w *Webhook) Send(ctx context.Context, cfg json.RawMessage, secret string, ev Event) error { var c struct { URL string `json:"url"`