crypto/x509: create CRLs with Issuer.RawSubject

Per discussion with Roland Shoemaker, this updates
x509.CreateRevocationList to mirror the behavior of
x509.CreateCertificate, creating an internal struct for the ASN.1
encoding of the CRL. This allows us to switch the Issuer field type to
asn1.RawValue, bypassing the round-tripping issues of pkix.Name in most
scenarios.

Per linked ticket, this resolves issues where a non-Go created
certificate can be used to create CRLs which aren't correctly attested
to that certificate. In rare cases where the CRL issuer is validated
against the certificate's issuer (such as the linked JDK example), this
can result in failing to check this CRL for the candidate certificate.

Fixes #53754

Change-Id: If0adc053c081d6fb0b1ce47324c877eb2429a51f
GitHub-Last-Rev: 033115dd5eb93295330eb151ff8557df5bffbec8
GitHub-Pull-Request: golang/go#53985
Reviewed-on: https://go-review.googlesource.com/c/go/+/418834
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>

diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index 35394bfc33dbda50133cb9bc491c14888ad35d87..df86c65939ad9af17c9055a5bab638c32a4976e1 100644 (file)
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -2149,6 +2149,29 @@ type RevocationList struct {
        ExtraExtensions []pkix.Extension
 }
 
+// These structures reflect the ASN.1 structure of X.509 CRLs better than
+// the existing crypto/x509/pkix variants do. These mirror the existing
+// certificate structs in this file.
+//
+// Notably, we include issuer as an asn1.RawValue, mirroring the behavior of
+// tbsCertificate and allowing raw (unparsed) subjects to be passed cleanly.
+type certificateList struct {
+       TBSCertList        tbsCertificateList
+       SignatureAlgorithm pkix.AlgorithmIdentifier
+       SignatureValue     asn1.BitString
+}
+
+type tbsCertificateList struct {
+       Raw                 asn1.RawContent
+       Version             int `asn1:"optional,default:0"`
+       Signature           pkix.AlgorithmIdentifier
+       Issuer              asn1.RawValue
+       ThisUpdate          time.Time
+       NextUpdate          time.Time                 `asn1:"optional"`
+       RevokedCertificates []pkix.RevokedCertificate `asn1:"optional"`
+       Extensions          []pkix.Extension          `asn1:"tag:0,optional,explicit"`
+}
+
 // CreateRevocationList creates a new X.509 v2 Certificate Revocation List,
 // according to RFC 5280, based on template.
 //
@@ -2206,10 +2229,16 @@ func CreateRevocationList(rand io.Reader, template *RevocationList, issuer *Cert
                return nil, err
        }
 
