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:

- [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.