Fix Secret Key decode rejecting valid keys; verify build

The first base32 symbol carries 3 data bits (26 symbols = 130 bits, 2 spare),
but decode only accepted values 0-3, so ~half of generated keys failed to parse.
Adds an edge-case test, rustfmt config, Cargo.lock, and on-demand cross targets
instead of installing all seven in rust-toolchain.toml.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
2026-10-15 21:14:15 +09:00
co-authored by Claude Opus 5.5
parent 462fbbc523
commit 4a152bde11
14 changed files with 1947 additions and 59 deletions
Generated
+1845
View File
File diff suppressed because it is too large Load Diff
+1 -4
View File
@@ -75,10 +75,7 @@ pub fn unpad(padded: &[u8]) -> Result<Zeroizing<Vec<u8>>> {
return Err(CryptoError::Malformed); return Err(CryptoError::Malformed);
} }
let len = u32::from_le_bytes(padded[..4].try_into().unwrap()) as usize; let len = u32::from_le_bytes(padded[..4].try_into().unwrap()) as usize;
padded padded.get(4..4 + len).map(|d| Zeroizing::new(d.to_vec())).ok_or(CryptoError::Malformed)
.get(4..4 + len)
.map(|d| Zeroizing::new(d.to_vec()))
.ok_or(CryptoError::Malformed)
} }
#[cfg(test)] #[cfg(test)]
+15 -16
View File
@@ -20,24 +20,28 @@ pub struct DerivedKeys {
/// Default parameters for new accounts: RFC 9106 second recommended option. /// Default parameters for new accounts: RFC 9106 second recommended option.
pub fn default_params() -> KdfParams { pub fn default_params() -> KdfParams {
KdfParams { KdfParams { alg: "argon2id".into(), m_kib: 64 * 1024, t: 3, p: 4, salt: random_bytes::<16>().to_vec() }
alg: "argon2id".into(),
m_kib: 64 * 1024,
t: 3,
p: 4,
salt: random_bytes::<16>().to_vec(),
}
} }
/// Rejects parameters a malicious server might use to downgrade the KDF. /// Rejects parameters a malicious server might use to downgrade the KDF.
pub fn validate(params: &KdfParams) -> Result<()> { pub fn validate(params: &KdfParams) -> Result<()> {
if params.alg != "argon2id" || params.m_kib < MIN_KDF_M_KIB || params.t < MIN_KDF_T || params.p == 0 || params.salt.len() < 16 { if params.alg != "argon2id"
|| params.m_kib < MIN_KDF_M_KIB
|| params.t < MIN_KDF_T
|| params.p == 0
|| params.salt.len() < 16
{
return Err(CryptoError::WeakKdfParams); return Err(CryptoError::WeakKdfParams);
} }
Ok(()) Ok(())
} }
pub fn derive_keys(password: &str, secret_key: &SecretKey, account_id: Uuid, params: &KdfParams) -> Result<DerivedKeys> { pub fn derive_keys(
password: &str,
secret_key: &SecretKey,
account_id: Uuid,
params: &KdfParams,
) -> Result<DerivedKeys> {
validate(params)?; validate(params)?;
derive_keys_unchecked(password, secret_key, account_id, params) derive_keys_unchecked(password, secret_key, account_id, params)
} }
@@ -54,9 +58,7 @@ pub(crate) fn derive_keys_unchecked(
let mut argon_salt = [0u8; 16]; let mut argon_salt = [0u8; 16];
let mut info = b"kestrove/v1/argon-salt".to_vec(); let mut info = b"kestrove/v1/argon-salt".to_vec();
info.extend_from_slice(account_id.as_bytes()); info.extend_from_slice(account_id.as_bytes());
Hkdf::<Sha256>::new(None, &params.salt) Hkdf::<Sha256>::new(None, &params.salt).expand(&info, &mut argon_salt).map_err(|_| CryptoError::Kdf)?;
.expand(&info, &mut argon_salt)
.map_err(|_| CryptoError::Kdf)?;
let argon = Argon2::new( let argon = Argon2::new(
Algorithm::Argon2id, Algorithm::Argon2id,
@@ -132,10 +134,7 @@ mod tests {
#[test] #[test]
fn rejects_weak_params() { fn rejects_weak_params() {
let sk = SecretKey::from_bytes([1u8; 16]); let sk = SecretKey::from_bytes([1u8; 16]);
assert_eq!( assert_eq!(derive_keys("pw", &sk, Uuid::nil(), &test_params()).err(), Some(CryptoError::WeakKdfParams));
derive_keys("pw", &sk, Uuid::nil(), &test_params()).err(),
Some(CryptoError::WeakKdfParams)
);
assert!(validate(&default_params()).is_ok()); assert!(validate(&default_params()).is_ok());
} }
} }
+26 -7
View File
@@ -74,7 +74,12 @@ impl AccountKeyset {
let mut pt = Zeroizing::new(Vec::with_capacity(48)); let mut pt = Zeroizing::new(Vec::with_capacity(48));
pt.extend_from_slice(vk.id.as_bytes()); pt.extend_from_slice(vk.id.as_bytes());
pt.extend_from_slice(vk.expose()); pt.extend_from_slice(vk.expose());
seal_to(self.enc.id, &self.enc.public_bytes(), &pt, &Aad::build(ObjectType::VaultKey, self.account_id, Some(vault_id), None)) seal_to(
self.enc.id,
&self.enc.public_bytes(),
&pt,
&Aad::build(ObjectType::VaultKey, self.account_id, Some(vault_id), None),
)
} }
pub fn open_vault_key(&self, vault_id: Uuid, env: &Envelope) -> Result<SymKey> { pub fn open_vault_key(&self, vault_id: Uuid, env: &Envelope) -> Result<SymKey> {
@@ -100,26 +105,40 @@ pub struct SealedItem {
} }
/// Encrypts an item's plaintext (already serialized) with a fresh per-item key. /// Encrypts an item's plaintext (already serialized) with a fresh per-item key.
pub fn seal_item(account_id: Uuid, vault_id: Uuid, item_id: Uuid, vault_key: &SymKey, plaintext: &[u8]) -> Result<SealedItem> { pub fn seal_item(
account_id: Uuid,
vault_id: Uuid,
item_id: Uuid,
vault_key: &SymKey,
plaintext: &[u8],
) -> Result<SealedItem> {
let item_key = SymKey::generate(); let item_key = SymKey::generate();
let mut key_pt = Zeroizing::new(Vec::with_capacity(48)); let mut key_pt = Zeroizing::new(Vec::with_capacity(48));
key_pt.extend_from_slice(item_key.id.as_bytes()); key_pt.extend_from_slice(item_key.id.as_bytes());
key_pt.extend_from_slice(item_key.expose()); key_pt.extend_from_slice(item_key.expose());
let wrapped_item_key = let wrapped_item_key =
vault_key.seal(&key_pt, &Aad::build(ObjectType::ItemKey, account_id, Some(vault_id), Some(item_id)))?; vault_key.seal(&key_pt, &Aad::build(ObjectType::ItemKey, account_id, Some(vault_id), Some(item_id)))?;
let payload = item_key.seal(&pad(plaintext), &Aad::build(ObjectType::Item, account_id, Some(vault_id), Some(item_id)))?; let payload =
item_key.seal(&pad(plaintext), &Aad::build(ObjectType::Item, account_id, Some(vault_id), Some(item_id)))?;
Ok(SealedItem { wrapped_item_key, payload }) Ok(SealedItem { wrapped_item_key, payload })
} }
pub fn open_item(account_id: Uuid, vault_id: Uuid, item_id: Uuid, vault_key: &SymKey, sealed: &SealedItem) -> Result<Zeroizing<Vec<u8>>> { pub fn open_item(
let key_pt = account_id: Uuid,
vault_key.open(&sealed.wrapped_item_key, &Aad::build(ObjectType::ItemKey, account_id, Some(vault_id), Some(item_id)))?; vault_id: Uuid,
item_id: Uuid,
vault_key: &SymKey,
sealed: &SealedItem,
) -> Result<Zeroizing<Vec<u8>>> {
let key_pt = vault_key
.open(&sealed.wrapped_item_key, &Aad::build(ObjectType::ItemKey, account_id, Some(vault_id), Some(item_id)))?;
if key_pt.len() != 48 { if key_pt.len() != 48 {
return Err(CryptoError::Malformed); return Err(CryptoError::Malformed);
} }
let id = Uuid::from_slice(&key_pt[..16]).map_err(|_| CryptoError::Malformed)?; let id = Uuid::from_slice(&key_pt[..16]).map_err(|_| CryptoError::Malformed)?;
let item_key = SymKey::from_bytes(id, key_pt[16..].try_into().map_err(|_| CryptoError::Malformed)?); let item_key = SymKey::from_bytes(id, key_pt[16..].try_into().map_err(|_| CryptoError::Malformed)?);
let padded = item_key.open(&sealed.payload, &Aad::build(ObjectType::Item, account_id, Some(vault_id), Some(item_id)))?; let padded =
item_key.open(&sealed.payload, &Aad::build(ObjectType::Item, account_id, Some(vault_id), Some(item_id)))?;
unpad(&padded) unpad(&padded)
} }
+18 -5
View File
@@ -1,8 +1,8 @@
//! HPKE (RFC 9180) sealing of small secrets (vault keys) to an account's X25519 public key. //! HPKE (RFC 9180) sealing of small secrets (vault keys) to an account's X25519 public key.
use hpke::{ use hpke::{
aead::ChaCha20Poly1305, kdf::HkdfSha256, kem::X25519HkdfSha256, Deserializable, Kem as KemTrait, OpModeR, aead::ChaCha20Poly1305, kdf::HkdfSha256, kem::X25519HkdfSha256, Deserializable, Kem as KemTrait, OpModeR, OpModeS,
OpModeS, Serializable, Serializable,
}; };
use kv_proto::{Alg, Envelope, FORMAT_VERSION}; use kv_proto::{Alg, Envelope, FORMAT_VERSION};
use rand_core::OsRng; use rand_core::OsRng;
@@ -45,7 +45,14 @@ impl EncKeypair {
return Err(CryptoError::Unsupported); return Err(CryptoError::Unsupported);
} }
let encapped = <Kem as KemTrait>::EncappedKey::from_bytes(&env.enc).map_err(|_| CryptoError::Malformed)?; let encapped = <Kem as KemTrait>::EncappedKey::from_bytes(&env.enc).map_err(|_| CryptoError::Malformed)?;
hpke::single_shot_open::<ChaCha20Poly1305, HkdfSha256, Kem>(&OpModeR::Base, &self.sk, &encapped, INFO, &env.ct, aad) hpke::single_shot_open::<ChaCha20Poly1305, HkdfSha256, Kem>(
&OpModeR::Base,
&self.sk,
&encapped,
INFO,
&env.ct,
aad,
)
.map(Zeroizing::new) .map(Zeroizing::new)
.map_err(|_| CryptoError::Decrypt) .map_err(|_| CryptoError::Decrypt)
} }
@@ -54,8 +61,14 @@ impl EncKeypair {
/// Seals `plaintext` to a recipient public key (raw 32-byte X25519). /// Seals `plaintext` to a recipient public key (raw 32-byte X25519).
pub fn seal_to(recipient_id: Uuid, recipient_pk: &[u8], plaintext: &[u8], aad: &[u8]) -> Result<Envelope> { pub fn seal_to(recipient_id: Uuid, recipient_pk: &[u8], plaintext: &[u8], aad: &[u8]) -> Result<Envelope> {
let pk = <Kem as KemTrait>::PublicKey::from_bytes(recipient_pk).map_err(|_| CryptoError::Malformed)?; let pk = <Kem as KemTrait>::PublicKey::from_bytes(recipient_pk).map_err(|_| CryptoError::Malformed)?;
let (encapped, ct) = let (encapped, ct) = hpke::single_shot_seal::<ChaCha20Poly1305, HkdfSha256, Kem, _>(
hpke::single_shot_seal::<ChaCha20Poly1305, HkdfSha256, Kem, _>(&OpModeS::Base, &pk, INFO, plaintext, aad, &mut OsRng) &OpModeS::Base,
&pk,
INFO,
plaintext,
aad,
&mut OsRng,
)
.map_err(|_| CryptoError::Encrypt)?; .map_err(|_| CryptoError::Encrypt)?;
Ok(Envelope { Ok(Envelope {
v: FORMAT_VERSION, v: FORMAT_VERSION,
+15 -5
View File
@@ -44,10 +44,7 @@ impl SecretKey {
/// Accepts any case, spaces or hyphens, and the usual Crockford confusables (O→0, I/L→1). /// Accepts any case, spaces or hyphens, and the usual Crockford confusables (O→0, I/L→1).
pub fn parse(s: &str) -> Result<Self> { pub fn parse(s: &str) -> Result<Self> {
let cleaned: Zeroizing<String> = Zeroizing::new( let cleaned: Zeroizing<String> = Zeroizing::new(
s.chars() s.chars().filter(|c| !c.is_whitespace() && *c != '-').map(|c| c.to_ascii_uppercase()).collect(),
.filter(|c| !c.is_whitespace() && *c != '-')
.map(|c| c.to_ascii_uppercase())
.collect(),
); );
let rest = cleaned.strip_prefix(PREFIX).ok_or(CryptoError::BadSecretKey)?; let rest = cleaned.strip_prefix(PREFIX).ok_or(CryptoError::BadSecretKey)?;
if rest.len() != 27 { if rest.len() != 27 {
@@ -86,7 +83,8 @@ fn decode(s: &str) -> Result<[u8; 16]> {
let mut n: u128 = 0; let mut n: u128 = 0;
for (i, c) in s.bytes().enumerate() { for (i, c) in s.bytes().enumerate() {
let v = symbol_value(c)? as u128; let v = symbol_value(c)? as u128;
if i == 0 && v > 0b11 { // 26 symbols carry 130 bits; the 2 spare bits sit at the top, so the first symbol holds 3 data bits.
if i == 0 && v > 0b111 {
return Err(CryptoError::BadSecretKey); // would overflow 128 bits return Err(CryptoError::BadSecretKey); // would overflow 128 bits
} }
n = (n << 5) | v; n = (n << 5) | v;
@@ -109,6 +107,18 @@ mod tests {
} }
} }
#[test]
fn extremes_roundtrip() {
for b in [[0u8; 16], [0xff; 16], [0x80; 16]] {
let k = SecretKey::from_bytes(b);
assert_eq!(SecretKey::parse(&k.to_display()).unwrap().expose(), &b);
}
// First symbol 8 would need bit 128: reject.
let max = SecretKey::from_bytes([0xff; 16]).to_display().to_string();
assert!(max.starts_with("K1-7"));
assert!(SecretKey::parse(&max.replacen("K1-7", "K1-8", 1)).is_err());
}
#[test] #[test]
fn detects_typo() { fn detects_typo() {
let k = SecretKey::from_bytes([0xAB; 16]); let k = SecretKey::from_bytes([0xAB; 16]);
+6 -2
View File
@@ -192,7 +192,9 @@ impl VaultCore {
.iter() .iter()
.map(|(id, sealed)| decrypt(st, u, *id, sealed).map(|i| summary(&i))) .map(|(id, sealed)| decrypt(st, u, *id, sealed).map(|i| summary(&i)))
.collect::<Result<Vec<_>>>()?; .collect::<Result<Vec<_>>>()?;
out.sort_by(|a, b| b.favorite.cmp(&a.favorite).then_with(|| a.title.to_lowercase().cmp(&b.title.to_lowercase()))); out.sort_by(|a, b| {
b.favorite.cmp(&a.favorite).then_with(|| a.title.to_lowercase().cmp(&b.title.to_lowercase()))
});
Ok(out) Ok(out)
}) })
} }
@@ -228,7 +230,9 @@ impl VaultCore {
model.tags = item.tags; model.tags = item.tags;
model.modified_at = now; model.modified_at = now;
let json = zeroize::Zeroizing::new(serde_json::to_vec(&model).map_err(|e| CoreError::Internal { msg: e.to_string() })?); let json = zeroize::Zeroizing::new(
serde_json::to_vec(&model).map_err(|e| CoreError::Internal { msg: e.to_string() })?,
);
let sealed = seal_item(st.account_id, st.vault_id, id, &u.vault_key, &json)?; let sealed = seal_item(st.account_id, st.vault_id, id, &u.vault_key, &json)?;
st.items.insert(id, sealed); st.items.insert(id, sealed);
Ok(id.to_string()) Ok(id.to_string())
+4 -7
View File
@@ -27,12 +27,8 @@ const AMBIGUOUS: &str = "0O1lI|";
/// Generates a password with at least one character from each enabled class. /// Generates a password with at least one character from each enabled class.
pub fn generate_password(opts: &PasswordOptions) -> String { pub fn generate_password(opts: &PasswordOptions) -> String {
let classes: Vec<Vec<char>> = [ let classes: Vec<Vec<char>> =
(opts.lowercase, LOWER), [(opts.lowercase, LOWER), (opts.uppercase, UPPER), (opts.digits, DIGITS), (opts.symbols, SYMBOLS)]
(opts.uppercase, UPPER),
(opts.digits, DIGITS),
(opts.symbols, SYMBOLS),
]
.into_iter() .into_iter()
.filter(|(on, _)| *on) .filter(|(on, _)| *on)
.map(|(_, set)| set.chars().filter(|c| !(opts.avoid_ambiguous && AMBIGUOUS.contains(*c))).collect()) .map(|(_, set)| set.chars().filter(|c| !(opts.avoid_ambiguous && AMBIGUOUS.contains(*c))).collect())
@@ -70,7 +66,8 @@ mod tests {
assert!(p.chars().any(|c| c.is_ascii_digit())); assert!(p.chars().any(|c| c.is_ascii_digit()));
assert!(!p.chars().any(|c| AMBIGUOUS.contains(c))); assert!(!p.chars().any(|c| AMBIGUOUS.contains(c)));
} }
let digits_only = PasswordOptions { length: 6, lowercase: false, uppercase: false, symbols: false, ..Default::default() }; let digits_only =
PasswordOptions { length: 6, lowercase: false, uppercase: false, symbols: false, ..Default::default() };
assert_eq!(generate_password(&digits_only).len(), 8); // minimum length 8 assert_eq!(generate_password(&digits_only).len(), 8); // minimum length 8
} }
} }
+2 -1
View File
@@ -86,7 +86,8 @@ fn hmac_bytes<M: Mac + hmac::digest::KeyInit>(key: &[u8], msg: &[u8]) -> Vec<u8>
} }
fn decode_b32(s: &str) -> Result<Vec<u8>, TotpError> { fn decode_b32(s: &str) -> Result<Vec<u8>, TotpError> {
let cleaned: String = s.chars().filter(|c| !c.is_whitespace() && *c != '=' && *c != '-').collect::<String>().to_ascii_uppercase(); let cleaned: String =
s.chars().filter(|c| !c.is_whitespace() && *c != '=' && *c != '-').collect::<String>().to_ascii_uppercase();
BASE32_NOPAD.decode(cleaned.as_bytes()).map_err(|_| TotpError::Invalid) BASE32_NOPAD.decode(cleaned.as_bytes()).map_err(|_| TotpError::Invalid)
} }
+3 -1
View File
@@ -27,7 +27,9 @@ pub enum StoreError {
pub enum SyncState { pub enum SyncState {
Clean, Clean,
/// Edited locally; `base_revision` is what the edit was made against. /// Edited locally; `base_revision` is what the edit was made against.
Pending { base_revision: u64 }, Pending {
base_revision: u64,
},
} }
/// Storage backend. The SQLite implementation and an in-memory one for tests come in M1. /// Storage backend. The SQLite implementation and an in-memory one for tests come in M1.
+1 -4
View File
@@ -1,7 +1,4 @@
[toolchain] [toolchain]
channel = "stable" channel = "stable"
components = ["rustfmt", "clippy"] components = ["rustfmt", "clippy"]
targets = [ # Cross targets are added on demand by scripts/build-apple.sh and scripts/build-android.sh.
"aarch64-apple-ios", "aarch64-apple-ios-sim", "aarch64-apple-darwin", "x86_64-apple-darwin",
"aarch64-linux-android", "armv7-linux-androideabi", "x86_64-linux-android",
]
+2
View File
@@ -0,0 +1,2 @@
max_width = 120
use_small_heuristics = "Max"
+1
View File
@@ -3,6 +3,7 @@
set -euo pipefail set -euo pipefail
cd "$(dirname "$0")/.." cd "$(dirname "$0")/.."
OUT=../kestrove-android/core-ffi/src/main OUT=../kestrove-android/core-ffi/src/main
rustup target add aarch64-linux-android armv7-linux-androideabi x86_64-linux-android
cargo ndk -t arm64-v8a -t armeabi-v7a -t x86_64 -o "$OUT/jniLibs" build -p kv-ffi --release cargo ndk -t arm64-v8a -t armeabi-v7a -t x86_64 -o "$OUT/jniLibs" build -p kv-ffi --release
cargo build -p kv-ffi cargo build -p kv-ffi
cargo run -p kv-ffi --bin uniffi-bindgen -- generate --library target/debug/libkv_ffi.dylib --language kotlin --out-dir "$OUT/java" cargo run -p kv-ffi --bin uniffi-bindgen -- generate --library target/debug/libkv_ffi.dylib --language kotlin --out-dir "$OUT/java"
+1
View File
@@ -4,6 +4,7 @@ set -euo pipefail
cd "$(dirname "$0")/.." cd "$(dirname "$0")/.."
OUT=../kestrove-apple/Packages/KestroveCore OUT=../kestrove-apple/Packages/KestroveCore
LIB=libkv_ffi.a LIB=libkv_ffi.a
rustup target add aarch64-apple-ios aarch64-apple-ios-sim aarch64-apple-darwin x86_64-apple-darwin
for t in aarch64-apple-ios aarch64-apple-ios-sim aarch64-apple-darwin x86_64-apple-darwin; do for t in aarch64-apple-ios aarch64-apple-ios-sim aarch64-apple-darwin x86_64-apple-darwin; do
cargo build -p kv-ffi --release --target "$t" cargo build -p kv-ffi --release --target "$t"