From d005736213d7c9518ea1e05c63826783839dbed6 Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Sun, 29 Oct 2017 19:20:33 -0700 Subject: [PATCH] crypto/x509: handle name constraints with cryptobyte 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 --- src/crypto/x509/name_constraints_test.go | 151 +++++++++++++-- src/crypto/x509/x509.go | 232 +++++++++++++++-------- src/go/build/deps_test.go | 1 + 3 files changed, 289 insertions(+), 95 deletions(-) diff --git a/src/crypto/x509/name_constraints_test.go b/src/crypto/x509/name_constraints_test.go index 8b7845ea42..e75770b84d 100644 --- a/src/crypto/x509/name_constraints_test.go +++ b/src/crypto/x509/name_constraints_test.go @@ -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) } } } diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go index 915cd2e454..db819b0142 100644 --- a/src/crypto/x509/x509.go +++ b/src/crypto/x509/x509.go @@ -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++ } diff --git a/src/go/build/deps_test.go b/src/go/build/deps_test.go index 66d4157d63..5ab4cedd51 100644 --- a/src/go/build/deps_test.go +++ b/src/go/build/deps_test.go @@ -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"}, -- 2.48.1