diff --git a/backend/app/cmd/server.go b/backend/app/cmd/server.go index 9b1aebb7..c2e7c475 100644 --- a/backend/app/cmd/server.go +++ b/backend/app/cmd/server.go @@ -39,6 +39,7 @@ import ( "github.com/umputun/remark42/backend/app/providers" "github.com/umputun/remark42/backend/app/rest/api" "github.com/umputun/remark42/backend/app/rest/proxy" + "github.com/umputun/remark42/backend/app/safehttp" "github.com/umputun/remark42/backend/app/store" "github.com/umputun/remark42/backend/app/store/admin" "github.com/umputun/remark42/backend/app/store/engine" @@ -621,7 +622,7 @@ func (s *ServerCommand) newServerApp(ctx context.Context) (*serverApp, error) { MaxVotes: s.MaxVotes, PositiveScore: s.PositiveScore, ImageService: imageService, - TitleExtractor: service.NewTitleExtractor(http.Client{Timeout: time.Second * 5}, s.getAllowedDomains()), + TitleExtractor: service.NewTitleExtractor(http.Client{Timeout: time.Second * 5, Transport: safehttp.Transport()}, s.getAllowedDomains()), RestrictedWordsMatcher: service.NewRestrictedWordsMatcher(service.StaticRestrictedWordsLister{Words: s.RestrictedWords}), } dataService.RestrictSameIPVotes.Enabled = s.RestrictVoteIP diff --git a/backend/app/rest/proxy/image.go b/backend/app/rest/proxy/image.go index 38bf2f7c..2589e099 100644 --- a/backend/app/rest/proxy/image.go +++ b/backend/app/rest/proxy/image.go @@ -6,7 +6,6 @@ import ( "encoding/base64" "fmt" "io" - "net" "net/http" "strings" "time" @@ -16,6 +15,7 @@ import ( "github.com/go-pkgz/repeater/v2" "github.com/umputun/remark42/backend/app/rest" + "github.com/umputun/remark42/backend/app/safehttp" "github.com/umputun/remark42/backend/app/store/image" ) @@ -153,7 +153,7 @@ func (p Image) downloadImage(ctx context.Context, imgURL string) ([]byte, error) transport := p.Transport if transport == nil { - transport = ssrfSafeTransport() + transport = safehttp.Transport() } client := http.Client{ Timeout: 30 * time.Second, @@ -163,11 +163,11 @@ func (p Image) downloadImage(ctx context.Context, imgURL string) ([]byte, error) var resp *http.Response err := repeater.NewFixed(5, time.Second).Do(ctx, func() error { var e error - req, e := http.NewRequest("GET", imgURL, http.NoBody) + req, e := http.NewRequest("GET", imgURL, http.NoBody) //nolint:gosec // SSRF mitigated by safehttp.Transport assigned above if e != nil { return fmt.Errorf("failed to make request for %s: %w", imgURL, e) } - resp, e = client.Do(req.WithContext(ctx)) //nolint:bodyclose,gosec // body closed in defer; SSRF mitigated by ssrfSafeTransport + resp, e = client.Do(req.WithContext(ctx)) //nolint:bodyclose,gosec // body closed in defer; SSRF mitigated by safehttp.Transport return e }) if err != nil { @@ -198,72 +198,3 @@ func (p Image) downloadImage(ctx context.Context, imgURL string) ([]byte, error) } return imgData, nil } - -// ssrfSafeTransport returns an http.Transport with a dialer that blocks connections to private IP addresses. -// it resolves the host, validates all IPs, then dials using the resolved IP to prevent DNS rebinding attacks. -// tries each resolved IP in order to handle dual-stack hosts where the first IP may be unreachable. -func ssrfSafeTransport() *http.Transport { - dialer := &net.Dialer{Timeout: 30 * time.Second} - return &http.Transport{ - DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { - host, port, err := net.SplitHostPort(addr) - if err != nil { - return nil, fmt.Errorf("invalid address %s: %w", addr, err) - } - - // resolve the host to IP addresses - ips, err := net.DefaultResolver.LookupIPAddr(ctx, host) - if err != nil { - return nil, fmt.Errorf("can't resolve host %s: %w", host, err) - } - if len(ips) == 0 { - return nil, fmt.Errorf("no IP addresses resolved for host %s", host) - } - - for _, ip := range ips { - if isPrivateIP(ip.IP) { - return nil, fmt.Errorf("access to private address is not allowed") - } - } - - // try each resolved IP to handle dual-stack hosts where some IPs may be unreachable - var lastErr error - for _, ip := range ips { - conn, dialErr := dialer.DialContext(ctx, network, net.JoinHostPort(ip.String(), port)) - if dialErr == nil { - return conn, nil - } - lastErr = dialErr - } - return nil, fmt.Errorf("can't connect to %s: %w", host, lastErr) - }, - } -} - -// privateCIDRs holds pre-parsed private/reserved CIDR blocks for SSRF protection. -var privateCIDRs = func() []*net.IPNet { - cidrs := []string{ - "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16", - "100.64.0.0/10", "127.0.0.0/8", "169.254.0.0/16", - "::1/128", "fc00::/7", "fe80::/10", - } - blocks := make([]*net.IPNet, 0, len(cidrs)) - for _, cidr := range cidrs { - _, block, _ := net.ParseCIDR(cidr) - blocks = append(blocks, block) - } - return blocks -}() - -// isPrivateIP checks if the given IP belongs to a private/reserved range. -func isPrivateIP(ip net.IP) bool { - if ip.IsUnspecified() { - return true - } - for _, block := range privateCIDRs { - if block.Contains(ip) { - return true - } - } - return false -} diff --git a/backend/app/rest/proxy/image_test.go b/backend/app/rest/proxy/image_test.go index 69f10dec..ed178433 100644 --- a/backend/app/rest/proxy/image_test.go +++ b/backend/app/rest/proxy/image_test.go @@ -4,7 +4,6 @@ import ( "encoding/base64" "fmt" "io" - "net" "net/http" "net/http/httptest" "strconv" @@ -421,41 +420,6 @@ func TestImage_ResponseSizeLimit(t *testing.T) { assert.Contains(t, string(b), "failed to fetch") } -func TestIsPrivateIP(t *testing.T) { - tbl := []struct { - ip string - private bool - }{ - {"127.0.0.1", true}, - {"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}, - {"192.168.255.255", true}, - {"169.254.1.1", true}, - {"100.64.0.1", true}, - {"100.127.255.255", true}, - {"::1", true}, - {"fc00::1", true}, - {"fe80::1", true}, - {"0.0.0.0", true}, - {"::", true}, - {"8.8.8.8", false}, - {"203.0.113.1", false}, - {"1.1.1.1", false}, - {"2001:db8::1", false}, - } - - for _, tt := range tbl { - t.Run(tt.ip, func(t *testing.T) { - ip := net.ParseIP(tt.ip) - require.NotNil(t, ip) - assert.Equal(t, tt.private, isPrivateIP(ip)) - }) - } -} - func imgHTTPTestsServer(t *testing.T) *httptest.Server { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/image/img1.png" { diff --git a/backend/app/safehttp/safehttp.go b/backend/app/safehttp/safehttp.go new file mode 100644 index 00000000..faf4adc5 --- /dev/null +++ b/backend/app/safehttp/safehttp.go @@ -0,0 +1,82 @@ +// Package safehttp provides HTTP transports hardened against SSRF: outbound +// connections are dialed using a pre-resolved IP, with a check that all +// resolved IPs sit outside private/reserved ranges. This blocks both naive +// SSRF (private IP literals in user-supplied URLs) and DNS rebinding. +package safehttp + +import ( + "context" + "fmt" + "net" + "net/http" + "time" +) + +// Transport returns an *http.Transport whose DialContext refuses any address +// that resolves to a private/reserved IP, choosing the IP itself for the dial +// to defeat DNS rebinding (an attacker cannot have the resolver hand back a +// public IP at the check and a private one at the connect). +func Transport() *http.Transport { + dialer := &net.Dialer{Timeout: 30 * time.Second} + return &http.Transport{ + DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { + host, port, err := net.SplitHostPort(addr) + if err != nil { + return nil, fmt.Errorf("invalid address %s: %w", addr, err) + } + + ips, err := net.DefaultResolver.LookupIPAddr(ctx, host) + if err != nil { + return nil, fmt.Errorf("can't resolve host %s: %w", host, err) + } + if len(ips) == 0 { + return nil, fmt.Errorf("no IP addresses resolved for host %s", host) + } + + for _, ip := range ips { + if IsPrivateIP(ip.IP) { + return nil, fmt.Errorf("access to private address is not allowed") + } + } + + var lastErr error + for _, ip := range ips { + conn, dialErr := dialer.DialContext(ctx, network, net.JoinHostPort(ip.String(), port)) + if dialErr == nil { + return conn, nil + } + lastErr = dialErr + } + return nil, fmt.Errorf("can't connect to %s: %w", host, lastErr) + }, + } +} + +// privateCIDRs holds pre-parsed private/reserved CIDR blocks. +var privateCIDRs = func() []*net.IPNet { + cidrs := []string{ + "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16", + "100.64.0.0/10", "127.0.0.0/8", "169.254.0.0/16", + "::1/128", "fc00::/7", "fe80::/10", + } + blocks := make([]*net.IPNet, 0, len(cidrs)) + for _, cidr := range cidrs { + _, block, _ := net.ParseCIDR(cidr) + blocks = append(blocks, block) + } + return blocks +}() + +// IsPrivateIP reports whether ip falls in any private, loopback, link-local, +// CGNAT, or reserved range — including IPv4 and IPv6 unspecified addresses. +func IsPrivateIP(ip net.IP) bool { + if ip.IsUnspecified() { + return true + } + for _, block := range privateCIDRs { + if block.Contains(ip) { + return true + } + } + return false +} diff --git a/backend/app/safehttp/safehttp_test.go b/backend/app/safehttp/safehttp_test.go new file mode 100644 index 00000000..b3fab00e --- /dev/null +++ b/backend/app/safehttp/safehttp_test.go @@ -0,0 +1,78 @@ +package safehttp + +import ( + "context" + "net" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestIsPrivateIP(t *testing.T) { + tbl := []struct { + ip string + private bool + }{ + {"127.0.0.1", true}, + {"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}, + {"192.168.255.255", true}, + {"169.254.1.1", true}, + {"100.64.0.1", true}, + {"100.127.255.255", true}, + {"::1", true}, + {"fc00::1", true}, + {"fe80::1", true}, + {"0.0.0.0", true}, + {"::", true}, + {"8.8.8.8", false}, + {"203.0.113.1", false}, + {"1.1.1.1", false}, + {"2001:db8::1", false}, + } + + for _, tt := range tbl { + t.Run(tt.ip, func(t *testing.T) { + ip := net.ParseIP(tt.ip) + require.NotNil(t, ip) + assert.Equal(t, tt.private, IsPrivateIP(ip)) + }) + } +} + +func TestTransport_BlocksPrivate(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + client := &http.Client{Transport: Transport(), Timeout: 2 * time.Second} + resp, err := client.Get(srv.URL) // httptest.NewServer binds 127.0.0.1 + if resp != nil { + _ = resp.Body.Close() + } + require.Error(t, err, "private address must be refused") + assert.Contains(t, err.Error(), "access to private address is not allowed") +} + +func TestTransport_AllowsPublic(t *testing.T) { + dialer := &net.Dialer{Timeout: 2 * time.Second} + tr := Transport() + // monkey-check the Dialer wiring directly: the transport must reject 127.0.0.1 + _, err := tr.DialContext(context.Background(), "tcp", "127.0.0.1:1") + require.Error(t, err) + assert.Contains(t, err.Error(), "access to private address is not allowed") + + // public IP literal goes through the dial path (will likely error on connect, but NOT on policy) + _, err = tr.DialContext(context.Background(), "tcp", "203.0.113.1:1") + require.Error(t, err) + assert.NotContains(t, err.Error(), "access to private address is not allowed") + _ = dialer +} diff --git a/backend/app/store/service/title_test.go b/backend/app/store/service/title_test.go index 3be3de1a..085ec8e0 100644 --- a/backend/app/store/service/title_test.go +++ b/backend/app/store/service/title_test.go @@ -14,6 +14,8 @@ import ( "github.com/go-pkgz/syncs" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/umputun/remark42/backend/app/safehttp" ) func TestTitle_GetTitle(t *testing.T) { @@ -128,3 +130,41 @@ func TestTitle_DoubleClosed(t *testing.T) { // second call should not result in panic assert.NoError(t, ex.Close()) } + +// TestTitle_GetBlocksPrivateIPViaSafeTransport reproduces the SSRF in TitleExtractor. +// In production (cmd/server.go) the TitleExtractor receives the comment's Locator.URL +// straight from the user JSON body. The domain allowlist alone is not enough — a +// hostname suffix-matching an allowed domain can resolve to a private IP (DNS rebinding) +// or an attacker can list 127.0.0.1 directly when AllowedHosts is empty. +// +// The fix is to wrap the http.Client with safehttp.Transport at construction time, +// matching what the image proxy already does. This test asserts the safehttp transport +// is honored by the title fetcher: even though "127.0.0.1" is in the allowed-domains +// list, the dialer refuses to connect to a private address. +// +// As a control, the second sub-test shows the same setup WITHOUT safehttp.Transport +// happily fetches the page — demonstrating the original vulnerability. +func TestTitle_GetBlocksPrivateIPViaSafeTransport(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`secret`)) + })) + defer ts.Close() + + t.Run("with safehttp transport: blocked", func(t *testing.T) { + client := http.Client{Timeout: 2 * time.Second, Transport: safehttp.Transport()} + ex := NewTitleExtractor(client, []string{"127.0.0.1"}) + defer ex.Close() + _, err := ex.Get(ts.URL) + require.Error(t, err) + assert.Contains(t, err.Error(), "access to private address is not allowed") + }) + + t.Run("control: default transport leaks", func(t *testing.T) { + client := http.Client{Timeout: 2 * time.Second} // no safehttp.Transport — vulnerable + ex := NewTitleExtractor(client, []string{"127.0.0.1"}) + defer ex.Close() + title, err := ex.Get(ts.URL) + require.NoError(t, err, "without safehttp.Transport the SSRF succeeds — this is the bug") + assert.Equal(t, "secret", title) + }) +}