From f96e440a479d1155486dc290be281e045bcaf883 Mon Sep 17 00:00:00 2001
From: ffishman <20211677+ffishman@users.noreply.github.com>
Date: Tue, 20 Oct 2026 16:46:26 +0000
Subject: [PATCH] M9: check the bundle's SignedData shape and refuse trailing
 bytes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

A .p7b must be SignedData with version, digestAlgorithms,
encapContentInfo, optional certificates and CRLs, then signerInfos
(RFC 5652 §5.1), and nothing may follow it (CodeRabbit on #3561).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
---
 crates/sign/src/cms.rs      | 20 ++++++++++++++------
 crates/sign/tests/crypto.rs | 13 +++++++++++++
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/crates/sign/src/cms.rs b/crates/sign/src/cms.rs
index bbdea4b14..c4cf5e027 100644
--- a/crates/sign/src/cms.rs
+++ b/crates/sign/src/cms.rs
@@ -74,19 +74,27 @@ fn carried_certificates(set: &Tlv) -> Result<Vec<Certificate>, SignError> {
 /// authorities hand out a chain or a set of CA certificates. Signers and CRLs, if present, are
 /// ignored; only the certificates are read. DER or BER.
 pub fn certificate_bundle(bytes: &[u8]) -> Result<Vec<Certificate>, SignError> {
-    let (ci, _) = Tlv::parse_ber(bytes)?;
+    let (ci, rest) = Tlv::parse_ber(bytes)?;
+    // A trust file is the bundle alone: bytes after it mean a damaged or spliced file.
+    if !rest.is_empty() {
+        return Err(bad("data after the bundle"));
+    }
     let ci = ci.expect(tag::SEQUENCE, "ContentInfo")?.children()?;
     let [ct, content] = ci.as_slice() else { return Err(bad("ContentInfo")) };
     if ct.oid()? != SIGNED_DATA {
         return Err(bad("not SignedData"));
     }
     let sd = content.expect(tag::ctx(0), "content")?.inner()?.children()?;
-    // version, digestAlgorithms, encapContentInfo, then the optional [0] certificates.
+    // version, digestAlgorithms, encapContentInfo, the optional [0] certificates and [1] CRLs,
+    // then signerInfos (RFC 5652 §5.1).
+    let [version, digests, encap, middle @ .., signers] = sd.as_slice() else { return Err(bad("SignedData")) };
+    let shape = [(version, tag::INTEGER), (digests, tag::SET), (encap, tag::SEQUENCE), (signers, tag::SET)];
+    if shape.iter().any(|(t, want)| t.tag != *want) || middle.iter().any(|t| t.tag != tag::ctx(0) && t.tag != tag::ctx(1)) {
+        return Err(bad("SignedData"));
+    }
     let mut certificates = Vec::new();
-    for t in sd.iter().skip(3) {
-        if t.tag == tag::ctx(0) {
-            certificates.extend(carried_certificates(t)?);
-        }
+    for t in middle.iter().filter(|t| t.tag == tag::ctx(0)) {
+        certificates.extend(carried_certificates(t)?);
     }
     if certificates.is_empty() {
         return Err(bad("the bundle holds no certificate"));
diff --git a/crates/sign/tests/crypto.rs b/crates/sign/tests/crypto.rs
index 3c3e51fbd..05eacfde4 100644
--- a/crates/sign/tests/crypto.rs
+++ b/crates/sign/tests/crypto.rs
@@ -295,6 +295,19 @@ fn certificates_load_from_pkcs7_bundles() {
     assert!(load_certificates(&p7b(&[])).is_err());
     let not_signed_data = pdfcraft_sign::der::seq(&[&pdfcraft_sign::der::oid(cms::DATA), &pdfcraft_sign::der::explicit(0, &[])]);
     assert!(load_certificates(&not_signed_data).is_err());
+    // A SignedData out of shape: no signerInfos SET, or a version that isn't an INTEGER.
+    {
+        use pdfcraft_sign::der::{explicit, int, oid, seq, set_of, tlv};
+        let wrap = |sd: &[&[u8]]| seq(&[&oid(cms::SIGNED_DATA), &explicit(0, &seq(sd))]);
+        let (certs, encap) = (tlv(0xA0, &[leaf.raw.as_slice(), &root.raw].concat()), seq(&[&oid(cms::DATA)]));
+        assert_eq!(load_certificates(&wrap(&[&int(1), &set_of(&[]), &encap, &certs, &set_of(&[])])).unwrap().len(), 2);
+        assert!(load_certificates(&wrap(&[&int(1), &set_of(&[]), &encap, &certs])).is_err());
+        assert!(load_certificates(&wrap(&[&set_of(&[]), &set_of(&[]), &encap, &certs, &set_of(&[])])).is_err());
+    }
+    let mut trailing = bundle.clone();
+    trailing.extend_from_slice(&[0x05, 0x00]);
+    assert!(load_certificates(&trailing).is_err());
+    assert!(cms::certificate_bundle(&bundle).is_ok());
     // Truncated anywhere: an error, never a panic.
     for n in 0..bundle.len() {
         let _ = load_certificates(&bundle[..n]);