Skip to content

Commit 9583b61

Browse files
jkujku
authored
Ensure correct certificate is used for TSA auth checks (GHSA-xm5m-wgh2-rrg3) (#1333)
Currently VerifyLeafCert and verifyTSRWithChain may disagree on which cert is the real leaf certificate (TSA certificate): VerifyLeafCert should use the leaf certificate identified by verifyTSRWithChain. * Return the signer cert from verifyTSRWithChain() so verifyLeafCert() can just use the correct cert * Make sure verifyTSRWithChain() ensures that we have signer cert (either embedded or provided as option) * Make sure verifyTSRWithChain() verifies that embedded and provided cert match if both are present * Modify verifyLeafCert() so it only operates on given leaf cert * Remove unused function verifyEmbeddedLeafCert is now not needed: the check is already done in verifyTSRWithChain. Remove the related test, add test cases to cover the same situatation in verifyTSRWithChain. Signed-off-by: Jussi Kukkonen <[email protected]>
1 parent 7aab8b4 commit 9583b61

3 files changed

Lines changed: 106 additions & 96 deletions

File tree

‎pkg/verification/testdata/response-injected-certs.tsr‎

1.87 KB
Binary file not shown.

‎pkg/verification/verify.go‎

Lines changed: 29 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -81,14 +81,6 @@ func verifySubjectCommonName(cert *x509.Certificate, opts VerifyOpts) error {
8181
return nil
8282
}
8383

84-
// If embedded in the TSR, verify the TSR's leaf certificate matches a provided TSA certificate
85-
func verifyEmbeddedLeafCert(tsaCert *x509.Certificate, opts VerifyOpts) error {
86-
if opts.TSACertificate != nil && !opts.TSACertificate.Equal(tsaCert) {
87-
return fmt.Errorf("certificate embedded in the TSR does not match the provided TSA certificate")
88-
}
89-
return nil
90-
}
91-
9284
// Verify the leaf's EKU is set to critical, per RFC 3161 2.3
9385
func verifyLeafCertCriticalEKU(cert *x509.Certificate) error {
9486
var criticalEKU bool
@@ -104,33 +96,14 @@ func verifyLeafCertCriticalEKU(cert *x509.Certificate) error {
10496
return nil
10597
}
10698

107-
func verifyLeafCert(ts timestamp.Timestamp, opts VerifyOpts) error {
108-
if len(ts.Certificates) == 0 && opts.TSACertificate == nil {
109-
return fmt.Errorf("leaf certificate must be present the in TSR or as a verify option")
99+
func verifyLeafCert(leafCert *x509.Certificate, opts VerifyOpts) error {
100+
if leafCert == nil {
101+
// should never happen
102+
return fmt.Errorf("signer certificate is required")
110103
}
111104

112105
errMsg := "failed to verify TSA certificate"
113106

114-
var leafCert *x509.Certificate
115-
if len(ts.Certificates) != 0 {
116-
for _, c := range ts.Certificates {
117-
if !c.IsCA {
118-
leafCert = c
119-
break
120-
}
121-
}
122-
if leafCert == nil {
123-
return fmt.Errorf("no leaf certificate found in chain")
124-
}
125-
126-
err := verifyEmbeddedLeafCert(leafCert, opts)
127-
if err != nil {
128-
return fmt.Errorf("%s: %w", errMsg, err)
129-
}
130-
} else {
131-
leafCert = opts.TSACertificate
132-
}
133-
134107
err := verifyLeafCertCriticalEKU(leafCert)
135108
if err != nil {
136109
return fmt.Errorf("%s: %w", errMsg, err)
@@ -252,7 +225,8 @@ func VerifyTimestampResponse(tsrBytes []byte, artifact io.Reader, opts VerifyOpt
252225
}
253226

254227
// verify the timestamp response signature using the provided certificate pool
255-
if err = verifyTSRWithChain(ts, opts); err != nil {
228+
signerCert, err := verifyTSRWithChain(ts, opts)
229+
if err != nil {
256230
return nil, err
257231
}
258232

@@ -264,7 +238,7 @@ func VerifyTimestampResponse(tsrBytes []byte, artifact io.Reader, opts VerifyOpt
264238
return nil, err
265239
}
266240

267-
if err = verifyLeafCert(*ts, opts); err != nil {
241+
if err = verifyLeafCert(signerCert, opts); err != nil {
268242
return nil, err
269243
}
270244

@@ -277,23 +251,29 @@ func VerifyTimestampResponse(tsrBytes []byte, artifact io.Reader, opts VerifyOpt
277251
return ts, nil
278252
}
279253

280-
func verifyTSRWithChain(ts *timestamp.Timestamp, opts VerifyOpts) error {
254+
// Returns the TSA signer certificate after verifying the certificate chain validity.
255+
func verifyTSRWithChain(ts *timestamp.Timestamp, opts VerifyOpts) (*x509.Certificate, error) {
281256
p7Message, err := pkcs7.Parse(ts.RawToken)
282257
if err != nil {
283-
return fmt.Errorf("error parsing hashed message: %w", err)
258+
return nil, fmt.Errorf("error parsing hashed message: %w", err)
284259
}
285260

286261
if len(opts.Roots) == 0 {
287-
return fmt.Errorf("no root certificates provided for verifying the certificate chain")
262+
return nil, fmt.Errorf("no root certificates provided for verifying the certificate chain")
288263
}
264+
265+
if p7Message.Certificates == nil && opts.TSACertificate == nil {
266+
return nil, fmt.Errorf("leaf certificate must be present in the TSR or as a verify option")
267+
}
268+
289269
rootCertPool := x509.NewCertPool()
290270
for _, cert := range opts.Roots {
291271
if cert != nil {
292272
rootCertPool.AddCert(cert)
293273
}
294274
}
295275
if rootCertPool.Equal(x509.NewCertPool()) {
296-
return fmt.Errorf("no valid root certificates provided for verifying the certificate chain")
276+
return nil, fmt.Errorf("no valid root certificates provided for verifying the certificate chain")
297277
}
298278
intermediateCertPool := x509.NewCertPool()
299279
for _, cert := range opts.Intermediates {
@@ -313,16 +293,25 @@ func verifyTSRWithChain(ts *timestamp.Timestamp, opts VerifyOpts) error {
313293
// leaf certificate issuer and serial number information is already part of
314294
// the PKCS7 object, adding the leaf certificate to the Certificates field
315295
// will allow verification to pass
316-
if p7Message.Certificates == nil && opts.TSACertificate != nil {
296+
if p7Message.Certificates == nil {
317297
p7Message.Certificates = []*x509.Certificate{opts.TSACertificate}
318298
}
319299

320300
err = p7Message.VerifyWithOpts(x509Opts)
321301
if err != nil {
322-
return fmt.Errorf("error while verifying with chain: %w", err)
302+
return nil, fmt.Errorf("error while verifying with chain: %w", err)
323303
}
324304

325-
return nil
305+
signerCert := p7Message.GetOnlySigner()
306+
if signerCert == nil {
307+
return nil, fmt.Errorf("signer certificate was not found")
308+
}
309+
310+
if opts.TSACertificate != nil && !opts.TSACertificate.Equal(signerCert) {
311+
return nil, fmt.Errorf("certificate embedded in the TSR does not match the provided TSA certificate")
312+
}
313+
314+
return signerCert, nil
326315
}
327316

328317
// Verify that the TSR's hashed message matches the digest of the artifact to be timestamped

‎pkg/verification/verify_test.go‎

Lines changed: 77 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
"fmt"
2828
"io"
2929
"math/big"
30+
"os"
3031
"strings"
3132
"testing"
3233
"time"
@@ -107,8 +108,9 @@ func TestVerifyArtifactHashedMessages(t *testing.T) {
107108
}
108109

109110
opts := VerifyOpts{
110-
Intermediates: certs[1:2],
111-
Roots: certs[2:],
111+
Intermediates: certs[1:2],
112+
Roots: certs[2:],
113+
TSACertificate: certs[0],
112114
}
113115

114116
ts, err := VerifyTimestampResponse(respBytes.Bytes(), strings.NewReader(tc.message), opts)
@@ -192,7 +194,7 @@ func TestVerifyLeafCert(t *testing.T) {
192194
useOptsCert: false,
193195
useTSCert: false,
194196
expectVerifySuccess: false,
195-
expectedErrMsg: "leaf certificate must be present the in TSR or as a verify option",
197+
expectedErrMsg: "signer certificate is required",
196198
},
197199
{
198200
useOptsCert: true,
@@ -213,7 +215,7 @@ func TestVerifyLeafCert(t *testing.T) {
213215
{
214216
onlyCACerts: true,
215217
expectVerifySuccess: false,
216-
expectedErrMsg: "no leaf certificate found in chain",
218+
expectedErrMsg: "signer certificate is required",
217219
},
218220
}
219221

@@ -246,12 +248,17 @@ func TestVerifyLeafCert(t *testing.T) {
246248
ts.Certificates = []*x509.Certificate{sampleCert}
247249
}
248250

251+
var testSigner *x509.Certificate
252+
if tc.useTSCert || tc.useOptsCert {
253+
testSigner = sampleCert
254+
}
255+
249256
if tc.onlyCACerts {
250257
sampleCert.IsCA = true
251258
ts.Certificates = []*x509.Certificate{sampleCert}
252259
}
253260

254-
err := verifyLeafCert(ts, opts)
261+
err := verifyLeafCert(testSigner, opts)
255262

256263
if err != nil && tc.expectVerifySuccess {
257264
t.Fatalf("expected error to be nil, actual error: %v", err)
@@ -267,56 +274,6 @@ func TestVerifyLeafCert(t *testing.T) {
267274
}
268275
}
269276

270-
func TestVerifyEmbeddedLeafCert(t *testing.T) {
271-
type test struct {
272-
optsCert *x509.Certificate
273-
providedCert *x509.Certificate
274-
expectVerifySuccess bool
275-
}
276-
277-
tests := []test{
278-
{
279-
optsCert: nil,
280-
providedCert: &x509.Certificate{
281-
Raw: []byte("abc123"),
282-
},
283-
expectVerifySuccess: true,
284-
},
285-
{
286-
optsCert: &x509.Certificate{
287-
Raw: []byte("abc123"),
288-
},
289-
providedCert: &x509.Certificate{
290-
Raw: []byte("abc123"),
291-
},
292-
expectVerifySuccess: true,
293-
},
294-
{
295-
optsCert: &x509.Certificate{
296-
Raw: []byte("abc123"),
297-
},
298-
providedCert: &x509.Certificate{
299-
Raw: []byte("def456"),
300-
},
301-
expectVerifySuccess: false,
302-
},
303-
}
304-
305-
for _, tc := range tests {
306-
opts := VerifyOpts{
307-
TSACertificate: tc.optsCert,
308-
}
309-
310-
err := verifyEmbeddedLeafCert(tc.providedCert, opts)
311-
if err == nil && !tc.expectVerifySuccess {
312-
t.Errorf("expected verification to fail: provided cert unexpectedly matches opts cert")
313-
}
314-
if err != nil && tc.expectVerifySuccess {
315-
t.Errorf("expected verification to pass: provided cert does not match opts cert")
316-
}
317-
}
318-
}
319-
320277
func TestVerifySubjectCommonName(t *testing.T) {
321278
type test struct {
322279
optsCommonName string
@@ -591,6 +548,11 @@ func TestVerifyTSRWithChain(t *testing.T) {
591548
t.Errorf("failed to create certificate chain: %v", err)
592549
}
593550

551+
altCertChain, _, err := createCertChainAndSigner()
552+
if err != nil {
553+
t.Errorf("failed to create certificate chain: %v", err)
554+
}
555+
594556
tsWithCerts, err := createSignedTimestamp(certChain, sv, true)
595557
if err != nil {
596558
t.Errorf("failed to create signed certificate: %v", err)
@@ -603,6 +565,7 @@ func TestVerifyTSRWithChain(t *testing.T) {
603565

604566
// get certificates
605567
leaf := certChain[0]
568+
altLeaf := altCertChain[0]
606569
intermediate := certChain[1]
607570
root := certChain[2]
608571

@@ -654,6 +617,26 @@ func TestVerifyTSRWithChain(t *testing.T) {
654617
},
655618
expectVerifySuccess: false,
656619
},
620+
{
621+
name: "Verification succeeds with same certificate embedded and provided",
622+
ts: tsWithCerts,
623+
opts: VerifyOpts{
624+
Roots: []*x509.Certificate{root},
625+
Intermediates: []*x509.Certificate{intermediate},
626+
TSACertificate: leaf,
627+
},
628+
expectVerifySuccess: true,
629+
},
630+
{
631+
name: "Verification fails with embedded and provided certificate mismatch",
632+
ts: tsWithCerts,
633+
opts: VerifyOpts{
634+
Roots: []*x509.Certificate{root},
635+
Intermediates: []*x509.Certificate{intermediate},
636+
TSACertificate: altLeaf,
637+
},
638+
expectVerifySuccess: false,
639+
},
657640
{
658641
name: "Verification fails due to missing root certificate",
659642
ts: tsWithCerts,
@@ -691,7 +674,7 @@ func TestVerifyTSRWithChain(t *testing.T) {
691674

692675
for _, tc := range tests {
693676
t.Run(tc.name, func(t *testing.T) {
694-
err = verifyTSRWithChain(tc.ts, tc.opts)
677+
_, err = verifyTSRWithChain(tc.ts, tc.opts)
695678
if tc.expectVerifySuccess && err != nil {
696679
t.Errorf("unexpectedly failed \nExpected verifyTSRWithChain to successfully verify certificate chain, err: %v", err)
697680
} else if !tc.expectVerifySuccess && err == nil {
@@ -700,3 +683,41 @@ func TestVerifyTSRWithChain(t *testing.T) {
700683
})
701684
}
702685
}
686+
687+
func TestVerifyTimestampResponseWithOutOfChainCert(t *testing.T) {
688+
// Read the fixture file: it contains 3 certs
689+
// * "Spoofed" TSA cert (not issued by the root cert)
690+
// * TSA cert issued by root cert
691+
// * root cert
692+
respBytes, err := os.ReadFile("testdata/response-injected-certs.tsr")
693+
if err != nil {
694+
t.Fatalf("failed to read fixture file: %v", err)
695+
}
696+
697+
// Parse fixture to find a valid root (or self-signed leaf) to bypass chain verification
698+
ts, err := timestamp.ParseResponse(respBytes)
699+
if err != nil {
700+
t.Fatalf("failed to parse response in test: %v", err)
701+
}
702+
703+
// Find the self-signed cert, use as the root
704+
var validRoots []*x509.Certificate
705+
for _, cert := range ts.Certificates {
706+
if cert.Subject.CommonName != "Spoofed TSA" && cert.Issuer.String() == cert.Subject.String() {
707+
validRoots = append(validRoots, cert)
708+
break
709+
}
710+
}
711+
712+
opts := VerifyOpts{
713+
CommonName: "Spoofed TSA", // Ask for the cert that is not part of the chain
714+
Roots: validRoots,
715+
}
716+
717+
// verify should fail as Spoofed TSA is not signed by root cert
718+
_, err = VerifyTimestampResponse(respBytes, strings.NewReader("hello"), opts)
719+
720+
if err == nil {
721+
t.Errorf("VerifyTimestampResponse succeeded with a certificate that is not part of the cert chain.")
722+
}
723+
}

0 commit comments

Comments
 (0)