Skip to content

Commit 07e4f9b

Browse files
authored
Merge pull request #3540 from dgageot/worktree-board-87979f1e33071010
fix(ssrf): avoid DNS resolution at construction time in NewSSRFSafeTransport
2 parents 769869b + 76e5cf5 commit 07e4f9b

3 files changed

Lines changed: 86 additions & 70 deletions

File tree

lint/constructor_network_io.go

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,20 +3,22 @@ package main
33
import (
44
"go/ast"
55
"go/types"
6+
"slices"
67

78
"github.com/dgageot/rubocop-go/cop"
89
)
910

1011
// ConstructorNetworkIO enforces that constructors do not perform network I/O.
1112
//
12-
// Constructors should assemble state and return it. Dialing, listening, or
13-
// issuing HTTP requests from New* hides network side effects before the caller
14-
// can decide when to connect, arrange cancellation, or surface failures from an
15-
// explicit operation.
13+
// Constructors should assemble state and return it. Dialing, listening,
14+
// issuing HTTP requests, or resolving DNS names from New* hides network side
15+
// effects before the caller can decide when to connect, arrange cancellation,
16+
// or surface failures from an explicit operation.
1617
//
17-
// Detection is intentionally low-noise: only direct calls to selected package
18-
// functions in net and net/http are flagged. Method calls such as .Do and
19-
// .Accept are out of scope for now.
18+
// Detection is intentionally low-noise: only calls to selected functions and
19+
// methods in net and net/http are flagged (net.Dial*/Listen*/Lookup*,
20+
// Resolver.Lookup*, http.Get/Head/Post/PostForm). Method calls such as .Do
21+
// and .Accept are out of scope for now.
2022
//
2123
// Calls inside nested function literals are ignored unless the literal is
2224
// immediately invoked as part of constructor execution.
@@ -52,7 +54,7 @@ func networkIOCall(p *cop.Pass, call *ast.CallExpr) (string, string, bool) {
5254
return pkg, name, true
5355
}
5456

