From 76f9bf3279eff2e596db4960a78a2665d0ff9405 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Tue, 21 Jan 2025 16:36:50 -0800 Subject: [PATCH] [internal-branch.go1.24-vendor] proxy, http/httpproxy: do not mismatch IPv6 zone ids against hosts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When matching against a host "example.com", don't match an IPv6 address like "[1000::1%25.example.com]:80". Thanks to Juho Forsén of Mattermost for reporting this issue. Fixes CVE-2025-22870 For #71984 For #71986 Change-Id: I0c4fdf18765decc27e6ddf220ebe3a9bf4a6454d Reviewed-on: https://go-review.googlesource.com/c/net/+/654696 LUCI-TryBot-Result: Go LUCI Auto-Submit: Junyang Shao Reviewed-by: Michael Pratt --- http/httpproxy/proxy.go | 10 ++- http/httpproxy/proxy_test.go | 7 ++ proxy/per_host.go | 8 +- proxy/per_host_test.go | 166 ++++++++++++++++++++++++----------- 4 files changed, 135 insertions(+), 56 deletions(-) diff --git a/http/httpproxy/proxy.go b/http/httpproxy/proxy.go index 6404aaf1..d89c257a 100644 --- a/http/httpproxy/proxy.go +++ b/http/httpproxy/proxy.go @@ -14,6 +14,7 @@ import ( "errors" "fmt" "net" + "net/netip" "net/url" "os" "strings" @@ -177,8 +178,10 @@ func (cfg *config) useProxy(addr string) bool { if host == "localhost" { return false } - ip := net.ParseIP(host) - if ip != nil { + nip, err := netip.ParseAddr(host) + var ip net.IP + if err == nil { + ip = net.IP(nip.AsSlice()) if ip.IsLoopback() { return false } @@ -360,6 +363,9 @@ type domainMatch struct { } func (m domainMatch) match(host, port string, ip net.IP) bool { + if ip != nil { + return false + } if strings.HasSuffix(host, m.host) || (m.matchHost && host == m.host[1:]) { return m.port == "" || m.port == port } diff --git a/http/httpproxy/proxy_test.go b/http/httpproxy/proxy_test.go index 790afdab..a1dd2e83 100644 --- a/http/httpproxy/proxy_test.go +++ b/http/httpproxy/proxy_test.go @@ -211,6 +211,13 @@ var proxyForURLTests = []proxyForURLTest{{ }, req: "http://www.xn--fsq092h.com", want: "", +}, { + cfg: httpproxy.Config{ + NoProxy: "example.com", + HTTPProxy: "proxy", + }, + req: "http://[1000::%25.example.com]:123", + want: "http://proxy", }, } diff --git a/proxy/per_host.go b/proxy/per_host.go index d7d4b8b6..32bdf435 100644 --- a/proxy/per_host.go +++ b/proxy/per_host.go @@ -7,6 +7,7 @@ package proxy import ( "context" "net" + "net/netip" "strings" ) @@ -57,7 +58,8 @@ func (p *PerHost) DialContext(ctx context.Context, network, addr string) (c net. } func (p *PerHost) dialerForRequest(host string) Dialer { - if ip := net.ParseIP(host); ip != nil { + if nip, err := netip.ParseAddr(host); err == nil { + ip := net.IP(nip.AsSlice()) for _, net := range p.bypassNetworks { if net.Contains(ip) { return p.bypass @@ -108,8 +110,8 @@ func (p *PerHost) AddFromString(s string) { } continue } - if ip := net.ParseIP(host); ip != nil { - p.AddIP(ip) + if nip, err := netip.ParseAddr(host); err == nil { + p.AddIP(net.IP(nip.AsSlice())) continue } if strings.HasPrefix(host, "*.") { diff --git a/proxy/per_host_test.go b/proxy/per_host_test.go index 0447eb42..b7bcec8a 100644 --- a/proxy/per_host_test.go +++ b/proxy/per_host_test.go @@ -7,8 +7,9 @@ package proxy import ( "context" "errors" + "fmt" "net" - "reflect" + "slices" "testing" ) @@ -22,55 +23,118 @@ func (r *recordingProxy) Dial(network, addr string) (net.Conn, error) { } func TestPerHost(t *testing.T) { - expectedDef := []string{ - "example.com:123", - "1.2.3.4:123", - "[1001::]:123", + for _, test := range []struct { + config string // passed to PerHost.AddFromString + nomatch []string // addrs using the default dialer + match []string // addrs using the bypass dialer + }{{ + config: "localhost,*.zone,127.0.0.1,10.0.0.1/8,1000::/16", + nomatch: []string{ + "example.com:123", + "1.2.3.4:123", + "[1001::]:123", + }, + match: []string{ + "localhost:123", + "zone:123", + "foo.zone:123", + "127.0.0.1:123", + "10.1.2.3:123", + "[1000::]:123", + "[1000::%25.example.com]:123", + }, + }, { + config: "localhost", + nomatch: []string{ + "127.0.0.1:80", + }, + match: []string{ + "localhost:80", + }, + }, { + config: "*.zone", + nomatch: []string{ + "foo.com:80", + }, + match: []string{ + "foo.zone:80", + "foo.bar.zone:80", + }, + }, { + config: "1.2.3.4", + nomatch: []string{ + "127.0.0.1:80", + "11.2.3.4:80", + }, + match: []string{ + "1.2.3.4:80", + }, + }, { + config: "10.0.0.0/24", + nomatch: []string{ + "10.0.1.1:80", + }, + match: []string{ + "10.0.0.1:80", + "10.0.0.255:80", + }, + }, { + config: "fe80::/10", + nomatch: []string{ + "[fec0::1]:80", + "[fec0::1%en0]:80", + }, + match: []string{ + "[fe80::1]:80", + "[fe80::1%en0]:80", + }, + }, { + // We don't allow zone IDs in network prefixes, + // so this config matches nothing. + config: "fe80::%en0/10", + nomatch: []string{ + "[fec0::1]:80", + "[fec0::1%en0]:80", + "[fe80::1]:80", + "[fe80::1%en0]:80", + "[fe80::1%en1]:80", + }, + }} { + for _, addr := range test.match { + testPerHost(t, test.config, addr, true) + } + for _, addr := range test.nomatch { + testPerHost(t, test.config, addr, false) + } + } +} + +func testPerHost(t *testing.T, config, addr string, wantMatch bool) { + name := fmt.Sprintf("config %q, dial %q", config, addr) + + var def, bypass recordingProxy + perHost := NewPerHost(&def, &bypass) + perHost.AddFromString(config) + perHost.Dial("tcp", addr) + + // Dial and DialContext should have the same results. + var defc, bypassc recordingProxy + perHostc := NewPerHost(&defc, &bypassc) + perHostc.AddFromString(config) + perHostc.DialContext(context.Background(), "tcp", addr) + if !slices.Equal(def.addrs, defc.addrs) { + t.Errorf("%v: Dial default=%v, bypass=%v; DialContext default=%v, bypass=%v", name, def.addrs, bypass.addrs, defc.addrs, bypass.addrs) + return + } + + if got, want := slices.Concat(def.addrs, bypass.addrs), []string{addr}; !slices.Equal(got, want) { + t.Errorf("%v: dialed %q, want %q", name, got, want) + return + } + + gotMatch := len(bypass.addrs) > 0 + if gotMatch != wantMatch { + t.Errorf("%v: matched=%v, want %v", name, gotMatch, wantMatch) + return } - expectedBypass := []string{ - "localhost:123", - "zone:123", - "foo.zone:123", - "127.0.0.1:123", - "10.1.2.3:123", - "[1000::]:123", - } - - t.Run("Dial", func(t *testing.T) { - var def, bypass recordingProxy - perHost := NewPerHost(&def, &bypass) - perHost.AddFromString("localhost,*.zone,127.0.0.1,10.0.0.1/8,1000::/16") - for _, addr := range expectedDef { - perHost.Dial("tcp", addr) - } - for _, addr := range expectedBypass { - perHost.Dial("tcp", addr) - } - - if !reflect.DeepEqual(expectedDef, def.addrs) { - t.Errorf("Hosts which went to the default proxy didn't match. Got %v, want %v", def.addrs, expectedDef) - } - if !reflect.DeepEqual(expectedBypass, bypass.addrs) { - t.Errorf("Hosts which went to the bypass proxy didn't match. Got %v, want %v", bypass.addrs, expectedBypass) - } - }) - - t.Run("DialContext", func(t *testing.T) { - var def, bypass recordingProxy - perHost := NewPerHost(&def, &bypass) - perHost.AddFromString("localhost,*.zone,127.0.0.1,10.0.0.1/8,1000::/16") - for _, addr := range expectedDef { - perHost.DialContext(context.Background(), "tcp", addr) - } - for _, addr := range expectedBypass { - perHost.DialContext(context.Background(), "tcp", addr) - } - - if !reflect.DeepEqual(expectedDef, def.addrs) { - t.Errorf("Hosts which went to the default proxy didn't match. Got %v, want %v", def.addrs, expectedDef) - } - if !reflect.DeepEqual(expectedBypass, bypass.addrs) { - t.Errorf("Hosts which went to the bypass proxy didn't match. Got %v, want %v", bypass.addrs, expectedBypass) - } - }) }