From 347ccf8152d49daa8121d78eb256827461e018a8 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Mon, 3 Jun 2024 09:52:22 -0400 Subject: [PATCH] ca/linter: Port safety checks from ceremony tool (#7498) In https://github.com/letsencrypt/boulder/pull/7005 several safety checks were added to the `ceremony` tool: This change extracts the `RawSubject` to `RawIssuer` DER byte comparison into the `//linter` package proper so that it can serve both `//ca` and `//cmd/ceremony`. Adds a helper function `verifyTBSCertificateDeterminism` to `//ca` similar to an existing check in `//cmd/ceremony`. This code is not shared because we want `//cmd/ceremony` to largely stand alone from boulder proper. The helper performs a byte comparison on the `RawTBSCertificate` DER bytes for a given linting certificate and leaf certificate. The goal is to verify that `x509.CreateCertificate` was deterministic and produced identical DER bytes after each signing operation. Fixes https://github.com/letsencrypt/boulder/issues/6965 --- ca/ca.go | 77 ++++++++++++++++++++++++++++++++- ca/ca_test.go | 100 +++++++++++++++++++++++++++++++++++++++++++ cmd/ceremony/main.go | 11 ----- linter/linter.go | 11 +++++ 4 files changed, 187 insertions(+), 12 deletions(-) diff --git a/ca/ca.go b/ca/ca.go index 3d870accc..fbecd91d3 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -24,6 +24,8 @@ import ( "github.com/miekg/pkcs11" "github.com/prometheus/client_golang/prometheus" "github.com/zmap/zlint/v3/lint" + "golang.org/x/crypto/cryptobyte" + cryptobyte_asn1 "golang.org/x/crypto/cryptobyte/asn1" "golang.org/x/crypto/ocsp" "google.golang.org/protobuf/types/known/timestamppb" @@ -421,7 +423,7 @@ func (ca *certificateAuthorityImpl) IssueCertificateForPrecertificate(ctx contex ca.log.AuditInfof("Signing cert: issuer=[%s] serial=[%s] regID=[%d] names=[%s] certProfileName=[%s] certProfileHash=[%x] precert=[%s]", issuer.Name(), serialHex, req.RegistrationID, names, certProfile.name, certProfile.hash, hex.EncodeToString(precert.Raw)) - _, issuanceToken, err := issuer.Prepare(certProfile.profile, issuanceReq) + lintCertBytes, issuanceToken, err := issuer.Prepare(certProfile.profile, issuanceReq) if err != nil { ca.log.AuditErrf("Preparing cert failed: issuer=[%s] serial=[%s] regID=[%d] names=[%s] certProfileName=[%s] certProfileHash=[%x] err=[%v]", issuer.Name(), serialHex, req.RegistrationID, names, certProfile.name, certProfile.hash, err) @@ -436,6 +438,11 @@ func (ca *certificateAuthorityImpl) IssueCertificateForPrecertificate(ctx contex return nil, berrors.InternalServerError("failed to sign certificate: %s", err) } + err = tbsCertIsDeterministic(lintCertBytes, certDER) + if err != nil { + return nil, err + } + ca.signatureCount.With(prometheus.Labels{"purpose": string(certType), "issuer": issuer.Name()}).Inc() ca.log.AuditInfof("Signing cert success: issuer=[%s] serial=[%s] regID=[%d] names=[%s] certificate=[%s] certProfileName=[%s] certProfileHash=[%x]", issuer.Name(), serialHex, req.RegistrationID, names, hex.EncodeToString(certDER), certProfile.name, certProfile.hash) @@ -611,9 +618,77 @@ func (ca *certificateAuthorityImpl) issuePrecertificateInner(ctx context.Context return nil, nil, berrors.InternalServerError("failed to sign precertificate: %s", err) } + err = tbsCertIsDeterministic(lintCertBytes, certDER) + if err != nil { + return nil, nil, err + } + ca.signatureCount.With(prometheus.Labels{"purpose": string(precertType), "issuer": issuer.Name()}).Inc() ca.log.AuditInfof("Signing precert success: issuer=[%s] serial=[%s] regID=[%d] names=[%s] precertificate=[%s] certProfileName=[%s] certProfileHash=[%x]", issuer.Name(), serialHex, issueReq.RegistrationID, strings.Join(csr.DNSNames, ", "), hex.EncodeToString(certDER), certProfile.name, certProfile.hash) return certDER, &certProfileWithID{certProfile.name, certProfile.hash, nil}, nil } + +// verifyTBSCertIsDeterministic verifies that x509.CreateCertificate signing +// operation is deterministic and produced identical DER bytes between the given +// lint certificate and leaf certificate. If the DER byte equality check fails +// it's mississuance, but it's better to know about the problem sooner than +// later. The caller is responsible for passing the appropriate valid +// certificate bytes in the correct position. +func tbsCertIsDeterministic(lintCertBytes []byte, leafCertBytes []byte) error { + if core.IsAnyNilOrZero(lintCertBytes, leafCertBytes) { + return fmt.Errorf("lintCertBytes of leafCertBytes were nil") + } + + // extractTBSCertBytes is a partial copy of //crypto/x509/parser.go to + // extract the RawTBSCertificate field from given DER bytes. It the + // RawTBSCertificate field bytes or an error if the given bytes cannot be + // parsed. This is far more performant than parsing the entire *Certificate + // structure with x509.ParseCertificate(). + // + // RFC 5280, Section 4.1 + // Certificate ::= SEQUENCE { + // tbsCertificate TBSCertificate, + // signatureAlgorithm AlgorithmIdentifier, + // signatureValue BIT STRING } + // + // TBSCertificate ::= SEQUENCE { + // .. + extractTBSCertBytes := func(inputDERBytes *[]byte) ([]byte, error) { + input := cryptobyte.String(*inputDERBytes) + + // Extract the Certificate bytes + if !input.ReadASN1(&input, cryptobyte_asn1.SEQUENCE) { + return nil, errors.New("malformed certificate") + } + + var tbs cryptobyte.String + // Extract the TBSCertificate bytes from the Certificate bytes + if !input.ReadASN1(&tbs, cryptobyte_asn1.SEQUENCE) { + return nil, errors.New("malformed tbs certificate") + } + + if tbs.Empty() { + return nil, errors.New("parsed RawTBSCertificate field was empty") + } + + return tbs, nil + } + + lintRawTBSCert, err := extractTBSCertBytes(&lintCertBytes) + if err != nil { + return fmt.Errorf("while extracting lint TBS cert: %w", err) + } + + leafRawTBSCert, err := extractTBSCertBytes(&leafCertBytes) + if err != nil { + return fmt.Errorf("while extracting leaf TBS cert: %w", err) + } + + if !bytes.Equal(lintRawTBSCert, leafRawTBSCert) { + return fmt.Errorf("mismatch between lintCert and leafCert RawTBSCertificate DER bytes: \"%x\" != \"%x\"", lintRawTBSCert, leafRawTBSCert) + } + + return nil +} diff --git a/ca/ca_test.go b/ca/ca_test.go index f697ff5ec..11f526eef 100644 --- a/ca/ca_test.go +++ b/ca/ca_test.go @@ -10,6 +10,7 @@ import ( "encoding/asn1" "errors" "fmt" + "math/big" "os" "strings" "testing" @@ -1295,3 +1296,102 @@ func TestGenerateSKID(t *testing.T) { test.AssertEquals(t, cap(sha256skid), 20) features.Reset() } + +func TestVerifyTBSCertIsDeterministic(t *testing.T) { + t.Parallel() + + // Create first keypair and cert + testKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + test.AssertNotError(t, err, "unable to generate ECDSA private key") + template := &x509.Certificate{ + NotAfter: time.Now().Add(1 * time.Hour), + DNSNames: []string{"example.com"}, + SerialNumber: big.NewInt(1), + } + certDer1, err := x509.CreateCertificate(rand.Reader, template, template, &testKey.PublicKey, testKey) + test.AssertNotError(t, err, "unable to create certificate") + + // Create second keypair and cert + testKey2, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + test.AssertNotError(t, err, "unable to generate ECDSA private key") + template2 := &x509.Certificate{ + NotAfter: time.Now().Add(2 * time.Hour), + DNSNames: []string{"example.net"}, + SerialNumber: big.NewInt(2), + } + certDer2, err := x509.CreateCertificate(rand.Reader, template2, template2, &testKey2.PublicKey, testKey2) + test.AssertNotError(t, err, "unable to create certificate") + + testCases := []struct { + name string + lintCertBytes []byte + leafCertBytes []byte + errorSubstr string + }{ + { + name: "Both nil", + lintCertBytes: nil, + leafCertBytes: nil, + errorSubstr: "were nil", + }, + { + name: "Missing a value, invalid input", + lintCertBytes: nil, + leafCertBytes: []byte{0x6, 0x6, 0x6}, + errorSubstr: "were nil", + }, + { + name: "Missing a value, valid input", + lintCertBytes: nil, + leafCertBytes: certDer1, + errorSubstr: "were nil", + }, + { + name: "Mismatched bytes, invalid input", + lintCertBytes: []byte{0x6, 0x6, 0x6}, + leafCertBytes: []byte{0x1, 0x2, 0x3}, + errorSubstr: "malformed certificate", + }, + { + name: "Mismatched bytes, invalider input", + lintCertBytes: certDer1, + leafCertBytes: []byte{0x1, 0x2, 0x3}, + errorSubstr: "malformed certificate", + }, + { + // This case is an example of when a linting cert's DER bytes are + // mismatched compared to then precert or final cert created from + // that linting cert's DER bytes. + name: "Mismatched bytes, valid input", + lintCertBytes: certDer1, + leafCertBytes: certDer2, + errorSubstr: "mismatch between", + }, + { + // Take this with a grain of salt since this test is not actually + // creating a linting certificate and performing two + // x509.CreateCertificate() calls like + // ca.IssueCertificateForPrecertificate and + // ca.issuePrecertificateInner do. However, we're still going to + // verify the equality. + name: "Valid", + lintCertBytes: certDer1, + leafCertBytes: certDer1, + }, + } + + for _, testCase := range testCases { + // TODO(#7454) Remove this rebinding + testCase := testCase + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + err := tbsCertIsDeterministic(testCase.lintCertBytes, testCase.leafCertBytes) + if testCase.errorSubstr != "" { + test.AssertError(t, err, "your lack of errors is disturbing") + test.AssertContains(t, err.Error(), testCase.errorSubstr) + } else { + test.AssertNotError(t, err, "unexpected error") + } + }) + } +} diff --git a/cmd/ceremony/main.go b/cmd/ceremony/main.go index a68512f9b..cb9838c8c 100644 --- a/cmd/ceremony/main.go +++ b/cmd/ceremony/main.go @@ -645,10 +645,6 @@ func rootCeremony(configBytes []byte) error { if err != nil { return err } - // Verify that the lintCert is self-signed. - if !bytes.Equal(lintCert.RawSubject, lintCert.RawIssuer) { - return fmt.Errorf("mismatch between self-signed lintCert RawSubject and RawIssuer DER bytes: \"%x\" != \"%x\"", lintCert.RawSubject, lintCert.RawIssuer) - } finalCert, err := signAndWriteCert(template, template, lintCert, keyInfo.key, signer, config.Outputs.CertificatePath) if err != nil { return err @@ -697,10 +693,6 @@ func intermediateCeremony(configBytes []byte, ct certType) error { if err != nil { return err } - // Verify that the lintCert (and therefore the eventual finalCert) corresponds to the specified issuer certificate. - if !bytes.Equal(issuer.RawSubject, lintCert.RawIssuer) { - return fmt.Errorf("mismatch between issuer RawSubject and lintCert RawIssuer DER bytes: \"%x\" != \"%x\"", issuer.RawSubject, lintCert.RawIssuer) - } finalCert, err := signAndWriteCert(template, issuer, lintCert, pub, signer, config.Outputs.CertificatePath) if err != nil { return err @@ -780,9 +772,6 @@ func crossCertCeremony(configBytes []byte, ct certType) error { if lintCert.NotBefore.Before(toBeCrossSigned.NotBefore) { return fmt.Errorf("cross-signed subordinate CA's NotBefore predates the existing CA's NotBefore") } - if !bytes.Equal(issuer.RawSubject, lintCert.RawIssuer) { - return fmt.Errorf("mismatch between issuer RawSubject and lintCert RawIssuer DER bytes: \"%x\" != \"%x\"", issuer.RawSubject, lintCert.RawIssuer) - } // BR 7.1.2.2.3 Cross-Certified Subordinate CA Extensions if !slices.Equal(lintCert.ExtKeyUsage, toBeCrossSigned.ExtKeyUsage) { return fmt.Errorf("lint cert and toBeCrossSigned cert EKUs differ") diff --git a/linter/linter.go b/linter/linter.go index 07ac8b029..e9bf33b85 100644 --- a/linter/linter.go +++ b/linter/linter.go @@ -1,6 +1,7 @@ package linter import ( + "bytes" "crypto" "crypto/ecdsa" "crypto/rand" @@ -239,6 +240,16 @@ func makeLintCert(tbs *x509.Certificate, subjectPubKey crypto.PublicKey, issuer if err != nil { return nil, nil, fmt.Errorf("failed to parse lint certificate: %w", err) } + // RFC 5280, Sections 4.1.2.6 and 8 + // + // When the subject of the certificate is a CA, the subject + // field MUST be encoded in the same way as it is encoded in the + // issuer field (Section 4.1.2.4) in all certificates issued by + // the subject CA. + if !bytes.Equal(issuer.RawSubject, lintCert.RawIssuer) { + return nil, nil, fmt.Errorf("mismatch between lint issuer RawSubject and lintCert.RawIssuer DER bytes: \"%x\" != \"%x\"", issuer.RawSubject, lintCert.RawIssuer) + } + return lintCertBytes, lintCert, nil }