55-
if name, ok := cop.CallTo(call, "net", "Dial", "DialTimeout", "Listen", "ListenPacket", "ListenTCP", "ListenUDP", "ListenUnix"); ok {
57+
if name, ok := cop.CallTo(call, "net", netIOFuncNames...); ok {
5658
return "net", name, true
5759
}
5860
if name, ok := cop.CallTo(call, "http", "Get", "Head", "Post", "PostForm"); ok {
@@ -61,6 +63,15 @@ func networkIOCall(p *cop.Pass, call *ast.CallExpr) (string, string, bool) {
6163
return "", "", false
6264
}
6365

66+
// netIOFuncNames are the functions (and Resolver methods, matched by the
67+
// type-based path) in package net that perform network I/O: dialing,
68+
// listening, and DNS resolution.
69+
var netIOFuncNames = []string{
70+
"Dial", "DialTimeout", "Listen", "ListenPacket", "ListenTCP", "ListenUDP", "ListenUnix",
71+
"LookupAddr", "LookupCNAME", "LookupHost", "LookupIP", "LookupIPAddr",
72+
"LookupMX", "LookupNS", "LookupNetIP", "LookupPort", "LookupSRV", "LookupTXT",
73+
}
74+
6475
func networkIOFuncName(obj types.Object) (string, string, bool) {
6576
fn, ok := obj.(*types.Func)
6677
if !ok {
@@ -73,8 +84,9 @@ func networkIOFuncName(obj types.Object) (string, string, bool) {
7384
name := fn.Name()
7485
switch pkg.Path() {
7586
case "net":
76-
switch name {
77-
case "Dial", "DialTimeout", "Listen", "ListenPacket", "ListenTCP", "ListenUDP", "ListenUnix":
87+
// Matches both package functions and *net.Resolver methods
88+
// (e.g. net.DefaultResolver.LookupIPAddr).
89+
if slices.Contains(netIOFuncNames, name) {
7890
return "net", name, true
7991
}
8092
case "net/http":

pkg/httpclient/ssrf.go

Lines changed: 31 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -119,22 +119,30 @@ func NewSSRFSafeTransport() *http.Transport {
119119
t = &http.Transport{Proxy: http.ProxyFromEnvironment}
120120
}
121121
proxies := proxyDialAllowlist()
122-
t.DialContext = (&net.Dialer{
122+
guarded := &net.Dialer{
123123
Timeout: 30 * time.Second,
124124
KeepAlive: 30 * time.Second,
125-
Control: func(network, address string, c syscall.RawConn) error {
126-
if _, ok := proxies[canonicalHostPort(address)]; ok {
127-
return nil
128-
}
129-
return SSRFDialControl(network, address, c)
130-
},
131-
}).DialContext
125+
Control: SSRFDialControl,
126+
}
127+
trusted := &net.Dialer{
128+
Timeout: 30 * time.Second,
129+
KeepAlive: 30 * time.Second,
130+
}
131+
// The address here is pre-resolution: with a proxy configured the
132+
// transport dials the proxy's literal host:port, so the allowlist can
133+
// be matched by string without ever resolving the proxy hostname.
134+
t.DialContext = func(ctx context.Context, network, address string) (net.Conn, error) {
135+
if _, ok := proxies[canonicalHostPort(address)]; ok {
136+
return trusted.DialContext(ctx, network, address)
137+
}
138+
return guarded.DialContext(ctx, network, address)
139+
}
132140
return t
133141
}
134142

135143
// canonicalHostPort normalises a "host:port" dial address so equivalent
136144
// IPv4 forms compare equal. An IPv4-mapped IPv6 address (::ffff:a.b.c.d)
137-
// is folded back to its dotted-quad form, matching what proxyHostPorts
145+
// is folded back to its dotted-quad form, matching what proxyHostPort
138146
// stores in the allowlist for an IPv4-literal proxy. Non-IP hosts and
139147
// malformed inputs are returned unchanged so the allowlist lookup
140148
// simply misses and the SSRF fall-through applies.
@@ -153,12 +161,13 @@ func canonicalHostPort(address string) string {
153161
return net.JoinHostPort(ip.String(), port)
154162
}
155163

156-
// proxyDialAllowlist returns the set of "host:port" strings that the
157-
// HTTP_PROXY / HTTPS_PROXY / ALL_PROXY environment variables resolve to.
158-
// Hostname-based proxies are resolved to all of their A/AAAA records so
159-
// the comparison can be done against the post-resolution dial address.
164+
// proxyDialAllowlist returns the set of "host:port" strings the
165+
// HTTP_PROXY / HTTPS_PROXY / ALL_PROXY environment variables name.
166+
// Hostnames are kept verbatim — never resolved — because the transport
167+
// dials a configured proxy by its literal host:port, so the comparison
168+
// happens pre-resolution. Constructors must not block on DNS.
160169
// The result is a snapshot taken at call time — safe to capture in a
161-
// dialer's Control closure.
170+
// dial closure.
162171
func proxyDialAllowlist() map[string]struct{} {
163172
out := map[string]struct{}{}
164173
raw := []string{
@@ -167,26 +176,26 @@ func proxyDialAllowlist() map[string]struct{} {
167176
os.Getenv("ALL_PROXY"), os.Getenv("all_proxy"),
168177
}
169178
for _, s := range raw {
170-
for addr := range proxyHostPorts(s) {
179+
if addr := proxyHostPort(s); addr != "" {
171180
out[addr] = struct{}{}
172181
}
173182
}
174183
return out
175184
}
176185

177-
// proxyHostPorts parses a single proxy specification (matching the syntax
186+
// proxyHostPort parses a single proxy specification (matching the syntax
178187
// accepted by net/http.ProxyFromEnvironment: a full URL or a bare host[:port]
179-
// in which case http:// is implied) and returns each "ip:port" it can be
180-
// reached at. A nil/empty input yields an empty set.
181-
func proxyHostPorts(spec string) map[string]struct{} {
188+
// in which case http:// is implied) and returns its canonical "host:port".
189+
// An empty or unparseable input yields "".
190+
func proxyHostPort(spec string) string {
182191
if spec == "" {
183-
return nil
192+
return ""
184193
}
185194
u, err := url.Parse(spec)
186195
if err != nil || u.Host == "" {
187196
u, err = url.Parse("http://" + spec)
188197
if err != nil || u.Host == "" {
189-
return nil
198+
return ""
190199
}
191200
}
192201
port := u.Port()
@@ -200,24 +209,7 @@ func proxyHostPorts(spec string) map[string]struct{} {
200209
port = "80"
201210
}
202211
}
203-
out := map[string]struct{}{}
204-
host := u.Hostname()
205-
if ip := net.ParseIP(host); ip != nil {
206-
out[net.JoinHostPort(ip.String(), port)] = struct{}{}
207-
return out
208-
}
209-
// The allowlist is resolved once, at transport-construction time, so its
210-
// (potentially blocking) DNS lookups are not repeated on every dial.
211-
// There is no caller context at construction; a bounded independent root
212-
// is the right base for the resolver timeout.
213-
//rubocop:disable Lint/ContextConnectivity
214-
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
215-
defer cancel()
216-
ips, _ := net.DefaultResolver.LookupIPAddr(ctx, host)
217-
for _, ip := range ips {
218-
out[net.JoinHostPort(ip.IP.String(), port)] = struct{}{}
219-
}
220-
return out
212+
return canonicalHostPort(net.JoinHostPort(u.Hostname(), port))
221213
}
222214

223215
// BoundedRedirects returns an http.Client.CheckRedirect that limits a

pkg/httpclient/ssrf_test.go

Lines changed: 33 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -292,42 +292,54 @@ func TestSSRFDialControl_IPv6ZoneID(t *testing.T) {
292292
assert.Contains(t, err.Error(), "not a valid IP")
293293
}
294294

295-
// TestProxyHostPorts pins the parsing rules for HTTP_PROXY-style values.
295+
// TestProxyHostPort pins the parsing rules for HTTP_PROXY-style values.
296296
// We support full URLs and bare host[:port] (matching net/http's
297-
// ProxyFromEnvironment), and we always emit a port — dial addresses
298-
// have one and we compare against them verbatim.
299-
func TestProxyHostPorts(t *testing.T) {
297+
// ProxyFromEnvironment), we always emit a port — dial addresses have
298+
// one and we compare against them verbatim — and hostnames are kept
299+
// literal, never DNS-resolved: parsing must not block.
300+
func TestProxyHostPort(t *testing.T) {
300301
t.Parallel()
301302

302303
tests := []struct {
303304
name string
304305
spec string
305-
want []string
306+
want string
306307
}{
307-
{"empty", "", nil},
308-
{"http URL with port", "http://172.17.0.0:3128", []string{"172.17.0.0:3128"}},
309-
{"http URL without port", "http://10.0.0.1", []string{"10.0.0.1:80"}},
310-
{"https URL without port", "https://10.0.0.1", []string{"10.0.0.1:443"}},
311-
{"socks5 URL without port", "socks5://10.0.0.1", []string{"10.0.0.1:1080"}},
312-
{"bare host:port", "172.17.0.0:3128", []string{"172.17.0.0:3128"}},
313-
{"bare host", "172.17.0.0", []string{"172.17.0.0:80"}},
314-
{"IPv6 with port", "http://[::1]:3128", []string{"[::1]:3128"}},
308+
{"empty", "", ""},
309+
{"http URL with port", "http://172.17.0.0:3128", "172.17.0.0:3128"},
310+
{"http URL without port", "http://10.0.0.1", "10.0.0.1:80"},
311+
{"https URL without port", "https://10.0.0.1", "10.0.0.1:443"},
312+
{"socks5 URL without port", "socks5://10.0.0.1", "10.0.0.1:1080"},
313+
{"bare host:port", "172.17.0.0:3128", "172.17.0.0:3128"},
314+
{"bare host", "172.17.0.0", "172.17.0.0:80"},
315+
{"IPv6 with port", "http://[::1]:3128", "[::1]:3128"},
316+
{"hostname kept literal", "http://proxy.internal:3128", "proxy.internal:3128"},
315317
}
316318
for _, tt := range tests {
317319
t.Run(tt.name, func(t *testing.T) {
318320
t.Parallel()
319-
got := proxyHostPorts(tt.spec)
320-
if tt.want == nil {
321-
assert.Empty(t, got)
322-
return
323-
}
324-
for _, w := range tt.want {
325-
assert.Contains(t, got, w)
326-
}
321+
assert.Equal(t, tt.want, proxyHostPort(tt.spec))
327322
})
328323
}
329324
}
330325

326+
// TestProxyDialAllowlist_NoDNSAtConstruction is the regression test for
327+
// NewSSRFSafeTransport blocking constructors (NewLocalRuntime among them)
328+
// on synchronous proxy-hostname resolution: a hostname proxy — even one
329+
// that can never resolve — must land in the allowlist verbatim, proving
330+
// the allowlist is built without DNS. Matching happens pre-resolution at
331+
// dial time instead.
332+
func TestProxyDialAllowlist_NoDNSAtConstruction(t *testing.T) {
333+
t.Setenv("HTTP_PROXY", "http://proxy.invalid:3128")
334+
t.Setenv("HTTPS_PROXY", "")
335+
t.Setenv("ALL_PROXY", "")
336+
t.Setenv("http_proxy", "")
337+
t.Setenv("https_proxy", "")
338+
t.Setenv("all_proxy", "")
339+
340+
assert.Contains(t, proxyDialAllowlist(), "proxy.invalid:3128")
341+
}
342+
331343
// TestNewSSRFSafeTransport_AllowsConfiguredProxy is the regression test
332344
// for the docker-agent sandbox bug: HTTP_PROXY=http://172.17.0.0:3128
333345
// must not be rejected by SSRFDialControl, because that's the only way

0 commit comments

Comments
 (0)