-       tbsCertList := pkix.TBSCertificateList{
+       // Correctly use the issuer's subject sequence if one is specified.
+       issuerSubject, err := subjectBytes(issuer)
+       if err != nil {
+               return nil, err
+       }
+
+       tbsCertList := tbsCertificateList{
                Version:    1, // v2
                Signature:  signatureAlgorithm,
-               Issuer:     issuer.Subject.ToRDNSequence(),
+               Issuer:     asn1.RawValue{FullBytes: issuerSubject},
                ThisUpdate: template.ThisUpdate.UTC(),
                NextUpdate: template.NextUpdate.UTC(),
                Extensions: []pkix.Extension{
@@ -2236,6 +2265,10 @@ func CreateRevocationList(rand io.Reader, template *RevocationList, issuer *Cert
                return nil, err
        }
 
+       // Optimization to only marshal this struct once, when signing and
+       // then embedding in certificateList below.
+       tbsCertList.Raw = tbsCertListContents
+
        input := tbsCertListContents
        if hashFunc != 0 {
                h := hashFunc.New()
@@ -2255,7 +2288,7 @@ func CreateRevocationList(rand io.Reader, template *RevocationList, issuer *Cert
                return nil, err
        }
 
-       return asn1.Marshal(pkix.CertificateList{
+       return asn1.Marshal(certificateList{
                TBSCertList:        tbsCertList,
                SignatureAlgorithm: signatureAlgorithm,
                SignatureValue:     asn1.BitString{Bytes: signature, BitLength: len(signature) * 8},

diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index 5690e200b271ba50890a692ab13408660400b704..44441d45d49d8ac51a8dcd6da84d77ba90b31e8d 100644 (file)
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -2419,6 +2419,18 @@ func TestCreateRevocationList(t *testing.T) {
        if err != nil {
                t.Fatalf("Failed to generate Ed25519 key: %s", err)
        }
+
+       // Generation command:
+       // openssl req -x509 -newkey rsa -keyout key.pem -out cert.pem -days 365 -nodes -subj '/C=US/ST=California/L=San Francisco/O=Internet Widgets, Inc./OU=WWW/CN=Root/emailAddress=admin@example.com' -sha256 -addext basicConstraints=CA:TRUE -addext "keyUsage = digitalSignature, keyEncipherment, dataEncipherment, cRLSign, keyCertSign" -utf8
+       utf8CAStr := "MIIEITCCAwmgAwIBAgIUXHXy7NdtDv+ClaHvIvlwCYiI4a4wDQYJKoZIhvcNAQELBQAwgZoxCzAJBgNVBAYTAlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMRYwFAYDVQQHDA1TYW4gRnJhbmNpc2NvMR8wHQYDVQQKDBZJbnRlcm5ldCBXaWRnZXRzLCBJbmMuMQwwCgYDVQQLDANXV1cxDTALBgNVBAMMBFJvb3QxIDAeBgkqhkiG9w0BCQEWEWFkbWluQGV4YW1wbGUuY29tMB4XDTIyMDcwODE1MzgyMFoXDTIzMDcwODE1MzgyMFowgZoxCzAJBgNVBAYTAlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMRYwFAYDVQQHDA1TYW4gRnJhbmNpc2NvMR8wHQYDVQQKDBZJbnRlcm5ldCBXaWRnZXRzLCBJbmMuMQwwCgYDVQQLDANXV1cxDTALBgNVBAMMBFJvb3QxIDAeBgkqhkiG9w0BCQEWEWFkbWluQGV4YW1wbGUuY29tMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAmXvp0WNjsZzySWT7Ce5zewQNKq8ujeZGphJ44Vdrwut/b6TcC4iYENds5+7/3PYwBllp3K5TRpCcafSxdhJsvA7/zWlHHNRcJhJLNt9qsKWP6ukI2Iw6OmFMg6kJQ8f67RXkT8HR3v0UqE+lWrA0g+oRuj4erLtfOtSpnl4nsE/Rs2qxbELFWAf7F5qMqH4dUyveWKrNT8eI6YQN+wBg0MAjoKRvDJnBhuo+IvvXX8Aq1QWUcBGPK3or/Ehxy5f/gEmSUXyEU1Ht/vATt2op+eRaEEpBdGRvO+DrKjlcQV2XMN18A9LAX6hCzH43sGye87dj7RZ9yj+waOYNaM7kFQIDAQABo10wWzAdBgNVHQ4EFgQUtbSlrW4hGL2kNjviM6wcCRwvOEEwHwYDVR0jBBgwFoAUtbSlrW4hGL2kNjviM6wcCRwvOEEwDAYDVR0TBAUwAwEB/zALBgNVHQ8EBAMCAbYwDQYJKoZIhvcNAQELBQADggEBAAko82YNNI2n/45L3ya21vufP6nZihIOIxgcRPUMX+IDJZk16qsFdcLgH3KAP8uiVLn8sULuCj35HpViR4IcAk2d+DqfG11l8kY+e5P7nYsViRfy0AatF59/sYlWf+3RdmPXfL70x4mE9OqlMdDm0kR2obps8rng83VLDNvj3R5sBnQwdw6LKLGzaE+RiCTmkH0+P6vnbOJ33su9+9al1+HvJUg3UM1Xq5Bw7TE8DQTetMV3c2Q35RQaJB9pQ4blJOnW9hfnt8yQzU6TU1bU4mRctTm1o1f8btPqUpi+/blhi5MUJK0/myj1XD00pmyfp8QAFl1EfqmTMIBMLg633A0="
+       utf8CABytes, _ := base64.StdEncoding.DecodeString(utf8CAStr)
+       utf8CA, _ := ParseCertificate(utf8CABytes)
+
+       utf8KeyStr := "MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQCZe+nRY2OxnPJJZPsJ7nN7BA0qry6N5kamEnjhV2vC639vpNwLiJgQ12zn7v/c9jAGWWncrlNGkJxp9LF2Emy8Dv/NaUcc1FwmEks232qwpY/q6QjYjDo6YUyDqQlDx/rtFeRPwdHe/RSoT6VasDSD6hG6Ph6su1861KmeXiewT9GzarFsQsVYB/sXmoyofh1TK95Yqs1Px4jphA37AGDQwCOgpG8MmcGG6j4i+9dfwCrVBZRwEY8reiv8SHHLl/+ASZJRfIRTUe3+8BO3ain55FoQSkF0ZG874OsqOVxBXZcw3XwD0sBfqELMfjewbJ7zt2PtFn3KP7Bo5g1ozuQVAgMBAAECggEAIscjKiD9PAe2Fs9c2tk/LYazfRKI1/pv072nylfGwToffCq8+ZgP7PEDamKLc4QNScME685MbFbkOlYJyBlQriQv7lmGlY/A+Zd3l410XWaGf9IiAP91Sjk13zd0M/micApf23qtlXt/LMwvSadXnvRw4+SjirxCTdBWRt5K2/ZAN550v7bHFk1EZc3UBF6sOoNsjQWh9Ek79UmQYJBPiZDBHO7O2fh2GSIbUutTma+Tb2i1QUZzg+AG3cseF3p1i3uhNrCh+p+01bJSzGTQsRod2xpD1tpWwR3kIftCOmD1XnhpaBQi7PXjEuNbfucaftnoYj2ShDdmgD5RkkbTAQKBgQC8Ghu5MQ/yIeqXg9IpcSxuWtUEAEfK33/cC/IvuntgbNEnWQm5Lif4D6a9zjkxiCS+9HhrUu5U2EV8NxOyaqmtub3Np1Z5mPuI9oiZ119bjUJd4X+jKOTaePWvOv/rL/pTHYqzXohVMrXy+DaTIq4lOcv3n72SuhuTcKU95rhKtQKBgQDQ4t+HsRZd5fJzoCgRQhlNK3EbXQDv2zXqMW3GfpF7GaDP18I530inRURSJa++rvi7/MCFg/TXVS3QC4HXtbzTYTqhE+VHzSr+/OcsqpLE8b0jKBDv/SBkz811PUJDs3LsX31DT3K0zUpMpNSd/5SYTyJKef9L6mxmwlC1S2Yv4QKBgQC57SiYDdnQIRwrtZ2nXvlm/xttAAX2jqJoU9qIuNA4yHaYaRcGVowlUvsiw9OelQ6VPTpGA0wWy0src5lhkrKzSFRHEe+U89U1VVJCljLoYKFIAJvUH5jOJh/am/vYca0COMIfeAJUDHLyfcwb9XyiyRVGZzvP62tUelSq8gIZvQKBgCAHeaDzzWsudCO4ngwvZ3PGwnwgoaElqrmzRJLYG3SVtGvKOJTpINnNLDGwZ6dEaw1gLyEJ38QY4oJxEULDMiXzVasXQuPkmMAqhUP7D7A1JPw8C4TQ+mOa3XUppHx/CpMl/S4SA5OnmsnvyE5Fv0IveCGVXUkFtAN5rihuXEfhAoGANUkuGU3A0Upk2mzv0JTGP4H95JFG93cqnyPNrYs30M6RkZNgTW27yyr+Nhs4/cMdrg1AYTB0+6ItQWSDmYLs7JEbBE/8L8fdD1irIcygjIHE9nJh96TgZCt61kVGLE8758lOdmoB2rZOpGwi16QIhdQb+IyozYqfX+lQUojL/W0="
+       utf8KeyBytes, _ := base64.StdEncoding.DecodeString(utf8KeyStr)
+       utf8KeyRaw, _ := ParsePKCS8PrivateKey(utf8KeyBytes)
+       utf8Key := utf8KeyRaw.(crypto.Signer)
+
        tests := []struct {
                name          string
                key           crypto.Signer
@@ -2687,6 +2699,16 @@ func TestCreateRevocationList(t *testing.T) {
                                NextUpdate: time.Time{}.Add(time.Hour * 48),
                        },
                },
+               {
+                       name:   "valid CA with utf8 Subject fields including Email, empty list",
+                       key:    utf8Key,
+                       issuer: utf8CA,
+                       template: &RevocationList{
+                               Number:     big.NewInt(5),
+                               ThisUpdate: time.Time{}.Add(time.Hour * 24),
+                               NextUpdate: time.Time{}.Add(time.Hour * 48),
+                       },
+               },
        }
 
        for _, tc := range tests {
@@ -2746,6 +2768,33 @@ func TestCreateRevocationList(t *testing.T) {
                                t.Fatalf("Unexpected second extension: got %v, want %v",
                                        parsedCRL.Extensions[1], crlExt)
                        }
+
+                       // With Go 1.19's updated RevocationList, we can now directly compare
+                       // the RawSubject of the certificate to RawIssuer on the parsed CRL.
+                       // However, this doesn't work with our hacked issuers above (that
+                       // aren't parsed from a proper DER bundle but are instead manually
+                       // constructed). Prefer RawSubject when it is set.
+                       if len(tc.issuer.RawSubject) > 0 {
+                               issuerSubj, err := subjectBytes(tc.issuer)
+                               if err != nil {
+                                       t.Fatalf("failed to get issuer subject: %s", err)
+                               }
+                               if !bytes.Equal(issuerSubj, parsedCRL.RawIssuer) {
+                                       t.Fatalf("Unexpected issuer subject; wanted: %v, got: %v", hex.EncodeToString(issuerSubj), hex.EncodeToString(parsedCRL.RawIssuer))
+                               }
+                       } else {
+                               // When we hack our custom Subject in the test cases above,
+                               // we don't set the additional fields (such as Names) in the
+                               // hacked issuer. Round-trip a parsing of pkix.Name so that
+                               // we add these missing fields for the comparison.
+                               issuerRDN := tc.issuer.Subject.ToRDNSequence()
+                               var caIssuer pkix.Name
+                               caIssuer.FillFromRDNSequence(&issuerRDN)
+                               if !reflect.DeepEqual(caIssuer, parsedCRL.Issuer) {
+                                       t.Fatalf("Expected issuer.Subject, parsedCRL.Issuer to be the same; wanted: %#v, got: %#v", caIssuer, parsedCRL.Issuer)
+                               }
+                       }
+
                        if len(parsedCRL.Extensions[2:]) == 0 && len(tc.template.ExtraExtensions) == 0 {
                                // If we don't have anything to check return early so we don't
                                // hit a [] != nil false positive below.