fix(header): preserve on-disk version through decode/encode

`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.
This commit is contained in:
2026-05-02 21:28:26 +02:00
parent 75afadb1ec
commit 53bb927a87
2 changed files with 140 additions and 3 deletions
+124 -1
View File
@@ -6,7 +6,9 @@ use std::io::{BufReader, Read, Seek, SeekFrom, Write};
use std::sync::Arc; use std::sync::Arc;
use crate::error::*; 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::pipeline;
use crate::reader::{AheadReader, ReadInfoChunk}; use crate::reader::{AheadReader, ReadInfoChunk};
use crate::secrets::{SecretBytes32, SecretVec}; use crate::secrets::{SecretBytes32, SecretVec};
@@ -96,6 +98,7 @@ pub fn encrypt<S: AsRef<str>>(
0 0
}; };
let header = Header { let header = Header {
version: VERSION_CURRENT,
alg: AlgId::XChaCha20Poly1305, alg: AlgId::XChaCha20Poly1305,
flags, flags,
chunk_size, chunk_size,
@@ -347,3 +350,123 @@ pub fn decrypt_range<S: AsRef<str>>(
out.commit()?; out.commit()?;
Ok(()) 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<u8> = (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<u8> = (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);
}
}
+16 -2
View File
@@ -34,7 +34,7 @@ use std::io::Read;
use crate::error::FcryError; use crate::error::FcryError;
const MAGIC: [u8; 4] = *b"fcry"; const MAGIC: [u8; 4] = *b"fcry";
const VERSION_CURRENT: u8 = 2; pub const VERSION_CURRENT: u8 = 2;
const VERSION_MIN: u8 = 1; const VERSION_MIN: u8 = 1;
pub const NONCE_PREFIX_LEN: usize = 19; pub const NONCE_PREFIX_LEN: usize = 19;
@@ -131,6 +131,10 @@ impl KdfParams {
#[derive(Clone, Debug)] #[derive(Clone, Debug)]
pub struct Header { 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 alg: AlgId,
pub flags: u8, pub flags: u8,
pub chunk_size: u32, pub chunk_size: u32,
@@ -144,7 +148,7 @@ impl Header {
pub fn encode(&self) -> Vec<u8> { pub fn encode(&self) -> Vec<u8> {
let mut out = Vec::with_capacity(72); let mut out = Vec::with_capacity(72);
out.extend_from_slice(&MAGIC); out.extend_from_slice(&MAGIC);
out.push(VERSION_CURRENT); out.push(self.version);
out.push(self.alg as u8); out.push(self.alg as u8);
out.push(self.flags); out.push(self.flags);
out.push(0); // reserved out.push(0); // reserved
@@ -210,6 +214,7 @@ impl Header {
}; };
Ok(Self { Ok(Self {
version,
alg, alg,
flags, flags,
chunk_size, chunk_size,
@@ -228,6 +233,7 @@ mod tests {
#[test] #[test]
fn roundtrip() { fn roundtrip() {
let h = Header { let h = Header {
version: VERSION_CURRENT,
alg: AlgId::XChaCha20Poly1305, alg: AlgId::XChaCha20Poly1305,
flags: 0, flags: 0,
chunk_size: 1024 * 1024, chunk_size: 1024 * 1024,
@@ -238,6 +244,7 @@ mod tests {
let bytes = h.encode(); let bytes = h.encode();
let mut cur = Cursor::new(&bytes); let mut cur = Cursor::new(&bytes);
let parsed = Header::read(&mut cur).unwrap(); let parsed = Header::read(&mut cur).unwrap();
assert_eq!(parsed.version, h.version);
assert_eq!(parsed.alg, h.alg); assert_eq!(parsed.alg, h.alg);
assert_eq!(parsed.flags, h.flags); assert_eq!(parsed.flags, h.flags);
assert_eq!(parsed.chunk_size, h.chunk_size); assert_eq!(parsed.chunk_size, h.chunk_size);
@@ -249,6 +256,7 @@ mod tests {
#[test] #[test]
fn roundtrip_length_committed() { fn roundtrip_length_committed() {
let h = Header { let h = Header {
version: VERSION_CURRENT,
alg: AlgId::XChaCha20Poly1305, alg: AlgId::XChaCha20Poly1305,
flags: FLAG_LENGTH_COMMITTED, flags: FLAG_LENGTH_COMMITTED,
chunk_size: 65536, chunk_size: 65536,
@@ -267,6 +275,7 @@ mod tests {
#[test] #[test]
fn rejects_bad_magic() { fn rejects_bad_magic() {
let mut bytes = Header { let mut bytes = Header {
version: VERSION_CURRENT,
alg: AlgId::XChaCha20Poly1305, alg: AlgId::XChaCha20Poly1305,
flags: 0, flags: 0,
chunk_size: 4096, chunk_size: 4096,
@@ -285,6 +294,7 @@ mod tests {
#[test] #[test]
fn rejects_unknown_flag_bits() { fn rejects_unknown_flag_bits() {
let mut bytes = Header { let mut bytes = Header {
version: VERSION_CURRENT,
alg: AlgId::XChaCha20Poly1305, alg: AlgId::XChaCha20Poly1305,
flags: 0, flags: 0,
chunk_size: 4096, chunk_size: 4096,
@@ -314,8 +324,12 @@ mod tests {
bytes.push(0); // kdf id raw bytes.push(0); // kdf id raw
bytes.extend_from_slice(&[3u8; NONCE_PREFIX_LEN]); bytes.extend_from_slice(&[3u8; NONCE_PREFIX_LEN]);
let parsed = Header::read(&mut Cursor::new(&bytes)).unwrap(); let parsed = Header::read(&mut Cursor::new(&bytes)).unwrap();
assert_eq!(parsed.version, 1);
assert_eq!(parsed.flags, 0); assert_eq!(parsed.flags, 0);
assert_eq!(parsed.chunk_size, 1024); assert_eq!(parsed.chunk_size, 1024);
assert_eq!(parsed.plaintext_length, None); 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);
} }
} }