Repository: google/certificate-transparency-go

Issue:
Certificate verifier doesn't reject mismatched signature algorithm
When verifying a certificate to see if see if should be logged, CTFE accepts certificates whose signatureAlgorithm field doesn't match the signature field in the tbsCertificate.

The implication is that it's possible to spam logs by taking a certificate signed by a trusted CA and replacing the signatureAlgorithm with an arbitrary OID.  Since this field isn't covered by the signature, CTFE accepts the certificate.  Since the OID space is infinitely large, a spammer can create as many certificates as they want this way.

For example, see the certificates in Submariner at entries 14260064, 14260065, and 14260088 - these certificates share the same tbsCertificate and signature but have different signatureAlgorithms.

There's a CL to fix this in crypto/x509 (https://go-review.googlesource.com/c/go/+/235118) but since certificate-transparency-go has its own fork of crypto/x509 I'm filing an issue here.

Discussion:

Solution patch:
diff --git a/x509/pkix/pkix.go b/x509/pkix/pkix.go
index 843fa1f2..1716f908 100644
--- a/x509/pkix/pkix.go
+++ b/x509/pkix/pkix.go
@@ -18,6 +18,7 @@ import (
 // AlgorithmIdentifier represents the ASN.1 structure of the same name. See RFC
 // 5280, section 4.1.1.2.
 type AlgorithmIdentifier struct {
+	Raw        asn1.RawContent
 	Algorithm  asn1.ObjectIdentifier
 	Parameters asn1.RawValue `asn1:"optional"`
 }
diff --git a/x509/x509.go b/x509/x509.go
index 140e7244..917d7877 100644
--- a/x509/x509.go
+++ b/x509/x509.go
@@ -1813,9 +1813,25 @@ func parseNameConstraintsExtension(out *Certificate, e pkix.Extension, nfe *NonF
 	return unhandled, nil
 }
 
-func parseCertificate(in *certificate) (*Certificate, error) {
+func parseCertificate(in *certificate, tbsOnly bool) (*Certificate, error) {
 	var nfe NonFatalErrors
 
+	// Certificates contain two signature algorithm identifier fields,
+	// one in the inner signed tbsCertificate structure and one in the
+	// outer unsigned certificate structure. RFC 5280 requires these
+	// fields match, but golang doesn't impose this restriction. Because
+	// the outer structure is not covered by the signature the algorithm
+	// field is entirely malleable. This allows a user to bypass the
+	// leaf data uniqueness check that happens in trillian by altering
+	// the unbounded OID or parameter fields of the algorithmIdentifier
+	// structure and submit an infinite number of duplicate but slightly
+	// different looking certificates to a log. To avoid this directly
+	// compare the bytes of the two algorithmIdentifier structures
+	// and reject the certificate if they do not match.
+	if !tbsOnly && !bytes.Equal(in.SignatureAlgorithm.Raw, in.TBSCertificate.SignatureAlgorithm.Raw) {
+		return nil, errors.New("x509: mismatching signature algorithm identifiers")
+	}
+
 	out := new(Certificate)
 	out.Raw = in.Raw
 	out.RawTBSCertificate = in.TBSCertificate.Raw
@@ -2095,7 +2111,7 @@ func ParseTBSCertificate(asn1Data []byte) (*Certificate, error) {
 	}
 	ret, err := parseCertificate(&certificate{
 		Raw:            tbsCert.Raw,
-		TBSCertificate: tbsCert})
+		TBSCertificate: tbsCert}, true)
 	if err != nil {
 		errs, ok := err.(NonFatalErrors)
 		if !ok {
@@ -2127,7 +2143,7 @@ func ParseCertificate(asn1Data []byte) (*Certificate, error) {
 	if len(rest) > 0 {
 		return nil, asn1.SyntaxError{Msg: "trailing data"}
 	}
-	ret, err := parseCertificate(&cert)
+	ret, err := parseCertificate(&cert, false)
 	if err != nil {
 		errs, ok := err.(NonFatalErrors)
 		if !ok {
@@ -2166,7 +2182,7 @@ func ParseCertificates(asn1Data []byte) ([]*Certificate, error) {
 
 	ret := make([]*Certificate, len(v))
 	for i, ci := range v {
-		cert, err := parseCertificate(ci)
+		cert, err := parseCertificate(ci, false)
 		if err != nil {
 			errs, ok := err.(NonFatalErrors)
 			if !ok {



Test patch:
diff --git a/x509/revoked_test.go b/x509/revoked_test.go
index b5158d1c..9dfd8142 100644
--- a/x509/revoked_test.go
+++ b/x509/revoked_test.go
@@ -94,6 +94,7 @@ func TestParseCertificateList(t *testing.T) {
 			want: TBSCertList{
 				Version: 1,
 				Signature: pkix.AlgorithmIdentifier{
+					Raw:        fromHex("300d06092a864886f70d01010b0500"),
 					Algorithm:  oidSignatureSHA256WithRSA,
 					Parameters: asn1.RawValue{Class: 0, Tag: 5, Bytes: []byte{}, FullBytes: []byte{5, 0}},
 				},
@@ -348,6 +349,7 @@ func TestParseCertificateList(t *testing.T) {
 			want: TBSCertList{
 				Version: 1,
 				Signature: pkix.AlgorithmIdentifier{
+					Raw:        fromHex("300d06092a864886f70d01010b0500"),
 					Algorithm:  oidSignatureSHA256WithRSA,
 					Parameters: asn1.RawValue{Class: 0, Tag: 5, Bytes: []byte{}, FullBytes: []byte{5, 0}},
 				},
@@ -416,6 +418,7 @@ func TestParseCertificateList(t *testing.T) {
 			want: TBSCertList{
 				Version: 1,
 				Signature: pkix.AlgorithmIdentifier{
+					Raw:        fromHex("300d06092a864886f70d01010b0500"),
 					Algorithm:  oidSignatureSHA256WithRSA,
 					Parameters: asn1.RawValue{Class: 0, Tag: 5, Bytes: []byte{}, FullBytes: []byte{5, 0}},
 				},
@@ -915,6 +918,7 @@ func TestParseCertificateList(t *testing.T) {
 			want: TBSCertList{
 				Version: 1,
 				Signature: pkix.AlgorithmIdentifier{
+					Raw:        fromHex("300d06092a864886f70d01010b0500"),
 					Algorithm:  oidSignatureSHA256WithRSA,
 					Parameters: asn1.RawValue{Class: 0, Tag: 5, Bytes: []byte{}, FullBytes: []byte{5, 0}},
 				},
@@ -987,6 +991,7 @@ func TestParseCertificateList(t *testing.T) {
 			want: TBSCertList{
 				Version: 1,
 				Signature: pkix.AlgorithmIdentifier{
+					Raw:        fromHex("300d06092a864886f70d01010b0500"),
 					Algorithm:  oidSignatureSHA256WithRSA,
 					Parameters: asn1.RawValue{Class: 0, Tag: 5, Bytes: []byte{}, FullBytes: []byte{5, 0}},
 				},
@@ -1059,6 +1064,7 @@ func TestParseCertificateList(t *testing.T) {
 			want: TBSCertList{
 				Version: 1,
 				Signature: pkix.AlgorithmIdentifier{
+					Raw:        fromHex("300d06092a864886f70d01010b0500"),
 					Algorithm:  oidSignatureSHA256WithRSA,
 					Parameters: asn1.RawValue{Class: 0, Tag: 5, Bytes: []byte{}, FullBytes: []byte{5, 0}},
 				},
@@ -1224,6 +1230,7 @@ func TestParseCertificateList(t *testing.T) {
 			want: TBSCertList{
 				Version: 1,
 				Signature: pkix.AlgorithmIdentifier{
+					Raw:        fromHex("300d06092a864886f70d01010b0500"),
 					Algorithm:  oidSignatureSHA256WithRSA,
 					Parameters: asn1.RawValue{Class: 0, Tag: 5, Bytes: []byte{}, FullBytes: []byte{5, 0}},
 				},
@@ -1343,6 +1350,7 @@ func TestParseCertificateList(t *testing.T) {
 			want: TBSCertList{
 				Version: 1,
 				Signature: pkix.AlgorithmIdentifier{
+					Raw:        fromHex("300d06092a864886f70d01010b0500"),
 					Algorithm:  oidSignatureSHA256WithRSA,
 					Parameters: asn1.RawValue{Class: 0, Tag: 5, Bytes: []byte{}, FullBytes: []byte{5, 0}},
 				},
@@ -1463,6 +1471,7 @@ func TestParseCertificateList(t *testing.T) {
 			want: TBSCertList{
 				Version: 1,
 				Signature: pkix.AlgorithmIdentifier{
+					Raw:        fromHex("300d06092a864886f70d01010b0500"),
 					Algorithm:  oidSignatureSHA256WithRSA,
 					Parameters: asn1.RawValue{Class: 0, Tag: 5, Bytes: []byte{}, FullBytes: []byte{5, 0}},
 				},
diff --git a/x509/testdata/invalid/mismatching-sig-alg.pem b/x509/testdata/invalid/mismatching-sig-alg.pem
new file mode 100644
index 00000000..049a344a
--- /dev/null
+++ b/x509/testdata/invalid/mismatching-sig-alg.pem
@@ -0,0 +1,8 @@
+-----BEGIN CERTIFICATE-----
+MIIBAjCBqqADAgECAgEAMAoGCCqGSM49BAMCMAAwHhcNMDkxMTEwMjMwMDAwWhcN
+MTkwODEwMjMwMDAwWjAAMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEJwxgz8ox
+ipHbj2fHlQRt3xgxmSyXinGNe8K5eT0ChiGpG1oMVQPMzBgsG5ZSgdxwYn/J9GfN
+0/RDMbSldQsZO6MVMBMwEQYDVR0RAQH/BAcwBYIDYXNkMAoGCCqGSM49BAMDA0cA
+MEQCIDu3tl2kE6Hy2EJeNrnlQXqQHOpFPxFut7B9sPe8Ios1AiAleoh3TIr9neiT
+M5Nt98OAuqEcUT4Etn/B/6I6xIes+Q==
+-----END CERTIFICATE-----
diff --git a/x509/x509_test.go b/x509/x509_test.go
index 03377bb2..fa3fc2c7 100644
--- a/x509/x509_test.go
+++ b/x509/x509_test.go
@@ -2861,6 +2861,7 @@ func TestParseCertificateFail(t *testing.T) {
 		{desc: "RSAIntegerNotMinimal", in: "testdata/invalid/xf-der-pubkey-rsa-nonminimal-int.pem", wantErr: "integer not minimally-encoded"},
 		{desc: "SubjectNonPrintable", in: "testdata/invalid/xf-subject-nonprintable.pem", wantErr: "PrintableString contains invalid character"},
 		{desc: "NegativeRSAModulus", in: "testdata/invalid/xf-pubkey-rsa-modulus-negative.pem", wantErr: "RSA modulus is not a positive number"},
+		{desc: "MismatchingSigAlg", in: "testdata/invalid/mismatching-sig-alg.pem", wantErr: "mismatching signature algorithm identifiers", wantFatal: true},
 	}
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {