Merge pull request #1341 from storytold/sec/pdf-no-rc4-create

craft-pdf: refuse to create RC4 encryption without an explicit legacy call

crates/pdf/README.md

@@ -202,9 +202,12 @@ PDF files. The reader decrypts on its own and does not use it. It covers revisio
   block is ignored and bad padding is left in.
 - `permissions()` gives the `/P` flags; for R5/R6 a permission counts only when `/Perms` grants
   it too. Owners have every permission.
-- `create(&NewEncryption, id0)` builds the `/Encrypt` values for RC4-40, RC4-128, AES-128 or
-  AES-256. The caller supplies 32 bytes of entropy (`seed`), so the module needs no OS access
-  and builds for the web. `file_id` derives a file identifier from the same seed.
+- `create(&NewEncryption, id0)` builds the `/Encrypt` values for AES-128 or AES-256. It refuses
+  the legacy RC4-40 and RC4-128 schemes, because RC4 is broken (RFC 7465); `create_legacy`
+  writes those, for a file that must stay readable by an old reader. `Document::set_encryption`
+  refuses them too, with `set_encryption_legacy` as the explicit way in. The caller supplies 32
+  bytes of entropy (`seed`), so the module needs no OS access and builds for the web. `file_id`
+  derives a file identifier from the same seed.
 - `EncryptDict` holds the dictionary's values. The caller reads them from, and writes them to,
   its own object model; the module does not parse PDF syntax. Keys and prepared passwords are
   zeroized when dropped.

crates/pdf/examples/cos_protect.rs

@@ -22,7 +22,8 @@ fn main() {
         _ => craft_pdf::cos::Algorithm::Aes256,
     };
     let mut doc = craft_pdf::cos::Document::open(std::sync::Arc::new(std::fs::read(input).unwrap())).unwrap();
