mirrors/remark42
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(ssrf): apply ssrf-safe transport to TitleExtractor

Browse Source 
The image proxy got an ssrfSafeTransport in commit aca0cff3 that resolves
DNS first, blocks any IP in private/reserved CIDRs, then dials by IP to
defeat DNS rebinding. The TitleExtractor used to construct comments'
PostTitle from Locator.URL — a user-supplied field — was missed by that
fix and kept using http.DefaultTransport. The hostname allowlist there
checks the parsed URL host but never the IP it resolves to, so a domain
suffix-matching an allowed host (or 127.0.0.1 itself when AllowedHosts
is empty) reaches the metadata service or any other internal endpoint.

The same gosec rule (G704) was excluded globally in .golangci.yml as part
of aca0cff3, so this gap was not caught by the linter either.

Extract the transport into a new safehttp package so it lives in one
place and can be reused, then pass safehttp.Transport() into the
TitleExtractor's http.Client at construction (cmd/server.go). The image
proxy switches to safehttp.Transport() too — same behaviour, no longer
duplicated.

Reproduction in title_test.go uses the production-style client to hit
an httptest.Server (always 127.0.0.1) and asserts the dialer refuses
even though "127.0.0.1" is in the allowed-domains list. A control case
shows the same setup without safehttp.Transport returns the page —
making the original vulnerability explicit.
This commit is contained in:
Dmitry Verkhoturov
2026-04-18 02:32:31 -05:00
committed by Umputun
parent 5d88c1b2fa
commit ff85bbc5ea
6 changed files with 206 additions and 110 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/app/cmd/server.go
+2 -1
View File
@@ -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
backend/app/rest/proxy/image.go
+4 -73
View File
@@ -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
}
backend/app/rest/proxy/image_test.go
-36
View File
@@ -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" {
backend/app/safehttp/safehttp.go
+82
View File
@@ -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
}
backend/app/safehttp/safehttp_test.go
+78
View File
@@ -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
}
backend/app/store/service/title_test.go
+40
View File
@@ -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(`<html><title>secret</title></html>`))
}))
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)
})
}