From 53bb927a8794635b484a277f0984d725ccb34565 Mon Sep 17 00:00:00 2001 From: ddidderr Date: Sat, 2 May 2026 21:28:26 +0200 Subject: [PATCH] fix(header): preserve on-disk version through decode/encode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Header::encode()` previously hard-coded `VERSION_CURRENT` (= 2) on every write. Because the encoded header is fed back as AEAD AAD on decrypt, this broke decryption of any v1 ciphertext written before commit 75afadb: the file's authenticated AAD has version byte 1, but the recomputed AAD has byte 2, so AEAD verification fails on every chunk. The release notes for 75afadb explicitly promised v1 compatibility, so this is a regression against the documented contract — caught by an external reviewer who reproduced it by encrypting with HEAD^ and decrypting with HEAD. Fix by adding a `version: u8` field to `Header`. `Header::read()` now captures the on-disk byte and `encode()` writes it back. New encrypts in `crypto::encrypt()` set `version = VERSION_CURRENT`, so freshly written files are unchanged on the wire; only the round-trip path through `read → encode` is now byte-identical for v1 inputs. This was the simplest fix that preserves the existing AAD design (header bytes verbatim → AAD). Alternatives considered: - Storing the raw header bytes alongside the parsed struct would also work but spends an extra allocation and adds a second source of truth for the same data. - Conditionally emitting v2 only when flags != 0 would happen to produce v1 bytes for a v1 input, but it conflates "what version does this file claim" with "does it carry length commitment" — two things that should stay independent for future flag bits. Test plan: - `header::tests::reads_v1_header` now also asserts that re-encoding a parsed v1 header reproduces the original bytes exactly (so the AAD round-trips). - New `crypto::tests::decrypts_v1_ciphertext` and `decrypts_v1_ciphertext_parallel` build a multi-chunk v1 fixture by hand (XChaCha20Poly1305 with version-byte-1 AAD) and confirm `decrypt()` succeeds on both the serial and parallel paths. - Manual: built `HEAD^` in a worktree, encrypted a 200-byte payload with `--chunk-size 64`, decrypted with the patched binary at `-j 1` and `-j 4`. Both round-trip; output bytes match input. Refs: external review identifying this regression. --- src/crypto.rs | 125 +++++++++++++++++++++++++++++++++++++++++++++++++- src/header.rs | 18 +++++++- 2 files changed, 140 insertions(+), 3 deletions(-) diff --git a/src/crypto.rs b/src/crypto.rs index 00a9515..5ac202a 100644 --- a/src/crypto.rs +++ b/src/crypto.rs @@ -6,7 +6,9 @@ use std::io::{BufReader, Read, Seek, SeekFrom, Write}; use std::sync::Arc; use crate::error::*; -use crate::header::{AlgId, FLAG_LENGTH_COMMITTED, Header, KdfParams, NONCE_PREFIX_LEN, TAG_LEN}; +use crate::header::{ + AlgId, FLAG_LENGTH_COMMITTED, Header, KdfParams, NONCE_PREFIX_LEN, TAG_LEN, VERSION_CURRENT, +}; use crate::pipeline; use crate::reader::{AheadReader, ReadInfoChunk}; use crate::secrets::{SecretBytes32, SecretVec}; @@ -96,6 +98,7 @@ pub fn encrypt>( 0 }; let header = Header { + version: VERSION_CURRENT, alg: AlgId::XChaCha20Poly1305, flags, chunk_size, @@ -347,3 +350,123 @@ pub fn decrypt_range>( out.commit()?; Ok(()) } + +#[cfg(test)] +mod tests { + //! Regression tests for cross-version compatibility. The on-disk header + //! is part of the AEAD AAD, so any byte that ends up in `Header::encode()` + //! must match the bytes that were authenticated when the file was + //! written. The v1 test below catches the regression where `encode()` + //! used to hard-code the current version on output. + use super::*; + use crate::header::{Header, KdfParams, NONCE_PREFIX_LEN}; + use std::fs; + use tempfile::TempDir; + + fn write_v1_ciphertext(path: &std::path::Path, key: &SecretBytes32, plaintext: &[u8]) { + // Build a v1 header by hand: same wire format as v2 with flags=0, + // but with version byte = 1. + let nonce_prefix = [0x42u8; NONCE_PREFIX_LEN]; + let header = Header { + version: 1, + alg: AlgId::XChaCha20Poly1305, + flags: 0, + chunk_size: 64, + kdf: KdfParams::Raw, + nonce_prefix, + plaintext_length: None, + }; + let aad = header.encode(); + // First byte after MAGIC is the version — verify our fixture really + // is v1 (so this test fails open if encode() ever reverts). + assert_eq!(aad[4], 1); + + let chunk_sz = header.chunk_size as usize; + let aead = build_aead(key); + + let mut out = Vec::new(); + out.extend_from_slice(&aad); + + let mut counter: u32 = 0; + let mut pos = 0; + while pos < plaintext.len() { + let end = (pos + chunk_sz).min(plaintext.len()); + let last = end == plaintext.len() && (end - pos) < chunk_sz; + let mut buf = plaintext[pos..end].to_vec(); + let nonce = make_nonce(&nonce_prefix, counter, last); + aead.encrypt_in_place(&nonce, &aad, &mut buf).unwrap(); + out.extend_from_slice(&buf); + pos = end; + if last { + break; + } + // If we hit a chunk-boundary EOF we still need a trailing "last" + // empty chunk to authenticate end-of-stream. + counter = bump_counter(counter).unwrap(); + if pos == plaintext.len() { + let mut empty = Vec::new(); + let nonce = make_nonce(&nonce_prefix, counter, true); + aead.encrypt_in_place(&nonce, &aad, &mut empty).unwrap(); + out.extend_from_slice(&empty); + break; + } + } + // Empty plaintext: emit a single empty "last" chunk. + if plaintext.is_empty() { + let mut empty = Vec::new(); + let nonce = make_nonce(&nonce_prefix, 0, true); + aead.encrypt_in_place(&nonce, &aad, &mut empty).unwrap(); + out.extend_from_slice(&empty); + } + fs::write(path, &out).unwrap(); + } + + #[test] + fn decrypts_v1_ciphertext() { + let dir = TempDir::new().unwrap(); + let ct = dir.path().join("v1.bin"); + let rt = dir.path().join("rt.bin"); + + let mut key = SecretBytes32::zeroed(); + key.with_mut_array(|k| k.copy_from_slice(b"0123456789abcdef0123456789abcdef")); + + // Multi-chunk plaintext (chunk_size = 64 in the fixture). + let plain: Vec = (0..200u8).collect(); + write_v1_ciphertext(&ct, &key, &plain); + + decrypt( + Some(ct.to_str().unwrap()), + Some(rt.to_str().unwrap()), + Some(&key), + None, + 1, + ) + .expect("v1 decrypt should succeed"); + let got = fs::read(&rt).unwrap(); + assert_eq!(got, plain); + } + + #[test] + fn decrypts_v1_ciphertext_parallel() { + // Same fixture, but exercising the multi-threaded pipeline. + let dir = TempDir::new().unwrap(); + let ct = dir.path().join("v1.bin"); + let rt = dir.path().join("rt.bin"); + + let mut key = SecretBytes32::zeroed(); + key.with_mut_array(|k| k.copy_from_slice(b"0123456789abcdef0123456789abcdef")); + + let plain: Vec = (0..200u8).collect(); + write_v1_ciphertext(&ct, &key, &plain); + + decrypt( + Some(ct.to_str().unwrap()), + Some(rt.to_str().unwrap()), + Some(&key), + None, + 4, + ) + .expect("v1 parallel decrypt should succeed"); + assert_eq!(fs::read(&rt).unwrap(), plain); + } +} diff --git a/src/header.rs b/src/header.rs index aef90d2..cd9fa06 100644 --- a/src/header.rs +++ b/src/header.rs @@ -34,7 +34,7 @@ use std::io::Read; use crate::error::FcryError; const MAGIC: [u8; 4] = *b"fcry"; -const VERSION_CURRENT: u8 = 2; +pub const VERSION_CURRENT: u8 = 2; const VERSION_MIN: u8 = 1; pub const NONCE_PREFIX_LEN: usize = 19; @@ -131,6 +131,10 @@ impl KdfParams { #[derive(Clone, Debug)] pub struct Header { + /// On-disk format version. Set to `VERSION_CURRENT` for new encrypts; + /// preserved as-read for decrypt so the AAD recomputed on decode matches + /// the bytes that were authenticated when the file was written. + pub version: u8, pub alg: AlgId, pub flags: u8, pub chunk_size: u32, @@ -144,7 +148,7 @@ impl Header { pub fn encode(&self) -> Vec { let mut out = Vec::with_capacity(72); out.extend_from_slice(&MAGIC); - out.push(VERSION_CURRENT); + out.push(self.version); out.push(self.alg as u8); out.push(self.flags); out.push(0); // reserved @@ -210,6 +214,7 @@ impl Header { }; Ok(Self { + version, alg, flags, chunk_size, @@ -228,6 +233,7 @@ mod tests { #[test] fn roundtrip() { let h = Header { + version: VERSION_CURRENT, alg: AlgId::XChaCha20Poly1305, flags: 0, chunk_size: 1024 * 1024, @@ -238,6 +244,7 @@ mod tests { let bytes = h.encode(); let mut cur = Cursor::new(&bytes); let parsed = Header::read(&mut cur).unwrap(); + assert_eq!(parsed.version, h.version); assert_eq!(parsed.alg, h.alg); assert_eq!(parsed.flags, h.flags); assert_eq!(parsed.chunk_size, h.chunk_size); @@ -249,6 +256,7 @@ mod tests { #[test] fn roundtrip_length_committed() { let h = Header { + version: VERSION_CURRENT, alg: AlgId::XChaCha20Poly1305, flags: FLAG_LENGTH_COMMITTED, chunk_size: 65536, @@ -267,6 +275,7 @@ mod tests { #[test] fn rejects_bad_magic() { let mut bytes = Header { + version: VERSION_CURRENT, alg: AlgId::XChaCha20Poly1305, flags: 0, chunk_size: 4096, @@ -285,6 +294,7 @@ mod tests { #[test] fn rejects_unknown_flag_bits() { let mut bytes = Header { + version: VERSION_CURRENT, alg: AlgId::XChaCha20Poly1305, flags: 0, chunk_size: 4096, @@ -314,8 +324,12 @@ mod tests { bytes.push(0); // kdf id raw bytes.extend_from_slice(&[3u8; NONCE_PREFIX_LEN]); let parsed = Header::read(&mut Cursor::new(&bytes)).unwrap(); + assert_eq!(parsed.version, 1); assert_eq!(parsed.flags, 0); assert_eq!(parsed.chunk_size, 1024); assert_eq!(parsed.plaintext_length, None); + // Re-encoding must reproduce the original v1 bytes exactly so the + // recomputed AAD matches what the file was authenticated with. + assert_eq!(parsed.encode(), bytes); } }