Issue #159: open .p12 files with trailing whitespace, PEM armour or base64 (#298)

pkcs12::open demanded clean DER at the first byte, so a file with one
extra newline, PEM armour or a bare base64 body (the shapes OpenSSL
wraps, editors and government portals emit) was refused before any
crypto ran, with "malformed data: PKCS #12: not a PKCS #12 file" and no
reason.

open now runs the bytes through normalize(): ASCII whitespace is
trimmed, "-----BEGIN" takes the base64 body between the armour markers
(byte offsets, so a marker glued to the body line works too), and a
bare base64 body is decoded — a real DER PFX always contains bytes
outside the base64 alphabet, so the fast path can't swallow one. The
exact parse reason is carried into the error instead of being dropped.

Truncated or empty files still fail, now saying why.

Test: crypto.rs::opens_wrapped_pkcs12_files_and_still_rejects_broken_ones
(a wrapped copy of rsa-aes.p12 yields the same certificate through
trailing whitespace, PEM armour and bare base64; truncated and empty
files error with the reason). It failed on all three wrapped forms
before the fix.

Co-authored-by: Mawfyy <mawfy12@gmail.com>
Co-authored-by: Brandon Thomas <echelon@gmail.com>

Cargo.lock

@@ -3751,6 +3751,7 @@ version = "0.4.0"
 dependencies = [
  "aes",
  "aws-lc-rs",
+ "base64 0.22.1",
  "cbc",
  "des",
  "getrandom 0.4.3",

crates/sign/Cargo.toml

@@ -15,6 +15,7 @@ pdfcraft-cos = { workspace = true }
 pdfcraft-annot = { workspace = true }
 pdfcraft-fonts = { workspace = true }
 thiserror.workspace = true
+base64 = "0.22"
 sha1 = "0.11"
 sha2 = "0.11"
 p256 = { version = "0.14", default-features = false, features = ["ecdsa", "std"] }

crates/sign/src/pkcs12.rs

@@ -3,12 +3,16 @@
 //!
 //! Reading supports PBES2 (PBKDF2 with HMAC-SHA-1/256/384/512; AES-128/192/256-CBC or
 //! 3DES-CBC) and the legacy PKCS #12 PBE schemes (SHA-1 with 3DES or RC2), and checks the MAC.
+//! Files that aren't clean DER still open: trailing whitespace, PEM armour or a bare
+//! base64 body (#159).
 //! Writing uses PBES2 AES-256-CBC with PBKDF2-HMAC-SHA-256 and an HMAC-SHA-256 MAC, OpenSSL 3's
 //! defaults.
 
+use base64::Engine as _;
 use cbc::cipher::block_padding::Pkcs7;
 use cbc::cipher::{BlockModeDecrypt, BlockModeEncrypt, InnerIvInit, KeyIvInit};
 use hmac::Mac;
+use std::borrow::Cow;
 
 use crate::SignError;
 use crate::der::{self, Tlv, tag};
@@ -282,9 +286,57 @@ fn bags(safe_contents: &[u8], password: &str, out: &mut Vec<Bag>) -> Result<(),
     Ok(())
 }
 
