Skip to content

Commit f0c69db

Browse files
thatnealpatelgopherbot
thatnealpatel
authored and
gopherbot
committed
[release-branch.go1.25] crypto/x509: improve domain name verification
Don't use domainToReverseLabels to check if domain names are valid, since it is not particularly performant, and can contribute to DoS vectors. Instead just iterate over the name and enforce the properties we care about. This also enforces that DNS names, both in SANs and name constraints, are valid. We previously allowed invalid SANs, because some intermediates had these weird names (see #23995), but there are currently no trusted intermediates that have this property, and since we target the web PKI, supporting this particular case is not a high priority. Thank you to Jakub Ciolek for reporting this issue. Fixes CVE-2025-58187 For #75681 Fixes #75715 Change-Id: I6ebce847dcbe5fc63ef2f9a74f53f11c4c56d3d1 Reviewed-on: https://go-internal-review.googlesource.com/c/go/+/2820 Reviewed-by: Damien Neil <[email protected]> Reviewed-by: Roland Shoemaker <[email protected]> Reviewed-on: https://go-internal-review.googlesource.com/c/go/+/2981 Commit-Queue: Roland Shoemaker <[email protected]> Reviewed-by: Nicholas Husin <[email protected]> Reviewed-on: https://go-review.googlesource.com/c/go/+/709848 Auto-Submit: Michael Pratt <[email protected]> TryBot-Bypass: Michael Pratt <[email protected]> Reviewed-by: Carlos Amedee <[email protected]>
1 parent 9fd3ac8 commit f0c69db

File tree

4 files changed

+96
-100
lines changed

4 files changed

+96
-100
lines changed

