1
0
mirror of https://github.com/golang/go synced 2024-09-29 16:34:31 -06:00

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: 033115dd5e
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>
This commit is contained in:
Alexander Scheel 2022-11-02 16:19:23 +00:00 committed by Gopher Robot
parent 1bfb51f8f7
commit a367981b4c
2 changed files with 85 additions and 3 deletions

View File

@ -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},

View File

@ -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.