+fn normalize(bytes: &[u8]) -> Result<Cow<'_, [u8]>, SignError> {
+    let start = bytes.iter().position(|b| !b.is_ascii_whitespace()).unwrap_or(0);
+    let end = bytes.iter().rposition(|b| !b.is_ascii_whitespace()).map_or(0, |i| i + 1);
+    let trimmed = bytes.get(start..end).unwrap_or(b""); // hostile input: no slicing by hand
+
+    // 1. A DER file starts with a SEQUENCE tag: borrow, no work.
+    if trimmed.first() == Some(&0x30) {
+        return Ok(Cow::Borrowed(trimmed));
+    }
+    // 2. PEM armor: base64 body between the BEGIN/END lines.
+    if trimmed.starts_with(b"-----BEGIN") {
+        let body = pem_body(trimmed)?;
+        return Ok(Cow::Owned(decode_base64(body)?));
+    }
+    // 3. Raw base64 text, armour or not (a real DER PFX always contains bytes
+    //    outside the base64 alphabet, so this can't swallow a valid file).
+    if trimmed.iter().all(|b| b.is_ascii_alphanumeric() || *b == b'+' || *b == b'/' || *b == b'=' || b.is_ascii_whitespace()) {
+        return Ok(Cow::Owned(decode_base64(trimmed)?));
+    }
+    // 4. Not recognisable: hand the bytes to Tlv::parse_all for a clear error.
+    Ok(Cow::Borrowed(trimmed))
+}
+
+fn decode_base64(input: &[u8]) -> Result<Vec<u8>, SignError> {
+    let compact: Vec<u8> = input.iter().copied().filter(|b| !b.is_ascii_whitespace()).collect();
+    base64::engine::general_purpose::STANDARD.decode(&compact).map_err(|e| SignError::Malformed(format!("PKCS #12: base64: {e}")))
+}
+
+/// The base64 lines between "-----BEGIN …-----" and "-----END …-----".
+fn pem_body(pem: &[u8]) -> Result<&[u8], SignError> {
+    let text = std::str::from_utf8(pem).map_err(|_| bad("PEM file is not valid text"))?;
+    let begin = text.find("-----BEGIN ").ok_or_else(|| bad("PEM file has no BEGIN marker"))?;
+    let body_start = text[begin..].find('\n').map_or(text.len(), |i| begin + i + 1);
+    let body_end = text.find("-----END ").filter(|end| *end >= body_start).ok_or_else(|| bad("PEM file has no END marker"))?;
+    let body = text.as_bytes().get(body_start..body_end).ok_or_else(|| bad("PEM body"))?;
+    if !body.iter().any(|b| !b.is_ascii_whitespace()) {
+        return Err(bad("PEM file has an empty body"));
+    }
+    Ok(body)
+}
+
 /// Open a `.p12` / `.pfx` file. A wrong password is [`SignError::WrongPassword`].
 pub fn open(bytes: &[u8], password: &str) -> Result<DigitalId, SignError> {
-    let pfx = Tlv::parse_all(bytes).map_err(|_| bad("not a PKCS #12 file"))?.children()?;
+    let der = normalize(bytes)?;
+    let pfx = Tlv::parse_all(&der)
+        .map_err(|e| match e {
+            // Keep "not a PKCS #12 file" but carry the exact reason through.
+            SignError::Malformed(why) => bad(&format!("not a PKCS #12 file: {why}")),
+            e => e,
+        })?
+        .children()?;
     let auth_safe = pfx.get(1).ok_or_else(|| bad("authSafe"))?.children()?;
     if auth_safe.first().map(|o| o.oid()).transpose()?.as_deref() != Some(DATA) {
         return Err(SignError::Unsupported("public-key protected PKCS #12 files".into()));

crates/sign/tests/crypto.rs

@@ -209,6 +209,40 @@ fn crls_verify_and_report_revocation() {
     );
 }
 
+/// Issue #159: `.p12` files that aren't clean DER still open — trailing
+/// whitespace, PEM armour or a bare base64 body — and real damage still fails,
+/// with the reason in the message.
+#[test]
+fn opens_wrapped_pkcs12_files_and_still_rejects_broken_ones() {
+    use base64::Engine as _;
+    let der = data("rsa-aes.p12");
+    let signer = pkcs12::open(&der, "test").unwrap();
+
+    // An editor or a download appends whitespace.
+    let mut trailing = der.clone();
+    trailing.extend_from_slice(b"\r\n \n");
+    assert_eq!(pkcs12::open(&trailing, "test").unwrap().certificate, signer.certificate);
+
+    // PEM armour, the way OpenSSL and government portals present them.
+    let b64 = base64::engine::general_purpose::STANDARD.encode(&der);
+    let mut pem = String::from("-----BEGIN PKCS12-----\n");
+    for line in b64.as_bytes().chunks(64) {
+        pem.push_str(std::str::from_utf8(line).unwrap());
+        pem.push('\n');
+    }
+    pem.push_str("-----END PKCS12-----\n");
+    assert_eq!(pkcs12::open(pem.as_bytes(), "test").unwrap().certificate, signer.certificate);
+
+    // The base64 body alone, armour stripped.
+    assert_eq!(pkcs12::open(b64.as_bytes(), "test").unwrap().certificate, signer.certificate);
+
+    // A truncated file still fails, and says why.
+    let err = pkcs12::open(&der[..64], "test").expect_err("truncated file");
+    assert!(err.to_string().contains("runs past the end"), "{err}");
+    let err = pkcs12::open(b"", "test").expect_err("empty file");
+    assert!(err.to_string().contains("truncated DER"), "{err}");
+}
+
 #[test]
 fn ocsp_responses_verify_and_match_the_certificate() {
     let id = pkcs12::open(&data("ec-p256.p12"), "test").unwrap();