‎src/crypto/x509/name_constraints_test.go‎

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

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.
1459+
// #77: if several EKUs are requested, satisfying any of them is sufficient.
15161460
{
15171461
roots: make([]constraintsSpec, 1),
15181462
intermediates: [][]constraintsSpec{
@@ -1527,7 +1471,7 @@ var nameConstraintsTests = []nameConstraintsTest{
15271471
requestedEKUs: []ExtKeyUsage{ExtKeyUsageClientAuth, ExtKeyUsageEmailProtection},
15281472
},
15291473

1530-
// #81: EKUs that are not asserted in VerifyOpts are not required to be
1474+
// #78: EKUs that are not asserted in VerifyOpts are not required to be
15311475
// nested.
15321476
{
15331477
roots: make([]constraintsSpec, 1),
@@ -1546,7 +1490,7 @@ var nameConstraintsTests = []nameConstraintsTest{
15461490
},
15471491
},
15481492

1549-
// #82: a certificate without SANs and CN is accepted in a constrained chain.
1493+
// #79: a certificate without SANs and CN is accepted in a constrained chain.
15501494
{
15511495
roots: []constraintsSpec{
15521496
{
@@ -1563,7 +1507,7 @@ var nameConstraintsTests = []nameConstraintsTest{
15631507
},
15641508
},
15651509

1566-
// #83: a certificate without SANs and with a CN that does not parse as a
1510+
// #80: a certificate without SANs and with a CN that does not parse as a
15671511
// hostname is accepted in a constrained chain.
15681512
{
15691513
roots: []constraintsSpec{
@@ -1582,7 +1526,7 @@ var nameConstraintsTests = []nameConstraintsTest{
15821526
},
15831527
},
15841528

1585-
// #84: a certificate with SANs and CN is accepted in a constrained chain.
1529+
// #81: a certificate with SANs and CN is accepted in a constrained chain.
15861530
{
15871531
roots: []constraintsSpec{
15881532
{
@@ -1600,14 +1544,7 @@ var nameConstraintsTests = []nameConstraintsTest{
16001544
},
16011545
},
16021546

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
1547+
// #82: URIs with IPv6 addresses with zones and ports are rejected
16111548
{
16121549
roots: []constraintsSpec{
16131550
{

‎src/crypto/x509/parser.go‎

Lines changed: 46 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -413,10 +413,14 @@ 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+
}
416420
emailAddresses = append(emailAddresses, email)
417421
case nameTypeDNS:
418422
name := string(data)
419-
if err := isIA5String(name); err != nil {
423+
if err := isIA5String(name); err != nil || (err == nil && !domainNameValid(name, false)) {
420424
return errors.New("x509: SAN dNSName is malformed")
421425
}
422426
dnsNames = append(dnsNames, string(name))
@@ -426,14 +430,9 @@ func parseSANExtension(der cryptobyte.String) (dnsNames, emailAddresses []string
426430
return errors.New("x509: SAN uniformResourceIdentifier is malformed")
427431
}
428432
uri, err := url.Parse(uriStr)
429-
if err != nil {
433+
if err != nil || (err == nil && uri.Host != "" && !domainNameValid(uri.Host, false)) {
430434
return fmt.Errorf("x509: cannot parse URI %q: %s", uriStr, err)
431435
}
432-
if len(uri.Host) > 0 {
433-
if _, ok := domainToReverseLabels(uri.Host); !ok {
434-
return fmt.Errorf("x509: cannot parse URI %q: invalid domain", uriStr)
435-
}
436-
}
437436
uris = append(uris, uri)
438437
case nameTypeIP:
439438
switch len(data) {
@@ -598,15 +597,7 @@ func parseNameConstraintsExtension(out *Certificate, e pkix.Extension) (unhandle
598597
return nil, nil, nil, nil, errors.New("x509: invalid constraint value: " + err.Error())
599598
}
600599

601-
trimmedDomain := domain
602-
if len(trimmedDomain) > 0 && trimmedDomain[0] == '.' {
603-
// constraints can have a leading
604-
// period to exclude the domain
605-
// itself, but that's not valid in a
606-
// normal domain name.
607-
trimmedDomain = trimmedDomain[1:]
608-
}
609-
if _, ok := domainToReverseLabels(trimmedDomain); !ok {
600+
if !domainNameValid(domain, true) {
610601
return nil, nil, nil, nil, fmt.Errorf("x509: failed to parse dnsName constraint %q", domain)
611602
}
612603
dnsNames = append(dnsNames, domain)
@@ -647,12 +638,7 @@ func parseNameConstraintsExtension(out *Certificate, e pkix.Extension) (unhandle
647638
return nil, nil, nil, nil, fmt.Errorf("x509: failed to parse rfc822Name constraint %q", constraint)
648639
}
649640
} else {
650-
// Otherwise it's a domain name.
651-
domain := constraint
652-
if len(domain) > 0 && domain[0] == '.' {
653-
domain = domain[1:]
654-
}
655-
if _, ok := domainToReverseLabels(domain); !ok {
641+
if !domainNameValid(constraint, true) {
656642
return nil, nil, nil, nil, fmt.Errorf("x509: failed to parse rfc822Name constraint %q", constraint)
657643
}
658644
}
@@ -668,15 +654,7 @@ func parseNameConstraintsExtension(out *Certificate, e pkix.Extension) (unhandle
668654
return nil, nil, nil, nil, fmt.Errorf("x509: failed to parse URI constraint %q: cannot be IP address", domain)
669655
}
670656

671-
trimmedDomain := domain
672-
if len(trimmedDomain) > 0 && trimmedDomain[0] == '.' {
673-
// constraints can have a leading
674-
// period to exclude the domain itself,
675-
// but that's not valid in a normal
676-
// domain name.
677-
trimmedDomain = trimmedDomain[1:]
678-
}
679-
if _, ok := domainToReverseLabels(trimmedDomain); !ok {
657+
if !domainNameValid(domain, true) {
680658
return nil, nil, nil, nil, fmt.Errorf("x509: failed to parse URI constraint %q", domain)
681659
}
682660
uriDomains = append(uriDomains, domain)
@@ -1317,3 +1295,40 @@ func ParseRevocationList(der []byte) (*RevocationList, error) {
13171295

13181296
return rl, nil
13191297
}
1298+
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.
1308+
func domainNameValid(s string, constraint bool) bool {
1309+
if len(s) == 0 && constraint {
1310+
return true
1311+
}
1312+
if len(s) == 0 || (!constraint && s[0] == '.') || s[len(s)-1] == '.' || len(s) > 253 {
1313+
return false
1314+
}
1315+
lastDot := -1
1316+
if constraint && s[0] == '.' {
1317+
s = s[1:]
1318+
}
1319+
1320+
for i := 0; i <= len(s); i++ {
1321+
if i == len(s) || s[i] == '.' {
1322+
labelLen := i
1323+
if lastDot >= 0 {
1324+
labelLen -= lastDot + 1
1325+
}
1326+
if labelLen == 0 || labelLen > 63 {
1327+
return false
1328+
}
1329+
lastDot = i
1330+
}
1331+
}
1332+
1333+
return true
1334+
}

‎src/crypto/x509/parser_test.go‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"encoding/asn1"
99
"encoding/pem"
1010
"os"
11+
"strings"
1112
"testing"
1213

1314
cryptobyte_asn1 "golang.org/x/crypto/cryptobyte/asn1"
@@ -251,3 +252,45 @@ d5l1tRhScKu2NBgm74nYmJxJYgvuTA38wGhRrGU=
251252
}
252253
}
253254
}
255+
256+
func TestDomainNameValid(t *testing.T) {
257+
for _, tc := range []struct {
258+
name string
259+
dnsName string
260+
constraint bool
261+
valid bool
262+
}{
263+
{"empty name, name", "", false, false},
264+
{"empty name, constraint", "", true, true},
265+
{"empty label, name", "a..a", false, false},
266+
{"empty label, constraint", "a..a", true, false},
267+
{"period, name", ".", false, false},
268+
{"period, constraint", ".", true, false}, // TODO(roland): not entirely clear if this is a valid constraint (require at least one label?)
269+
{"valid, name", "a.b.c", false, true},
270+
{"valid, constraint", "a.b.c", true, true},
271+
{"leading period, name", ".a.b.c", false, false},
272+
{"leading period, constraint", ".a.b.c", true, true},
273+
{"trailing period, name", "a.", false, false},
274+
{"trailing period, constraint", "a.", true, false},
275+
{"bare label, name", "a", false, true},
276+
{"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},
283+
{"63 char single label, name", strings.Repeat("a", 63), false, true},
284+
{"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},
287+
{"63 char label, name", "a." + strings.Repeat("a", 63), false, true},
288+
{"63 char label, constraint", "a." + strings.Repeat("a", 63), true, true},
289+
} {
290+
t.Run(tc.name, func(t *testing.T) {
291+
if tc.valid != domainNameValid(tc.dnsName, tc.constraint) {
292+
t.Errorf("domainNameValid(%q, %t) = %v; want %v", tc.dnsName, tc.constraint, !tc.valid, tc.valid)
293+
}
294+
})
295+
}
296+
}

‎src/crypto/x509/verify.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -391,6 +391,7 @@ func parseRFC2821Mailbox(in string) (mailbox rfc2821Mailbox, ok bool) {
391391
// domainToReverseLabels converts a textual domain name like foo.example.com to
392392
// the list of labels in reverse order, e.g. ["com", "example", "foo"].
393393
func domainToReverseLabels(domain string) (reverseLabels []string, ok bool) {
394+
reverseLabels = make([]string, 0, strings.Count(domain, ".")+1)
394395
for len(domain) > 0 {
395396
if i := strings.LastIndexByte(domain, '.'); i == -1 {
396397
reverseLabels = append(reverseLabels, domain)

0 commit comments

Comments
 (0)