From 5e779ac56f832d8d34d4b80f1a798aef29d6d562 Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Tue, 2 Jun 2020 16:03:30 -0700 Subject: [PATCH 1/2] x509: disallow mismatching signature algorithm identifiers This prevents an easy method for spamming a log by modifying one of the malleable fields in the certificate structure. Fixes #699 --- x509/pkix/pkix.go | 1 + x509/revoked_test.go | 9 +++++++++ x509/x509.go | 16 ++++++++++++++++ x509/x509_test.go | 1 + 4 files changed, 27 insertions(+) diff --git a/x509/pkix/pkix.go b/x509/pkix/pkix.go index 843fa1f2cd..1716f908ab 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/revoked_test.go b/x509/revoked_test.go index b5158d1c60..9dfd8142f5 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/x509.go b/x509/x509.go index 140e7244bc..36ab5d38a8 100644 --- a/x509/x509.go +++ b/x509/x509.go @@ -1816,6 +1816,22 @@ func parseNameConstraintsExtension(out *Certificate, e pkix.Extension, nfe *NonF func parseCertificate(in *certificate) (*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 !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 diff --git a/x509/x509_test.go b/x509/x509_test.go index 03377bb247..fa3fc2c760 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) { From 95f9514dbed272b51b8a0405b818de6bc8c44b17 Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Mon, 13 Jul 2020 15:45:41 -0700 Subject: [PATCH 2/2] gate check on parsing full cert --- x509/testdata/invalid/mismatching-sig-alg.pem | 8 ++++++++ x509/x509.go | 10 +++++----- 2 files changed, 13 insertions(+), 5 deletions(-) create mode 100644 x509/testdata/invalid/mismatching-sig-alg.pem diff --git a/x509/testdata/invalid/mismatching-sig-alg.pem b/x509/testdata/invalid/mismatching-sig-alg.pem new file mode 100644 index 0000000000..049a344af6 --- /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.go b/x509/x509.go index 36ab5d38a8..917d78779f 100644 --- a/x509/x509.go +++ b/x509/x509.go @@ -1813,7 +1813,7 @@ 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, @@ -1828,7 +1828,7 @@ func parseCertificate(in *certificate) (*Certificate, error) { // 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 !bytes.Equal(in.SignatureAlgorithm.Raw, in.TBSCertificate.SignatureAlgorithm.Raw) { + if !tbsOnly && !bytes.Equal(in.SignatureAlgorithm.Raw, in.TBSCertificate.SignatureAlgorithm.Raw) { return nil, errors.New("x509: mismatching signature algorithm identifiers") } @@ -2111,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 { @@ -2143,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 { @@ -2182,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 {