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 0431f87..bbc63241 100644
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -1707,9 +1707,7 @@
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 @@
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 @@
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 36dbc47..61b1773 100644
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -1074,6 +1074,40 @@
}
}
+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)
