]> Cypherpunks repositories - gostls13.git/commitdiff
crypto/x509: handle name constraints with cryptobyte
authorAdam Langley <agl@golang.org>
Mon, 30 Oct 2017 02:20:33 +0000 (19:20 -0700)
committerAdam Langley <agl@golang.org>
Sun, 12 Nov 2017 01:19:40 +0000 (01:19 +0000)
This allows better precision and (the motivation) empty strings to
be handled correctly. With that in place tests for the behaviour of
empty name constraints can be added.

Also fixes a compatibility issue with NSS. See #22616.

Fixes #22616

Change-Id: I5139439bb58435d5f769828a4eebf8bed2d858e8
Reviewed-on: https://go-review.googlesource.com/74271
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
src/crypto/x509/name_constraints_test.go
src/crypto/x509/x509.go
src/go/build/deps_test.go

index 8b7845ea421eaac1ec81a6969eb289f38fdf3689..e75770b84d619776c98edee3601fc6c34ebdcce4 100644 (file)
@@ -1185,9 +1185,6 @@ var nameConstraintsTests = []nameConstraintsTest{
                },
        },
 
-       // TODO(agl): handle empty name constraints. Currently this doesn't
-       // work because empty values are treated as missing.
-
        // #61: omitting extended key usage in a CA certificate implies that
        // any usage is ok.
        nameConstraintsTest{
@@ -1345,6 +1342,111 @@ var nameConstraintsTests = []nameConstraintsTest{
                        ekus: []string{"serverAuth", "clientAuth"},
                },
        },
