crypto/x509: parse CSRs with a critical flag in the requested extensions.

The format for a CSR is horribly underspecified and we had a mistake.
The code was parsing the attributes from the CSR as a
pkix.AttributeTypeAndValueSET, which is only almost correct: it works so
long as the requested extensions don't contain the optional “critical”
flag.

Unfortunately this mistake is exported somewhat in the API and the
Attributes field of a CSR actually has the wrong type. I've moved this
field to the bottom of the structure and updated the comment to reflect
this.

The Extensions and other fields of the CSR structure can be saved
however and this change does that.

Fixes #11897.

Change-Id: If8e2f5c21934800b72b041e38691efc3e897ecf1
Reviewed-on: https://go-review.googlesource.com/12717
Reviewed-by: Rob Pike <r@golang.org>

diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index 0431f87cd00ac23a219b97b3f47b7831701d21bc..bbc63241c649e5672621dbcede3ad0b7e2dc8d3f 100644 (file)
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -1707,9 +1707,7 @@ type CertificateRequest struct {
 
        Subject pkix.Name
 
-       // Attributes is a collection of attributes providing
-       // additional information about the subject of the certificate.
-       // See RFC 2986 section 4.1.
+       // Attributes is the dried husk of a bug and shouldn't be used.
        Attributes []pkix.AttributeTypeAndValueSET
 
        // Extensions contains raw X.509 extensions. When parsing CSRs, this
@@ -1784,6 +1782,38 @@ func parseRawAttributes(rawAttributes []asn1.RawValue) []pkix.AttributeTypeAndVa
        return attributes
 }
 
+// parseCSRExtensions parses the attributes from a CSR and extracts any
+// requested extensions.
+func parseCSRExtensions(rawAttributes []asn1.RawValue) ([]pkix.Extension, error) {
+       // pkcs10Attribute reflects the Attribute structure from section 4.1 of
+       // https://tools.ietf.org/html/rfc2986.
+       type pkcs10Attribute struct {
+               Id     asn1.ObjectIdentifier
+               Values []asn1.RawValue `asn1:"set"`
+       }
+
+       var ret []pkix.Extension
+       for _, rawAttr := range rawAttributes {
+               var attr pkcs10Attribute
+               if rest, err := asn1.Unmarshal(rawAttr.FullBytes, &attr); err != nil || len(rest) != 0 || len(attr.Values) == 0 {
+                       // Ignore attributes that don't parse.
+                       continue
+               }
+
+               if !attr.Id.Equal(oidExtensionRequest) {
+                       continue
+               }
+
+               var extensions []pkix.Extension
+               if _, err := asn1.Unmarshal(attr.Values[0].FullBytes, &extensions); err != nil {
+                       return nil, err
+               }
+               ret = append(ret, extensions...)
+       }
+
+       return ret, nil
+}
+
 // CreateCertificateRequest creates a new certificate based on a template. The
 // following members of template are used: Subject, Attributes,
 // SignatureAlgorithm, Extensions, DNSNames, EmailAddresses, and IPAddresses.
