From 8e6593ba335e8ae86df745a61e07e403f793caba Mon Sep 17 00:00:00 2001 From: Aporis3674 <233422463+Aporis3674@users.noreply.github.com> Date: Sun, 26 Jul 2026 21:11:00 +0000 Subject: [PATCH] fix(config): don't panic on a corrupt config, and don't leave keys world-readable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Loading an identity went through `impl From for Identity`, which decoded the two WireGuard keys with `.expect()` and then `copy_from_slice()` into `[u8; 32]`. Both panic on input the user can easily end up with: $ cargo test bug_repro -- --nocapture panicked at src/config.rs:55:24: copy_from_slice: source slice length (20) does not match destination slice length (32) panicked at src/config.rs:46:14: # non-base64 value `config::save` writes with `std::fs::write`, which truncates in place, so a process killed mid-save leaves exactly that truncated file — and every later start panics instead of re-registering. `config::load` already returns `Result`, so the caller is prepared for an error; it cannot catch a panic. The `client_id` field a few lines below is length-checked, which is the pattern the two key fields were missing. - `TryFrom` replaces `From`, and `decode_key` names the offending field: "config: wg_private_key decodes to 20 bytes, expected 32" - the config holds a WireGuard private key, a MASQUE client private key and an access token, and was created with the process umask — 0644 on a typical Linux box. It is now written to a temp file, chmod 0600, then renamed, which fixes the world-readable mode and makes the write atomic so the truncation cannot happen in the first place - five tests cover both malformed shapes, an oversized key, the happy path and the resulting file mode --- aether/src/config.rs | 145 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 119 insertions(+), 26 deletions(-) diff --git a/aether/src/config.rs b/aether/src/config.rs index addc2ae..0064a3b 100644 --- a/aether/src/config.rs +++ b/aether/src/config.rs @@ -6,6 +6,21 @@ use serde::{Deserialize, Serialize}; use crate::account::Identity; use crate::error::{AetherError, Result}; +/// Decode a base64 field into a fixed-size key, reporting the field name on +/// failure instead of panicking. A truncated or hand-edited config must be a +/// recoverable error: the caller can re-register, but it cannot catch a panic. +fn decode_key(field: &str, value: &str) -> Result<[u8; N]> { + let raw = base64::engine::general_purpose::STANDARD + .decode(value.trim()) + .map_err(|e| AetherError::Other(format!("config: {field} is not valid base64: {e}")))?; + <[u8; N]>::try_from(raw.as_slice()).map_err(|_| { + AetherError::Other(format!( + "config: {field} decodes to {} bytes, expected {N}", + raw.len() + )) + }) +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct PersistedIdentity { pub device_id: String, @@ -39,31 +54,25 @@ impl From<&Identity> for PersistedIdentity { } } -impl From for Identity { - fn from(p: PersistedIdentity) -> Self { - let wg_priv = base64::engine::general_purpose::STANDARD - .decode(&p.wg_private_key) - .expect("decode wg private key"); - - let wg_peer = base64::engine::general_purpose::STANDARD - .decode(&p.wg_peer_public_key) - .expect("decode wg peer public key"); - - let mut wg_private_key = [0u8; 32]; - let mut wg_peer_public_key = [0u8; 32]; - let mut client_id_arr = [0u8; 3]; - wg_private_key.copy_from_slice(&wg_priv); - wg_peer_public_key.copy_from_slice(&wg_peer); - +impl TryFrom for Identity { + type Error = AetherError; + + fn try_from(p: PersistedIdentity) -> Result { + let wg_private_key = decode_key::<32>("wg_private_key", &p.wg_private_key)?; + let wg_peer_public_key = decode_key::<32>("wg_peer_public_key", &p.wg_peer_public_key)?; + + // client_id predates the current format, so an absent or unreadable + // value stays a zeroed default rather than an error. + let mut client_id = [0u8; 3]; if !p.client_id.is_empty() { if let Ok(decoded) = base64::engine::general_purpose::STANDARD.decode(&p.client_id) { - if decoded.len() == 3 { - client_id_arr.copy_from_slice(&decoded); + if decoded.len() == client_id.len() { + client_id.copy_from_slice(&decoded); } } } - Identity { + Ok(Identity { device_id: p.device_id, access_token: p.access_token, cert_pem: p.cert_pem.into_bytes(), @@ -72,11 +81,29 @@ impl From for Identity { ipv6: p.ipv6, wg_private_key, wg_peer_public_key, - client_id: client_id_arr, - } + client_id, + }) } } +/// Write a file that holds private keys: atomically, and readable only by the +/// current user. `std::fs::write` truncates in place — a crash mid-write leaves +/// a half-written config — and it creates the file with the process umask, +/// which is world-readable on a typical Linux box. +fn write_private(path: &str, contents: &str) -> Result<()> { + let tmp = format!("{path}.tmp"); + std::fs::write(&tmp, contents)?; + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&tmp, std::fs::Permissions::from_mode(0o600))?; + } + + std::fs::rename(&tmp, path)?; + Ok(()) +} + pub fn load(path: &str) -> Result> { if !Path::new(path).exists() { return Ok(None); @@ -84,15 +111,14 @@ pub fn load(path: &str) -> Result> { let text = std::fs::read_to_string(path)?; let persisted: PersistedIdentity = toml::from_str(&text).map_err(|e| AetherError::Other(format!("config parse: {e}")))?; - Ok(Some(persisted.into())) + Ok(Some(Identity::try_from(persisted)?)) } pub fn save(path: &str, identity: &Identity) -> Result<()> { let persisted = PersistedIdentity::from(identity); let text = toml::to_string_pretty(&persisted) .map_err(|e| AetherError::Other(format!("config encode: {e}")))?; - std::fs::write(path, text)?; - Ok(()) + write_private(path, &text) } pub fn save_masque_creds(path: &str, cert_pem: &[u8], key_pem: &[u8]) -> Result<()> { @@ -106,6 +132,73 @@ pub fn save_masque_creds(path: &str, cert_pem: &[u8], key_pem: &[u8]) -> Result< persisted.key_pem = String::from_utf8_lossy(key_pem).to_string(); let updated = toml::to_string_pretty(&persisted) .map_err(|e| AetherError::Other(format!("config encode: {e}")))?; - std::fs::write(path, updated)?; - Ok(()) + write_private(path, &updated) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn persisted(wg_private_key: &str, wg_peer_public_key: &str) -> PersistedIdentity { + PersistedIdentity { + device_id: "d".into(), + access_token: "t".into(), + cert_pem: String::new(), + key_pem: String::new(), + ipv4: "10.0.0.2".into(), + ipv6: "::1".into(), + wg_private_key: wg_private_key.into(), + wg_peer_public_key: wg_peer_public_key.into(), + client_id: String::new(), + } + } + + fn b64(bytes: &[u8]) -> String { + base64::engine::general_purpose::STANDARD.encode(bytes) + } + + #[test] + fn reads_a_well_formed_identity() { + let id = Identity::try_from(persisted(&b64(&[1u8; 32]), &b64(&[2u8; 32]))).unwrap(); + assert_eq!(id.wg_private_key, [1u8; 32]); + assert_eq!(id.wg_peer_public_key, [2u8; 32]); + } + + #[test] + fn a_truncated_key_is_an_error_not_a_panic() { + // What a config left half-written by a killed process looks like. + let err = Identity::try_from(persisted(&b64(&[1u8; 20]), &b64(&[2u8; 32]))).unwrap_err(); + assert!(err.to_string().contains("wg_private_key"), "{err}"); + assert!(err.to_string().contains("expected 32"), "{err}"); + } + + #[test] + fn a_key_that_is_not_base64_is_an_error_not_a_panic() { + let err = + Identity::try_from(persisted("!!! not base64 !!!", &b64(&[2u8; 32]))).unwrap_err(); + assert!(err.to_string().contains("not valid base64"), "{err}"); + } + + #[test] + fn an_oversized_key_is_rejected_too() { + let err = Identity::try_from(persisted(&b64(&[1u8; 33]), &b64(&[2u8; 32]))).unwrap_err(); + assert!(err.to_string().contains("expected 32"), "{err}"); + } + + #[cfg(unix)] + #[test] + fn the_saved_config_is_not_world_readable() { + use std::os::unix::fs::PermissionsExt; + + let dir = std::env::temp_dir().join(format!("aether-cfg-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("aether.toml"); + let path = path.to_str().unwrap(); + + write_private(path, "device_id = \"d\"\n").unwrap(); + + let mode = std::fs::metadata(path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "config holds private keys, mode was {mode:o}"); + let _ = std::fs::remove_dir_all(&dir); + } }