Skip to content

Commit 1cd7168

Browse files
rolandshoemakerrolandshoemaker
committed
crypto/x509: rework fix for CVE-2025-58187
In CL 709854 we enabled strict validation for a number of properties of domain names (and their constraints). This caused significant breakage, since we didn't previously disallow the creation of certificates which contained these malformed domains. Rollback a number of the properties we enforced, making domainNameValid only enforce the same properties that domainToReverseLabels does. Since this also undoes some of the DoS protections our initial fix enabled, this change also adds caching of constraints in isValid (which perhaps is the fix we should've initially chosen). Updates #75835 Fixes #75828 Change-Id: Ie6ca6b4f30e9b8a143692b64757f7bbf4671ed0e Reviewed-on: https://go-review.googlesource.com/c/go/+/710735 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Damien Neil <[email protected]>
1 parent 8aa1efa commit 1cd7168

5 files changed

Lines changed: 221 additions & 50 deletions

File tree

‎src/crypto/x509/name_constraints_test.go‎

Lines changed: 69 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1456,7 +1456,63 @@ var nameConstraintsTests = []nameConstraintsTest{
14561456
expectedError: "incompatible key usage",
14571457
},
14581458

1459-
// #77: if several EKUs are requested, satisfying any of them is sufficient.
1459+
// An invalid DNS SAN should be detected only at validation time so
1460+
// that we can process CA certificates in the wild that have invalid SANs.
1461+
// See https://github.com/golang/go/issues/23995
1462+
1463+
// #77: an invalid DNS or mail SAN will not be detected if name constraint
1464+
// checking is not triggered.
1465+
{
1466+
roots: make([]constraintsSpec, 1),
1467+
intermediates: [][]constraintsSpec{
1468+
{
1469+
{},
1470+
},
1471+
},
1472+
leaf: leafSpec{
1473+
sans: []string{"dns:this is invalid", "email:this @ is invalid"},
1474+
},
1475+
},
1476+
1477+
// #78: an invalid DNS SAN will be detected if any name constraint checking
1478+
// is triggered.
1479+
{
1480+
roots: []constraintsSpec{
1481+
{
1482+
bad: []string{"uri:"},
1483+
},
1484+
},
1485+
intermediates: [][]constraintsSpec{
1486+
{
1487+
{},
1488+
},
1489+
},
1490+
leaf: leafSpec{
1491+
sans: []string{"dns:this is invalid"},
1492+
},
1493+
expectedError: "cannot parse dnsName",
1494+
},
1495+
1496+
// #79: an invalid email SAN will be detected if any name constraint
1497+
// checking is triggered.
1498+
{
1499+
roots: []constraintsSpec{
1500+
{
1501+
bad: []string{"uri:"},
1502+
},
1503+
},
1504+
intermediates: [][]constraintsSpec{
1505+
{
1506+
{},
1507+
},
1508+
},
1509+
leaf: leafSpec{
1510+
sans: []string{"email:this @ is invalid"},
1511+
},
1512+
expectedError: "cannot parse rfc822Name",
1513+
},
1514+
1515+
// #80: if several EKUs are requested, satisfying any of them is sufficient.
14601516
{
14611517
roots: make([]constraintsSpec, 1),
14621518
intermediates: [][]constraintsSpec{
@@ -1471,7 +1527,7 @@ var nameConstraintsTests = []nameConstraintsTest{
14711527
requestedEKUs: []ExtKeyUsage{ExtKeyUsageClientAuth, ExtKeyUsageEmailProtection},
14721528
},
14731529

1474-
// #78: EKUs that are not asserted in VerifyOpts are not required to be
1530+
// #81: EKUs that are not asserted in VerifyOpts are not required to be
14751531
// nested.
14761532
{
14771533
roots: make([]constraintsSpec, 1),
@@ -1490,7 +1546,7 @@ var nameConstraintsTests = []nameConstraintsTest{
14901546
},
14911547
},
14921548

1493-
// #79: a certificate without SANs and CN is accepted in a constrained chain.
1549+
// #82: a certificate without SANs and CN is accepted in a constrained chain.
14941550
{
14951551
roots: []constraintsSpec{
14961552
{
@@ -1507,7 +1563,7 @@ var nameConstraintsTests = []nameConstraintsTest{
15071563
},
15081564
},
15091565

1510-
// #80: a certificate without SANs and with a CN that does not parse as a
1566+
// #83: a certificate without SANs and with a CN that does not parse as a
15111567
// hostname is accepted in a constrained chain.
15121568
{
15131569
roots: []constraintsSpec{
@@ -1526,7 +1582,7 @@ var nameConstraintsTests = []nameConstraintsTest{
15261582
},
15271583
},
15281584

1529-
// #81: a certificate with SANs and CN is accepted in a constrained chain.
1585+
// #84: a certificate with SANs and CN is accepted in a constrained chain.
15301586
{
15311587
roots: []constraintsSpec{
15321588
{
@@ -1544,7 +1600,14 @@ var nameConstraintsTests = []nameConstraintsTest{
15441600
},
15451601
},
15461602

1547-
// #82: URIs with IPv6 addresses with zones and ports are rejected
1603+
// #85: .example.com is an invalid DNS name, it should not match the
1604+
// constraint example.com.
1605+
{
1606+
roots: []constraintsSpec{{ok: []string{"dns:example.com"}}},
1607+
leaf: leafSpec{sans: []string{"dns:.example.com"}},
1608+
expectedError: "cannot parse dnsName \".example.com\"",
1609+
},
1610+
// #86: URIs with IPv6 addresses with zones and ports are rejected
15481611
{
15491612
roots: []constraintsSpec{
15501613
{

‎src/crypto/x509/parser.go‎

Lines changed: 39 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -413,14 +413,10 @@ func parseSANExtension(der cryptobyte.String) (dnsNames, emailAddresses []string
413413
if err := isIA5String(email); err != nil {
414414
return errors.New("x509: SAN rfc822Name is malformed")
415415
}
416-
parsed, ok := parseRFC2821Mailbox(email)
417-
if !ok || (ok && !domainNameValid(parsed.domain, false)) {
418-
return errors.New("x509: SAN rfc822Name is malformed")
419-
}
420416
emailAddresses = append(emailAddresses, email)
421417
case nameTypeDNS:
422418
name := string(data)
423-
if err := isIA5String(name); err != nil || (err == nil && !domainNameValid(name, false)) {
419+
if err := isIA5String(name); err != nil {
424420
return errors.New("x509: SAN dNSName is malformed")
425421
}
426422
dnsNames = append(dnsNames, string(name))
@@ -430,9 +426,12 @@ func parseSANExtension(der cryptobyte.String) (dnsNames, emailAddresses []string
430426
return errors.New("x509: SAN uniformResourceIdentifier is malformed")
431427
}
432428
uri, err := url.Parse(uriStr)
433-
if err != nil || (err == nil && uri.Host != "" && !domainNameValid(uri.Host, false)) {
429+
if err != nil {
434430
return fmt.Errorf("x509: cannot parse URI %q: %s", uriStr, err)
435431
}
432+
if len(uri.Host) > 0 && !domainNameValid(uri.Host, false) {
433+
return fmt.Errorf("x509: cannot parse URI %q: invalid domain", uriStr)
434+
}
436435
uris = append(uris, uri)
437436
case nameTypeIP:
438437
switch len(data) {
@@ -1296,36 +1295,58 @@ func ParseRevocationList(der []byte) (*RevocationList, error) {
12961295
return rl, nil
12971296
}
12981297

1299-
// domainNameValid does minimal domain name validity checking. In particular it
1300-
// enforces the following properties:
1301-
// - names cannot have the trailing period
1302-
// - names can only have a leading period if constraint is true
1303-
// - names must be <= 253 characters
1304-
// - names cannot have empty labels
1305-
// - names cannot labels that are longer than 63 characters
1306-
//
1307-
// Note that this does not enforce the LDH requirements for domain names.
1298+
// domainNameValid is an alloc-less version of the checks that
1299+
// domainToReverseLabels does.
13081300
func domainNameValid(s string, constraint bool) bool {
1309-
if len(s) == 0 && constraint {
1301+
// TODO(#75835): This function omits a number of checks which we
1302+
// really should be doing to enforce that domain names are valid names per
1303+
// RFC 1034. We previously enabled these checks, but this broke a
1304+
// significant number of certificates we previously considered valid, and we
1305+
// happily create via CreateCertificate (et al). We should enable these
1306+
// checks, but will need to gate them behind a GODEBUG.
1307+
//
1308+
// I have left the checks we previously enabled, noted with "TODO(#75835)" so
1309+
// that we can easily re-enable them once we unbreak everyone.
1310+
1311+
// TODO(#75835): this should only be true for constraints.
1312+
if len(s) == 0 {
13101313
return true
13111314
}
1312-
if len(s) == 0 || (!constraint && s[0] == '.') || s[len(s)-1] == '.' || len(s) > 253 {
1315+
1316+
// Do not allow trailing period (FQDN format is not allowed in SANs or
1317+
// constraints).
1318+
if s[len(s)-1] == '.' {
13131319
return false
13141320
}
1321+
1322+
// TODO(#75835): domains must have at least one label, cannot have
1323+
// a leading empty label, and cannot be longer than 253 characters.
1324+
// if len(s) == 0 || (!constraint && s[0] == '.') || len(s) > 253 {
1325+
// return false
1326+
// }
1327+
13151328
lastDot := -1
13161329
if constraint && s[0] == '.' {
13171330
s = s[1:]
13181331
}
13191332

13201333
for i := 0; i <= len(s); i++ {
1334+
if i < len(s) && (s[i] < 33 || s[i] > 126) {
1335+
// Invalid character.
1336+
return false
1337+
}
13211338
if i == len(s) || s[i] == '.' {
13221339
labelLen := i
13231340
if lastDot >= 0 {
13241341
labelLen -= lastDot + 1
13251342
}
1326-
if labelLen == 0 || labelLen > 63 {
1343+
if labelLen == 0 {
13271344
return false
13281345
}
1346+
// TODO(#75835): labels cannot be longer than 63 characters.
1347+
// if labelLen > 63 {
1348+
// return false
1349+
// }
13291350
lastDot = i
13301351
}
13311352
}

‎src/crypto/x509/parser_test.go‎

Lines changed: 74 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@
55
package x509
66

77
import (
8+
"crypto/ecdsa"
9+
"crypto/elliptic"
10+
"crypto/rand"
811
"encoding/asn1"
912
"encoding/pem"
1013
"os"
@@ -260,7 +263,31 @@ func TestDomainNameValid(t *testing.T) {
260263
constraint bool
261264
valid bool
262265
}{
263-
{"empty name, name", "", false, false},
266+
// TODO(#75835): these tests are for stricter name validation, which we
267+
// had to disable. Once we reenable these strict checks, behind a
268+
// GODEBUG, we should add them back in.
269+
// {"empty name, name", "", false, false},
270+
// {"254 char label, name", strings.Repeat("a.a", 84) + "aaa", false, false},
271+
// {"254 char label, constraint", strings.Repeat("a.a", 84) + "aaa", true, false},
272+
// {"253 char label, name", strings.Repeat("a.a", 84) + "aa", false, false},
273+
// {"253 char label, constraint", strings.Repeat("a.a", 84) + "aa", true, false},
274+
// {"64 char single label, name", strings.Repeat("a", 64), false, false},
275+
// {"64 char single label, constraint", strings.Repeat("a", 64), true, false},
276+
// {"64 char label, name", "a." + strings.Repeat("a", 64), false, false},
277+
// {"64 char label, constraint", "a." + strings.Repeat("a", 64), true, false},
278+
279+
// TODO(#75835): these are the inverse of the tests above, they should be removed
280+
// once the strict checking is enabled.
281+
{"254 char label, name", strings.Repeat("a.a", 84) + "aaa", false, true},
282+
{"254 char label, constraint", strings.Repeat("a.a", 84) + "aaa", true, true},
283+
{"253 char label, name", strings.Repeat("a.a", 84) + "aa", false, true},
284+
{"253 char label, constraint", strings.Repeat("a.a", 84) + "aa", true, true},
285+
{"64 char single label, name", strings.Repeat("a", 64), false, true},
286+
{"64 char single label, constraint", strings.Repeat("a", 64), true, true},
287+
{"64 char label, name", "a." + strings.Repeat("a", 64), false, true},
288+
{"64 char label, constraint", "a." + strings.Repeat("a", 64), true, true},
289+
290+
// Check we properly enforce properties of domain names.
264291
{"empty name, constraint", "", true, true},
265292
{"empty label, name", "a..a", false, false},
266293
{"empty label, constraint", "a..a", true, false},
@@ -274,23 +301,60 @@ func TestDomainNameValid(t *testing.T) {
274301
{"trailing period, constraint", "a.", true, false},
275302
{"bare label, name", "a", false, true},
276303
{"bare label, constraint", "a", true, true},
277-
{"254 char label, name", strings.Repeat("a.a", 84) + "aaa", false, false},
278-
{"254 char label, constraint", strings.Repeat("a.a", 84) + "aaa", true, false},
279-
{"253 char label, name", strings.Repeat("a.a", 84) + "aa", false, false},
280-
{"253 char label, constraint", strings.Repeat("a.a", 84) + "aa", true, false},
281-
{"64 char single label, name", strings.Repeat("a", 64), false, false},
282-
{"64 char single label, constraint", strings.Repeat("a", 64), true, false},
283304
{"63 char single label, name", strings.Repeat("a", 63), false, true},
284305
{"63 char single label, constraint", strings.Repeat("a", 63), true, true},
285-
{"64 char label, name", "a." + strings.Repeat("a", 64), false, false},
286-
{"64 char label, constraint", "a." + strings.Repeat("a", 64), true, false},
287306
{"63 char label, name", "a." + strings.Repeat("a", 63), false, true},
288307
{"63 char label, constraint", "a." + strings.Repeat("a", 63), true, true},
289308
} {
290309
t.Run(tc.name, func(t *testing.T) {
291-
if tc.valid != domainNameValid(tc.dnsName, tc.constraint) {
310+
valid := domainNameValid(tc.dnsName, tc.constraint)
311+
if tc.valid != valid {
292312
t.Errorf("domainNameValid(%q, %t) = %v; want %v", tc.dnsName, tc.constraint, !tc.valid, tc.valid)
293313
}
314+
// Also check that we enforce the same properties as domainToReverseLabels
315+
trimmedName := tc.dnsName
316+
if tc.constraint && len(trimmedName) > 1 && trimmedName[0] == '.' {
317+
trimmedName = trimmedName[1:]
318+
}
319+
_, revValid := domainToReverseLabels(trimmedName)
320+
if valid != revValid {
321+
t.Errorf("domainNameValid(%q, %t) = %t != domainToReverseLabels(%q) = %t", tc.dnsName, tc.constraint, valid, trimmedName, revValid)
322+
}
294323
})
295324
}
296325
}
326+
327+
func TestRoundtripWeirdSANs(t *testing.T) {
328+
// TODO(#75835): check that certificates we create with CreateCertificate that have malformed SAN values
329+
// can be parsed by ParseCertificate. We should eventually restrict this, but for now we have to maintain
330+
// this property as people have been relying on it.
331+
k, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
332+
if err != nil {
333+
t.Fatal(err)
334+
}
335+
badNames := []string{
336+
"baredomain",
337+
"baredomain.",
338+
strings.Repeat("a", 255),
339+
strings.Repeat("a", 65) + ".com",
340+
}
341+
tmpl := &Certificate{
342+
EmailAddresses: badNames,
343+
DNSNames: badNames,
344+
}
345+
b, err := CreateCertificate(rand.Reader, tmpl, tmpl, &k.PublicKey, k)
346+
if err != nil {
347+
t.Fatal(err)
348+
}
349+
_, err = ParseCertificate(b)
350+
if err != nil {
351+
t.Fatalf("Couldn't roundtrip certificate: %v", err)
352+
}
353+
}
354+
355+
func FuzzDomainNameValid(f *testing.F) {
356+
f.Fuzz(func(t *testing.T, data string) {
357+
domainNameValid(data, false)
358+
domainNameValid(data, true)
359+
})
360+
}

0 commit comments

Comments
 (0)