From 5ec14155ff077f103b152b2cea1cb7365d1d723e Mon Sep 17 00:00:00 2001 From: Mawfyy Date: Thu, 8 Oct 2026 11:15:01 -0500 Subject: [PATCH] Issue #159: open .p12 files with trailing whitespace, PEM armour or base64 (#298) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Co-authored-by: Brandon Thomas --- Cargo.lock | 1 + crates/sign/Cargo.toml | 1 + crates/sign/src/pkcs12.rs | 54 ++++++++++++++++++++++++++++++++++++- crates/sign/tests/crypto.rs | 34 +++++++++++++++++++++++ 4 files changed, 89 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 9384aeee8..8e207b71a 100644 --- a/Cargo.lock +++ b/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", diff --git a/crates/sign/Cargo.toml b/crates/sign/Cargo.toml index ddb23cdd7..a09a78bde 100644 --- a/crates/sign/Cargo.toml +++ b/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"] } diff --git a/crates/sign/src/pkcs12.rs b/crates/sign/src/pkcs12.rs index 419d25c11..11d1e717d 100644 --- a/crates/sign/src/pkcs12.rs +++ b/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) -> Result<(), Ok(()) } +fn normalize(bytes: &[u8]) -> Result, 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, SignError> { + let compact: Vec = 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 { - 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())); diff --git a/crates/sign/tests/crypto.rs b/crates/sign/tests/crypto.rs index f771911f2..f66179bfd 100644 --- a/crates/sign/tests/crypto.rs +++ b/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();