ddidderr/fcry
1
0
Fork 0
Code Issues 2 Pull Requests Projects Releases Wiki Activity

refactor(secrets): back SecretBytes32/SecretVec with the secrets crate

Browse Source 
Replace the homegrown `region::lock` + `Zeroizing` wrappers with thin
adapters over `secrets::SecretBox` and `secrets::SecretVec`. The
upstream crate already provides everything the local types were doing
by hand (mlock, zero-on-drop) and adds protections we didn't have:
guard pages around the allocation and `mprotect`-based access control
(`PROT_NONE` at rest, `PROT_READ` during a borrow, `PROT_READ|WRITE`
during a mut borrow). Net result is a security upgrade, not just a
dependency swap.

Why a local adapter still exists
--------------------------------
`secrets::SecretVec` is fixed-length — no `push`. The tty passphrase
reader needs to append bytes one at a time without ever reallocating
(a panicking partial read must not leave stale plaintext on the heap),
so `SecretVec` keeps a separate logical `len` over a fixed protected
allocation of `MAX_PASSPHRASE_LEN` bytes. Bytes past `len` stay
zero-padding and are never exposed through `with_slice`.

API shape: closure-scoped borrows
---------------------------------
The previous `as_slice` / `as_array` returned long-lived `&[u8]`
references, which would have kept the upstream pages in `PROT_READ`
for the full lifetime of the borrow. The new API uses
`with_array(|s| ...)`, `with_mut_array(|s| ...)`, `with_slice(|s| ...)`
so the unprotected window is exactly the closure body. This is uglier
at call sites (notably the nested closures in `derive_key`) but it's
the right tradeoff — minimizing the unprotected window is the whole
point of using the crate.

AEAD key copy footnote
----------------------
`XChaCha20Poly1305::new` copies the key into its own (unprotected)
state, which then lives in the `aead` binding for the entire
encrypt/decrypt loop. This is unchanged from before — the cipher
state was never protected — but it's now called out explicitly with
a comment at both call sites noting that `chacha20poly1305` zeroizes
that internal copy on drop. Future readers shouldn't have to
rediscover this by reading upstream source.

`from_vec` zeroing
------------------
`SecretVec::from_vec(v: Vec<u8>)` is used on the env-var path. It
calls `secrets::SecretVec::from(&mut [u8])`, which (verified against
secrets-1.3.0: `Box::from` -> `transfer` -> `memtransfer`) copies the
bytes into protected storage and zeroes the source slice. The
original Vec's allocation is then released through the normal
allocator — the bytes inside it are zero, but the heap block itself
isn't specially handled. The doc comment on `from_vec` reflects this
precisely. As before, the env-var path also leaves a copy in the
process `environ` table, which is a known accepted leak.

Cargo.toml
----------
Use `protected-secrets = { package = "secrets", version = "1.3" }`
with default features. The `secrets` crate has no pure-Rust backend
at v1.3 — disabling default features only switches *how* libsodium
is linked (bundled `libsodium-sys` vs. the crate's own bindings to a
system libsodium), and can break builds where the chosen path isn't
set up. Defaults are correct here. The `region` dependency is
dropped.

Test plan
---------
- `cargo build` clean.
- `cargo test` — 2 unit + 18 integration tests pass, including
  `roundtrip_passphrase_argon2id` which exercises the full
  passphrase -> argon2id -> AEAD key path through the new wrappers.
