Skip to content

Commit 25219f3

Browse files
jdomeracki-coderjdomeracki-coder
authored
fix(coderd): harden Azure identity certificate fetch (cherry-pick v2.32) (#25277)
Cherry-pick of 57b11d4 to `release/2.32`. Backport of #25274. > [!NOTE] > This PR was created by Coder Agents on behalf of a human.
1 parent d944b92 commit 25219f3

4 files changed

Lines changed: 296 additions & 10 deletions

File tree

‎coderd/azureidentity/azureidentity.go‎

Lines changed: 176 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,9 @@ import (
88
"encoding/pem"
99
"errors"
1010
"io"
11+
"net"
1112
"net/http"
13+
"net/url"
1214
"regexp"
1315
"sync"
1416
"time"
@@ -25,6 +27,158 @@ var allowedSigners = regexp.MustCompile(`^(.*\.)?metadata\.(azure\.(com|us|cn)|m
2527
// each time a parse occurs.
2628
var pkcs7Mutex sync.Mutex
2729

30+
// allowedCertHosts contains the hosts Azure intermediate
31+
// certificates are served from. Only these hosts are permitted
32+
// when fetching issuing certificates referenced in the signer
33+
// certificate. This prevents SSRF via crafted
34+
// IssuingCertificateURL values.
35+
//
36+
// Source: https://learn.microsoft.com/en-us/azure/security/fundamentals/azure-ca-details
37+
var allowedCertHosts = map[string]bool{
38+
"www.microsoft.com": true,
39+
"cacerts.digicert.com": true,
40+
}
41+
42+
// maxCertResponseBytes is the maximum size of a certificate
43+
// response body we will read. Azure intermediate certificates
44+
// are typically under 4 KiB; 1 MiB is a generous upper bound
45+
// that prevents memory exhaustion from malicious responses.
46+
const maxCertResponseBytes = 1 << 20 // 1 MiB
47+
48+
// extraBlockedNetworks lists special-use CIDR ranges that the
49+
// stdlib classification methods (IsLoopback, IsPrivate, etc.) do
50+
// not cover. Blocking these prevents SSRF against carrier-grade
51+
// NAT, network-benchmarking, documentation, discard-only, and
52+
// the all-zeros "this network" range.
53+
//
54+
// IPv6 ranges already handled by stdlib:
55+
// - ::1/128 (IsLoopback)
56+
// - fc00::/7 (IsPrivate, ULA)
57+
// - fe80::/10 (IsLinkLocalUnicast)
58+
// - ff00::/8 (IsMulticast)
59+
// - ::/128 (IsUnspecified)
60+
var extraBlockedNetworks []*net.IPNet
61+
62+
func init() {
63+
for _, cidr := range []string{
64+
// IPv4 special-use ranges.
65+
"0.0.0.0/8", // RFC 1122 "this network".
66+
"100.64.0.0/10", // RFC 6598 carrier-grade NAT.
67+
"198.18.0.0/15", // RFC 2544 benchmarking.
68+
69+
// IPv6 special-use ranges not covered by stdlib.
70+
"64:ff9b:1::/48", // RFC 8215 IPv4/IPv6 translation.
71+
"100::/64", // RFC 6666 discard-only.
72+
"2001:2::/48", // RFC 5180 benchmarking.
73+
"2001:db8::/32", // RFC 3849 documentation.
74+
} {
75+
_, network, _ := net.ParseCIDR(cidr)
76+
extraBlockedNetworks = append(extraBlockedNetworks, network)
77+
}
78+
}
79+
80+
// isPrivateIP reports whether the IP is on a network that must
81+
// not be reachable when fetching certificates. IPv4-mapped IPv6
82+
// addresses are canonicalized to IPv4 first so a literal like
83+
// ::ffff:169.254.169.254 cannot bypass the IPv4 ranges.
84+
func isPrivateIP(ip net.IP) bool {
85+
if v4 := ip.To4(); v4 != nil {
86+
ip = v4
87+
}
88+
if ip.IsLoopback() ||
89+
ip.IsPrivate() ||
90+
ip.IsLinkLocalUnicast() ||
91+
ip.IsLinkLocalMulticast() ||
92+
ip.IsMulticast() ||
93+
ip.IsUnspecified() ||
94+
ip.IsInterfaceLocalMulticast() {
95+
return true
96+
}
97+
for _, network := range extraBlockedNetworks {
98+
if network.Contains(ip) {
99+
return true
100+
}
101+
}
102+
return false
103+
}
104+
105+
// certFetchClient is an HTTP client that refuses to connect
106+
// to private or link-local IP addresses. This provides
107+
// defense-in-depth against SSRF even if the host allowlist is
108+
// somehow bypassed (e.g. via DNS rebinding).
109+
var certFetchClient = &http.Client{
110+
Timeout: 5 * time.Second,
111+
Transport: &http.Transport{
112+
DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
113+
host, port, err := net.SplitHostPort(addr)
114+
if err != nil {
115+
return nil, xerrors.Errorf("split host/port: %w", err)
116+
}
117+
ips, err := net.DefaultResolver.LookupIPAddr(ctx, host)
118+
if err != nil {
119+
return nil, xerrors.Errorf("resolve host: %w", err)
120+
}
121+
if len(ips) == 0 {
122+
return nil, xerrors.Errorf("no addresses for %q", host)
123+
}
124+
// Reject up front so a single tainted answer
125+
// short-circuits the dial rather than racing it.
126+
for _, ip := range ips {
127+
if isPrivateIP(ip.IP) {
128+
return nil, xerrors.Errorf(
129+
"certificate fetch blocked: %q resolved to private IP %s",
130+
host, ip.IP,
131+
)
132+
}
133+
}
134+
// Dial the validated IP directly. If we dialed by
135+
// hostname here, Go's stdlib would re-resolve and a
136+
// hostile resolver could swap in a private IP after
137+
// validation (DNS rebinding). TLS verification still
138+
// uses the URL host via the Transport's TLS config.
139+
var d net.Dialer
140+
var firstErr error
141+
for _, ip := range ips {
142+
conn, derr := d.DialContext(ctx, network, net.JoinHostPort(ip.IP.String(), port))
143+
if derr == nil {
144+
return conn, nil
145+
}
146+
if firstErr == nil {
147+
firstErr = derr
148+
}
149+
}
150+
return nil, firstErr
151+
},
152+
},
153+
}
154+
155+
// IsAllowedCertificateURL reports whether rawURL points to a
156+
// host on the allowlist, uses http or https, and targets a
157+
// standard PKI distribution port. Microsoft and DigiCert serve
158+
// these artifacts on 80/443 only; any other port is rejected to
159+
// keep the SSRF surface as narrow as the hostname itself.
160+
func IsAllowedCertificateURL(rawURL string) bool {
161+
if rawURL == "" {
162+
return false
163+
}
164+
u, err := url.Parse(rawURL)
165+
if err != nil {
166+
return false
167+
}
168+
if u.Scheme != "http" && u.Scheme != "https" {
169+
return false
170+
}
171+
if !allowedCertHosts[u.Hostname()] {
172+
return false
173+
}
174+
switch u.Port() {
175+
case "", "80", "443":
176+
return true
177+
default:
178+
return false
179+
}
180+
}
181+
28182
type metadata struct {
29183
VMID string `json:"vmId"`
30184
}
@@ -81,29 +235,42 @@ func Validate(ctx context.Context, signature string, options Options) (string, e
81235
ctx, cancelFunc := context.WithTimeout(ctx, 5*time.Second)
82236
defer cancelFunc()
83237
for _, certURL := range signer.IssuingCertificateURL {
238+
if !IsAllowedCertificateURL(certURL) {
239+
return "", xerrors.New("issuing certificate URL not on allowlist")
240+
}
84241
req, err := http.NewRequestWithContext(ctx, "GET", certURL, nil)
85242
if err != nil {
86-
return "", xerrors.Errorf("new request %q: %w", certURL, err)
243+
return "", xerrors.New("construct certificate request")
87244
}
88-
res, err := http.DefaultClient.Do(req)
245+
res, err := certFetchClient.Do(req)
89246
if err != nil {
90-
return "", xerrors.Errorf("no cached certificate for %q found. error fetching: %w", certURL, err)
247+
return "", xerrors.New("certificate fetch unsuccessful")
91248
}
92-
data, err := io.ReadAll(res.Body)
249+
limited := io.LimitReader(res.Body, maxCertResponseBytes+1)
250+
data, err := io.ReadAll(limited)
251+
_ = res.Body.Close()
93252
if err != nil {
94-
_ = res.Body.Close()
95-
return "", xerrors.Errorf("read body %q: %w", certURL, err)
253+
return "", xerrors.New("read certificate response body")
254+
}
255+
if int64(len(data)) > maxCertResponseBytes {
256+
return "", xerrors.New(
257+
"certificate response exceeds maximum size",
258+
)
96259
}
97-
_ = res.Body.Close()
98260
cert, err := x509.ParseCertificate(data)
99261
if err != nil {
100-
return "", xerrors.Errorf("parse certificate %q: %w", certURL, err)
262+
// Do not wrap the parse error; it may contain
263+
// fragments of the HTTP response body, which
264+
// could leak internal data to the caller.
265+
return "", xerrors.New(
266+
"fetched data is not a valid certificate",
267+
)
101268
}
102269
options.Intermediates.AddCert(cert)
103270
}
104271
_, err = signer.Verify(options.VerifyOptions)
105272
if err != nil {
106-
return "", err
273+
return "", xerrors.New("signature verification failed after fetching issuing certificates")
107274
}
108275
}
109276

‎coderd/azureidentity/azureidentity_internal_test.go‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
package azureidentity
2+
3+
import (
4+
"context"
5+
"net"
6+
"net/http"
7+
"net/http/httptest"
8+
"testing"
9+
10+
"github.com/stretchr/testify/require"
11+
)
12+
13+
func TestIsPrivateIP(t *testing.T) {
14+
t.Parallel()
15+
cases := []struct {
16+
name string
17+
ip string
18+
blocked bool
19+
}{
20+
{"loopback v4", "127.0.0.1", true},
21+
{"loopback v6", "::1", true},
22+
{"link local v4 (azure metadata)", "169.254.169.254", true},
23+
{"link local v6", "fe80::1", true},
24+
{"rfc1918 10/8", "10.0.0.1", true},
25+
{"rfc1918 172.16/12", "172.16.0.1", true},
26+
{"rfc1918 192.168/16", "192.168.0.1", true},
27+
{"ipv6 ula", "fc00::1", true},
28+
{"unspecified v4", "0.0.0.0", true},
29+
{"unspecified v6", "::", true},
30+
{"this-network 0.0.0.0/8", "0.1.2.3", true},
31+
{"cgnat 100.64/10", "100.64.0.1", true},
32+
{"benchmarking 198.18/15", "198.18.0.1", true},
33+
{"multicast v4", "224.0.0.1", true},
34+
{"ipv6 nat64 well-known", "64:ff9b:1::1", true},
35+
{"ipv6 discard-only", "100::1", true},
36+
{"ipv6 benchmarking", "2001:2::1", true},
37+
{"ipv6 documentation", "2001:db8::1", true},
38+
// IPv4-mapped IPv6: must canonicalize to v4 before
39+
// classification, otherwise an attacker could bypass
40+
// the metadata block via ::ffff:169.254.169.254.
41+
{"ipv4-mapped metadata", "::ffff:169.254.169.254", true},
42+
{"ipv4-mapped rfc1918", "::ffff:10.0.0.1", true},
43+
44+
{"public v4", "8.8.8.8", false},
45+
{"public v6", "2606:4700:4700::1111", false},
46+
}
47+
for _, tc := range cases {
48+
t.Run(tc.name, func(t *testing.T) {
49+
t.Parallel()
50+
ip := net.ParseIP(tc.ip)
51+
require.NotNil(t, ip, "parse %q", tc.ip)
52+
require.Equal(t, tc.blocked, isPrivateIP(ip))
53+
})
54+
}
55+
}
56+
57+
// TestCertFetchClientRejectsLoopback proves the dialer refuses
58+
// to connect even when the URL itself would have passed an
59+
// allowlist (httptest.Server always binds to 127.0.0.1, so a
60+
// successful fetch here would mean the SSRF guard had failed).
61+
func TestCertFetchClientRejectsLoopback(t *testing.T) {
62+
t.Parallel()
63+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
64+
_, _ = w.Write([]byte("should never be reached"))
65+
}))
66+
t.Cleanup(srv.Close)
67+
68+
req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, srv.URL, nil)
69+
require.NoError(t, err)
70+
resp, err := certFetchClient.Do(req)
71+
if resp != nil {
72+
defer resp.Body.Close()
73+
}
74+
require.Error(t, err)
75+
require.Contains(t, err.Error(), "private IP")
76+
}

‎coderd/azureidentity/azureidentity_test.go‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,3 +87,37 @@ func TestExpiresSoon(t *testing.T) {
8787
}
8888
}
8989
}
90+
91+
func TestIsAllowedCertificateURL(t *testing.T) {
92+
t.Parallel()
93+
tests := []struct {
94+
name string
95+
url string
96+
allowed bool
97+
}{
98+
{"microsoft http", "http://www.microsoft.com/pki/mscorp/cert.crt", true},
99+
{"microsoft https", "https://www.microsoft.com/pkiops/certs/cert.crt", true},
100+
{"digicert http", "http://cacerts.digicert.com/DigiCertGlobalRootG2.crt", true},
101+
{"digicert https", "https://cacerts.digicert.com/DigiCertGlobalRootG3.crt", true},
102+
{"evil domain", "http://evil.example.com/cert.crt", false},
103+
{"metadata endpoint", "http://169.254.169.254/latest/meta-data/", false},
104+
{"localhost", "http://localhost/secret", false},
105+
{"subdomain trick", "http://www.microsoft.com.evil.com/cert.crt", false},
106+
{"empty string", "", false},
107+
{"ftp scheme", "ftp://www.microsoft.com/cert.crt", false},
108+
{"no scheme", "www.microsoft.com/cert.crt", false},
109+
{"javascript scheme", "javascript:alert(1)", false},
110+
{"microsoft with path", "http://www.microsoft.com/pkiops/certs/cert.crt", true},
111+
{"microsoft explicit port 80", "http://www.microsoft.com:80/cert.crt", true},
112+
{"microsoft explicit port 443", "https://www.microsoft.com:443/cert.crt", true},
113+
{"microsoft non-standard port", "http://www.microsoft.com:8080/cert.crt", false},
114+
{"microsoft port 22", "http://www.microsoft.com:22/cert.crt", false},
115+
}
116+
for _, tc := range tests {
117+
t.Run(tc.name, func(t *testing.T) {
118+
t.Parallel()
119+
result := azureidentity.IsAllowedCertificateURL(tc.url)
120+
require.Equal(t, tc.allowed, result, "URL: %s", tc.url)
121+
})
122+
}
123+
}

‎coderd/workspaceresourceauth.go‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77

88
"github.com/mitchellh/mapstructure"
99

10+
"cdr.dev/slog/v3"
1011
"github.com/coder/coder/v2/coderd/awsidentity"
1112
"github.com/coder/coder/v2/coderd/azureidentity"
1213
"github.com/coder/coder/v2/coderd/database"
@@ -39,9 +40,17 @@ func (api *API) postWorkspaceAuthAzureInstanceIdentity(rw http.ResponseWriter, r
3940
VerifyOptions: api.AzureCertificates,
4041
})
4142
if err != nil {
43+
// Log the full error for operators but return only a
44+
// generic message to the caller. Errors from the
45+
// certificate fetch path may contain fragments of
46+
// internal HTTP responses, so exposing them would be
47+
// an information disclosure risk.
48+
api.Logger.Warn(ctx, "azure identity validation failed",
49+
slog.Error(err),
50+
)
4251
httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{
4352
Message: "Invalid Azure identity.",
44-
Detail: err.Error(),
53+
Detail: "Signature verification failed.",
4554
})
4655
return
4756
}

0 commit comments

Comments
 (0)