From a367981b4c8e3ae955eca9cc597d9622201155f3 Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Wed, 2 Nov 2022 16:19:23 +0000 Subject: [PATCH] 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 Reviewed-by: Bryan Mills Run-TryBot: Roland Shoemaker TryBot-Result: Gopher Robot Auto-Submit: Roland Shoemaker --- src/crypto/x509/x509.go | 39 +++++++++++++++++++++++++--- src/crypto/x509/x509_test.go | 49 ++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go index 35394bfc33..df86c65939 100644 --- 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 5690e200b2..44441d45d4 100644 --- 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. -- 2.48.1