Skip to content

Commit 59d15e2

Browse files
committed
signature/pkcs7: tighten validation per Copilot review
- ParseEnvelope rejects non-1 signer count and empty EncryptedDigest - validateSignRequest rejects empty Payload.Content (matches base.Envelope) - verifySignerOutput guards against nil leaf certificate - signerAdapter.Sign returns error instead of panicking on second call - Content().Payload.ContentType is empty (PKCS#7 detached doesn't carry it) - add tests for new validation checks Signed-off-by: Dallas Delaney <dadelan@microsoft.com>
1 parent cf8313d commit 59d15e2

2 files changed

Lines changed: 93 additions & 12 deletions

File tree

signature/pkcs7/envelope.go

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,15 @@ func ParseEnvelope(envelopeBytes []byte) (env signature.Envelope, err error) {
7474
return nil, &signature.InvalidSignatureError{Msg: err.Error()}
7575
}
7676

77-
var sigBytes []byte
78-
if len(p7.Signers) > 0 {
79-
sigBytes = p7.Signers[0].EncryptedDigest
77+
// dm-verity profile: exactly one signer with a non-empty signature.
78+
if len(p7.Signers) != 1 {
79+
return nil, &signature.InvalidSignatureError{
80+
Msg: fmt.Sprintf("dm-verity PKCS#7 envelope requires exactly one signer; got %d", len(p7.Signers)),
81+
}
82+
}
83+
sigBytes := p7.Signers[0].EncryptedDigest
84+
if len(sigBytes) == 0 {
85+
return nil, &signature.InvalidSignatureError{Msg: "dm-verity PKCS#7 envelope has empty signature"}
8086
}
8187

8288
return &envelope{
@@ -142,9 +148,12 @@ func (e *envelope) Sign(req *signature.SignRequest) ([]byte, error) {
142148
return encoded, nil
143149
}
144150

145-
// validateSignRequest enforces the dm-verity profile: no signed-attribute
146-
// fields, RSA-2048 key only.
151+
// validateSignRequest enforces the dm-verity profile: non-empty payload,
152+
// no signed-attribute fields, RSA-2048 key only.
147153
func validateSignRequest(req *signature.SignRequest) error {
154+
if len(req.Payload.Content) == 0 {
155+
return &signature.InvalidSignRequestError{Msg: "dm-verity PKCS#7 envelope requires a non-empty Payload.Content"}
156+
}
148157
if !req.SigningTime.IsZero() {
149158
return &signature.InvalidSignRequestError{Msg: "dm-verity PKCS#7 envelope does not support SigningTime"}
150159
}
@@ -189,6 +198,9 @@ func verifySignerOutput(payload, sig []byte, certs []*x509.Certificate) error {
189198
if len(certs) == 0 {
190199
return &signature.InvalidSignatureError{Msg: "no certificates returned from signer"}
191200
}
201+
if certs[0] == nil {
202+
return &signature.InvalidSignatureError{Msg: "signer returned a nil leaf certificate"}
203+
}
192204
pub, ok := certs[0].PublicKey.(*rsa.PublicKey)
193205
if !ok {
194206
return &signature.UnsupportedSigningKeyError{
@@ -222,10 +234,10 @@ func (a *signerAdapter) Public() crypto.PublicKey {
222234
}
223235

224236
// Sign returns the pre-computed signature. The digest and opts arguments
225-
// are ignored. Panics if called more than once.
237+
// are ignored. Returns an error if called more than once.
226238
func (a *signerAdapter) Sign(rand io.Reader, digest []byte, opts crypto.SignerOpts) ([]byte, error) {
227239
if a.used {
228-
panic("pkcs7: signerAdapter.Sign called more than once")
240+
return nil, errors.New("pkcs7: signerAdapter.Sign called more than once")
229241
}
230242
a.used = true
231243
return a.sig, nil
@@ -253,9 +265,10 @@ func (e *envelope) Content() (*signature.EnvelopeContent, error) {
253265
CertificateChain: e.certs,
254266
Signature: e.sigBytes,
255267
},
256-
Payload: signature.Payload{
257-
ContentType: MediaTypeEnvelope,
258-
},
268+
// Payload.ContentType identifies the signed payload's media type,
269+
// not the envelope format. PKCS#7 detached signatures don't carry
270+
// the payload's content type, so it is unknown after parsing.
271+
Payload: signature.Payload{},
259272
}, nil
260273
}
261274

signature/pkcs7/envelope_test.go

Lines changed: 70 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525

2626
"github.com/notaryproject/notation-core-go/signature"
2727
"github.com/notaryproject/notation-core-go/testhelper"
28+
gopkcs7 "go.mozilla.org/pkcs7"
2829
)
2930

3031
const testPayload = "test dm-verity root hash payload"
@@ -94,8 +95,10 @@ func TestSignParseVerifyRoundTrip(t *testing.T) {
9495
if len(content.SignerInfo.CertificateChain) == 0 {
9596
t.Fatal("Content() returned no certificates")
9697
}
97-
if content.Payload.ContentType != MediaTypeEnvelope {
98-
t.Fatalf("ContentType = %q, want %q", content.Payload.ContentType, MediaTypeEnvelope)
98+
// PKCS#7 detached signatures don't carry the signed payload's content
99+
// type, so Content() reports it as unknown.
100+
if content.Payload.ContentType != "" {
101+
t.Fatalf("ContentType = %q, want empty", content.Payload.ContentType)
99102
}
100103
}
101104

@@ -259,3 +262,68 @@ func (s *customSignSigner) Sign(payload []byte) ([]byte, []*x509.Certificate, er
259262
return sig, s.certs, nil
260263
}
261264
func (s *customSignSigner) KeySpec() (signature.KeySpec, error) { return s.keySpec, nil }
265+
266+
// TestSignRejectsEmptyPayload verifies that an empty Payload.Content is
267+
// rejected with InvalidSignRequestError.
268+
func TestSignRejectsEmptyPayload(t *testing.T) {
269+
req := newSignRequest()
270+
req.Payload.Content = nil
271+
_, err := NewEnvelope().Sign(req)
272+
var want *signature.InvalidSignRequestError
273+
if !errors.As(err, &want) {
274+
t.Fatalf("want InvalidSignRequestError, got %T: %v", err, err)
275+
}
276+
}
277+
278+
// TestSignRejectsNilLeafCertificate verifies that a signer returning a
279+
// nil leaf certificate is rejected with InvalidSignatureError.
280+
func TestSignRejectsNilLeafCertificate(t *testing.T) {
281+
base := newRSATestSigner()
282+
req := &signature.SignRequest{
283+
Payload: signature.Payload{ContentType: MediaTypeEnvelope, Content: []byte(testPayload)},
284+
Signer: &customSignSigner{
285+
certs: []*x509.Certificate{nil, base.certs[1]},
286+
keySpec: base.keySpec,
287+
sign: func(payload []byte) ([]byte, error) {
288+
h := sha256.Sum256(payload)
289+
return rsa.SignPKCS1v15(rand.Reader, base.key.(*rsa.PrivateKey), crypto.SHA256, h[:])
290+
},
291+
},
292+
}
293+
_, err := NewEnvelope().Sign(req)
294+
var want *signature.InvalidSignatureError
295+
if !errors.As(err, &want) {
296+
t.Fatalf("want InvalidSignatureError, got %T: %v", err, err)
297+
}
298+
}
299+
300+
// TestParseEnvelopeRejectsMultipleSigners verifies that the dm-verity
301+
// profile's "exactly one signer" guard fires on multi-signer envelopes.
302+
// Empty-EncryptedDigest is also exercised by FuzzSignaturePkcs7.
303+
func TestParseEnvelopeRejectsMultipleSigners(t *testing.T) {
304+
tuple := testhelper.GetRSACertTuple(2048)
305+
sd, err := gopkcs7.NewSignedData([]byte(testPayload))
306+
if err != nil {
307+
t.Fatalf("NewSignedData() error: %v", err)
308+
}
309+
sd.SetDigestAlgorithm(gopkcs7.OIDDigestAlgorithmSHA256)
310+
sd.SetEncryptionAlgorithm(gopkcs7.OIDEncryptionAlgorithmRSA)
311+
cfg := gopkcs7.SignerInfoConfig{}
312+
if err := sd.SignWithoutAttr(tuple.Cert, tuple.PrivateKey, cfg); err != nil {
313+
t.Fatalf("SignWithoutAttr() #1 error: %v", err)
314+
}
315+
if err := sd.SignWithoutAttr(tuple.Cert, tuple.PrivateKey, cfg); err != nil {
316+
t.Fatalf("SignWithoutAttr() #2 error: %v", err)
317+
}
318+
sd.Detach()
319+
encoded, err := sd.Finish()
320+
if err != nil {
321+
t.Fatalf("Finish() error: %v", err)
322+
}
323+
324+
_, err = ParseEnvelope(encoded)
325+
var want *signature.InvalidSignatureError
326+
if !errors.As(err, &want) {
327+
t.Fatalf("want InvalidSignatureError, got %T: %v", err, err)
328+
}
329+
}

0 commit comments

Comments
 (0)