@@ -1986,38 +2016,15 @@ func parseCertificateRequest(in *certificateRequest) (*CertificateRequest, error
 
        out.Subject.FillFromRDNSequence(&subject)
 
-       var extensions []pkix.AttributeTypeAndValue
-
-       for _, atvSet := range out.Attributes {
-               if !atvSet.Type.Equal(oidExtensionRequest) {
-                       continue
-               }
-
-               for _, atvs := range atvSet.Value {
-                       extensions = append(extensions, atvs...)
-               }
+       if out.Extensions, err = parseCSRExtensions(in.TBSCSR.RawAttributes); err != nil {
+               return nil, err
        }
 
-       out.Extensions = make([]pkix.Extension, 0, len(extensions))
-
-       for _, e := range extensions {
-               value, ok := e.Value.([]byte)
-               if !ok {
-                       return nil, errors.New("x509: extension attribute contained non-OCTET STRING data")
-               }
-
-               out.Extensions = append(out.Extensions, pkix.Extension{
-                       Id:    e.Type,
-                       Value: value,
-               })
-
-               if len(e.Type) == 4 && e.Type[0] == 2 && e.Type[1] == 5 && e.Type[2] == 29 {
-                       switch e.Type[3] {
-                       case 17:
-                               out.DNSNames, out.EmailAddresses, out.IPAddresses, err = parseSANExtension(value)
-                               if err != nil {
-                                       return nil, err
-                               }
+       for _, extension := range out.Extensions {
+               if extension.Id.Equal(oidExtensionSubjectAltName) {
+                       out.DNSNames, out.EmailAddresses, out.IPAddresses, err = parseSANExtension(extension.Value)
+                       if err != nil {
+                               return nil, err
                        }
                }
        }

diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index 36dbc479319250010f900285a4834bb53625e7d7..61b1773745363789455c5b051d1a7fd893688464 100644 (file)
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -1074,6 +1074,40 @@ func TestParseCertificateRequest(t *testing.T) {
        }
 }
 
+func TestCriticalFlagInCSRRequestedExtensions(t *testing.T) {
+       // This CSR contains an extension request where the extensions have a
+       // critical flag in them. In the past we failed to handle this.
+       const csrBase64 = "MIICrTCCAZUCAQIwMzEgMB4GA1UEAwwXU0NFUCBDQSBmb3IgRGV2ZWxlciBTcmwxDzANBgNVBAsMBjQzNTk3MTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBALFMAJ7Zy9YyfgbNlbUWAW0LalNRMPs7aXmLANsCpjhnw3lLlfDPaLeWyKh1nK5I5ojaJOW6KIOSAcJkDUe3rrE0wR0RVt3UxArqs0R/ND3u5Q+bDQY2X1HAFUHzUzcdm5JRAIA355v90teMckaWAIlkRQjDE22Lzc6NAl64KOd1rqOUNj8+PfX6fSo20jm94Pp1+a6mfk3G/RUWVuSm7owO5DZI/Fsi2ijdmb4NUar6K/bDKYTrDFkzcqAyMfP3TitUtBp19Mp3B1yAlHjlbp/r5fSSXfOGHZdgIvp0WkLuK2u5eQrX5l7HMB/5epgUs3HQxKY6ljhh5wAjDwz//LsCAwEAAaA1MDMGCSqGSIb3DQEJDjEmMCQwEgYDVR0TAQH/BAgwBgEB/wIBADAOBgNVHQ8BAf8EBAMCAoQwDQYJKoZIhvcNAQEFBQADggEBAAMq3bxJSPQEgzLYR/yaVvgjCDrc3zUbIwdOis6Go06Q4RnjH5yRaSZAqZQTDsPurQcnz2I39VMGEiSkFJFavf4QHIZ7QFLkyXadMtALc87tm17Ej719SbHcBSSZayR9VYJUNXRLayI6HvyUrmqcMKh+iX3WY3ICr59/wlM0tYa8DYN4yzmOa2Onb29gy3YlaF5A2AKAMmk003cRT9gY26mjpv7d21czOSSeNyVIoZ04IR9ee71vWTMdv0hu/af5kSjQ+ZG5/Qgc0+mnECLz/1gtxt1srLYbtYQ/qAY8oX1DCSGFS61tN/vl+4cxGMD/VGcGzADRLRHSlVqy2Qgss6Q="
+
+       csrBytes := fromBase64(csrBase64)
+       csr, err := ParseCertificateRequest(csrBytes)
+       if err != nil {
+               t.Fatalf("failed to parse CSR: %s", err)
+       }
+
+       expected := []struct{
+               Id asn1.ObjectIdentifier
+               Value []byte
+       }{
+               {oidExtensionBasicConstraints, fromBase64("MAYBAf8CAQA=")},
+               {oidExtensionKeyUsage, fromBase64("AwIChA==")},
+       }
+
+       if n := len(csr.Extensions); n != len(expected) {
+               t.Fatalf("expected to find %d extensions but found %d", len(expected), n)
+       }
+
+       for i, extension := range csr.Extensions {
+               if !extension.Id.Equal(expected[i].Id) {
+                       t.Fatalf("extension #%d has unexpected type %v (expected %v)", i, extension.Id, expected[i].Id)
+               }
+
+               if !bytes.Equal(extension.Value, expected[i].Value) {
+                       t.Fatalf("extension #%d has unexpected contents %x (expected %x)", i, extension.Value, expected[i].Value)
+               }
+       }
+}
+
 func TestMaxPathLen(t *testing.T) {
        block, _ := pem.Decode([]byte(pemPrivateKey))
        rsaPriv, err := ParsePKCS1PrivateKey(block.Bytes)