- `cargo clippy` (and `--tests`, `--benches`) clean.
- `cargo +nightly fmt` applied.
This commit is contained in:
ddidderr
2026-05-02 19:14:42 +02:00
parent fe65e1f899
commit 898697016a
5 changed files with 142 additions and 133 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Cargo.lock
Generated
+58 -38
View File
@@ -95,12 +95,6 @@ version = "1.8.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "2af50177e190e07a26ab74f8b1efbfe2ef87da2116221318cb1c2e82baf7de06"
[[package]]
name = "bitflags"
version = "1.3.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "bef38d45163c2f1dde094a7dfd33ccf595c92905c8f8f4fdc18d06fb1037718a"
[[package]]
name = "bitflags"
version = "2.11.1"
@@ -286,8 +280,8 @@ dependencies = [
"clap",
"getrandom 0.3.4",
"libc",
"region",
"rlimit",
"secrets",
"tempfile",
"windows-sys 0.59.0",
"zeroize",
@@ -359,15 +353,6 @@ version = "0.12.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "32a66949e030da00e8c7d4434b251670a91556f4144941d37452769c25d58a53"
[[package]]
name = "mach2"
version = "0.4.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "d640282b302c0bb0a2a8e0233ead9035e3bed871f0b7e81fe4a1ec829765db44"
dependencies = [
"libc",
]
[[package]]
name = "memchr"
version = "2.8.0"
@@ -392,6 +377,16 @@ version = "0.3.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "c08d65885ee38876c4f86fa503fb49d7b507c2b62552df7c70b2fce627e06381"
[[package]]
name = "page_size"
version = "0.6.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "30d5b2194ed13191c1999ae0704b7839fb18384fa22e49b57eeaa97d79ce40da"
dependencies = [
"libc",
"winapi",
]
[[package]]
name = "password-hash"
version = "0.5.0"
@@ -403,6 +398,12 @@ dependencies = [
"subtle",
]
[[package]]
name = "pkg-config"
version = "0.3.33"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "19f132c84eca552bf34cab8ec81f1c1dcc229b811638f9d283dceabe58c5569e"
[[package]]
name = "poly1305"
version = "0.8.0"
@@ -480,18 +481,6 @@ version = "0.4.14"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "6e1dd4122fc1595e8162618945476892eefca7b88c52820e74af6262213cae8f"
[[package]]
name = "region"
version = "3.0.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "e6b6ebd13bc009aef9cd476c1310d49ac354d36e240cf1bd753290f3dc7199a7"
dependencies = [
"bitflags 1.3.2",
"libc",
"mach2",
"windows-sys 0.52.0",
]
[[package]]
name = "rlimit"
version = "0.10.2"
@@ -507,13 +496,25 @@ version = "1.1.4"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "b6fe4565b9518b83ef4f91bb47ce29620ca828bd32cb7e408f0062e9930ba190"
dependencies = [
"bitflags 2.11.1",
"bitflags",
"errno",
"libc",
"linux-raw-sys",
"windows-sys 0.61.2",
]
[[package]]
name = "secrets"
version = "1.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "71f5325144404085953b8078fa4a0b4d224d13f17ee3854534260ad7ff3dfb5c"
dependencies = [
"libc",
"page_size",
"pkg-config",
"vcpkg",
]
[[package]]
name = "serde"
version = "1.0.228"
@@ -613,6 +614,12 @@ version = "0.2.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "06abde3611657adf66d383f00b093d7faecc7fa57071cce2578660c9f1010821"
[[package]]
name = "vcpkg"
version = "0.2.15"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "accd4ea62f7bb7a82fe23066fb0957d48ef677f6eeb8215f372f52e48bb32426"
[[package]]
name = "version_check"
version = "0.9.5"
@@ -643,21 +650,34 @@ dependencies = [
"wit-bindgen",
]
[[package]]
name = "winapi"
version = "0.3.9"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "5c839a674fcd7a98952e593242ea400abe93992746761e38641405d28b00f419"
dependencies = [
"winapi-i686-pc-windows-gnu",
"winapi-x86_64-pc-windows-gnu",
]
[[package]]
name = "winapi-i686-pc-windows-gnu"
version = "0.4.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6"
[[package]]
name = "winapi-x86_64-pc-windows-gnu"
version = "0.4.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f"
[[package]]
name = "windows-link"
version = "0.2.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5"
[[package]]
name = "windows-sys"
version = "0.52.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "282be5f36a8ce781fad8c8ae18fa3f9beff57ec1b52cb3de0789201425d9a33d"
dependencies = [
"windows-targets",
]
[[package]]
name = "windows-sys"
version = "0.59.0"
Cargo.toml
+1 -1
View File
@@ -9,7 +9,7 @@ argon2 = "0.5"
chacha20poly1305 = "0.10"
clap = {version = "4", features = ["derive"]}
getrandom = {version = "0.3"}
region = "3"
protected-secrets = {package = "secrets", version = "1.3"}
zeroize = {version = "1", features = ["derive"]}
[target.'cfg(unix)'.dependencies]
src/crypto.rs
+13 -9
View File
@@ -6,7 +6,7 @@ use std::io::Write;
use crate::error::*;
use crate::header::{AlgId, Header, KdfParams, NONCE_PREFIX_LEN, TAG_LEN};
use crate::reader::{AheadReader, ReadInfoChunk};
use crate::secrets::SecretBytes32;
use crate::secrets::{SecretBytes32, SecretVec};
use crate::utils::*;
/// XChaCha20Poly1305 nonce: 24 bytes total. STREAM splits the trailing 5 bytes
@@ -28,15 +28,15 @@ fn make_nonce(prefix: &[u8; NONCE_PREFIX_LEN], counter: u32, last: bool) -> XNon
/// For `KdfParams::Argon2id`, `passphrase` must be supplied.
pub fn derive_key(
kdf: &KdfParams,
raw_key: Option<&[u8; 32]>,
passphrase: Option<&[u8]>,
raw_key: Option<&SecretBytes32>,
passphrase: Option<&SecretVec>,
) -> Result<SecretBytes32, FcryError> {
let mut out = SecretBytes32::zeroed();
match kdf {
KdfParams::Raw => {
let raw =
raw_key.ok_or_else(|| FcryError::Format("raw kdf requires --raw-key".into()))?;
out.as_mut_array().copy_from_slice(raw);
raw.with_array(|raw| out.with_mut_array(|out| out.copy_from_slice(raw)));
}
KdfParams::Argon2id {
salt,
@@ -49,7 +49,7 @@ pub fn derive_key(
let params = argon2::Params::new(*m_cost, *t_cost, *p_cost, Some(32))?;
let argon =
argon2::Argon2::new(argon2::Algorithm::Argon2id, argon2::Version::V0x13, params);
argon.hash_password_into(pw, salt, out.as_mut_array())?;
pw.with_slice(|pw| out.with_mut_array(|out| argon.hash_password_into(pw, salt, out)))?;
}
}
Ok(out)
@@ -79,7 +79,9 @@ pub fn encrypt<S: AsRef<str>>(
let aad = header.encode();
f_encrypted.write_all(&aad)?;
let aead = XChaCha20Poly1305::new(key.as_array().into());
// The AEAD keeps its own unprotected key copy while the loop runs.
// chacha20poly1305 zeroizes that copy on drop.
let aead = key.with_array(|key| XChaCha20Poly1305::new(key.into()));
let mut buf = vec![0u8; chunk_sz];
let mut counter: u32 = 0;
@@ -121,8 +123,8 @@ pub fn encrypt<S: AsRef<str>>(
pub fn decrypt<S: AsRef<str>>(
input_file: Option<S>,
output_file: Option<S>,
raw_key: Option<&[u8; 32]>,
passphrase: Option<&[u8]>,
raw_key: Option<&SecretBytes32>,
passphrase: Option<&SecretVec>,
) -> Result<(), FcryError> {
let mut reader = open_input(input_file)?;
let header = Header::read(&mut reader)?;
@@ -136,7 +138,9 @@ pub fn decrypt<S: AsRef<str>>(
let mut f_encrypted = AheadReader::from(reader, cipher_chunk);
let mut f_plain = OutSink::open(output_file)?;
let aead = XChaCha20Poly1305::new(key.as_array().into());
// The AEAD keeps its own unprotected key copy while the loop runs.
// chacha20poly1305 zeroizes that copy on drop.
let aead = key.with_array(|key| XChaCha20Poly1305::new(key.into()));
let mut buf = vec![0u8; cipher_chunk];
let mut counter: u32 = 0;
src/main.rs
+6 -12
View File
@@ -73,7 +73,7 @@ fn parse_raw_key(s: &str) -> Result<SecretBytes32, FcryError> {
)));
}
let mut key = SecretBytes32::zeroed();
key.as_mut_array().copy_from_slice(raw);
key.with_mut_array(|key| key.copy_from_slice(raw));
Ok(key)
}
@@ -86,9 +86,8 @@ enum PassphraseSource {
fn read_passphrase(src: &PassphraseSource, confirm: bool) -> Result<SecretVec, FcryError> {
match src {
PassphraseSource::EnvVar(var) => {
// Take the env value, then immediately convert to a Zeroize+mlock'd
// buffer. The original `String` from `env::var` is consumed by
// `into_bytes()`, so its allocation moves into our SecretVec.
// Take the env value, then immediately copy it into upstream
// protected storage. The source Vec is zeroed after the copy.
// Note: a copy still exists in the process `environ` table; that is
// a known and accepted leak for the env-var path.
let v = std::env::var(var).map_err(|_| {
@@ -105,7 +104,7 @@ fn read_passphrase(src: &PassphraseSource, confirm: bool) -> Result<SecretVec, F
if pw != pw2 {
return Err(FcryError::Passphrase("passphrases do not match".into()));
}
// pw2 dropped here -> zeroized + munlocked.
// pw2 dropped here -> zeroized + unlocked by the upstream crate.
}
Ok(pw)
}
@@ -160,12 +159,7 @@ fn run(mut cli: Cli) -> Result<(), FcryError> {
Some(src) => Some(read_passphrase(src, false)?),
None => None,
};
decrypt(
input,
output,
raw_key.as_ref().map(|k| k.as_array()),
pw.as_ref().map(|p| p.as_slice()),
)?;
decrypt(input, output, raw_key.as_ref(), pw.as_ref())?;
} else {
let (key, kdf) = if let Some(src) = &pw_src {
let mut salt = [0u8; ARGON2_SALT_LEN];
@@ -180,7 +174,7 @@ fn run(mut cli: Cli) -> Result<(), FcryError> {
p_cost: argon_parallelism,
};
let pw = read_passphrase(src, true)?;
let key = derive_key(&kdf, None, Some(pw.as_slice()))?;
let key = derive_key(&kdf, None, Some(&pw))?;
(key, kdf)
} else {
let key = parse_raw_key(raw_key_str.as_deref().unwrap())?;
src/secrets.rs
+52 -61
View File
@@ -2,120 +2,111 @@
//! Secret-handling primitives.
//!
//! Two wrappers and a cross-platform passphrase reader:
//! Thin local adapters around the upstream `secrets` crate plus a
//! cross-platform passphrase reader:
//!
//! * [`SecretBytes32`] — heap-allocated 32-byte buffer, mlock'd, zero on drop.
//! * [`SecretVec`] — heap-allocated `Vec<u8>` with stable capacity, mlock'd,
//! zero on drop.
//! * [`SecretBytes32`] — heap-allocated 32-byte buffer protected by
//! `secrets::SecretBox`.
//! * [`SecretVec`] — fixed-allocation byte buffer protected by
//! `secrets::SecretVec`, with a separate logical length so tty input can be
//! appended without reallocations.
//! * [`read_passphrase_tty`] — direct tty reader (Linux/macOS termios,
//! Windows Console API). Reads into a pre-reserved `SecretVec` so no
//! reallocation can leave stale unzeroed copies on the heap.
//!
//! mlock is provided via the `region` crate (portable across Linux/macOS/Windows).
//! The lock is dropped *before* the underlying buffer is freed (field order
//! matters in Rust drop semantics).
use std::io;
use zeroize::Zeroizing;
use protected_secrets::{SecretBox as ProtectedSecretBox, SecretVec as ProtectedSecretVec};
/// Maximum passphrase length we accept on the tty.
/// Pre-reserved so the underlying Vec never reallocates while reading.
pub const MAX_PASSPHRASE_LEN: usize = 1024;
/// Heap-allocated 32-byte secret. mlock'd; zeroed on drop.
/// Heap-allocated 32-byte secret protected by the upstream `secrets` crate.
pub struct SecretBytes32 {
// Field order matters: `_lock` is dropped first (munlock the page), then
// `inner` is dropped (zeroize the bytes, then free).
_lock: Option<region::LockGuard>,
inner: Box<Zeroizing<[u8; 32]>>,
inner: ProtectedSecretBox<[u8; 32]>,
}
impl SecretBytes32 {
pub fn zeroed() -> Self {
let inner = Box::new(Zeroizing::new([0u8; 32]));
let lock = region::lock(inner.as_ptr(), inner.len()).ok();
Self { _lock: lock, inner }
Self {
inner: ProtectedSecretBox::zero(),
}
}
pub fn as_array(&self) -> &[u8; 32] {
&self.inner
pub fn with_array<R>(&self, f: impl FnOnce(&[u8; 32]) -> R) -> R {
let inner = self.inner.borrow();
f(&inner)
}
pub fn as_mut_array(&mut self) -> &mut [u8; 32] {
&mut self.inner
pub fn with_mut_array<R>(&mut self, f: impl FnOnce(&mut [u8; 32]) -> R) -> R {
let mut inner = self.inner.borrow_mut();
f(&mut inner)
}
}
/// Heap-allocated byte buffer with **fixed capacity** that is mlock'd and
/// zeroed on drop. Pushing beyond the reserved capacity is rejected so the
/// underlying allocation never moves (which would invalidate the lock and
/// leave a stale unzeroed copy behind).
/// Heap-allocated byte buffer with **fixed capacity** protected by upstream
/// `secrets::SecretVec`.
///
/// Upstream `SecretVec` is fixed-length, so this adapter stores a separate
/// logical length. Bytes after `len` remain zero-filled padding and are never
/// exposed through [`SecretVec::with_slice`].
pub struct SecretVec {
_lock: Option<region::LockGuard>,
inner: Zeroizing<Vec<u8>>,
capacity: usize,
inner: ProtectedSecretVec<u8>,
len: usize,
}
impl SecretVec {
/// Allocate a buffer with fixed `capacity` and mlock it.
/// Allocate a protected buffer with fixed `capacity`.
pub fn with_capacity(capacity: usize) -> Self {
let inner = Zeroizing::new(Vec::with_capacity(capacity));
let lock = if capacity > 0 {
region::lock(inner.as_ptr(), capacity).ok()
} else {
None
};
Self {
_lock: lock,
inner,
capacity,
inner: ProtectedSecretVec::zero(capacity),
len: 0,
}
}
/// Wrap an already-allocated `Vec<u8>` (e.g. one we got from
/// `String::into_bytes()` for the env-var path). The Vec's `capacity`
/// is mlock'd as-is. Pushing afterwards is forbidden.
pub fn from_vec(v: Vec<u8>) -> Self {
let cap = v.capacity();
let inner = Zeroizing::new(v);
let lock = if cap > 0 {
region::lock(inner.as_ptr(), cap).ok()
} else {
None
};
/// Copy bytes from an already-allocated `Vec<u8>` into protected storage.
/// The upstream conversion zeroes the source bytes after copying; the
/// allocation itself is then released normally when the Vec is dropped.
pub fn from_vec(mut v: Vec<u8>) -> Self {
let len = v.len();
Self {
_lock: lock,
inner,
capacity: cap,
inner: ProtectedSecretVec::from(&mut v[..]),
len,
}
}
pub fn push(&mut self, b: u8) -> io::Result<()> {
if self.inner.len() >= self.capacity {
if self.len >= self.inner.len() {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"secret buffer full",
));
}
self.inner.push(b);
{
let mut inner = self.inner.borrow_mut();
inner[self.len] = b;
}
self.len += 1;
Ok(())
}
pub fn as_slice(&self) -> &[u8] {
&self.inner
pub fn with_slice<R>(&self, f: impl FnOnce(&[u8]) -> R) -> R {
let inner = self.inner.borrow();
f(&inner[..self.len])
}
}
impl PartialEq for SecretVec {
fn eq(&self, other: &Self) -> bool {
// constant-time-ish: compare full slices, no early return on mismatch.
let a = self.as_slice();
let b = other.as_slice();
if a.len() != b.len() {
// Constant-time-ish: length still leaks, but contents do not early-out.
if self.len != other.len {
return false;
}
let a = self.inner.borrow();
let b = other.inner.borrow();
let mut diff: u8 = 0;
for (x, y) in a.iter().zip(b.iter()) {
for (x, y) in a[..self.len].iter().zip(b[..other.len].iter()) {
diff |= x ^ y;
}
diff == 0