expectedError: "incompatible key usage",
},
- // #77: if several EKUs are requested, satisfying any of them is sufficient.
+ // An invalid DNS SAN should be detected only at validation time so
+ // that we can process CA certificates in the wild that have invalid SANs.
+ // See https://github.com/golang/go/issues/23995
+
+ // #77: an invalid DNS or mail SAN will not be detected if name constraint
+ // checking is not triggered.
+ {
+ roots: make([]constraintsSpec, 1),
+ intermediates: [][]constraintsSpec{
+ {
+ {},
+ },
+ },
+ leaf: leafSpec{
+ sans: []string{"dns:this is invalid", "email:this @ is invalid"},
+ },
+ },
+
+ // #78: an invalid DNS SAN will be detected if any name constraint checking
+ // is triggered.
+ {
+ roots: []constraintsSpec{
+ {
+ bad: []string{"uri:"},
+ },
+ },
+ intermediates: [][]constraintsSpec{
+ {
+ {},
+ },
+ },
+ leaf: leafSpec{
+ sans: []string{"dns:this is invalid"},
+ },
+ expectedError: "cannot parse dnsName",
+ },
+
+ // #79: an invalid email SAN will be detected if any name constraint
+ // checking is triggered.
+ {
+ roots: []constraintsSpec{
+ {
+ bad: []string{"uri:"},
+ },
+ },
+ intermediates: [][]constraintsSpec{
+ {
+ {},
+ },
+ },
+ leaf: leafSpec{
+ sans: []string{"email:this @ is invalid"},
+ },
+ expectedError: "cannot parse rfc822Name",
+ },
+
+ // #80: if several EKUs are requested, satisfying any of them is sufficient.
{
roots: make([]constraintsSpec, 1),
intermediates: [][]constraintsSpec{
requestedEKUs: []ExtKeyUsage{ExtKeyUsageClientAuth, ExtKeyUsageEmailProtection},
},
- // #78: EKUs that are not asserted in VerifyOpts are not required to be
+ // #81: EKUs that are not asserted in VerifyOpts are not required to be
// nested.
{
roots: make([]constraintsSpec, 1),
},
},
- // #79: a certificate without SANs and CN is accepted in a constrained chain.
+ // #82: a certificate without SANs and CN is accepted in a constrained chain.
{
roots: []constraintsSpec{
{
},
},
- // #80: a certificate without SANs and with a CN that does not parse as a
+ // #83: a certificate without SANs and with a CN that does not parse as a
// hostname is accepted in a constrained chain.
{
roots: []constraintsSpec{
},
},
- // #81: a certificate with SANs and CN is accepted in a constrained chain.
+ // #84: a certificate with SANs and CN is accepted in a constrained chain.
{
roots: []constraintsSpec{
{
},
},
- // #82: URIs with IPv6 addresses with zones and ports are rejected
+ // #85: .example.com is an invalid DNS name, it should not match the
+ // constraint example.com.
+ {
+ roots: []constraintsSpec{{ok: []string{"dns:example.com"}}},
+ leaf: leafSpec{sans: []string{"dns:.example.com"}},
+ expectedError: "cannot parse dnsName \".example.com\"",
+ },
+ // #86: URIs with IPv6 addresses with zones and ports are rejected
{
roots: []constraintsSpec{
{
if err := isIA5String(email); err != nil {
return errors.New("x509: SAN rfc822Name is malformed")
}
- parsed, ok := parseRFC2821Mailbox(email)
- if !ok || (ok && !domainNameValid(parsed.domain, false)) {
- return errors.New("x509: SAN rfc822Name is malformed")
- }
emailAddresses = append(emailAddresses, email)
case nameTypeDNS:
name := string(data)
- if err := isIA5String(name); err != nil || (err == nil && !domainNameValid(name, false)) {
+ if err := isIA5String(name); err != nil {
return errors.New("x509: SAN dNSName is malformed")
}
dnsNames = append(dnsNames, string(name))
return errors.New("x509: SAN uniformResourceIdentifier is malformed")
}
uri, err := url.Parse(uriStr)
- if err != nil || (err == nil && uri.Host != "" && !domainNameValid(uri.Host, false)) {
+ if err != nil {
return fmt.Errorf("x509: cannot parse URI %q: %s", uriStr, err)
}
+ if len(uri.Host) > 0 && !domainNameValid(uri.Host, false) {
+ return fmt.Errorf("x509: cannot parse URI %q: invalid domain", uriStr)
+ }
uris = append(uris, uri)
case nameTypeIP:
switch len(data) {
return rl, nil
}
-// domainNameValid does minimal domain name validity checking. In particular it
-// enforces the following properties:
-// - names cannot have the trailing period
-// - names can only have a leading period if constraint is true
-// - names must be <= 253 characters
-// - names cannot have empty labels
-// - names cannot labels that are longer than 63 characters
-//
-// Note that this does not enforce the LDH requirements for domain names.
+// domainNameValid is an alloc-less version of the checks that
+// domainToReverseLabels does.
func domainNameValid(s string, constraint bool) bool {
- if len(s) == 0 && constraint {
+ // TODO(#75835): This function omits a number of checks which we
+ // really should be doing to enforce that domain names are valid names per
+ // RFC 1034. We previously enabled these checks, but this broke a
+ // significant number of certificates we previously considered valid, and we
+ // happily create via CreateCertificate (et al). We should enable these
+ // checks, but will need to gate them behind a GODEBUG.
+ //
+ // I have left the checks we previously enabled, noted with "TODO(#75835)" so
+ // that we can easily re-enable them once we unbreak everyone.
+
+ // TODO(#75835): this should only be true for constraints.
+ if len(s) == 0 {
return true
}
- if len(s) == 0 || (!constraint && s[0] == '.') || s[len(s)-1] == '.' || len(s) > 253 {
+
+ // Do not allow trailing period (FQDN format is not allowed in SANs or
+ // constraints).
+ if s[len(s)-1] == '.' {
return false
}
+
+ // TODO(#75835): domains must have at least one label, cannot have
+ // a leading empty label, and cannot be longer than 253 characters.
+ // if len(s) == 0 || (!constraint && s[0] == '.') || len(s) > 253 {
+ // return false
+ // }
+
lastDot := -1
if constraint && s[0] == '.' {
s = s[1:]
}
for i := 0; i <= len(s); i++ {
+ if i < len(s) && (s[i] < 33 || s[i] > 126) {
+ // Invalid character.
+ return false
+ }
if i == len(s) || s[i] == '.' {
labelLen := i
if lastDot >= 0 {
labelLen -= lastDot + 1
}
- if labelLen == 0 || labelLen > 63 {
+ if labelLen == 0 {
return false
}
+ // TODO(#75835): labels cannot be longer than 63 characters.
+ // if labelLen > 63 {
+ // return false
+ // }
lastDot = i
}
}
package x509
import (
+ "crypto/ecdsa"
+ "crypto/elliptic"
+ "crypto/rand"
"encoding/asn1"
"encoding/pem"
"os"
constraint bool
valid bool
}{
- {"empty name, name", "", false, false},
+ // TODO(#75835): these tests are for stricter name validation, which we
+ // had to disable. Once we reenable these strict checks, behind a
+ // GODEBUG, we should add them back in.
+ // {"empty name, name", "", false, false},
+ // {"254 char label, name", strings.Repeat("a.a", 84) + "aaa", false, false},
+ // {"254 char label, constraint", strings.Repeat("a.a", 84) + "aaa", true, false},
+ // {"253 char label, name", strings.Repeat("a.a", 84) + "aa", false, false},
+ // {"253 char label, constraint", strings.Repeat("a.a", 84) + "aa", true, false},
+ // {"64 char single label, name", strings.Repeat("a", 64), false, false},
+ // {"64 char single label, constraint", strings.Repeat("a", 64), true, false},
+ // {"64 char label, name", "a." + strings.Repeat("a", 64), false, false},
+ // {"64 char label, constraint", "a." + strings.Repeat("a", 64), true, false},
+
+ // TODO(#75835): these are the inverse of the tests above, they should be removed
+ // once the strict checking is enabled.
+ {"254 char label, name", strings.Repeat("a.a", 84) + "aaa", false, true},
+ {"254 char label, constraint", strings.Repeat("a.a", 84) + "aaa", true, true},
+ {"253 char label, name", strings.Repeat("a.a", 84) + "aa", false, true},
+ {"253 char label, constraint", strings.Repeat("a.a", 84) + "aa", true, true},
+ {"64 char single label, name", strings.Repeat("a", 64), false, true},
+ {"64 char single label, constraint", strings.Repeat("a", 64), true, true},
+ {"64 char label, name", "a." + strings.Repeat("a", 64), false, true},
+ {"64 char label, constraint", "a." + strings.Repeat("a", 64), true, true},
+
+ // Check we properly enforce properties of domain names.
{"empty name, constraint", "", true, true},
{"empty label, name", "a..a", false, false},
{"empty label, constraint", "a..a", true, false},
{"trailing period, constraint", "a.", true, false},
{"bare label, name", "a", false, true},
{"bare label, constraint", "a", true, true},
- {"254 char label, name", strings.Repeat("a.a", 84) + "aaa", false, false},
- {"254 char label, constraint", strings.Repeat("a.a", 84) + "aaa", true, false},
- {"253 char label, name", strings.Repeat("a.a", 84) + "aa", false, false},
- {"253 char label, constraint", strings.Repeat("a.a", 84) + "aa", true, false},
- {"64 char single label, name", strings.Repeat("a", 64), false, false},
- {"64 char single label, constraint", strings.Repeat("a", 64), true, false},
{"63 char single label, name", strings.Repeat("a", 63), false, true},
{"63 char single label, constraint", strings.Repeat("a", 63), true, true},
- {"64 char label, name", "a." + strings.Repeat("a", 64), false, false},
- {"64 char label, constraint", "a." + strings.Repeat("a", 64), true, false},
{"63 char label, name", "a." + strings.Repeat("a", 63), false, true},
{"63 char label, constraint", "a." + strings.Repeat("a", 63), true, true},
} {
t.Run(tc.name, func(t *testing.T) {
- if tc.valid != domainNameValid(tc.dnsName, tc.constraint) {
+ valid := domainNameValid(tc.dnsName, tc.constraint)
+ if tc.valid != valid {
t.Errorf("domainNameValid(%q, %t) = %v; want %v", tc.dnsName, tc.constraint, !tc.valid, tc.valid)
}
+ // Also check that we enforce the same properties as domainToReverseLabels
+ trimmedName := tc.dnsName
+ if tc.constraint && len(trimmedName) > 1 && trimmedName[0] == '.' {
+ trimmedName = trimmedName[1:]
+ }
+ _, revValid := domainToReverseLabels(trimmedName)
+ if valid != revValid {
+ t.Errorf("domainNameValid(%q, %t) = %t != domainToReverseLabels(%q) = %t", tc.dnsName, tc.constraint, valid, trimmedName, revValid)
+ }
})
}
}
+
+func TestRoundtripWeirdSANs(t *testing.T) {
+ // TODO(#75835): check that certificates we create with CreateCertificate that have malformed SAN values
+ // can be parsed by ParseCertificate. We should eventually restrict this, but for now we have to maintain
+ // this property as people have been relying on it.
+ k, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+ if err != nil {
+ t.Fatal(err)
+ }
+ badNames := []string{
+ "baredomain",
+ "baredomain.",
+ strings.Repeat("a", 255),
+ strings.Repeat("a", 65) + ".com",
+ }
+ tmpl := &Certificate{
+ EmailAddresses: badNames,
+ DNSNames: badNames,
+ }
+ b, err := CreateCertificate(rand.Reader, tmpl, tmpl, &k.PublicKey, k)
+ if err != nil {
+ t.Fatal(err)
+ }
+ _, err = ParseCertificate(b)
+ if err != nil {
+ t.Fatalf("Couldn't roundtrip certificate: %v", err)
+ }
+}
+
+func FuzzDomainNameValid(f *testing.F) {
+ f.Fuzz(func(t *testing.T, data string) {
+ domainNameValid(data, false)
+ domainNameValid(data, true)
+ })
+}
return reverseLabels, true
}
-func matchEmailConstraint(mailbox rfc2821Mailbox, constraint string) (bool, error) {
+func matchEmailConstraint(mailbox rfc2821Mailbox, constraint string, reversedDomainsCache map[string][]string, reversedConstraintsCache map[string][]string) (bool, error) {
// If the constraint contains an @, then it specifies an exact mailbox
// name.
if strings.Contains(constraint, "@") {
// Otherwise the constraint is like a DNS constraint of the domain part
// of the mailbox.
- return matchDomainConstraint(mailbox.domain, constraint)
+ return matchDomainConstraint(mailbox.domain, constraint, reversedDomainsCache, reversedConstraintsCache)
}
-func matchURIConstraint(uri *url.URL, constraint string) (bool, error) {
+func matchURIConstraint(uri *url.URL, constraint string, reversedDomainsCache map[string][]string, reversedConstraintsCache map[string][]string) (bool, error) {
// From RFC 5280, Section 4.2.1.10:
// “a uniformResourceIdentifier that does not include an authority
// component with a host name specified as a fully qualified domain
return false, fmt.Errorf("URI with IP (%q) cannot be matched against constraints", uri.String())
}
- return matchDomainConstraint(host, constraint)
+ return matchDomainConstraint(host, constraint, reversedDomainsCache, reversedConstraintsCache)
}
func matchIPConstraint(ip net.IP, constraint *net.IPNet) (bool, error) {
return true, nil
}
-func matchDomainConstraint(domain, constraint string) (bool, error) {
+func matchDomainConstraint(domain, constraint string, reversedDomainsCache map[string][]string, reversedConstraintsCache map[string][]string) (bool, error) {
// The meaning of zero length constraints is not specified, but this
// code follows NSS and accepts them as matching everything.
if len(constraint) == 0 {
return true, nil
}
- domainLabels, ok := domainToReverseLabels(domain)
- if !ok {
- return false, fmt.Errorf("x509: internal error: cannot parse domain %q", domain)
+ domainLabels, found := reversedDomainsCache[domain]
+ if !found {
+ var ok bool
+ domainLabels, ok = domainToReverseLabels(domain)
+ if !ok {
+ return false, fmt.Errorf("x509: internal error: cannot parse domain %q", domain)
+ }
+ reversedDomainsCache[domain] = domainLabels
}
// RFC 5280 says that a leading period in a domain name means that at
constraint = constraint[1:]
}
- constraintLabels, ok := domainToReverseLabels(constraint)
- if !ok {
- return false, fmt.Errorf("x509: internal error: cannot parse domain %q", constraint)
+ constraintLabels, found := reversedConstraintsCache[constraint]
+ if !found {
+ var ok bool
+ constraintLabels, ok = domainToReverseLabels(constraint)
+ if !ok {
+ return false, fmt.Errorf("x509: internal error: cannot parse domain %q", constraint)
+ }
+ reversedConstraintsCache[constraint] = constraintLabels
}
if len(domainLabels) < len(constraintLabels) ||
}
}
+ // Each time we do constraint checking, we need to check the constraints in
+ // the current certificate against all of the names that preceded it. We
+ // reverse these names using domainToReverseLabels, which is a relatively
+ // expensive operation. Since we check each name against each constraint,
+ // this requires us to do N*C calls to domainToReverseLabels (where N is the
+ // total number of names that preceed the certificate, and C is the total
+ // number of constraints in the certificate). By caching the results of
+ // calling domainToReverseLabels, we can reduce that to N+C calls at the
+ // cost of keeping all of the parsed names and constraints in memory until
+ // we return from isValid.
+ reversedDomainsCache := map[string][]string{}
+ reversedConstraintsCache := map[string][]string{}
+
if (certType == intermediateCertificate || certType == rootCertificate) &&
c.hasNameConstraints() {
toCheck := []*Certificate{}
if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "email address", name, mailbox,
func(parsedName, constraint any) (bool, error) {
- return matchEmailConstraint(parsedName.(rfc2821Mailbox), constraint.(string))
+ return matchEmailConstraint(parsedName.(rfc2821Mailbox), constraint.(string), reversedDomainsCache, reversedConstraintsCache)
}, c.PermittedEmailAddresses, c.ExcludedEmailAddresses); err != nil {
return err
}
case nameTypeDNS:
name := string(data)
- if _, ok := domainToReverseLabels(name); !ok {
+ if !domainNameValid(name, false) {
return fmt.Errorf("x509: cannot parse dnsName %q", name)
}
if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "DNS name", name, name,
func(parsedName, constraint any) (bool, error) {
- return matchDomainConstraint(parsedName.(string), constraint.(string))
+ return matchDomainConstraint(parsedName.(string), constraint.(string), reversedDomainsCache, reversedConstraintsCache)
}, c.PermittedDNSDomains, c.ExcludedDNSDomains); err != nil {
return err
}
if err := c.checkNameConstraints(&comparisonCount, maxConstraintComparisons, "URI", name, uri,
func(parsedName, constraint any) (bool, error) {
- return matchURIConstraint(parsedName.(*url.URL), constraint.(string))
+ return matchURIConstraint(parsedName.(*url.URL), constraint.(string), reversedDomainsCache, reversedConstraintsCache)
}, c.PermittedURIDomains, c.ExcludedURIDomains); err != nil {
return err
}
func TestNameConstraints(t *testing.T) {
for i, test := range nameConstraintTests {
- result, err := matchDomainConstraint(test.domain, test.constraint)
+ result, err := matchDomainConstraint(test.domain, test.constraint, map[string][]string{}, map[string][]string{})
if err != nil && !test.expectError {
t.Errorf("unexpected error for test #%d: domain=%s, constraint=%s, err=%s", i, test.domain, test.constraint, err)