# ADR 0024: encryption on save — reversing "never write encryption" - **Status:** Accepted - **Date:** 2026-08-18 - **Review item:** `NIGIG_PDF_FEATURE_PARITY_PLAN.md` §1 Phase 6, "encryption applied on save for AES-128/256 (writer side, not just parser side)" and the exit criterion "encrypted-write round-trip" - **Supersedes:** ADR 0005's decision to "never write encryption" - **Related:** ADR 0005 (reading the standard security handler), ADR 0019 (document creation), ADR 0023 (reproducible output — deliberately given up here, for a reason) ## Context ADR 0005 implemented the standard security handler for **reading** and committed to never writing it. That was the right call at the time and for the reason it gave: a crate that only parses cannot produce weak ciphertext if it produces none. Phases 4 and 5 changed the facts. The crate now creates documents and edits them, and an editor that cannot write encryption does something worse than refuse — it **silently drops the protection**. Open a password-protected document, change one annotation, save, and the output is plaintext. No error, no warning; the file simply stops being encrypted. That is the failure mode this project keeps finding, in the one place where the consequence is a data breach rather than a wrong glyph. So the refusal now protects nobody, and the decision is reversed. The principle behind ADR 0005 is kept intact in a narrower form: **no hand-rolled cryptography, and no weak cipher offered as an option.** ## Decision ### What is written, and what is refused | Cipher | Read | Write | |---|---|---| | RC4 40/128-bit (`V2`) | yes | **refused** | | AES-128 (`AESV2`, `/V 4 /R 4`) | yes | yes | | AES-256 (`AESV3`, `/V 5 /R 6`) | yes | yes, default | RC4 stays readable because files exist that use it. It is **not writable**: it is broken, and a library that offers a broken cipher as an option will have it selected by someone who does not know that. `EncryptionAlgorithm` has no RC4 variant, so the refusal is enforced by the type rather than by a runtime check that could be bypassed. AES-256 is the default. AES-128 is kept because Acrobat 7-9 and a long tail of enterprise tooling cannot open AES-256. ### The encryptor is the exact inverse of the decryptor `encrypt_write.rs` imports `hash_r6`, `pad_password` and `rc4_apply` from `encrypt.rs` rather than restating them, and `Encryptor::object_key` is a line-for-line mirror of `Decryptor::object_key`. Two implementations of one algorithm drift, and the direction they drift in here is "decrypts to garbage" — which is exactly what an encryption round-trip test would catch and a unit test of either half alone would not. Every unit test in the module round-trips through the **existing** `Decryptor`. Nothing asserts against a hardcoded ciphertext, because a hardcoded expectation only proves the code still does what it did. ### Encryption lives at one choke point `PdfWriter` holds an optional `Encryptor`, and `write_object_at` encrypts every string and stream on the way out. It is not applied at each call site, and that is the important design decision: > Encryption is all-or-nothing. One stream written in the clear inside an > encrypted document is not a partial failure, it is a leak, and no reader > will report it because the file is otherwise perfectly valid. Putting it at the single point every object passes through makes "forgot to encrypt that one" unrepresentable rather than merely unlikely. There are twenty-two `write_object_at` call sites in `PdfDocBuilder`; auditing all of them on every future change is not a plan. One object is exempt and must be: `write_object_at_plain` writes the `/Encrypt` dictionary itself. It carries the salts and validation hashes a reader needs **before** it can derive any key. Encrypting it makes the document permanently unopenable — by us and by everyone else. ### `/ID` is generated with the key, not after it Revisions 2-4 mix the first `/ID` element into key derivation. A document whose `/ID` is regenerated after the key is derived cannot be opened at all. `PdfDocBuilder::finish` therefore generates the file ID, feeds it to `Encryptor::new`, and writes the same bytes into the trailer, so the two cannot disagree. ### Failing closed `getrandom` supplies salts and IVs. When the OS random source fails there is no safe fallback — a predictable IV in CBC is a real break, not a degradation — so `finish` returns an empty `Vec` rather than a document. Writing the file in the clear when the caller asked for a password would be the worst available outcome. ## Verification ### Round trip, through our own reader 15 integration tests in `pdf-document/tests/encryption_write.rs` and 18 unit tests in the module. The two that carry the weight: - **`the_plaintext_is_not_in_the_file`** searches the raw bytes, not any API. An API that decrypts is precisely the thing that cannot see a leak. It has a control: the same document unencrypted *does* contain the payload, so the test cannot pass by the fixture having lost it. - **`strings_are_encrypted_too_not_just_streams`** puts the payload in the `/Info` title. Encrypting streams and forgetting strings is the easy half-implementation, and it leaks document titles, form values and annotation contents. ### Round trip, through implementations we share no code with Added to `tools/check-pdf-external-readers.sh`, which CI runs: ``` == encrypted document (ADR 0024) == ok qpdf opens it with the password ok it really is AES-256 ok the wrong password is refused ok poppler decrypts the content ok no plaintext in the encrypted file ``` `qpdf --check` reports `R = 6`, `stream encryption method: AESv3` and **no syntax or stream encoding errors**; `pdfinfo` reports `Encrypted: yes (... algorithm:AES-256)`; `pdftotext -upw` extracts the same text as from the unencrypted control. AES-128 gives `R = 4` and `AESv2`. ### Mutation testing Four mutations, all killed — but **two only after the tests were strengthened**, and both failures are worth recording: | Mutation | First run | After | |---|---|---| | strings not encrypted | 3 tests fail | — | | `/Length` not updated for ciphertext | **survived** | 1 fails | | fixed IV instead of random | **survived** | 1 fails | | `/Encrypt` dict itself encrypted | 8 tests fail | — | **The fixed IV survived because of the test I thought covered it.** `two_saves_of_one_document_are_not_byte_identical` passes with a constant IV, because the AES-256 file key is regenerated on every save and that alone makes the output differ. The property I actually needed is narrower: *one* encryptor, encrypting *identical* plaintext, must produce different bytes. Holding the key fixed isolates the IV, which is the thing that matters — in CBC, a repeated IV under one key leaks that two plaintexts are equal. **The `/Length` mutation survived because our own reader is too forgiving.** It recovers from a wrong `/Length` by scanning for `endstream` — a robustness fix from ADR 0023 — so a round-trip through our stack cannot see the defect. An independent reader that trusts `/Length` would read a truncated stream and decrypt garbage. The new test measures the written bytes directly. That second one is a general lesson: **a lenient reader hides a broken writer.** Round-tripping through your own stack is necessary and not sufficient, which is why the external-reader gate exists. The `/Length` test also had to be rewritten. Its first draft searched a `String::from_utf8_lossy` view of the file and reported a stream declaring 80 bytes while holding 156 — ciphertext is not valid UTF-8, and the replacement characters shifted every offset. The bug was in the test. ## Merge criteria Taken from the Phase 6 plan text: - [x] Encryption applied on save, writer side - [x] AES-128 (`/V 4 /R 4`, `AESV2`) - [x] AES-256 (`/V 5 /R 6`, `AESV3`) - [x] Permission flags carried into the saved file and read back - [x] Encrypted-write round trip through our own reader - [x] Encrypted-write round trip through qpdf and poppler - [x] The plaintext is absent from the written bytes, asserted on bytes - [x] Strings encrypted as well as streams - [x] Wrong password refused, by us and by qpdf - [x] Owner password opens the document - [x] Empty user password still encrypts (permissions-only case) - [x] `/Encrypt` written unencrypted; encrypting it fails 8 tests - [x] `/ID` generated with the key it feeds - [x] Fresh IV per object, isolated from key freshness - [x] `/Length` describes the ciphertext - [x] Unencrypted output stays byte-reproducible (ADR 0023 preserved) - [x] No hand-rolled primitives; `getrandom` for all randomness - [x] RC4 not writable, enforced by the type - [x] Four mutations, all killed - [ ] **Digital signature writing, PKIX validation, revocation** — the rest of Phase 6. Not started; see below. ## Consequences **Positive.** A document that arrives encrypted can be edited and saved still encrypted. Phase 6's encryption exit criterion is met and gated in CI against two external implementations. **Negative, and deliberate.** Encrypted output is **not reproducible**. ADR 0023 made byte-identical output a property worth having — it makes files diffable in review and cacheable — and encryption gives it up, because the file key, the salts and every IV must be fresh. `two_saves_of_one_document_ are_not_byte_identical` asserts the loss rather than leaving it implicit, and unencrypted output is still reproducible. **Risk.** This code protects real secrets, and a subtle error here is worth more to an attacker than any other defect in this repository. The mitigations are: audited RustCrypto primitives only, the encryptor written as the literal inverse of a decryptor that already reads other producers' files, round-trips verified against qpdf and poppler, and a byte-level leak test with a control. What it has **not** had is an independent security review, and it should have one before it is offered as a product feature. **Not done:** public-key security handlers (`/Filter /Adobe.PubSec`), crypt filters other than the standard handler, encrypting only attachments (`/EFF`), and the entire signing half of Phase 6 — CMS/PKCS#7 writing, RSA and Ed25519 identities, PKIX chain validation, and revocation. That work needs the trust-anchor decision ADR 0010 identified: `VerificationStatus:: Valid` is currently unreachable by construction, and making it reachable is a policy change, not an implementation detail.