nigig-org/REVIEWS/adr/0024-pdf-encryption-on-save.md

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:

• Encryption applied on save, writer side
• AES-128 (/V 4 /R 4, AESV2)
• AES-256 (/V 5 /R 6, AESV3)
• Permission flags carried into the saved file and read back
• Encrypted-write round trip through our own reader
• Encrypted-write round trip through qpdf and poppler
• The plaintext is absent from the written bytes, asserted on bytes
• Strings encrypted as well as streams
• Wrong password refused, by us and by qpdf
• Owner password opens the document
• Empty user password still encrypts (permissions-only case)
• /Encrypt written unencrypted; encrypting it fails 8 tests
• /ID generated with the key it feeds
• Fresh IV per object, isolated from key freshness
• /Length describes the ciphertext
• Unencrypted output stays byte-reproducible (ADR 0023 preserved)
• No hand-rolled primitives; getrandom for all randomness
• RC4 not writable, enforced by the type
• 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.