+
+       // #69: an empty DNS constraint should allow anything.
+       nameConstraintsTest{
+               roots: []constraintsSpec{
+                       constraintsSpec{
+                               ok: []string{"dns:"},
+                       },
+               },
+               intermediates: [][]constraintsSpec{
+                       []constraintsSpec{
+                               constraintsSpec{},
+                       },
+               },
+               leaf: leafSpec{
+                       sans: []string{"dns:example.com"},
+               },
+       },
+
+       // #70: an empty DNS constraint should also reject everything.
+       nameConstraintsTest{
+               roots: []constraintsSpec{
+                       constraintsSpec{
+                               bad: []string{"dns:"},
+                       },
+               },
+               intermediates: [][]constraintsSpec{
+                       []constraintsSpec{
+                               constraintsSpec{},
+                       },
+               },
+               leaf: leafSpec{
+                       sans: []string{"dns:example.com"},
+               },
+               expectedError: "\"example.com\" is excluded",
+       },
+
+       // #71: an empty email constraint should allow anything
+       nameConstraintsTest{
+               roots: []constraintsSpec{
+                       constraintsSpec{
+                               ok: []string{"email:"},
+                       },
+               },
+               intermediates: [][]constraintsSpec{
+                       []constraintsSpec{
+                               constraintsSpec{},
+                       },
+               },
+               leaf: leafSpec{
+                       sans: []string{"email:foo@example.com"},
+               },
+       },
+
+       // #72: an empty email constraint should also reject everything.
+       nameConstraintsTest{
+               roots: []constraintsSpec{
+                       constraintsSpec{
+                               bad: []string{"email:"},
+                       },
+               },
+               intermediates: [][]constraintsSpec{
+                       []constraintsSpec{
+                               constraintsSpec{},
+                       },
+               },
+               leaf: leafSpec{
+                       sans: []string{"email:foo@example.com"},
+               },
+               expectedError: "\"foo@example.com\" is excluded",
+       },
+
+       // #73: an empty URI constraint should allow anything
+       nameConstraintsTest{
+               roots: []constraintsSpec{
+                       constraintsSpec{
+                               ok: []string{"uri:"},
+                       },
+               },
+               intermediates: [][]constraintsSpec{
+                       []constraintsSpec{
+                               constraintsSpec{},
+                       },
+               },
+               leaf: leafSpec{
+                       sans: []string{"uri:https://example.com/test"},
+               },
+       },
+
+       // #74: an empty URI constraint should also reject everything.
+       nameConstraintsTest{
+               roots: []constraintsSpec{
+                       constraintsSpec{
+                               bad: []string{"uri:"},
+                       },
+               },
+               intermediates: [][]constraintsSpec{
+                       []constraintsSpec{
+                               constraintsSpec{},
+                       },
+               },
+               leaf: leafSpec{
+                       sans: []string{"uri:https://example.com/test"},
+               },
+               expectedError: "\"https://example.com/test\" is excluded",
+       },
 }
 
 func makeConstraintsCACert(constraints constraintsSpec, name string, key *ecdsa.PrivateKey, parent *Certificate, parentKey *ecdsa.PrivateKey) (*Certificate, error) {
@@ -1826,14 +1928,29 @@ func TestRFC2821Parsing(t *testing.T) {
 }
 
 func TestBadNamesInConstraints(t *testing.T) {
+       constraintParseError := func(err error) bool {
+               str := err.Error()
+               return strings.Contains(str, "failed to parse ") && strings.Contains(str, "constraint")
+       }
+
+       encodingError := func(err error) bool {
+               return strings.Contains(err.Error(), "cannot be encoded as an IA5String")
+       }
+
        // Bad names in constraints should not parse.
-       badNames := []string{
-               "dns:foo.com.",
-               "email:abc@foo.com.",
-               "email:foo.com.",
-               "uri:example.com.",
-               "uri:1.2.3.4",
-               "uri:ffff::1",
+       badNames := []struct {
+               name    string
+               matcher func(error) bool
+       }{
+               {"dns:foo.com.", constraintParseError},
+               {"email:abc@foo.com.", constraintParseError},
+               {"email:foo.com.", constraintParseError},
+               {"uri:example.com.", constraintParseError},
+               {"uri:1.2.3.4", constraintParseError},
+               {"uri:ffff::1", constraintParseError},
+               {"dns:not–hyphen.com", encodingError},
+               {"email:foo@not–hyphen.com", encodingError},
+               {"uri:not–hyphen.com", encodingError},
        }
 
        priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
@@ -1841,19 +1958,17 @@ func TestBadNamesInConstraints(t *testing.T) {
                panic(err)
        }
 
-       for _, badName := range badNames {
+       for _, test := range badNames {
                _, err := makeConstraintsCACert(constraintsSpec{
-                       ok: []string{badName},
+                       ok: []string{test.name},
                }, "TestAbsoluteNamesInConstraints", priv, nil, priv)
 
                if err == nil {
-                       t.Errorf("bad name %q unexpectedly accepted in name constraint", badName)
+                       t.Errorf("bad name %q unexpectedly accepted in name constraint", test.name)
                        continue
-               }
-
-               if err != nil {
-                       if str := err.Error(); !strings.Contains(str, "failed to parse ") && !strings.Contains(str, "constraint") {
-                               t.Errorf("bad name %q triggered unrecognised error: %s", badName, str)
+               } else {
+                       if !test.matcher(err) {
+                               t.Errorf("bad name %q triggered unrecognised error: %s", test.name, err)
                        }
                }
        }
index 915cd2e4548d5eaaed86db08079fd7e0ae7b9ae3..db819b0142e8b3f689c2c92312d5e664a1d419e8 100644 (file)
@@ -31,6 +31,10 @@ import (
        "strconv"
        "strings"
        "time"
+       "unicode/utf8"
+
+       "golang_org/x/crypto/cryptobyte"
+       cryptobyte_asn1 "golang_org/x/crypto/cryptobyte/asn1"
 )
 
 // pkixPublicKey reflects a PKIX public key structure. See SubjectPublicKeyInfo
@@ -953,12 +957,6 @@ type policyInformation struct {
        // policyQualifiers omitted
 }
 
-// RFC 5280, 4.2.1.10
-type nameConstraints struct {
-       Permitted []generalSubtree `asn1:"optional,tag:0"`
-       Excluded  []generalSubtree `asn1:"optional,tag:1"`
-}
-
 const (
        nameTypeEmail = 1
        nameTypeDNS   = 2
@@ -966,13 +964,6 @@ const (
        nameTypeIP    = 7
 )
 
-type generalSubtree struct {
-       Email     string `asn1:"tag:1,optional,ia5"`
-       Name      string `asn1:"tag:2,optional,ia5"`
-       URIDomain string `asn1:"tag:6,optional,ia5"`
-       IPAndMask []byte `asn1:"tag:7,optional"`
-}
-
 // RFC 5280, 4.2.2.1
 type authorityInfoAccess struct {
        Method   asn1.ObjectIdentifier
@@ -1207,14 +1198,18 @@ func parseNameConstraintsExtension(out *Certificate, e pkix.Extension) (unhandle
        //
        // BaseDistance ::= INTEGER (0..MAX)
 
-       var constraints nameConstraints
-       if rest, err := asn1.Unmarshal(e.Value, &constraints); err != nil {
-               return false, err
-       } else if len(rest) != 0 {
-               return false, errors.New("x509: trailing data after X.509 NameConstraints")
+       outer := cryptobyte.String(e.Value)
+       var toplevel, permitted, excluded cryptobyte.String
+       var havePermitted, haveExcluded bool
+       if !outer.ReadASN1(&toplevel, cryptobyte_asn1.SEQUENCE) ||
+               !outer.Empty() ||
+               !toplevel.ReadOptionalASN1(&permitted, &havePermitted, cryptobyte_asn1.Tag(0).ContextSpecific().Constructed()) ||
+               !toplevel.ReadOptionalASN1(&excluded, &haveExcluded, cryptobyte_asn1.Tag(1).ContextSpecific().Constructed()) ||
+               !toplevel.Empty() {
+               return false, errors.New("x509: invalid NameConstraints extension")
        }
 
-       if len(constraints.Permitted) == 0 && len(constraints.Excluded) == 0 {
+       if !havePermitted && !haveExcluded || len(permitted) == 0 && len(excluded) == 0 {
                // https://tools.ietf.org/html/rfc5280#section-4.2.1.10:
                //   “either the permittedSubtrees field
                //   or the excludedSubtrees MUST be
@@ -1222,35 +1217,55 @@ func parseNameConstraintsExtension(out *Certificate, e pkix.Extension) (unhandle
                return false, errors.New("x509: empty name constraints extension")
        }
 
-       getValues := func(subtrees []generalSubtree) (dnsNames []string, ips []*net.IPNet, emails, uriDomains []string, err error) {
-               for _, subtree := range subtrees {
-                       switch {
-                       case len(subtree.Name) != 0:
-                               domain := subtree.Name
-                               if len(domain) > 0 && domain[0] == '.' {
+       getValues := func(subtrees cryptobyte.String) (dnsNames []string, ips []*net.IPNet, emails, uriDomains []string, err error) {
+               for !subtrees.Empty() {
+                       var seq, value cryptobyte.String
+                       var tag cryptobyte_asn1.Tag
+                       if !subtrees.ReadASN1(&seq, cryptobyte_asn1.SEQUENCE) ||
+                               !seq.ReadAnyASN1(&value, &tag) ||
+                               !seq.Empty() {
+                               return nil, nil, nil, nil, fmt.Errorf("x509: invalid NameConstraints extension")
+                       }
+
+                       var (
+                               dnsTag   = cryptobyte_asn1.Tag(2).ContextSpecific()
+                               emailTag = cryptobyte_asn1.Tag(1).ContextSpecific()
+                               ipTag    = cryptobyte_asn1.Tag(7).ContextSpecific()
+                               uriTag   = cryptobyte_asn1.Tag(6).ContextSpecific()
+                       )
+
+                       switch tag {
+                       case dnsTag:
+                               domain := string(value)
+                               if err := isIA5String(domain); err != nil {
+                                       return nil, nil, nil, nil, errors.New("x509: invalid constraint value: " + err.Error())
+                               }
+
+                               trimmedDomain := domain
+                               if len(trimmedDomain) > 0 && trimmedDomain[0] == '.' {
                                        // constraints can have a leading
                                        // period to exclude the domain
                                        // itself, but that's not valid in a
                                        // normal domain name.
-                                       domain = domain[1:]
+                                       trimmedDomain = trimmedDomain[1:]
                                }
-                               if _, ok := domainToReverseLabels(domain); !ok {
-                                       return nil, nil, nil, nil, fmt.Errorf("x509: failed to parse dnsName constraint %q", subtree.Name)
+                               if _, ok := domainToReverseLabels(trimmedDomain); !ok {
+                                       return nil, nil, nil, nil, fmt.Errorf("x509: failed to parse dnsName constraint %q", domain)
                                }
-                               dnsNames = append(dnsNames, subtree.Name)
+                               dnsNames = append(dnsNames, domain)
 
-                       case len(subtree.IPAndMask) != 0:
-                               l := len(subtree.IPAndMask)
+                       case ipTag:
+                               l := len(value)
                                var ip, mask []byte
 
                                switch l {
                                case 8:
-                                       ip = subtree.IPAndMask[:4]
-                                       mask = subtree.IPAndMask[4:]
+                                       ip = value[:4]
+                                       mask = value[4:]
 
                                case 32:
-                                       ip = subtree.IPAndMask[:16]
-                                       mask = subtree.IPAndMask[16:]
+                                       ip = value[:16]
+                                       mask = value[16:]
 
                                default:
                                        return nil, nil, nil, nil, fmt.Errorf("x509: IP constraint contained value of length %d", l)
@@ -1262,8 +1277,12 @@ func parseNameConstraintsExtension(out *Certificate, e pkix.Extension) (unhandle
 
                                ips = append(ips, &net.IPNet{IP: net.IP(ip), Mask: net.IPMask(mask)})
 
-                       case len(subtree.Email) != 0:
-                               constraint := subtree.Email
+                       case emailTag:
+                               constraint := string(value)
+                               if err := isIA5String(constraint); err != nil {
+                                       return nil, nil, nil, nil, errors.New("x509: invalid constraint value: " + err.Error())
+                               }
+
                                // If the constraint contains an @ then
                                // it specifies an exact mailbox name.
                                if strings.Contains(constraint, "@") {
@@ -1282,24 +1301,28 @@ func parseNameConstraintsExtension(out *Certificate, e pkix.Extension) (unhandle
                                }
                                emails = append(emails, constraint)
 
-                       case len(subtree.URIDomain) != 0:
-                               domain := subtree.URIDomain
+                       case uriTag:
+                               domain := string(value)
+                               if err := isIA5String(domain); err != nil {
+                                       return nil, nil, nil, nil, errors.New("x509: invalid constraint value: " + err.Error())
+                               }
 
                                if net.ParseIP(domain) != nil {
-                                       return nil, nil, nil, nil, fmt.Errorf("x509: failed to parse URI constraint %q: cannot be IP address", subtree.URIDomain)
+                                       return nil, nil, nil, nil, fmt.Errorf("x509: failed to parse URI constraint %q: cannot be IP address", domain)
                                }
 
-                               if len(domain) > 0 && domain[0] == '.' {
+                               trimmedDomain := domain
+                               if len(trimmedDomain) > 0 && trimmedDomain[0] == '.' {
                                        // constraints can have a leading
-                                       // period to exclude the domain
-                                       // itself, but that's not valid in a
-                                       // normal domain name.
-                                       domain = domain[1:]
+                                       // period to exclude the domain itself,
+                                       // but that's not valid in a normal
+                                       // domain name.
+                                       trimmedDomain = trimmedDomain[1:]
                                }
-                               if _, ok := domainToReverseLabels(domain); !ok {
-                                       return nil, nil, nil, nil, fmt.Errorf("x509: failed to parse URI constraint %q", subtree.URIDomain)
+                               if _, ok := domainToReverseLabels(trimmedDomain); !ok {
+                                       return nil, nil, nil, nil, fmt.Errorf("x509: failed to parse URI constraint %q", domain)
                                }
-                               uriDomains = append(uriDomains, subtree.URIDomain)
+                               uriDomains = append(uriDomains, domain)
 
                        default:
                                unhandled = true
@@ -1309,10 +1332,10 @@ func parseNameConstraintsExtension(out *Certificate, e pkix.Extension) (unhandle
                return dnsNames, ips, emails, uriDomains, nil
        }
 
-       if out.PermittedDNSDomains, out.PermittedIPRanges, out.PermittedEmailAddresses, out.PermittedURIDomains, err = getValues(constraints.Permitted); err != nil {
+       if out.PermittedDNSDomains, out.PermittedIPRanges, out.PermittedEmailAddresses, out.PermittedURIDomains, err = getValues(permitted); err != nil {
                return false, err
        }
-       if out.ExcludedDNSDomains, out.ExcludedIPRanges, out.ExcludedEmailAddresses, out.ExcludedURIDomains, err = getValues(constraints.Excluded); err != nil {
+       if out.ExcludedDNSDomains, out.ExcludedIPRanges, out.ExcludedEmailAddresses, out.ExcludedURIDomains, err = getValues(excluded); err != nil {
                return false, err
        }
        out.PermittedDNSDomainsCritical = e.Critical
@@ -1670,6 +1693,16 @@ func marshalSANs(dnsNames, emailAddresses []string, ipAddresses []net.IP, uris [
        return asn1.Marshal(rawValues)
 }
 
+func isIA5String(s string) error {
+       for _, r := range s {
+               if r >= utf8.RuneSelf {
+                       return fmt.Errorf("x509: %q cannot be encoded as an IA5String", s)
+               }
+       }
+
+       return nil
+}
+
 func buildExtensions(template *Certificate, authorityKeyId []byte) (ret []pkix.Extension, err error) {
        ret = make([]pkix.Extension, 10 /* maximum number of elements. */)
        n := 0
@@ -1808,8 +1841,6 @@ func buildExtensions(template *Certificate, authorityKeyId []byte) (ret []pkix.E
                ret[n].Id = oidExtensionNameConstraints
                ret[n].Critical = template.PermittedDNSDomainsCritical
 
-               var out nameConstraints
-
                ipAndMask := func(ipNet *net.IPNet) []byte {
                        maskedIP := ipNet.IP.Mask(ipNet.Mask)
                        ipAndMask := make([]byte, 0, len(maskedIP)+len(ipNet.Mask))
@@ -1818,37 +1849,84 @@ func buildExtensions(template *Certificate, authorityKeyId []byte) (ret []pkix.E
                        return ipAndMask
                }
 
-               out.Permitted = make([]generalSubtree, 0, len(template.PermittedDNSDomains)+len(template.PermittedIPRanges))
-               for _, permitted := range template.PermittedDNSDomains {
-                       out.Permitted = append(out.Permitted, generalSubtree{Name: permitted})
-               }
-               for _, permitted := range template.PermittedIPRanges {
-                       out.Permitted = append(out.Permitted, generalSubtree{IPAndMask: ipAndMask(permitted)})
-               }
-               for _, permitted := range template.PermittedEmailAddresses {
-                       out.Permitted = append(out.Permitted, generalSubtree{Email: permitted})
-               }
-               for _, permitted := range template.PermittedURIDomains {
-                       out.Permitted = append(out.Permitted, generalSubtree{URIDomain: permitted})
-               }
+               serialiseConstraints := func(dns []string, ips []*net.IPNet, emails []string, uriDomains []string) (der []byte, err error) {
+                       var b cryptobyte.Builder
 
-               out.Excluded = make([]generalSubtree, 0, len(template.ExcludedDNSDomains)+len(template.ExcludedIPRanges))
-               for _, excluded := range template.ExcludedDNSDomains {
-                       out.Excluded = append(out.Excluded, generalSubtree{Name: excluded})
-               }
-               for _, excluded := range template.ExcludedIPRanges {
-                       out.Excluded = append(out.Excluded, generalSubtree{IPAndMask: ipAndMask(excluded)})
+                       for _, name := range dns {
+                               if err = isIA5String(name); err != nil {
+                                       return nil, err
+                               }
+
+                               b.AddASN1(cryptobyte_asn1.SEQUENCE, func(b *cryptobyte.Builder) {
+                                       b.AddASN1(cryptobyte_asn1.Tag(2).ContextSpecific(), func(b *cryptobyte.Builder) {
+                                               b.AddBytes([]byte(name))
+                                       })
+                               })
+                       }
+
+                       for _, ipNet := range ips {
+                               b.AddASN1(cryptobyte_asn1.SEQUENCE, func(b *cryptobyte.Builder) {
+                                       b.AddASN1(cryptobyte_asn1.Tag(7).ContextSpecific(), func(b *cryptobyte.Builder) {
+                                               b.AddBytes(ipAndMask(ipNet))
+                                       })
+                               })
+                       }
+
+                       for _, email := range emails {
+                               if err = isIA5String(email); err != nil {
+                                       return nil, err
+                               }
+
+                               b.AddASN1(cryptobyte_asn1.SEQUENCE, func(b *cryptobyte.Builder) {
+                                       b.AddASN1(cryptobyte_asn1.Tag(1).ContextSpecific(), func(b *cryptobyte.Builder) {
+                                               b.AddBytes([]byte(email))
+                                       })
+                               })
+                       }
+
+                       for _, uriDomain := range uriDomains {
+                               if err = isIA5String(uriDomain); err != nil {
+                                       return nil, err
+                               }
+
+                               b.AddASN1(cryptobyte_asn1.SEQUENCE, func(b *cryptobyte.Builder) {
+                                       b.AddASN1(cryptobyte_asn1.Tag(6).ContextSpecific(), func(b *cryptobyte.Builder) {
+                                               b.AddBytes([]byte(uriDomain))
+                                       })
+                               })
+                       }
+
+                       return b.Bytes()
                }
-               for _, excluded := range template.ExcludedEmailAddresses {
-                       out.Excluded = append(out.Excluded, generalSubtree{Email: excluded})
+
+               permitted, err := serialiseConstraints(template.PermittedDNSDomains, template.PermittedIPRanges, template.PermittedEmailAddresses, template.PermittedURIDomains)
+               if err != nil {
+                       return nil, err
                }
-               for _, excluded := range template.ExcludedURIDomains {
-                       out.Excluded = append(out.Excluded, generalSubtree{URIDomain: excluded})
+
+               excluded, err := serialiseConstraints(template.ExcludedDNSDomains, template.ExcludedIPRanges, template.ExcludedEmailAddresses, template.ExcludedURIDomains)
+               if err != nil {
+                       return nil, err
                }
 
-               ret[n].Value, err = asn1.Marshal(out)
+               var b cryptobyte.Builder
+               b.AddASN1(cryptobyte_asn1.SEQUENCE, func(b *cryptobyte.Builder) {
+                       if len(permitted) > 0 {
+                               b.AddASN1(cryptobyte_asn1.Tag(0).ContextSpecific().Constructed(), func(b *cryptobyte.Builder) {
+                                       b.AddBytes(permitted)
+                               })
+                       }
+
+                       if len(excluded) > 0 {
+                               b.AddASN1(cryptobyte_asn1.Tag(1).ContextSpecific().Constructed(), func(b *cryptobyte.Builder) {
+                                       b.AddBytes(excluded)
+                               })
+                       }
+               })
+
+               ret[n].Value, err = b.Bytes()
                if err != nil {
-                       return
+                       return nil, err
                }
                n++
        }
index 66d4157d636f30fc343a1899563f8f14a32842bc..5ab4cedd5170fdb06593fd796f4eebae2e510744 100644 (file)
@@ -378,6 +378,7 @@ var pkgDeps = map[string][]string{
        "crypto/x509": {
                "L4", "CRYPTO-MATH", "OS", "CGO",
                "crypto/x509/pkix", "encoding/pem", "encoding/hex", "net", "os/user", "syscall", "net/url",
+               "golang_org/x/crypto/cryptobyte", "golang_org/x/crypto/cryptobyte/asn1",
        },
        "crypto/x509/pkix": {"L4", "CRYPTO-MATH", "encoding/hex"},