-    doc.set_encryption(&craft_pdf::cos::NewEncryption {
+    // `rc4-40` and `rc4-128` are the legacy schemes, which `set_encryption` refuses.
+    doc.set_encryption_legacy(&craft_pdf::cos::NewEncryption {
         algorithm,
         user_password: user,
         owner_password: owner,

crates/pdf/src/cos/document/open.rs

@@ -414,7 +414,20 @@ impl Document {
 
     /// Protect the document with a password (§7.6.4). Takes effect on the next save, which is
     /// always a full rewrite. Returns the handler (authenticated as owner).
+    ///
+    /// The legacy RC4 schemes are refused (RC4 is broken, `RFC 7465`): [`Self::set_encryption_legacy`]
+    /// writes them for a file an old reader must open.
     pub fn set_encryption(&mut self, params: &crate::crypt::NewEncryption) -> Result<(), CosError> {
+        self.set_encryption_with(params, false)
+    }
+
+    /// [`Self::set_encryption`] with the legacy RC4-40 and RC4-128 schemes allowed. Pass it only
+    /// for a file that must stay readable by a reader without AES.
+    pub fn set_encryption_legacy(&mut self, params: &crate::crypt::NewEncryption) -> Result<(), CosError> {
+        self.set_encryption_with(params, true)
+    }
+
+    fn set_encryption_with(&mut self, params: &crate::crypt::NewEncryption, legacy: bool) -> Result<(), CosError> {
         // The file identifier is part of the key; make sure it exists and keep it.
         let id = self.trailer.get(b"ID").map(|id| self.resolve(id));
         let id0 = match id.as_deref() {
@@ -426,7 +439,8 @@ impl Document {
                 id
             }
         };
-        let h = crate::crypt::create(params, &id0).map_err(|e| CosError::Security(e.to_string()))?;
+        let built = if legacy { crate::crypt::create_legacy(params, &id0) } else { crate::crypt::create(params, &id0) };
+        let h = built.map_err(|e| CosError::Security(e.to_string()))?;
         let d = h.dict();
         let s = |b: &[u8]| Object::String(crate::cos::PdfString { bytes: b.to_vec(), hex: true });
         let mut e = Dict::new();

crates/pdf/src/crypt.rs

@@ -658,9 +658,9 @@ fn key_r6(d: &EncryptDict, pw: &[u8], owner: bool) -> Result<Option<Zeroizing<Ve
 /// Which encryption to create.
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub enum Algorithm {
-    /// R2, 40-bit RC4 (legacy; for tests and compatibility only).
+    /// R2, 40-bit RC4 (legacy: [`create`] refuses it, [`create_legacy`] writes it).
     Rc4_40,
-    /// R3, 128-bit RC4.
+    /// R3, 128-bit RC4 (legacy: [`create`] refuses it, [`create_legacy`] writes it).
     Rc4_128,
     /// R4, AES-128 crypt filters.
     Aes128,
@@ -668,6 +668,14 @@ pub enum Algorithm {
     Aes256,
 }
 
+impl Algorithm {
+    /// Whether this is one of the broken legacy ciphers: read for compatibility, and refused for
+    /// new encryption by [`create`].
+    pub fn is_legacy(self) -> bool {
+        matches!(self, Algorithm::Rc4_40 | Algorithm::Rc4_128)
+    }
+}
+
 /// A password as [`create`] prepares it: for R2–R4 its PDFDocEncoding (characters it lacks
 /// dropped), at most 32 bytes; for R6 its SASLprep (RFC 4013: soft hyphens and the like removed,
 /// other spaces made plain, then NFKC; the password as given if SASLprep refuses it), in UTF-8,
@@ -732,7 +740,23 @@ pub fn file_id(seed: &[u8; 32]) -> Vec<u8> {
 }
 
 /// Build the `/Encrypt` values and an authenticated (owner) handler for a new encryption.
+///
+/// The legacy RC4 schemes are refused, because RC4 is broken (`RFC 7465`) and a file written with
+/// it protects nothing: [`create_legacy`] is the explicit way to write them.
 pub fn create(params: &NewEncryption, id0: &[u8]) -> Result<SecurityHandler, CryptError> {
+    if params.algorithm.is_legacy() {
+        return Err(CryptError::Unsupported("RC4 encryption is legacy (RFC 7465); use create_legacy for a file an old reader must open".into()));
+    }
+    create_any(params, id0)
+}
+
+/// [`create`] without the RC4 gate: writes RC4-40 and RC4-128 too. For a file that must stay
+/// readable by a reader without AES, and for tests that read and write every algorithm.
+pub fn create_legacy(params: &NewEncryption, id0: &[u8]) -> Result<SecurityHandler, CryptError> {
+    create_any(params, id0)
+}
+
+fn create_any(params: &NewEncryption, id0: &[u8]) -> Result<SecurityHandler, CryptError> {
     // R2 reserves bits 7-32; later revisions reserve bits 7-8 and 13-32.
     // Reserved bits must be 1, and bits 1-2 must be 0 (§7.6.4.2).
     let reserved = if params.algorithm == Algorithm::Rc4_40 { !0x3F } else { !0xFFF | 0xC0 };

crates/pdf/src/crypt/tests.rs

@@ -52,13 +52,35 @@ fn aes_padding_is_added_and_tolerantly_removed() {
 const ID: &[u8] = b"0123456789abcdef";
 
 fn new(alg: Algorithm, user: &str, owner: &str, perms: i32) -> SecurityHandler {
-    create(
+    // These tests read and write every algorithm, RC4 included.
+    create_legacy(
         &NewEncryption { algorithm: alg, user_password: user, owner_password: owner, permissions: perms, encrypt_metadata: true, seed: [42; 32] },
         ID,
     )
     .unwrap()
 }
 
+/// New files are not written with RC4: `create` refuses it, and the explicitly named
+/// `create_legacy` is the only way to write it.
+#[test]
+fn create_refuses_rc4_unless_asked() {
+    let params = |algorithm| NewEncryption {
+        algorithm,
+        user_password: "user",
+        owner_password: "owner",
+        permissions: -1,
+        encrypt_metadata: true,
+        seed: [42; 32],
+    };
+    for (alg, revision) in [(Algorithm::Rc4_40, 2), (Algorithm::Rc4_128, 3)] {
+        assert!(matches!(create(&params(alg), ID), Err(CryptError::Unsupported(_))), "{alg:?}");
+        assert_eq!(create_legacy(&params(alg), ID).unwrap().dict().r, revision, "{alg:?}");
+    }
+    for alg in [Algorithm::Aes128, Algorithm::Aes256] {
+        assert!(create(&params(alg), ID).is_ok(), "AES is not legacy: {alg:?}");
+    }
+}
+
 /// Every verifier check goes through this comparison: it reads both sides in full and a value
 /// that merely starts the same never matches.
 #[test]

crates/pdf/tests/cos/bounded_writer.rs

@@ -139,7 +139,8 @@ fn full_rewrites_preserve_graph_across_multiple_object_streams_and_evictions() {
 }
 
 fn protect(doc: &mut Document, algorithm: Algorithm) {
-    doc.set_encryption(&NewEncryption {
+    // Every algorithm, RC4 included, has to survive a rewrite.
+    doc.set_encryption_legacy(&NewEncryption {
         algorithm,
         user_password: "user",
         owner_password: "owner",

crates/pdf/tests/cos/encryption.rs

@@ -39,9 +39,11 @@ fn fixture_with(extra: &str) -> Vec<u8> {
     out
 }
 
+/// Every algorithm, RC4-40 and RC4-128 included: those two are the legacy schemes, which
+/// `set_encryption` refuses.
 fn protect(alg: Algorithm, user: &str, owner: &str, permissions: i32) -> Vec<u8> {
     let mut doc = Document::open(Arc::new(fixture())).unwrap();
-    doc.set_encryption(&NewEncryption {
+    doc.set_encryption_legacy(&NewEncryption {
         algorithm: alg,
         user_password: user,
         owner_password: owner,
@@ -287,7 +289,7 @@ fn signature_contents_stay_unencrypted() {
             c.set(b"Test".to_vec(), Object::Array(vec![Object::Ref(sig), Object::Ref(untyped), Object::Ref(note)]))
         })
         .unwrap();
-        doc.set_encryption(&NewEncryption {
+        doc.set_encryption_legacy(&NewEncryption {
             algorithm: alg,
             user_password: "",
             owner_password: "owner",
@@ -423,7 +425,7 @@ fn an_earlier_revision_opens_with_the_documents_own_credentials() {
         // Another file, encrypted with other keys: the document's key is no use, and it says so.
         let other = {
             let mut doc = Document::open(Arc::new(fixture())).unwrap();
-            doc.set_encryption(&NewEncryption {
+            doc.set_encryption_legacy(&NewEncryption {
                 algorithm: alg,
                 user_password: "user",
                 owner_password: "owner",
@@ -444,7 +446,7 @@ fn an_earlier_revision_opens_with_the_documents_own_credentials() {
 /// instead of as an indirect object.
 fn protect_inline(alg: Algorithm, user: &str, owner: &str) -> Vec<u8> {
     let mut doc = Document::open(Arc::new(fixture())).unwrap();
-    doc.set_encryption(&NewEncryption {
+    doc.set_encryption_legacy(&NewEncryption {
         algorithm: alg,
         user_password: user,
         owner_password: owner,

crates/pdf/tests/cos/file_identifier.rs

@@ -122,7 +122,8 @@ fn caller_entropy_makes_the_new_identifier_reproducible() {
 fn an_encrypted_rewrite_keeps_the_first_identifier_string_its_key_depends_on() {
     for algorithm in [Algorithm::Rc4_128, Algorithm::Aes128, Algorithm::Aes256] {
         let mut doc = reopen(source(true));
-        doc.set_encryption(&NewEncryption {
+        // The loop covers RC4-128, which `set_encryption` refuses.
+        doc.set_encryption_legacy(&NewEncryption {
             algorithm,
             user_password: "user",
             owner_password: "owner",
@@ -160,7 +161,7 @@ fn unusable_ids() -> Vec<(&'static str, Option<Object>)> {
 /// An encrypted file whose key is derived from the empty string, with its trailer `/ID` replaced by `id`.
 fn protected_with_id(algorithm: Algorithm, id: Option<Object>) -> Vec<u8> {
     let mut doc = reopen(source_with(Some("/ID [<> <>]")));
-    doc.set_encryption(&NewEncryption {
+    doc.set_encryption_legacy(&NewEncryption {
         algorithm,
         user_password: "user",
         owner_password: "owner",

crates/pdf/tests/crypt/permissions_revision.rs

@@ -3,7 +3,7 @@
 //!
 //! Bit n has the mask `1 << (n - 1)`. Readers use bits 3 to 6 and 9 to 12 and ignore the rest.
 
-use craft_pdf::crypt::{Algorithm, Auth, NewEncryption, Permissions, SecurityHandler, create};
+use craft_pdf::crypt::{Algorithm, Auth, NewEncryption, Permissions, SecurityHandler, create_legacy};
 
 const ID: &[u8] = b"0123456789abcdef";
 const T: bool = true;
@@ -18,7 +18,7 @@ fn cells(p: &Permissions) -> [bool; 8] {
 /// Creates a file with `flags` at revision `revision`, opens it with the user password, and
 /// returns the user's permissions.
 fn user_view(alg: Algorithm, revision: i64, flags: i32) -> Permissions {
-    let made = create(
+    let made = create_legacy(
         &NewEncryption { algorithm: alg, user_password: "user", owner_password: "owner", permissions: flags, encrypt_metadata: true, seed: [42; 32] },
         ID,
     )
@@ -56,7 +56,7 @@ fn revision_three_rows_hold_through_a_created_and_reopened_file() {
 
 #[test]
 fn owner_access_gives_every_flag() {
-    let made = create(
+    let made = create_legacy(
         &NewEncryption {
             algorithm: Algorithm::Rc4_128,
             user_password: "user",

crates/pdf/tests/crypt/proptests.rs

@@ -19,7 +19,7 @@
 
 use craft_pdf::crypt::{
     Algorithm, Auth, CryptError, Method, NewEncryption, PADDING, SecurityHandler, StreamKind, aes_cbc_decrypt, aes_cbc_encrypt, create,
-    effective_password, pdfdoc_byte, rc4,
+    create_legacy, effective_password, pdfdoc_byte, rc4,
 };
 use proptest::prelude::*;
 
@@ -92,8 +92,12 @@ fn expected_auth(algorithm: Algorithm, user_target: &[u8], owner_target: &[u8],
 }
 
 fn make(algorithm: Algorithm, user: &str, owner: &str) -> SecurityHandler {
-    create(&NewEncryption { algorithm, user_password: user, owner_password: owner, permissions: -1, encrypt_metadata: true, seed: [0x5e; 32] }, ID)
-        .unwrap()
+    // These properties read and write every algorithm, RC4 included.
+    create_legacy(
+        &NewEncryption { algorithm, user_password: user, owner_password: owner, permissions: -1, encrypt_metadata: true, seed: [0x5e; 32] },
+        ID,
+    )
+    .unwrap()
 }
 
 proptest! {
@@ -457,7 +461,7 @@ proptest! {
         }
 
         // Revisions below 4 always encrypt metadata, whatever the caller asked for.
-        let legacy = create(&NewEncryption { algorithm: Algorithm::Rc4_128, user_password: "user", owner_password: "owner", permissions: -1, encrypt_metadata: false, seed: [0x5e; 32] }, ID).unwrap();
+        let legacy = create_legacy(&NewEncryption { algorithm: Algorithm::Rc4_128, user_password: "user", owner_password: "owner", permissions: -1, encrypt_metadata: false, seed: [0x5e; 32] }, ID).unwrap();
         prop_assert!(legacy.dict().encrypt_metadata && legacy.encrypts_metadata(), "V<4 always encrypts metadata, as create() documents");
     }