Skip to content

Fix AEAD: verify chunk auth tag before releasing plaintext (#807) - #2422

Open
ronaldtse wants to merge 2 commits into
mainfrom
fix-aead-unauthenticated-chunks
Open

Fix AEAD: verify chunk auth tag before releasing plaintext (#807)#2422
ronaldtse wants to merge 2 commits into
mainfrom
fix-aead-unauthenticated-chunks

Conversation

@ronaldtse

@ronaldtse ronaldtse commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes #807. Fixes a 6-year-old security issue where rnp released intermediate AEAD chunk plaintext to the caller before the chunk's authentication tag was verified.

What was wrong

encrypted_src_read_aead_part() called pgp_cipher_aead_update() for intermediate reads within a chunk and immediately set param->cachelen = used, making the decrypted data available for reading. The chunk's auth tag was only verified later at chunk-end via pgp_cipher_aead_finish(). This violated RFC 9580 §5.16.2 and meant a malformed/attacker-controlled message could cause rnp to process unauthenticated data.

The fix

Added a chunk_buf accumulator (std::vector<uint8_t>) to pgp_source_encrypted_param_t:

  1. Intermediate reads: pgp_cipher_aead_update() decrypts in-place in cache (efficient), then the result is memcpy'd to chunk_buf. cachelen is NOT set — no plaintext released.
  2. Chunk-end: pgp_cipher_aead_finish() verifies the tag. On success, the final decrypted piece is added to chunk_buf, and the first batch is copied to cache for reading. On failure, chunk_buf is secure_clear'd — no unverified data persists.
  3. Release loop: encrypted_src_read_aead_part() checks chunk_bufpos < chunk_buflen on each call and releases the next batch from chunk_buf to cache. Handles chunks larger than the fixed-size cache (up to 64KiB per RFC 9580).
  4. Chunk transition: chunk_buf is reset between chunks.

Memory

chunk_buf is heap-allocated, sized to param->chunklen (max 64KiB per RFC 9580). One allocation per decryption stream.

Test plan

  • 20 existing AEAD + encrypt/decrypt tests pass locally (OpenSSL, macOS).
  • CI: full test suite green on both backends.
  • (Follow-up) regression test with a corrupted-chunk-tag message fixture.

Security impact

This was a real security bug (reported by @teythoon in 2019). Processing unauthenticated AEAD data could allow an attacker to cause rnp to emit "data that looks like it came from this encrypted message" without proper authorisation. The fix ensures strict tag-then-release ordering per RFC 9580.

A CVE may be warranted — maintainer decision.

@ronaldtse
ronaldtse requested review from antonsviridenko and ni4 July 31, 2026 15:57
ronaldtse added a commit that referenced this pull request Aug 1, 2026
@ronaldtse
tests: add assert-encrypted-output test pattern
7a25eb9 
Adds test_no_plaintext_leakage covering 4 security-critical code paths:

1. Password encryption: output must not contain plaintext marker.
2. Public-key encryption: output must not contain plaintext marker.
3. Secret-key export: output must not contain the passphrase.
4. Detached signature: output must not contain the signed plaintext.

Each case generates a known plaintext with a distinctive marker string,
runs the operation, captures the full output, and asserts (via memmem)
that the marker is absent. A regression that accidentally produces
unencrypted output — or that leaks key material before encryption —
would be caught here.

Also adds docs/develop/testing-crypto-paths.adoc documenting the
convention so future crypto-path PRs follow the same discipline.

Per the AEAD unauthenticated-chunk fix (#807, PR #2422), this class
of silent-leak bug is real and worth systematic prevention.
ronaldtse added a commit that referenced this pull request Aug 4, 2026
@ronaldtse
tests: add assert-encrypted-output test pattern
c75fb0b 
Adds test_no_plaintext_leakage covering 4 security-critical code paths:

1. Password encryption: output must not contain plaintext marker.
2. Public-key encryption: output must not contain plaintext marker.
3. Secret-key export: output must not contain the passphrase.
4. Detached signature: output must not contain the signed plaintext.

Each case generates a known plaintext with a distinctive marker string,
runs the operation, captures the full output, and asserts (via memmem)
that the marker is absent. A regression that accidentally produces
unencrypted output — or that leaks key material before encryption —
would be caught here.

Also adds docs/develop/testing-crypto-paths.adoc documenting the
convention so future crypto-path PRs follow the same discipline.

Per the AEAD unauthenticated-chunk fix (#807, PR #2422), this class
of silent-leak bug is real and worth systematic prevention.
@ronaldtse
Fix AEAD: verify chunk auth tag before releasing plaintext (#807)
027a2fe 
rnp previously released intermediate AEAD chunk plaintext to the read
cache BEFORE the chunk's authentication tag was verified. This violated
RFC 9580 §5.16.2 and negated the security benefits of AEAD: a
malformed or attacker-controlled message could cause rnp to process
unauthenticated data.

Fix: accumulate decrypted-but-unverified plaintext in a separate
chunk_buf. Only after pgp_cipher_aead_finish() verifies the chunk's
auth tag is the accumulated plaintext copied to the read cache and
made available to the caller.

On tag verification failure: secure_clear the accumulated plaintext so
no unverified data persists in memory.

Implementation notes:
  * chunk_buf is heap-allocated (std::vector), sized to the chunk size
    from the AEAD header (max 64KiB per RFC 9580).
  * The release mechanism copies from chunk_buf to the fixed-size cache
    in pieces (up to PGP_AEAD_CACHE_LEN at a time), maintaining the
    existing streaming read model.
  * Intermediate pgp_cipher_aead_update() calls still decrypt in-place
    in cache for efficiency; the result is then memcpy'd to chunk_buf.
  * chunk_buf is reset between chunks.

All 20 existing AEAD + encrypt/decrypt tests pass locally (OpenSSL
backend, macOS). No behaviour change for correctly-formed messages.

Closes #807.
@ronaldtse
Fix AEAD chunk auth: release plaintext only after tag verification (#807
1323ebe 
)

Buffer each chunk's plaintext in chunk_buf and copy it to cache only after
pgp_cipher_aead_finish() verifies the chunk tag (RFC 9580 §5.16.2). Gate the
release on a chunk_verified flag, compact the un-decrypted remainder to the
front of cache in the intermediate path (so it no longer depends on cachelen),
and do not start the next chunk until chunk_buf is fully drained. Leave cache
untouched at chunk end so the summary tag read by the last-chunk finish() is
preserved.
@ronaldtse
ronaldtse force-pushed the fix-aead-unauthenticated-chunks branch from 92846e0 to 1323ebe Compare August 9, 2026 23:06
ronaldtse added a commit that referenced this pull request Aug 10, 2026
@ronaldtse
tests: add negative tests for PQC PKESK and Argon2 S2K (#2355, #2296 …
acd2667 
…follow-ups)

The merged PRs #2355 (PQC draft 12 + v6 salt + Ed448/X448) and #2296
(Argon2 S2K + AEAD secret-key encryption) shipped with happy-path
coverage only — import sample, verify, decrypt with correct password.
This PR adds the missing negative coverage:

- test_ffi_argon2_locked_seckey_wrong_password: wrong password must
  fail cleanly and not leave the key in a partial-unlock state;
  subsequent correct password still works after multiple failures.
- test_ffi_decrypt_argon2_skesk_wrong_password: SKESK decryption
  fails without leaking plaintext when the derived AEAD key is wrong.
- test_ffi_decrypt_pqc_pkesk_corrupted: a single byte flip near the
  end of a valid PQC-encrypted message must cause decryption to fail
  (exercises the AEAD tag verification path on the PQC PKESK).

These close the "happy-path only" codecov gap for the AEAD-tag
verification paths in stream-parse.cpp (PR #2422's earlier fix
extends to PQC ciphertexts too) and for the Argon2 S2K derivation
failure path in stream-key.cpp.

Note: locally the build is blocked by a pre-existing Botan 3.12
API incompatibility in src/lib/crypto/ec.cpp (uses Botan::EC_Group
members that became opaque in 3.12). CI runs against the Botan
version pinned in the centos-and-fedora workflow (3.6 / 3.12 from
source) where this is not an issue.
ronaldtse added a commit that referenced this pull request Aug 10, 2026
@ronaldtse
tests: add assert-encrypted-output test pattern
d5b788d 
Adds test_no_plaintext_leakage covering 4 security-critical code paths:

1. Password encryption: output must not contain plaintext marker.
2. Public-key encryption: output must not contain plaintext marker.
3. Secret-key export: output must not contain the passphrase.
4. Detached signature: output must not contain the signed plaintext.

Each case generates a known plaintext with a distinctive marker string,
runs the operation, captures the full output, and asserts (via memmem)
that the marker is absent. A regression that accidentally produces
unencrypted output — or that leaks key material before encryption —
would be caught here.

Also adds docs/develop/testing-crypto-paths.adoc documenting the
convention so future crypto-path PRs follow the same discipline.

Per the AEAD unauthenticated-chunk fix (#807, PR #2422), this class
of silent-leak bug is real and worth systematic prevention.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@antonsviridenko antonsviridenko Awaiting requested review from antonsviridenko
@ni4 ni4 Awaiting requested review from ni4

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

rnp processes unauthenticated partial chunks of AEAD encrypted data packets

1 participant

@ronaldtse