diff --git a/crates/pdf/src/crypt.rs b/crates/pdf/src/crypt.rs index 069627db9..3d541704f 100644 --- a/crates/pdf/src/crypt.rs +++ b/crates/pdf/src/crypt.rs @@ -283,7 +283,7 @@ impl SecurityHandler { /// different key is refused too. pub fn authenticate_owner(&self, id0: &[u8], password: &str) -> Result { let owner = Self::open(self.dict.clone(), id0, Some(password))?; - if owner.auth != Auth::Owner || owner.key != self.key { + if owner.auth != Auth::Owner || !same(owner.key.as_slice(), self.key.as_slice()) { return Err(CryptError::WrongPassword); } Ok(owner) @@ -539,6 +539,12 @@ fn u_value_r4(d: &EncryptDict, id0: &[u8], key: &[u8]) -> Vec { x } +/// Whether `a` and `b` are equal, in time that depends only on their lengths: password-derived +/// digests, keys and verifiers are compared without stopping at the first differing byte. +fn same(a: &[u8], b: &[u8]) -> bool { + a.len() == b.len() && a.iter().zip(b).fold(0u8, |acc, (x, y)| acc | (x ^ y)) == 0 +} + /// Algorithm 6: authenticate a user password; returns the file key. /// /// Some producers ignore `/EncryptMetadata false` when deriving the key (they omit the @@ -549,7 +555,7 @@ fn user_key_r4(d: &EncryptDict, id0: &[u8], pw: &[u8]) -> Option= n && u[..n] == d.u[..n]).then_some(key) + (d.u.len() >= n && same(&u[..n], &d.u[..n])).then_some(key) }; check(d).or_else(|| (d.r >= 4 && !d.encrypt_metadata).then(|| check(&EncryptDict { encrypt_metadata: true, ..d.clone() })).flatten()) } @@ -636,7 +642,8 @@ fn key_r6(d: &EncryptDict, pw: &[u8], owner: bool) -> Result SecurityHandler { .unwrap() } +/// Every verifier check goes through this comparison: it reads both sides in full and a value +/// that merely starts the same never matches. +#[test] +fn verifier_comparison_reads_every_byte() { + assert!(super::same(b"", b"")); + assert!(super::same(b"0123456789abcdef", b"0123456789abcdef")); + assert!(!super::same(b"0123456789abcdef", b"0123456789abcdee"), "difference in the last byte"); + assert!(!super::same(b"0123456789abcdef", b"0123456789abcde"), "shorter value"); +} + const ALL: [Algorithm; 4] = [Algorithm::Rc4_40, Algorithm::Rc4_128, Algorithm::Aes128, Algorithm::Aes256]; #[test] diff --git a/crates/pkcs7/src/pkcs12.rs b/crates/pkcs7/src/pkcs12.rs index f14b2297e..ee245f380 100644 --- a/crates/pkcs7/src/pkcs12.rs +++ b/crates/pkcs7/src/pkcs12.rs @@ -184,6 +184,12 @@ impl Cipher { } } +/// Whether `a` and `b` are equal, in time that depends only on their lengths: the MAC is derived +/// from the password, so it is compared without stopping at the first differing byte. +fn same(a: &[u8], b: &[u8]) -> bool { + a.len() == b.len() && a.iter().zip(b).fold(0u8, |acc, (x, y)| acc | (x ^ y)) == 0 +} + fn hmac(alg: DigestAlg, key: &[u8], data: &[u8]) -> Result, Error> { fn run(key: &[u8], data: &[u8]) -> Result, Error> where @@ -411,7 +417,8 @@ pub fn open(bytes: &[u8], password: &str) -> Result { let key_len = alg.digest(&[]).len(); let iterations = budget.spend(alg, iterations, key_len)?; let key = pkcs12_kdf(alg, &bmp(password), &salt, 3, iterations, key_len); - if hmac(alg, &key, &content)? != *expected { + let computed = hmac(alg, &key, &content)?; + if !same(&computed, &expected) { return Err(Error::WrongPassword); } }