From 91d49aeecf828d9417ecf46b4baecf87b5a9d770 Mon Sep 17 00:00:00 2001 From: Andy Dixon Date: Thu, 8 Oct 2026 12:23:27 +0100 Subject: [PATCH] Fix review findings: PAX sizes, agent stdin, PEM parity, sticky writer - absorb uses the PAX-corrected entry size (large Go/bsdtar archives were stored truncated) and refuses GNU sparse entries - the agent reads the key from fd 0 straight into locked memory, and never unlinks a live agent's socket - the key file PEM parser is a port of Go's pem.Decode - the STREAM writer fails permanently after a write error; plaintext is cleared on reader error paths; AEAD, HMAC and hash states are zeroized - add/migrate refuse paths inside the vault; publish reports filesystems without an atomic no-replace primitive clearly and treats a leftover temporary name as a warning; Windows rejects NTFS-special names - the leak test runs in its own process Co-Authored-By: Claude --- Cargo.lock | 4 ++ Cargo.toml | 8 +-- SECURITY.md | 20 ++++-- docs/MIGRATION.md | 8 +++ src/absorb.rs | 60 ++++++++++++++++-- src/agent.rs | 25 +++++++- src/cli.rs | 10 ++- src/crypto/stream.rs | 71 +++++++++++++++++++-- src/keyfile.rs | 148 ++++++++++++++++++++++++++++++++----------- src/platform/fs.rs | 53 ++++++++++++++-- src/store/tests.rs | 11 ++++ tests/cli.rs | 9 +++ 12 files changed, 358 insertions(+), 69 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 131a37a..96e14ff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -88,6 +88,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d2f6c7dbe95a6ed67ad9f18e57daf93a2f034c524b99fd2b76d18fdfeb6660aa" dependencies = [ "hybrid-array", + "zeroize", ] [[package]] @@ -121,6 +122,7 @@ dependencies = [ "cfg-if", "cipher", "cpufeatures", + "zeroize", ] [[package]] @@ -133,6 +135,7 @@ dependencies = [ "chacha20", "cipher", "poly1305", + "zeroize", ] [[package]] @@ -277,6 +280,7 @@ dependencies = [ "block-buffer", "crypto-common", "ctutils", + "zeroize", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 5a47ebf..0fe0956 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -11,11 +11,11 @@ publish = false [dependencies] argon2 = { version = "0.6", default-features = false, features = ["alloc", "zeroize"] } -chacha20poly1305 = { version = "0.11", default-features = false, features = ["alloc"] } +chacha20poly1305 = { version = "0.11", default-features = false, features = ["alloc", "zeroize"] } hkdf = "0.13" -hmac = "0.13" -sha2 = { version = "0.11", default-features = false } -sha3 = { version = "0.11", default-features = false } +hmac = { version = "0.13", features = ["zeroize"] } +sha2 = { version = "0.11", default-features = false, features = ["zeroize"] } +sha3 = { version = "0.11", default-features = false, features = ["zeroize"] } ml-kem = { version = "0.3.2", features = ["zeroize", "hazmat"] } zeroize = { version = "1.9", features = ["derive", "serde"] } getrandom = "0.4" diff --git a/SECURITY.md b/SECURITY.md index 4153f20..9afe0ad 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -63,7 +63,8 @@ None of this is erasure from physical memory. `VAULT_PASSPHRASE` and `VAULT_DECOY_PASSPHRASE` removed from its environment, no secret in its arguments, its working directory `/` and standard error closed. It hands over the 34-byte suite ID and seed through a pipe. Before -reading it, the agent: +reading it (straight from the pipe into the locked page, with no +intermediate buffer), the agent: 1. sets the core-dump size limit to zero; 2. on Linux, sets `PR_SET_DUMPABLE` to 0 (no core dumps, no same-user @@ -108,16 +109,25 @@ for the passphrase. A failure after a record is already in place never deletes its objects. Windows has no directory fsync through the APIs used; NTFS journals metadata. +- `migrate` checks an original is still the file it stored (device, inode, + size, time; on Windows only size and time) and then deletes it by name; a + replacement in the instant between the check and the deletion is not + excluded. Paths inside the vault directory are refused outright. +- A filesystem with neither `RENAME_NOREPLACE` nor hard links (some FUSE + mounts, FAT) cannot publish without risking a replace, so `get` fails + there with a message saying so rather than fall back. - The vault lock uses the same POSIX `fcntl` locks as the Go version, so mixed Go and Rust processes exclude each other. Only a read-only filesystem lets a reader proceed without the lock. ## Plaintext on disk -- `vault add URL` and `vault mirror` stream non-HTML downloads to a private - (0600) temporary file in `$TMPDIR` before storing them, because the size - must be known first; it is removed on every normal path, including - errors. HTML and CSS are rewritten in memory. `vault absorb` stores tar +- `vault add URL` and `vault mirror` stream non-HTML downloads to a 0600 + temporary file in `$TMPDIR` before storing them, because the size must be + known first. On Unix the file is unlinked as soon as it is created, so it + has no name and disappears even if vault is killed; on other systems it + is a named file removed on every normal path, including errors. HTML and + CSS are rewritten in memory. `vault absorb` stores tar entries straight from the stream with no temporary file. - `vault get` writes plaintext where you ask; `vault migrate` deletes the originals with `unlink`, which is not secure deletion. diff --git a/docs/MIGRATION.md b/docs/MIGRATION.md index f547fa2..70c1ed5 100644 --- a/docs/MIGRATION.md +++ b/docs/MIGRATION.md @@ -23,6 +23,14 @@ Safety and correctness: use, see `SECURITY.md`.) - **Extracted files are synced** to disk before they appear under their name, and their directory afterwards. +- **`get` needs an atomic no-replace primitive.** On a filesystem with + neither `RENAME_NOREPLACE` nor hard links (some FUSE mounts, FAT) it fails + with a message saying so, where Go would rename. +- **`add`/`migrate` refuse paths inside the vault directory** (Go refused + only the directory itself; `migrate store/records` would have deleted the + vault's records). +- **`absorb` refuses GNU sparse files** (Go expanded them), and honours PAX + size records as Go does. - **A source file that changes while being added is not stored.** If it grows, shrinks, is modified or replaced during `add`/`migrate`, the command fails and nothing is stored. Go stored whatever it read (for a diff --git a/src/absorb.rs b/src/absorb.rs index 868d6e2..5b07c65 100644 --- a/src/absorb.rs +++ b/src/absorb.rs @@ -52,10 +52,14 @@ pub fn absorb( }; let name = join(&[dest, &rel]); let name = VaultName::new(&name).ok_or_else(|| Error::new(format!("invalid vault path {name:?}")))?; - let size = entry - .header() - .size() - .map_err(|e| Error::new(format!("reading archive: {}", crate::util::io_reason(&e))))?; + // The entry's size, with a PAX "size" override applied, as Go's + // hdr.Size; the fixed header field alone would truncate files that + // Go's archive/tar writes for sizes past 8 GiB. + let size = entry.size(); + if is_gnu_sparse(&mut entry) { + eprintln!("skipping {shown}: sparse files are not supported"); + continue; + } let mode = entry.header().mode().unwrap_or(0) & 0o777; let mtime = mtime(&mut entry)?; match store.add_reader(cat, &mut entry, size, mode, mtime, &name) { @@ -84,6 +88,18 @@ pub fn safe_name(name: &str) -> Option { Some(n) } +/// A PAX-format GNU sparse file, whose data is an encoded map, not the +/// file's bytes. +fn is_gnu_sparse(entry: &mut tar::Entry) -> bool { + if entry.header().entry_type() == tar::EntryType::GNUSparse { + return true; + } + match entry.pax_extensions() { + Ok(Some(exts)) => exts.flatten().any(|e| e.key_bytes().starts_with(b"GNU.sparse.")), + _ => false, + } +} + /// The entry's modification time: the PAX `mtime` (with its fraction) if /// present, else the header's whole seconds. fn mtime(entry: &mut tar::Entry) -> Result { @@ -236,6 +252,42 @@ mod tests { assert_eq!(s.catalogue().unwrap().len(), before); } + /// A PAX "size" record overrides the header's size field, as Go writes + /// for files over 8 GiB; the whole body must be stored. + #[test] + fn pax_size_override() { + let (d, s) = store("absorb-pax"); + let mut t = Vec::new(); + let rec = |k: &str, v: &str| { + let body = format!(" {k}={v}\n"); + let mut n = body.len() + 1; + while format!("{n}{body}").len() != n { + n += 1; + } + format!("{n}{body}") + }; + let pax = rec("size", "11"); + let mut h = header("PaxHeaders/big", tar::EntryType::XHeader, pax.len() as u64, 0o644, 0); + h.set_cksum(); + t.extend_from_slice(h.as_bytes()); + let mut p = pax.into_bytes(); + p.resize(p.len().div_ceil(512) * 512, 0); + t.extend_from_slice(&p); + let h = header("big.bin", tar::EntryType::Regular, 0, 0o644, 0); + t.extend_from_slice(h.as_bytes()); + let mut body = b"hello world".to_vec(); + body.resize(512, 0); + t.extend_from_slice(&body); + t.extend_from_slice(&[0u8; 1024]); + let mut cat = s.catalogue().unwrap(); + absorb(&s, &mut cat, &t[..], "", &mut |_, _| {}).unwrap(); + let r = cat.current("big.bin").unwrap(); + assert_eq!(r.size, 11); + let dir = crate::platform::fs::Dir::open(&d.0).unwrap(); + s.extract(r, &dir, std::ffi::OsStr::new("o")).unwrap(); + assert_eq!(std::fs::read(d.0.join("o")).unwrap(), b"hello world"); + } + #[test] fn names_and_times() { // As in Go, only names that would climb out are refused; others are diff --git a/src/agent.rs b/src/agent.rs index 35df901..df32ce6 100644 --- a/src/agent.rs +++ b/src/agent.rs @@ -226,6 +226,19 @@ mod imp { } } + fn read_fd0(buf: &mut [u8]) -> std::io::Result<()> { + let mut done = 0; + while done < buf.len() { + match rustix::io::read(rustix::stdio::stdin(), &mut buf[done..]) { + Ok(0) => return Err(std::io::ErrorKind::UnexpectedEof.into()), + Ok(n) => done += n, + Err(rustix::io::Errno::INTR) => {} + Err(e) => return Err(e.into()), + } + } + Ok(()) + } + /// The agent process. Returns when it expires or is told to quit. pub fn run(sock: &Path, ttl: &str, mode: &str) { let mut out = std::io::stdout(); @@ -249,7 +262,9 @@ mod imp { Ok(b) => b, Err(e) => return report(&mut out, &e.to_string()), }; - if std::io::stdin().lock().read_exact(secret.as_mut_slice()).is_err() { + // Read straight from fd 0 into the locked page: std's buffered + // stdin would leave a copy in ordinary heap memory. + if read_fd0(secret.as_mut_slice()).is_err() { return report(&mut out, "no key received"); } { @@ -258,8 +273,12 @@ mod imp { return report(&mut out, &e); } } - // A socket left by an agent that died stops bind; start has already - // stopped any live one. + // A socket left by an agent that died stops bind. Remove it only if + // nothing answers on it, so a concurrent unlock's live agent is not + // cut off from `vault lock`. + if UnixStream::connect(sock).is_ok() { + return report(&mut out, "another agent is already running for this key"); + } let _ = std::fs::remove_file(sock); let l = match UnixListener::bind(sock) { Ok(l) => l, diff --git a/src/cli.rs b/src/cli.rs index 02af8ba..568089d 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -477,8 +477,14 @@ fn run(cmd: &str, args: &[String]) -> Result { } let (s, _lock) = ctx.open(true)?; let mut cat = s.catalogue()?; - // Never ingest, or delete, the vault or its key. - let skip: Vec = [ctx.vault_dir()?, ctx.key()?].iter().filter_map(|p| FileId::of(p, true)).collect(); + // Never ingest, or delete, the vault, anything inside it, or its key. + let vault_dir = ctx.vault_dir()?; + if let (Ok(v), Ok(src)) = (std::fs::canonicalize(&vault_dir), std::fs::canonicalize(&pos[0])) { + if src != v && src.starts_with(&v) { + crate::bail!("{} is inside the vault", pos[0]); + } + } + let skip: Vec = [vault_dir, ctx.key()?].iter().filter_map(|p| FileId::of(p, true)).collect(); let yes = ctx.flags.yes; add_path(&s, &mut cat, Path::new(&pos[0]), &dest, migrate, &skip, &mut |names| { yes || confirm_overwrite(names) diff --git a/src/crypto/stream.rs b/src/crypto/stream.rs index e3d434e..4f9f88d 100644 --- a/src/crypto/stream.rs +++ b/src/crypto/stream.rs @@ -122,6 +122,8 @@ pub struct StreamWriter { dst: Option, buf: Vec, index: u64, + /// Set by any failure: like Go's sticky error, every later call fails. + failed: bool, } impl StreamWriter { @@ -131,6 +133,7 @@ impl StreamWriter { dst: Some(dst), buf: Vec::with_capacity(ENC_CHUNK_SIZE), index: 0, + failed: false, } } @@ -141,7 +144,24 @@ impl StreamWriter { w } + fn check(&self) -> io::Result<()> { + if self.failed { + Err(io::Error::other("encrypted stream writer already failed")) + } else { + Ok(()) + } + } + fn flush_chunk(&mut self, last: bool) -> io::Result<()> { + let r = self.seal_and_write(last); + if r.is_err() { + self.failed = true; + self.buf.zeroize(); + } + r + } + + fn seal_and_write(&mut self, last: bool) -> io::Result<()> { let n = self.buf.len(); self.buf.resize(n + TAG_SIZE, 0); self.cipher.seal(self.index, last, &mut self.buf); @@ -152,6 +172,7 @@ impl StreamWriter { } pub fn finish(mut self) -> io::Result { + self.check()?; self.flush_chunk(true)?; Ok(self.dst.take().expect("writer in use")) } @@ -159,6 +180,7 @@ impl StreamWriter { impl Write for StreamWriter { fn write(&mut self, mut p: &[u8]) -> io::Result { + self.check()?; let total = p.len(); while !p.is_empty() { let take = p.len().min(CHUNK_SIZE - self.buf.len()); @@ -174,6 +196,7 @@ impl Write for StreamWriter { } fn flush(&mut self) -> io::Result<()> { + self.check()?; self.dst.as_mut().expect("writer in use").flush() } } @@ -287,8 +310,11 @@ impl StreamReader { } if raw.len() < ENC_CHUNK_SIZE { // A short first chunk is final; empty is the empty file. - let p = self.open(raw, true)?; - expect_eof(&mut self.src)?; + let mut p = self.open(raw, true)?; + if let Err(e) = expect_eof(&mut self.src) { + p.zeroize(); + return Err(e); + } self.deliver(p); self.done = true; return Ok(()); @@ -307,9 +333,19 @@ impl StreamReader { if next.len() == TAG_SIZE { return Err(StreamError::EmptyLastChunk.into()); } - let p = self.open(pending, false)?; - let q = self.open(next, true)?; - expect_eof(&mut self.src)?; + let mut p = self.open(pending, false)?; + let mut q = match self.open(next, true) { + Ok(q) => q, + Err(e) => { + p.zeroize(); + return Err(e); + } + }; + if let Err(e) = expect_eof(&mut self.src) { + p.zeroize(); + q.zeroize(); + return Err(e); + } self.deliver(p); self.queued = Some(q); self.done = true; @@ -514,7 +550,8 @@ impl<'a, S: ReadAt + ?Sized> StreamReaderAt<'a, S> { )); } let mut n = 0; - let mut chunk = vec![0u8; ENC_CHUNK_SIZE]; + // Cleared on every exit, early returns included. + let mut chunk = zeroize::Zeroizing::new(vec![0u8; ENC_CHUNK_SIZE]); let mut cache = self.cache.lock().unwrap_or_else(|e| e.into_inner()); while n < p.len() && off < self.size { let idx = off / CHUNK_SIZE as u64; @@ -538,7 +575,7 @@ impl<'a, S: ReadAt + ?Sized> StreamReaderAt<'a, S> { n += take; off += take as u64; } - chunk.zeroize(); + Ok(n) } } @@ -750,6 +787,26 @@ pub(crate) mod tests { assert!(encrypted_chunk_count(ENC_CHUNK_SIZE as u64 + 16).is_err()); } + /// After a failed write the writer refuses everything, as Go's does, + /// instead of growing past a chunk. + #[test] + fn writer_failure_is_sticky() { + struct Full; + impl Write for Full { + fn write(&mut self, _: &[u8]) -> io::Result { + Err(io::Error::other("disk full")) + } + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } + } + let mut w = StreamWriter::new(&[1u8; 32], Full); + assert!(w.write_all(&pt(CHUNK_SIZE + 1)).is_err()); + assert!(w.write_all(&pt(10)).is_err()); + assert!(w.flush().is_err()); + assert!(w.finish().is_err()); + } + #[test] fn counter_overflow_is_refused() { let mut w = StreamWriter::starting_at(&[1u8; 32], Vec::new(), u64::MAX); diff --git a/src/keyfile.rs b/src/keyfile.rs index badf294..0e31af9 100644 --- a/src/keyfile.rs +++ b/src/keyfile.rs @@ -124,49 +124,102 @@ pub fn encode_pem(body: &[u8]) -> String { } /// The block body if `data` is one `VAULT KEY` PEM block, optionally with -/// text before it (as Go's `pem.Decode` allows) and only whitespace after. +/// text before it and only whitespace after: a port of Go's `pem.Decode` +/// followed by vault's checks on its result. pub fn decode_pem(data: &[u8]) -> Option> { - let begin = format!("-----BEGIN {PEM_TYPE}-----"); - let end = format!("-----END {PEM_TYPE}-----"); - let start = if data.starts_with(b"-----BEGIN ") { - 0 - } else { - find(data, b"\n-----BEGIN ")? + 1 - }; - let mut lines = data[start..].split_inclusive(|&c| c == b'\n'); - let first = lines.next()?; - if trim_right(first) != begin.as_bytes() { + let (typ, body, rest) = go_pem_decode(data)?; + if typ != PEM_TYPE.as_bytes() || !go_trim_space(rest).is_empty() { return None; } - let mut b64 = Vec::new(); - let mut after = None; - let mut consumed = start + first.len(); - for line in lines.by_ref() { - consumed += line.len(); - let t = trim_right(line); - if t == end.as_bytes() { - after = Some(consumed); - break; - } - if t.contains(&b':') { - // A PEM header line; Go accepts and ignores them. - continue; - } - b64.extend(t.iter().filter(|c| !matches!(c, b' ' | b'\t'))); - } - let after = after?; - if !data[after..].iter().all(|c| c.is_ascii_whitespace()) { - return None; - } - base64::engine::general_purpose::STANDARD.decode(&b64).ok() + Some(body) } -fn trim_right(l: &[u8]) -> &[u8] { - let mut e = l.len(); - while e > 0 && matches!(l[e - 1], b' ' | b'\t' | b'\r' | b'\n') { - e -= 1; +/// Go's `getLine`: up to the next LF (dropping one CR before it), with +/// trailing spaces and tabs removed, and the rest after the LF. +fn get_line(data: &[u8]) -> (&[u8], &[u8]) { + let (mut i, j) = match data.iter().position(|&c| c == b'\n') { + Some(i) => (i, i + 1), + None => (data.len(), data.len()), + }; + if j > i && i > 0 && data[i - 1] == b'\r' { + i -= 1; + } + let mut line = &data[..i]; + while let [rest @ .., b' ' | b'\t'] = line { + line = rest; + } + (line, &data[j..]) +} + +/// Go's `bytes.TrimSpace` (Unicode white space in UTF-8 text). +fn go_trim_space(b: &[u8]) -> &[u8] { + match std::str::from_utf8(b) { + Ok(s) => s.trim_matches(char::is_whitespace).as_bytes(), + Err(_) => b.trim_ascii(), + } +} + +/// Go's `encoding/pem.Decode`: the first well-formed block's type, bytes +/// and the data after it. Malformed candidates are skipped, as in Go. +fn go_pem_decode(data: &[u8]) -> Option<(Vec, Vec, &[u8])> { + const START: &[u8] = b"\n-----BEGIN "; + const END: &[u8] = b"\n-----END "; + const EOL: &[u8] = b"-----"; + let mut rest = data; + loop { + if rest.starts_with(&START[1..]) { + rest = &rest[START.len() - 1..]; + } else if let Some(i) = find(rest, START) { + rest = &rest[i + START.len()..]; + } else { + return None; + } + let (type_line, r) = get_line(rest); + rest = r; + let Some(typ) = type_line.strip_suffix(EOL) else { continue }; + let mut headers = 0; + loop { + if rest.is_empty() { + return None; + } + let (line, next) = get_line(rest); + if !line.contains(&b':') { + break; + } + headers += 1; + rest = next; + } + let (end_index, trailer_index) = if headers == 0 && rest.starts_with(&END[1..]) { + (0, END.len() - 1) + } else { + match find(rest, END) { + Some(i) => (i, i + END.len()), + None => continue, + } + }; + let trailer = &rest[trailer_index..]; + let tlen = typ.len() + EOL.len(); + if trailer.len() < tlen { + continue; + } + let (t, rest_of_end) = trailer.split_at(tlen); + if !t.starts_with(typ) || !t.ends_with(EOL) || !get_line(rest_of_end).0.is_empty() { + continue; + } + // Go's StdEncoding skips CR and LF; removeSpacesAndTabs drops the rest. + let b64: Vec = rest[..end_index] + .iter() + .copied() + .filter(|c| !matches!(c, b' ' | b'\t' | b'\r' | b'\n')) + .collect(); + let lenient = base64::engine::GeneralPurpose::new( + &base64::alphabet::STANDARD, + base64::engine::GeneralPurposeConfig::new().with_decode_allow_trailing_bits(true), + ); + let Ok(bytes) = lenient.decode(&b64) else { continue }; + let after = get_line(&rest[end_index + END.len() - 1..]).1; + return Some((typ.to_vec(), bytes, after)); } - &l[..e] } fn find(h: &[u8], n: &[u8]) -> Option { @@ -327,6 +380,27 @@ mod tests { assert!(decode_pem(format!("{pem}x").as_bytes()).is_none()); assert!(decode_pem(pem.replace("VAULT KEY", "VAULT KEX").as_bytes()).is_none()); assert!(decode_pem(pem.replace("-----END", "-----FIN").as_bytes()).is_none()); + // Cases where an earlier, simpler parser differed from Go's pem.Decode. + // (In the second body line: a colon in the first makes it a header.) + let first = pem.find('\n').unwrap(); + let mid = first + 1 + pem[first + 1..].find('\n').unwrap() + 30; + assert!( + decode_pem(format!("{}:{}", &pem[..mid], &pem[mid..]).as_bytes()).is_none(), + "colon inside the body" + ); + assert!( + decode_pem(format!("{pem}\u{b}\u{a0}").as_bytes()).is_some(), + "Unicode space after the block" + ); + assert!(decode_pem(pem.replacen("\n", "\r\n", 3).as_bytes()).is_some(), "CRLF lines"); + assert!( + decode_pem(format!("-----BEGIN X\n{pem}").as_bytes()).is_some(), + "a bad BEGIN line before the block" + ); + assert!( + decode_pem(format!("-----BEGIN VAULT KEY-----\nProc-Type: 4,x\n\n{}", &pem[26..]).as_bytes()).is_some(), + "headers" + ); let mut short = body.clone(); short.pop(); assert_eq!(parse_body(&short).err(), Some(Error::NotAKeyFile)); diff --git a/src/platform/fs.rs b/src/platform/fs.rs index 5bac771..80ddd0b 100644 --- a/src/platform/fs.rs +++ b/src/platform/fs.rs @@ -165,10 +165,33 @@ fn publish_inner(tmp: &Path, dst: &Path) -> io::Result<()> { /// atomic no-clobber publish wherever hard links work. #[cfg(not(windows))] fn link_publish(tmp: &Path, dst: &Path) -> io::Result<()> { - std::fs::hard_link(tmp, dst)?; - // The data is safely at dst; a failure to drop the temporary name is - // reported but dst stays. - std::fs::remove_file(tmp) + std::fs::hard_link(tmp, dst).map_err(no_atomic)?; + // The data is safely at dst; failing to drop the temporary name is only + // worth a warning. + if let Err(e) = std::fs::remove_file(tmp) { + eprintln!( + "vault: could not remove temporary file {}: {}", + tmp.display(), + crate::util::io_reason(&e) + ); + } + Ok(()) +} + +/// A filesystem with neither RENAME_NOREPLACE nor hard links cannot publish +/// without risking a replace; say so rather than fall back. +#[cfg(not(windows))] +fn no_atomic(e: io::Error) -> io::Error { + if e.kind() == io::ErrorKind::AlreadyExists { + return e; + } + io::Error::new( + e.kind(), + format!( + "this filesystem offers no atomic no-replace rename or hard link ({})", + crate::util::io_reason(&e) + ), + ) } #[cfg(windows)] @@ -325,9 +348,11 @@ impl Dir { match linkat(&self.fd, tmp, &self.fd, name, AtFlags::empty()) { Ok(()) => {} Err(rustix::io::Errno::EXIST) => return Err(exists_error(&dst)), - Err(e) => return Err(e.into()), + Err(e) => return Err(no_atomic(e.into())), + } + if let Err(e) = unlinkat(&self.fd, tmp, AtFlags::empty()) { + eprintln!("vault: could not remove temporary file {}: {e}", self.path.join(tmp).display()); } - unlinkat(&self.fd, tmp, AtFlags::empty())?; Ok(()) } #[cfg(not(unix))] @@ -380,13 +405,27 @@ fn is_reparse(_: &std::fs::Metadata) -> bool { false } +/// On Windows, also refuse names NTFS would treat specially: alternate +/// data streams (`a:b`), reserved device names, trailing dots or spaces. +fn windows_safe(name: &OsStr) -> bool { + if !cfg!(windows) { + return true; + } + let Some(n) = name.to_str() else { return false }; + let stem = n.split('.').next().unwrap_or("").trim_end().to_ascii_uppercase(); + let reserved = matches!(stem.as_str(), "CON" | "PRN" | "AUX" | "NUL" | "CONIN$" | "CONOUT$") + || ((stem.starts_with("COM") || stem.starts_with("LPT")) && stem.len() == 4 && stem.as_bytes()[3].is_ascii_digit()); + !n.contains([':', '<', '>', '"', '|', '?', '*']) && !n.ends_with(['.', ' ']) && !reserved +} + /// A single, plain path component: no separators, not "." or "..". fn check_component(name: &OsStr) -> io::Result<()> { let ok = !name.is_empty() && name != "." && name != ".." && Path::new(name).components().count() == 1 - && matches!(Path::new(name).components().next(), Some(std::path::Component::Normal(_))); + && matches!(Path::new(name).components().next(), Some(std::path::Component::Normal(_))) + && windows_safe(name); if ok { Ok(()) } else { diff --git a/src/store/tests.rs b/src/store/tests.rs index 60705c8..f358ca2 100644 --- a/src/store/tests.rs +++ b/src/store/tests.rs @@ -458,6 +458,17 @@ fn duplicate_records_recover() { #[cfg(target_os = "linux")] #[test] fn repeated_failures_do_not_leak() { + // Thread counts are per process, and other tests run in parallel: run + // the real check alone in a child copy of this test binary. + if std::env::var_os("VAULT_LEAK_CHILD").is_none() { + let o = std::process::Command::new(std::env::current_exe().unwrap()) + .args(["--exact", "store::tests::repeated_failures_do_not_leak", "--test-threads=1"]) + .env("VAULT_LEAK_CHILD", "1") + .output() + .unwrap(); + assert!(o.status.success(), "{}", String::from_utf8_lossy(&o.stdout)); + return; + } let threads = || std::fs::read_dir("/proc/self/task").unwrap().count(); let (d, s) = vault("leak"); let mut cat = s.catalogue().unwrap(); diff --git a/tests/cli.rs b/tests/cli.rs index 9dcbf6c..4cd69a6 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -256,6 +256,15 @@ fn migrate_deletes_only_stored_originals() { // Never the vault or the key. let o = e.run(RUST, Some(REAL), &["add", "store"]); assert!(String::from_utf8_lossy(&o.stderr).contains("is the vault or its key")); + // Nor anything inside the vault: migrating its records would delete them. + let before = e.names(RUST, REAL); + let o = e.run(RUST, Some(REAL), &["migrate", "--yes", "store/records"]); + assert!( + String::from_utf8_lossy(&o.stderr).contains("is inside the vault"), + "{}", + String::from_utf8_lossy(&o.stderr) + ); + assert_eq!(e.names(RUST, REAL), before); } #[cfg(unix)]