From 5d5a84778655e15352817a4b8d69a57b57906164 Mon Sep 17 00:00:00 2001 From: fufesou Date: Thu, 17 Sep 2026 11:14:45 +0800 Subject: [PATCH 1/8] fix: key file perm Signed-off-by: fufesou --- src/common.rs | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 98 insertions(+), 1 deletion(-) diff --git a/src/common.rs b/src/common.rs index df1a7df47..187b8758b 100644 --- a/src/common.rs +++ b/src/common.rs @@ -193,6 +193,46 @@ pub fn now() -> u64 { .unwrap_or_default() } +#[cfg(unix)] +const PRIVATE_KEY_FILE_MODE: u32 = 0o600; + +fn create_private_key_file(path: &str) -> std::io::Result { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + + match std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(path) + { + Ok(file) => { + if let Err(err) = + file.set_permissions(std::fs::Permissions::from_mode(PRIVATE_KEY_FILE_MODE)) + { + log::warn!( + "Failed to set permissions for private key file {}: {}", + path, + err + ); + } + Ok(file) + } + Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => { + std::fs::OpenOptions::new() + .write(true) + .truncate(true) + .open(path) + } + Err(err) => Err(err), + } + } + #[cfg(not(unix))] + { + std::fs::File::create(path) + } +} + pub fn gen_sk(wait: u64) -> (String, Option) { let sk_file = "id_ed25519"; if wait > 0 && !std::path::Path::new(sk_file).exists() { @@ -230,7 +270,7 @@ pub fn gen_sk(wait: u64) -> (String, Option) { let pub_file = format!("{sk_file}.pub"); if let Ok(mut f) = std::fs::File::create(&pub_file) { f.write_all(pk.as_bytes()).ok(); - if let Ok(mut f) = std::fs::File::create(sk_file) { + if let Ok(mut f) = create_private_key_file(sk_file) { let s = base64::encode(&sk); if f.write_all(s.as_bytes()).is_ok() { log::info!("Private/public key written to {}/{}", sk_file, pub_file); @@ -399,4 +439,61 @@ mod tests { .unwrap() .is_none()); } + + #[cfg(unix)] + #[test] + fn private_key_file_permissions() { + use std::process::Command; + + const CHILD_ENV: &str = "RUSTDESK_PRIVATE_KEY_TEST_CHILD"; + const CHILD_COMPLETED: &str = "private_key_test_child_completed"; + const TEST_NAME: &str = "common::tests::private_key_file_permissions"; + if std::env::var_os(CHILD_ENV).is_some() { + const TEST_UMASK: hbb_common::libc::mode_t = 0o022; + // The child process isolates the umask from other tests. + unsafe { hbb_common::libc::umask(TEST_UMASK) }; + assert_private_key_file_permissions(); + println!("{CHILD_COMPLETED}"); + return; + } + + let output = Command::new(std::env::current_exe().unwrap()) + .args(["--exact", TEST_NAME, "--nocapture"]) + .env(CHILD_ENV, "1") + .output() + .unwrap(); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + output.status.success() && stdout.contains(CHILD_COMPLETED), + "key-file test failed ({}):\n{stdout}\n{}", + output.status, + String::from_utf8_lossy(&output.stderr) + ); + } + + #[cfg(unix)] + fn assert_private_key_file_permissions() { + use std::os::unix::fs::PermissionsExt; + + const PERMISSION_BITS: u32 = 0o777; + const NEW_FILE_MODE: u32 = 0o600; + const EXISTING_MODE: u32 = 0o644; + let path = + std::env::temp_dir().join(format!("rustdesk-private-key-{}", uuid::Uuid::new_v4())); + let file = create_private_key_file(path.to_str().unwrap()).unwrap(); + assert_eq!( + file.metadata().unwrap().permissions().mode() & PERMISSION_BITS, + NEW_FILE_MODE + ); + file.set_permissions(std::fs::Permissions::from_mode(EXISTING_MODE)) + .unwrap(); + drop(file); + let file = create_private_key_file(path.to_str().unwrap()).unwrap(); + assert_eq!( + file.metadata().unwrap().permissions().mode() & PERMISSION_BITS, + EXISTING_MODE + ); + drop(file); + std::fs::remove_file(path).unwrap(); + } } From 7169b48a8613d0d009d592c961eb6f98ddadba8f Mon Sep 17 00:00:00 2001 From: fufesou Date: Fri, 18 Sep 2026 14:45:45 +0800 Subject: [PATCH 2/8] fix: enforce secure private key permissions on Unix Create new keys with mode 0600 and verify permissions before writing. Tighten existing keys to 0600 and propagate permission failures. Preserve dangling symlinks and clean up failed key creation. Signed-off-by: fufesou --- src/common.rs | 211 ++++++++++++++++++++++++--------------- src/relay_server.rs | 8 +- src/rendezvous_server.rs | 8 +- 3 files changed, 136 insertions(+), 91 deletions(-) diff --git a/src/common.rs b/src/common.rs index 187b8758b..45c65de18 100644 --- a/src/common.rs +++ b/src/common.rs @@ -196,91 +196,115 @@ pub fn now() -> u64 { #[cfg(unix)] const PRIVATE_KEY_FILE_MODE: u32 = 0o600; -fn create_private_key_file(path: &str) -> std::io::Result { - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - - match std::fs::OpenOptions::new() - .write(true) - .create_new(true) - .open(path) - { +#[cfg(not(unix))] +fn create_private_key_file(path: &str) -> ResultType { + Ok(std::fs::File::create(path)?) +} + +#[cfg(unix)] +fn create_private_key_file(path: &str) -> ResultType { + use std::os::unix::fs::OpenOptionsExt; + + let mut path = std::path::PathBuf::from(path); + let mut options = std::fs::OpenOptions::new(); + options + .write(true) + .create_new(true) + .mode(PRIVATE_KEY_FILE_MODE); + loop { + match options.open(&path) { Ok(file) => { - if let Err(err) = - file.set_permissions(std::fs::Permissions::from_mode(PRIVATE_KEY_FILE_MODE)) - { - log::warn!( - "Failed to set permissions for private key file {}: {}", - path, - err - ); + if let Err(err) = set_private_key_permissions(&file, true) { + drop(file); + std::fs::remove_file(&path).with_context(|| { + format!("Failed to remove {} after {err:#}", path.display()) + })?; + return Err(err); } - Ok(file) + return Ok(file); } Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => { - std::fs::OpenOptions::new() - .write(true) - .truncate(true) - .open(path) + // Let the OS reject symlink loops before following a dangling link. + match std::fs::metadata(&path) { + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) => return Err(err.into()), + Ok(_) => return Err(err.into()), + } + let target = std::fs::read_link(&path)?; + path.pop(); + path.push(target); } - Err(err) => Err(err), + Err(err) => return Err(err.into()), } } - #[cfg(not(unix))] - { - std::fs::File::create(path) +} + +#[cfg(unix)] +fn set_private_key_permissions(file: &std::fs::File, newly_created: bool) -> ResultType<()> { + use std::os::unix::fs::PermissionsExt; + + const PERMISSION_BITS: u32 = 0o7777; + let mode = file.metadata()?.permissions().mode() & PERMISSION_BITS; + hbb_common::anyhow::ensure!( + !newly_created || mode & !PRIVATE_KEY_FILE_MODE == 0, + "Unsafe initial private key permissions: {mode:04o}" + ); + if mode != PRIVATE_KEY_FILE_MODE { + file.set_permissions(std::fs::Permissions::from_mode(PRIVATE_KEY_FILE_MODE)) + .context("Failed to set private key permissions to 0600")?; + let mode = file.metadata()?.permissions().mode() & PERMISSION_BITS; + hbb_common::anyhow::ensure!( + mode == PRIVATE_KEY_FILE_MODE, + "Private key permissions are {mode:04o}, expected 0600" + ); } + Ok(()) } -pub fn gen_sk(wait: u64) -> (String, Option) { +pub fn gen_sk(wait: u64) -> ResultType<(String, Option)> { let sk_file = "id_ed25519"; if wait > 0 && !std::path::Path::new(sk_file).exists() { std::thread::sleep(std::time::Duration::from_millis(wait)); } - if let Ok(mut file) = std::fs::File::open(sk_file) { - let mut contents = String::new(); - if file.read_to_string(&mut contents).is_ok() { - let contents = contents.trim(); - let sk = base64::decode(contents).unwrap_or_default(); - if sk.len() == sign::SECRETKEYBYTES { - let mut tmp = [0u8; sign::SECRETKEYBYTES]; - tmp[..].copy_from_slice(&sk); - let pk = base64::encode(&tmp[sign::SECRETKEYBYTES / 2..]); - log::info!("Private key comes from {}", sk_file); - return (pk, Some(sign::SecretKey(tmp))); - } else { - // don't use log here, since it is async - println!("Fatal error: malformed private key in {sk_file}."); - std::process::exit(1); - } + match std::fs::File::open(sk_file) { + Ok(mut file) => { + #[cfg(unix)] + set_private_key_permissions(&file, false)?; + let mut contents = String::new(); + file.read_to_string(&mut contents) + .context("Failed to read private key")?; + let sk = base64::decode(contents.trim()).context("Malformed private key")?; + hbb_common::anyhow::ensure!(sk.len() == sign::SECRETKEYBYTES, "Malformed private key"); + let mut tmp = [0u8; sign::SECRETKEYBYTES]; + tmp[..].copy_from_slice(&sk); + let pk = base64::encode(&tmp[sign::SECRETKEYBYTES / 2..]); + log::info!("Private key comes from {}", sk_file); + return Ok((pk, Some(sign::SecretKey(tmp)))); } - } else { - let gen_func = || { - let (tmp, sk) = sign::gen_keypair(); - (base64::encode(tmp), sk) - }; - let (mut pk, mut sk) = gen_func(); - for _ in 0..300 { - if !pk.contains('/') && !pk.contains(':') { - break; - } - (pk, sk) = gen_func(); - } - let pub_file = format!("{sk_file}.pub"); - if let Ok(mut f) = std::fs::File::create(&pub_file) { - f.write_all(pk.as_bytes()).ok(); - if let Ok(mut f) = create_private_key_file(sk_file) { - let s = base64::encode(&sk); - if f.write_all(s.as_bytes()).is_ok() { - log::info!("Private/public key written to {}/{}", sk_file, pub_file); - log::debug!("Public key: {}", pk); - return (pk, Some(sk)); - } - } + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) => return Err(err).context("Failed to open private key"), + } + let gen_func = || { + let (tmp, sk) = sign::gen_keypair(); + (base64::encode(tmp), sk) + }; + let (mut pk, mut sk) = gen_func(); + for _ in 0..300 { + if !pk.contains('/') && !pk.contains(':') { + break; } + (pk, sk) = gen_func(); } - ("".to_owned(), None) + let pub_file = format!("{sk_file}.pub"); + let mut f = std::fs::File::create(&pub_file).context("Failed to create public key file")?; + f.write_all(pk.as_bytes()) + .context("Failed to write public key")?; + let mut f = create_private_key_file(sk_file).context("Failed to create private key file")?; + f.write_all(base64::encode(&sk).as_bytes()) + .context("Failed to write private key")?; + log::info!("Private/public key written to {}/{}", sk_file, pub_file); + log::debug!("Public key: {}", pk); + Ok((pk, Some(sk))) } #[cfg(unix)] @@ -473,27 +497,48 @@ mod tests { #[cfg(unix)] fn assert_private_key_file_permissions() { - use std::os::unix::fs::PermissionsExt; + use std::os::unix::fs::{symlink, PermissionsExt}; const PERMISSION_BITS: u32 = 0o777; const NEW_FILE_MODE: u32 = 0o600; const EXISTING_MODE: u32 = 0o644; - let path = + const RESTRICTIVE_UMASK: hbb_common::libc::mode_t = 0o777; + let directory = std::env::temp_dir().join(format!("rustdesk-private-key-{}", uuid::Uuid::new_v4())); - let file = create_private_key_file(path.to_str().unwrap()).unwrap(); - assert_eq!( - file.metadata().unwrap().permissions().mode() & PERMISSION_BITS, - NEW_FILE_MODE - ); - file.set_permissions(std::fs::Permissions::from_mode(EXISTING_MODE)) - .unwrap(); - drop(file); - let file = create_private_key_file(path.to_str().unwrap()).unwrap(); - assert_eq!( - file.metadata().unwrap().permissions().mode() & PERMISSION_BITS, - EXISTING_MODE - ); + std::fs::create_dir(&directory).unwrap(); + std::env::set_current_dir(&directory).unwrap(); + let path = std::path::Path::new("id_ed25519"); + let mode = || std::fs::metadata(path).unwrap().permissions().mode() & PERMISSION_BITS; + + gen_sk(0).unwrap(); + assert_eq!(mode(), NEW_FILE_MODE); + let contents = std::fs::read(path).unwrap(); + std::fs::set_permissions(path, std::fs::Permissions::from_mode(EXISTING_MODE)).unwrap(); + let file = std::fs::File::open(path).unwrap(); + assert!(set_private_key_permissions(&file, true).is_err()); + assert_eq!(mode(), EXISTING_MODE); drop(file); + gen_sk(0).unwrap(); + assert_eq!(mode(), NEW_FILE_MODE); + assert_eq!(std::fs::read(path).unwrap(), contents); + assert!(create_private_key_file("id_ed25519").is_err()); + assert_eq!(std::fs::read(path).unwrap(), contents); + + std::fs::remove_file(path).unwrap(); + symlink("key-alias", path).unwrap(); + symlink("key-target", "key-alias").unwrap(); + gen_sk(0).unwrap(); + assert_eq!(mode(), NEW_FILE_MODE); + assert!(!std::fs::read("key-target").unwrap().is_empty()); + std::fs::remove_file(path).unwrap(); + unsafe { hbb_common::libc::umask(RESTRICTIVE_UMASK) }; + gen_sk(0).unwrap(); + assert_eq!(mode(), NEW_FILE_MODE); + let contents = std::fs::read(path).unwrap(); + gen_sk(0).unwrap(); + assert_eq!(std::fs::read(path).unwrap(), contents); + std::env::set_current_dir(directory.parent().unwrap()).unwrap(); + std::fs::remove_dir_all(directory).unwrap(); } } diff --git a/src/relay_server.rs b/src/relay_server.rs index de1a7eae9..c57267412 100644 --- a/src/relay_server.rs +++ b/src/relay_server.rs @@ -51,7 +51,7 @@ pub async fn start_with_bind( port: &str, key: &str, ) -> ResultType<()> { - let key = get_server_sk(key); + let key = get_server_sk(key)?; if let Ok(mut file) = std::fs::File::open(BLACKLIST_FILE) { let mut contents = String::new(); if file.read_to_string(&mut contents).is_ok() { @@ -601,7 +601,7 @@ async fn relay( Ok(()) } -fn get_server_sk(key: &str) -> String { +fn get_server_sk(key: &str) -> ResultType { let mut key = key.to_owned(); if let Ok(sk) = base64::decode(&key) { if sk.len() == sign::SECRETKEYBYTES { @@ -611,7 +611,7 @@ fn get_server_sk(key: &str) -> String { } if key == "-" || key == "_" { - let (pk, _) = crate::common::gen_sk(300); + let (pk, _) = crate::common::gen_sk(300)?; key = pk; } @@ -619,7 +619,7 @@ fn get_server_sk(key: &str) -> String { log::info!("Key: {}", key); } - key + Ok(key) } #[async_trait] diff --git a/src/rendezvous_server.rs b/src/rendezvous_server.rs index eaf7190f9..ce81efeb1 100644 --- a/src/rendezvous_server.rs +++ b/src/rendezvous_server.rs @@ -111,7 +111,7 @@ impl RendezvousServer { key: &str, rmem: usize, ) -> ResultType<()> { - let (key, sk) = Self::get_server_sk(key); + let (key, sk) = Self::get_server_sk(key)?; let nat_port = port - 1; let ws_port = port + 2; let pm = PeerMap::new().await?; @@ -1256,7 +1256,7 @@ impl RendezvousServer { } #[inline] - fn get_server_sk(key: &str) -> (String, Option) { + fn get_server_sk(key: &str) -> ResultType<(String, Option)> { let mut out_sk = None; let mut key = key.to_owned(); if let Ok(sk) = base64::decode(&key) { @@ -1270,7 +1270,7 @@ impl RendezvousServer { } if key.is_empty() || key == "-" || key == "_" { - let (pk, sk) = crate::common::gen_sk(0); + let (pk, sk) = crate::common::gen_sk(0)?; out_sk = sk; if !key.is_empty() { key = pk; @@ -1280,7 +1280,7 @@ impl RendezvousServer { if !key.is_empty() { log::info!("Key: {}", key); } - (key, out_sk) + Ok((key, out_sk)) } #[inline] From 3b1c6d81493d100e0c04d19b30f70af1474e5694 Mon Sep 17 00:00:00 2001 From: fufesou Date: Fri, 18 Sep 2026 17:02:24 +0800 Subject: [PATCH 3/8] fix: preserve read-only keys and prevent public key clobbering Accept existing 0400 keys without changing their permissions. Write the public key only after private key creation and writing succeed. Signed-off-by: fufesou --- src/common.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/common.rs b/src/common.rs index 45c65de18..6df433ad8 100644 --- a/src/common.rs +++ b/src/common.rs @@ -245,6 +245,9 @@ fn set_private_key_permissions(file: &std::fs::File, newly_created: bool) -> Res const PERMISSION_BITS: u32 = 0o7777; let mode = file.metadata()?.permissions().mode() & PERMISSION_BITS; + if !newly_created && mode & !PRIVATE_KEY_FILE_MODE == 0 { + return Ok(()); + } hbb_common::anyhow::ensure!( !newly_created || mode & !PRIVATE_KEY_FILE_MODE == 0, "Unsafe initial private key permissions: {mode:04o}" @@ -295,13 +298,13 @@ pub fn gen_sk(wait: u64) -> ResultType<(String, Option)> { } (pk, sk) = gen_func(); } + let mut f = create_private_key_file(sk_file).context("Failed to create private key file")?; + f.write_all(base64::encode(&sk).as_bytes()) + .context("Failed to write private key")?; let pub_file = format!("{sk_file}.pub"); let mut f = std::fs::File::create(&pub_file).context("Failed to create public key file")?; f.write_all(pk.as_bytes()) .context("Failed to write public key")?; - let mut f = create_private_key_file(sk_file).context("Failed to create private key file")?; - f.write_all(base64::encode(&sk).as_bytes()) - .context("Failed to write private key")?; log::info!("Private/public key written to {}/{}", sk_file, pub_file); log::debug!("Public key: {}", pk); Ok((pk, Some(sk))) @@ -502,6 +505,7 @@ mod tests { const PERMISSION_BITS: u32 = 0o777; const NEW_FILE_MODE: u32 = 0o600; const EXISTING_MODE: u32 = 0o644; + const READ_ONLY_MODE: u32 = 0o400; const RESTRICTIVE_UMASK: hbb_common::libc::mode_t = 0o777; let directory = std::env::temp_dir().join(format!("rustdesk-private-key-{}", uuid::Uuid::new_v4())); @@ -510,7 +514,7 @@ mod tests { let path = std::path::Path::new("id_ed25519"); let mode = || std::fs::metadata(path).unwrap().permissions().mode() & PERMISSION_BITS; - gen_sk(0).unwrap(); + let public_key = gen_sk(0).unwrap().0; assert_eq!(mode(), NEW_FILE_MODE); let contents = std::fs::read(path).unwrap(); std::fs::set_permissions(path, std::fs::Permissions::from_mode(EXISTING_MODE)).unwrap(); @@ -523,7 +527,14 @@ mod tests { assert_eq!(std::fs::read(path).unwrap(), contents); assert!(create_private_key_file("id_ed25519").is_err()); assert_eq!(std::fs::read(path).unwrap(), contents); + std::fs::set_permissions(path, std::fs::Permissions::from_mode(READ_ONLY_MODE)).unwrap(); + assert_eq!(gen_sk(0).unwrap().0, public_key); + assert_eq!(mode(), READ_ONLY_MODE); + std::fs::remove_file(path).unwrap(); + symlink("missing/key", path).unwrap(); + assert!(gen_sk(0).is_err()); + assert!(std::fs::read_to_string("id_ed25519.pub").unwrap() == public_key); std::fs::remove_file(path).unwrap(); symlink("key-alias", path).unwrap(); symlink("key-target", "key-alias").unwrap(); From 26635f16764cf587f49791895b7057ab97fbc4a2 Mon Sep 17 00:00:00 2001 From: fufesou Date: Fri, 18 Sep 2026 23:32:15 +0800 Subject: [PATCH 4/8] fix: generate missing public keys and reject mismatched key pairs Signed-off-by: fufesou --- src/common.rs | 74 ++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 68 insertions(+), 6 deletions(-) diff --git a/src/common.rs b/src/common.rs index 6df433ad8..c38c89f61 100644 --- a/src/common.rs +++ b/src/common.rs @@ -264,8 +264,46 @@ fn set_private_key_permissions(file: &std::fs::File, newly_created: bool) -> Res Ok(()) } +fn write_public_key(path: &str, pk: &str, required: bool) -> ResultType<()> { + match std::fs::read_to_string(path) { + Ok(contents) if contents.trim() == pk => return Ok(()), + Ok(_) => { + let err = hbb_common::anyhow::anyhow!( + "Public key in {path} does not match private key in id_ed25519" + ); + log::error!("{err}"); + return Err(err); + } + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) => return Err(err).context("Failed to read public key file"), + } + let mut file = match std::fs::File::create(path) { + Ok(file) => file, + // Existing private keys may be provisioned in a read-only directory. + Err(err) if !required => { + log::warn!("Failed to create {path}: {err}; using the existing private key"); + return Ok(()); + } + Err(err) => return Err(err).context("Failed to create public key file"), + }; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + + const OWNER_READ_WRITE: u32 = 0o600; + let mode = file.metadata()?.permissions().mode(); + if mode & OWNER_READ_WRITE != OWNER_READ_WRITE { + file.set_permissions(std::fs::Permissions::from_mode(mode | OWNER_READ_WRITE)) + .context("Failed to set public key owner permissions")?; + } + } + file.write_all(pk.as_bytes()) + .context("Failed to write public key") +} + pub fn gen_sk(wait: u64) -> ResultType<(String, Option)> { let sk_file = "id_ed25519"; + let pub_file = format!("{sk_file}.pub"); if wait > 0 && !std::path::Path::new(sk_file).exists() { std::thread::sleep(std::time::Duration::from_millis(wait)); } @@ -281,6 +319,7 @@ pub fn gen_sk(wait: u64) -> ResultType<(String, Option)> { let mut tmp = [0u8; sign::SECRETKEYBYTES]; tmp[..].copy_from_slice(&sk); let pk = base64::encode(&tmp[sign::SECRETKEYBYTES / 2..]); + write_public_key(&pub_file, &pk, false)?; log::info!("Private key comes from {}", sk_file); return Ok((pk, Some(sign::SecretKey(tmp)))); } @@ -301,10 +340,7 @@ pub fn gen_sk(wait: u64) -> ResultType<(String, Option)> { let mut f = create_private_key_file(sk_file).context("Failed to create private key file")?; f.write_all(base64::encode(&sk).as_bytes()) .context("Failed to write private key")?; - let pub_file = format!("{sk_file}.pub"); - let mut f = std::fs::File::create(&pub_file).context("Failed to create public key file")?; - f.write_all(pk.as_bytes()) - .context("Failed to write public key")?; + write_public_key(&pub_file, &pk, true)?; log::info!("Private/public key written to {}/{}", sk_file, pub_file); log::debug!("Public key: {}", pk); Ok((pk, Some(sk))) @@ -498,6 +534,31 @@ mod tests { ); } + #[cfg(unix)] + fn assert_public_key_recovery() -> String { + let sk_file = "id_ed25519"; + let pub_file = "id_ed25519.pub"; + std::fs::create_dir(pub_file).unwrap(); + assert!(gen_sk(0).is_err()); + let private_key = std::fs::read(sk_file).unwrap(); + std::fs::remove_dir(pub_file).unwrap(); + let public_key = gen_sk(0).unwrap().0; + assert_eq!(std::fs::read_to_string(pub_file).unwrap(), public_key); + let mismatched_key = base64::encode(sign::gen_keypair().0); + std::fs::write(pub_file, &mismatched_key).unwrap(); + assert!(gen_sk(0).is_err()); + assert!(std::fs::read(sk_file).unwrap() == private_key); + assert_eq!(std::fs::read_to_string(pub_file).unwrap(), mismatched_key); + std::fs::write(pub_file, &public_key).unwrap(); + let permissions = std::fs::metadata(pub_file).unwrap().permissions(); + let mut read_only = permissions.clone(); + read_only.set_readonly(true); + std::fs::set_permissions(pub_file, read_only).unwrap(); + assert_eq!(gen_sk(0).unwrap().0, public_key); + std::fs::set_permissions(pub_file, permissions).unwrap(); + public_key + } + #[cfg(unix)] fn assert_private_key_file_permissions() { use std::os::unix::fs::{symlink, PermissionsExt}; @@ -514,14 +575,13 @@ mod tests { let path = std::path::Path::new("id_ed25519"); let mode = || std::fs::metadata(path).unwrap().permissions().mode() & PERMISSION_BITS; - let public_key = gen_sk(0).unwrap().0; + let public_key = assert_public_key_recovery(); assert_eq!(mode(), NEW_FILE_MODE); let contents = std::fs::read(path).unwrap(); std::fs::set_permissions(path, std::fs::Permissions::from_mode(EXISTING_MODE)).unwrap(); let file = std::fs::File::open(path).unwrap(); assert!(set_private_key_permissions(&file, true).is_err()); assert_eq!(mode(), EXISTING_MODE); - drop(file); gen_sk(0).unwrap(); assert_eq!(mode(), NEW_FILE_MODE); assert_eq!(std::fs::read(path).unwrap(), contents); @@ -536,6 +596,7 @@ mod tests { assert!(gen_sk(0).is_err()); assert!(std::fs::read_to_string("id_ed25519.pub").unwrap() == public_key); std::fs::remove_file(path).unwrap(); + std::fs::remove_file("id_ed25519.pub").unwrap(); symlink("key-alias", path).unwrap(); symlink("key-target", "key-alias").unwrap(); gen_sk(0).unwrap(); @@ -543,6 +604,7 @@ mod tests { assert!(!std::fs::read("key-target").unwrap().is_empty()); std::fs::remove_file(path).unwrap(); + std::fs::remove_file("id_ed25519.pub").unwrap(); unsafe { hbb_common::libc::umask(RESTRICTIVE_UMASK) }; gen_sk(0).unwrap(); assert_eq!(mode(), NEW_FILE_MODE); From 5532aacdb1dcccfc1441a1d7450c5863d6cfcfe3 Mon Sep 17 00:00:00 2001 From: fufesou Date: Sat, 19 Sep 2026 11:22:36 +0800 Subject: [PATCH 5/8] fix: publish public keys atomically without overwriting existing files Signed-off-by: fufesou --- Cargo.lock | 1 + Cargo.toml | 1 + src/common.rs | 119 ++++++++++++++++++++++++++++++++++++++++++++------ 3 files changed, 108 insertions(+), 13 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7c76b2b87..7a95f8a46 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1712,6 +1712,7 @@ dependencies = [ "serde_json", "sodiumoxide", "sqlx", + "tempfile", "tokio-tungstenite 0.17.1", "tower-http", "tungstenite 0.17.2", diff --git a/Cargo.toml b/Cargo.toml index 43963d125..b1c9715ce 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -41,6 +41,7 @@ jsonwebtoken = "8" headers = "0.3" once_cell = "1.8" sodiumoxide = "0.2" +tempfile = "3" tokio-tungstenite = "0.17" tungstenite = "0.17" regex = "1.4" diff --git a/src/common.rs b/src/common.rs index c38c89f61..35c2fdf2c 100644 --- a/src/common.rs +++ b/src/common.rs @@ -8,6 +8,7 @@ use std::{ io::prelude::*, io::Read, net::{IpAddr, Ipv4Addr, SocketAddr}, + path::{Path, PathBuf}, time::{Instant, SystemTime}, }; @@ -264,28 +265,71 @@ fn set_private_key_permissions(file: &std::fs::File, newly_created: bool) -> Res Ok(()) } -fn write_public_key(path: &str, pk: &str, required: bool) -> ResultType<()> { - match std::fs::read_to_string(path) { - Ok(contents) if contents.trim() == pk => return Ok(()), - Ok(_) => { - let err = hbb_common::anyhow::anyhow!( - "Public key in {path} does not match private key in id_ed25519" +fn missing_public_key_path(path: &Path, pk: &str) -> ResultType> { + let mut path = path.to_path_buf(); + loop { + match std::fs::read_to_string(&path) { + Ok(contents) if contents.trim() == pk => return Ok(None), + Ok(_) => { + let err = hbb_common::anyhow::anyhow!( + "Public key in {} does not match private key in id_ed25519", + path.display() + ); + log::error!("{err}"); + return Err(err); + } + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) => return Err(err).context("Failed to read public key file"), + } + match std::fs::symlink_metadata(&path) { + Ok(metadata) if metadata.file_type().is_symlink() => { + let target = std::fs::read_link(&path)?; + path.pop(); + path.push(target); + } + Ok(_) => continue, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(Some(path)), + Err(err) => return Err(err).context("Failed to resolve public key path"), + } + } +} + +fn publish_public_key(file: tempfile::TempPath, path: &Path, pk: &str) -> ResultType<()> { + match file.persist_noclobber(path) { + Ok(_) => Ok(()), + Err(err) if err.error.kind() == std::io::ErrorKind::AlreadyExists => { + hbb_common::anyhow::ensure!( + missing_public_key_path(path, pk)?.is_none(), + "Public key disappeared during publication" ); - log::error!("{err}"); - return Err(err); + Ok(()) } - Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} - Err(err) => return Err(err).context("Failed to read public key file"), + Err(err) => Err(err.error).context("Failed to publish public key file"), } - let mut file = match std::fs::File::create(path) { +} + +fn write_public_key(path: &str, pk: &str, required: bool) -> ResultType<()> { + let Some(path) = missing_public_key_path(Path::new(path), pk)? else { + return Ok(()); + }; + let temporary = path.with_file_name(format!(".id_ed25519.pub.{}", uuid::Uuid::new_v4())); + let mut file = match std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(&temporary) + { Ok(file) => file, // Existing private keys may be provisioned in a read-only directory. Err(err) if !required => { - log::warn!("Failed to create {path}: {err}; using the existing private key"); + log::warn!( + "Failed to create {}: {err}; using the existing private key", + path.display() + ); return Ok(()); } Err(err) => return Err(err).context("Failed to create public key file"), }; + let temporary = tempfile::TempPath::from_path(temporary); #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; @@ -298,7 +342,10 @@ fn write_public_key(path: &str, pk: &str, required: bool) -> ResultType<()> { } } file.write_all(pk.as_bytes()) - .context("Failed to write public key") + .context("Failed to write public key")?; + file.sync_all().context("Failed to sync public key")?; + drop(file); + publish_public_key(temporary, &path, pk) } pub fn gen_sk(wait: u64) -> ResultType<(String, Option)> { @@ -534,8 +581,38 @@ mod tests { ); } + #[cfg(target_os = "linux")] + fn assert_public_key_write_failure(public_key: &str) { + use hbb_common::libc; + + const WRITE_LIMIT: libc::rlim_t = 16; + let pub_file = "id_ed25519.pub"; + std::fs::remove_file(pub_file).unwrap(); + let result = unsafe { + let mut limits = std::mem::zeroed(); + assert_eq!(libc::getrlimit(libc::RLIMIT_FSIZE, &mut limits), 0); + let handler = libc::signal(libc::SIGXFSZ, libc::SIG_IGN); + assert_ne!(handler, libc::SIG_ERR); + let restricted = libc::rlimit { + rlim_cur: WRITE_LIMIT, + ..limits + }; + assert_eq!(libc::setrlimit(libc::RLIMIT_FSIZE, &restricted), 0); + let result = gen_sk(0); + assert_eq!(libc::setrlimit(libc::RLIMIT_FSIZE, &limits), 0); + assert_ne!(libc::signal(libc::SIGXFSZ, handler), libc::SIG_ERR); + result + }; + assert!(result.is_err()); + assert!(!std::path::Path::new(pub_file).exists()); + assert_eq!(gen_sk(0).unwrap().0, public_key); + assert_eq!(std::fs::read_to_string(pub_file).unwrap(), public_key); + } + #[cfg(unix)] fn assert_public_key_recovery() -> String { + use std::os::unix::fs::MetadataExt; + let sk_file = "id_ed25519"; let pub_file = "id_ed25519.pub"; std::fs::create_dir(pub_file).unwrap(); @@ -544,7 +621,23 @@ mod tests { std::fs::remove_dir(pub_file).unwrap(); let public_key = gen_sk(0).unwrap().0; assert_eq!(std::fs::read_to_string(pub_file).unwrap(), public_key); + #[cfg(target_os = "linux")] + assert_public_key_write_failure(&public_key); + let path = Path::new(pub_file); + let prepare = |pk: &str| { + let mut file = tempfile::NamedTempFile::new_in(".").unwrap(); + file.write_all(pk.as_bytes()).unwrap(); + file.into_temp_path() + }; + std::fs::remove_file(path).unwrap(); + let pending = prepare(&public_key); + gen_sk(0).unwrap(); + let inode = std::fs::metadata(path).unwrap().ino(); + publish_public_key(pending, path, &public_key).unwrap(); let mismatched_key = base64::encode(sign::gen_keypair().0); + assert!(publish_public_key(prepare(&mismatched_key), path, &mismatched_key).is_err()); + assert_eq!(std::fs::metadata(path).unwrap().ino(), inode); + assert_eq!(std::fs::read_to_string(path).unwrap(), public_key); std::fs::write(pub_file, &mismatched_key).unwrap(); assert!(gen_sk(0).is_err()); assert!(std::fs::read(sk_file).unwrap() == private_key); From f1268def6302cf75b60e252cc47449d621c3dfcd Mon Sep 17 00:00:00 2001 From: fufesou Date: Sat, 19 Sep 2026 14:02:32 +0800 Subject: [PATCH 6/8] fix: key file perms, comments Signed-off-by: fufesou --- src/common.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/common.rs b/src/common.rs index 35c2fdf2c..a80aab12e 100644 --- a/src/common.rs +++ b/src/common.rs @@ -202,6 +202,9 @@ fn create_private_key_file(path: &str) -> ResultType { Ok(std::fs::File::create(path)?) } +/// Restricts access from creation so another user cannot retain a readable descriptor. +/// Follows dangling symlinks for configured key paths, but never overwrites an existing key. +/// Verifies permissions before returning; on failure, closes and tries to remove the empty file. #[cfg(unix)] fn create_private_key_file(path: &str) -> ResultType { use std::os::unix::fs::OpenOptionsExt; @@ -308,6 +311,9 @@ fn publish_public_key(file: tempfile::TempPath, path: &Path, pk: &str) -> Result } } +/// Publishes a fully written temporary file so interrupted writes cannot expose partial public keys. +/// Preserves existing files: matching public keys are accepted, while mismatches return errors. +/// With `required = false`, temp-file creation failure only warns to support read-only provisioning. fn write_public_key(path: &str, pk: &str, required: bool) -> ResultType<()> { let Some(path) = missing_public_key_path(Path::new(path), pk)? else { return Ok(()); From d25975a7084f54dca9afc9c564a182f46fdc47fd Mon Sep 17 00:00:00 2001 From: fufesou Date: Sat, 19 Sep 2026 19:01:41 +0800 Subject: [PATCH 7/8] fix: reject key conflicts and clean up failed private key writes Reuse symlink resolution and simplify key loading and regression tests. Signed-off-by: fufesou --- src/common.rs | 293 +++++++++++++++++++++++++++----------------------- 1 file changed, 156 insertions(+), 137 deletions(-) diff --git a/src/common.rs b/src/common.rs index a80aab12e..5497ced73 100644 --- a/src/common.rs +++ b/src/common.rs @@ -5,6 +5,7 @@ use hbb_common::{ use ini::Ini; use sodiumoxide::crypto::sign; use std::{ + fs, io::prelude::*, io::Read, net::{IpAddr, Ipv4Addr, SocketAddr}, @@ -196,55 +197,61 @@ pub fn now() -> u64 { #[cfg(unix)] const PRIVATE_KEY_FILE_MODE: u32 = 0o600; +const MAX_KEY_SYMLINKS: usize = 40; -#[cfg(not(unix))] -fn create_private_key_file(path: &str) -> ResultType { - Ok(std::fs::File::create(path)?) +fn resolve_key_path(path: &Path) -> ResultType { + let mut path = path.to_path_buf(); + // Bound traversal even if symlinks change while they are being resolved. + for hops in 0..=MAX_KEY_SYMLINKS { + match fs::symlink_metadata(&path) { + Ok(metadata) if metadata.file_type().is_symlink() => {} + Ok(_) => return Ok(path), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(path), + Err(err) => return Err(err.into()), + } + if hops == MAX_KEY_SYMLINKS { + break; + } + let target = fs::read_link(&path)?; + path.pop(); + path.push(target); + } + hbb_common::anyhow::bail!( + "Too many key symlinks (limit: {MAX_KEY_SYMLINKS}): {}", + path.display() + ); } -/// Restricts access from creation so another user cannot retain a readable descriptor. +/// On Unix, restricts access from creation so another user cannot retain a readable descriptor. /// Follows dangling symlinks for configured key paths, but never overwrites an existing key. -/// Verifies permissions before returning; on failure, closes and tries to remove the empty file. -#[cfg(unix)] -fn create_private_key_file(path: &str) -> ResultType { +/// Writes after verifying Unix permissions; failures close and try to remove the new file. +fn create_private_key_file(path: &str, contents: &[u8]) -> ResultType<()> { + #[cfg(unix)] use std::os::unix::fs::OpenOptionsExt; - let mut path = std::path::PathBuf::from(path); - let mut options = std::fs::OpenOptions::new(); - options - .write(true) - .create_new(true) - .mode(PRIVATE_KEY_FILE_MODE); - loop { - match options.open(&path) { - Ok(file) => { - if let Err(err) = set_private_key_permissions(&file, true) { - drop(file); - std::fs::remove_file(&path).with_context(|| { - format!("Failed to remove {} after {err:#}", path.display()) - })?; - return Err(err); - } - return Ok(file); - } - Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => { - // Let the OS reject symlink loops before following a dangling link. - match std::fs::metadata(&path) { - Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} - Err(err) => return Err(err.into()), - Ok(_) => return Err(err.into()), - } - let target = std::fs::read_link(&path)?; - path.pop(); - path.push(target); - } - Err(err) => return Err(err.into()), - } + let path = resolve_key_path(Path::new(path))?; + let mut options = fs::OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + options.mode(PRIVATE_KEY_FILE_MODE); + let mut file = options.open(&path)?; + let result = (|| { + #[cfg(unix)] + set_private_key_permissions(&file, true)?; + file.write_all(contents) + .context("Failed to write private key") + })(); + if let Err(err) = result { + drop(file); + fs::remove_file(&path) + .with_context(|| format!("Failed to remove {} after {err:#}", path.display()))?; + return Err(err); } + Ok(()) } #[cfg(unix)] -fn set_private_key_permissions(file: &std::fs::File, newly_created: bool) -> ResultType<()> { +fn set_private_key_permissions(file: &fs::File, newly_created: bool) -> ResultType<()> { use std::os::unix::fs::PermissionsExt; const PERMISSION_BITS: u32 = 0o7777; @@ -257,7 +264,7 @@ fn set_private_key_permissions(file: &std::fs::File, newly_created: bool) -> Res "Unsafe initial private key permissions: {mode:04o}" ); if mode != PRIVATE_KEY_FILE_MODE { - file.set_permissions(std::fs::Permissions::from_mode(PRIVATE_KEY_FILE_MODE)) + file.set_permissions(fs::Permissions::from_mode(PRIVATE_KEY_FILE_MODE)) .context("Failed to set private key permissions to 0600")?; let mode = file.metadata()?.permissions().mode() & PERMISSION_BITS; hbb_common::anyhow::ensure!( @@ -269,31 +276,19 @@ fn set_private_key_permissions(file: &std::fs::File, newly_created: bool) -> Res } fn missing_public_key_path(path: &Path, pk: &str) -> ResultType> { - let mut path = path.to_path_buf(); - loop { - match std::fs::read_to_string(&path) { - Ok(contents) if contents.trim() == pk => return Ok(None), - Ok(_) => { - let err = hbb_common::anyhow::anyhow!( - "Public key in {} does not match private key in id_ed25519", - path.display() - ); - log::error!("{err}"); - return Err(err); - } - Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} - Err(err) => return Err(err).context("Failed to read public key file"), - } - match std::fs::symlink_metadata(&path) { - Ok(metadata) if metadata.file_type().is_symlink() => { - let target = std::fs::read_link(&path)?; - path.pop(); - path.push(target); - } - Ok(_) => continue, - Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(Some(path)), - Err(err) => return Err(err).context("Failed to resolve public key path"), + let path = resolve_key_path(path).context("Failed to resolve public key path")?; + match fs::read_to_string(&path) { + Ok(contents) if contents.trim() == pk => Ok(None), + Ok(_) => { + let err = hbb_common::anyhow::anyhow!( + "Public key in {} does not match private key in id_ed25519", + path.display() + ); + log::error!("{err}"); + Err(err) } + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(Some(path)), + Err(err) => Err(err).context("Failed to read public key file"), } } @@ -319,7 +314,7 @@ fn write_public_key(path: &str, pk: &str, required: bool) -> ResultType<()> { return Ok(()); }; let temporary = path.with_file_name(format!(".id_ed25519.pub.{}", uuid::Uuid::new_v4())); - let mut file = match std::fs::OpenOptions::new() + let mut file = match fs::OpenOptions::new() .write(true) .create_new(true) .open(&temporary) @@ -343,7 +338,7 @@ fn write_public_key(path: &str, pk: &str, required: bool) -> ResultType<()> { const OWNER_READ_WRITE: u32 = 0o600; let mode = file.metadata()?.permissions().mode(); if mode & OWNER_READ_WRITE != OWNER_READ_WRITE { - file.set_permissions(std::fs::Permissions::from_mode(mode | OWNER_READ_WRITE)) + file.set_permissions(fs::Permissions::from_mode(mode | OWNER_READ_WRITE)) .context("Failed to set public key owner permissions")?; } } @@ -360,7 +355,7 @@ pub fn gen_sk(wait: u64) -> ResultType<(String, Option)> { if wait > 0 && !std::path::Path::new(sk_file).exists() { std::thread::sleep(std::time::Duration::from_millis(wait)); } - match std::fs::File::open(sk_file) { + match fs::File::open(sk_file) { Ok(mut file) => { #[cfg(unix)] set_private_key_permissions(&file, false)?; @@ -368,13 +363,11 @@ pub fn gen_sk(wait: u64) -> ResultType<(String, Option)> { file.read_to_string(&mut contents) .context("Failed to read private key")?; let sk = base64::decode(contents.trim()).context("Malformed private key")?; - hbb_common::anyhow::ensure!(sk.len() == sign::SECRETKEYBYTES, "Malformed private key"); - let mut tmp = [0u8; sign::SECRETKEYBYTES]; - tmp[..].copy_from_slice(&sk); - let pk = base64::encode(&tmp[sign::SECRETKEYBYTES / 2..]); + let sk = sign::SecretKey::from_slice(&sk).context("Malformed private key")?; + let pk = base64::encode(sk.public_key()); write_public_key(&pub_file, &pk, false)?; log::info!("Private key comes from {}", sk_file); - return Ok((pk, Some(sign::SecretKey(tmp)))); + return Ok((pk, Some(sk))); } Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} Err(err) => return Err(err).context("Failed to open private key"), @@ -390,9 +383,9 @@ pub fn gen_sk(wait: u64) -> ResultType<(String, Option)> { } (pk, sk) = gen_func(); } - let mut f = create_private_key_file(sk_file).context("Failed to create private key file")?; - f.write_all(base64::encode(&sk).as_bytes()) - .context("Failed to write private key")?; + missing_public_key_path(Path::new(&pub_file), &pk)?; + create_private_key_file(sk_file, base64::encode(&sk).as_bytes()) + .context("Failed to create private key file")?; write_public_key(&pub_file, &pk, true)?; log::info!("Private/public key written to {}/{}", sk_file, pub_file); log::debug!("Public key: {}", pk); @@ -556,6 +549,30 @@ mod tests { .is_none()); } + #[cfg(unix)] + #[test] + fn key_path_symlinks_are_bounded() { + use std::os::unix::fs::symlink; + + let directory = tempfile::tempdir().unwrap(); + let target = directory.path().join("target"); + let mut path = target.clone(); + for index in 0..MAX_KEY_SYMLINKS { + let link = directory.path().join(format!("link-{index}")); + symlink(&path, &link).unwrap(); + path = link; + } + assert_eq!(resolve_key_path(&path).unwrap(), target); + let extra = directory.path().join("extra"); + symlink(&path, &extra).unwrap(); + let cycle = directory.path().join("cycle"); + symlink(&cycle, &cycle).unwrap(); + for path in [&extra, &cycle] { + let error = resolve_key_path(path).unwrap_err(); + assert!(error.to_string().starts_with("Too many key symlinks")); + } + } + #[cfg(unix)] #[test] fn private_key_file_permissions() { @@ -588,31 +605,35 @@ mod tests { } #[cfg(target_os = "linux")] - fn assert_public_key_write_failure(public_key: &str) { + fn with_file_size_limit(limit: hbb_common::libc::rlim_t, run: impl FnOnce() -> T) -> T { use hbb_common::libc; - const WRITE_LIMIT: libc::rlim_t = 16; - let pub_file = "id_ed25519.pub"; - std::fs::remove_file(pub_file).unwrap(); - let result = unsafe { + unsafe { let mut limits = std::mem::zeroed(); assert_eq!(libc::getrlimit(libc::RLIMIT_FSIZE, &mut limits), 0); let handler = libc::signal(libc::SIGXFSZ, libc::SIG_IGN); assert_ne!(handler, libc::SIG_ERR); let restricted = libc::rlimit { - rlim_cur: WRITE_LIMIT, + rlim_cur: limit, ..limits }; assert_eq!(libc::setrlimit(libc::RLIMIT_FSIZE, &restricted), 0); - let result = gen_sk(0); + let result = run(); assert_eq!(libc::setrlimit(libc::RLIMIT_FSIZE, &limits), 0); assert_ne!(libc::signal(libc::SIGXFSZ, handler), libc::SIG_ERR); result - }; - assert!(result.is_err()); - assert!(!std::path::Path::new(pub_file).exists()); + } + } + + #[cfg(target_os = "linux")] + fn assert_public_key_write_failure(public_key: &str) { + const WRITE_LIMIT: hbb_common::libc::rlim_t = 16; + let pub_file = "id_ed25519.pub"; + fs::remove_file(pub_file).unwrap(); + assert!(with_file_size_limit(WRITE_LIMIT, || gen_sk(0)).is_err()); + assert!(!Path::new(pub_file).exists()); assert_eq!(gen_sk(0).unwrap().0, public_key); - assert_eq!(std::fs::read_to_string(pub_file).unwrap(), public_key); + assert_eq!(fs::read_to_string(pub_file).unwrap(), public_key); } #[cfg(unix)] @@ -621,12 +642,20 @@ mod tests { let sk_file = "id_ed25519"; let pub_file = "id_ed25519.pub"; - std::fs::create_dir(pub_file).unwrap(); + let mismatched_key = base64::encode(sign::gen_keypair().0); + fs::write(pub_file, &mismatched_key).unwrap(); assert!(gen_sk(0).is_err()); - let private_key = std::fs::read(sk_file).unwrap(); - std::fs::remove_dir(pub_file).unwrap(); + assert!(!Path::new(sk_file).exists()); + assert_eq!(fs::read_to_string(pub_file).unwrap(), mismatched_key); + fs::remove_file(pub_file).unwrap(); let public_key = gen_sk(0).unwrap().0; - assert_eq!(std::fs::read_to_string(pub_file).unwrap(), public_key); + let private_key = fs::read(sk_file).unwrap(); + fs::remove_file(pub_file).unwrap(); + fs::create_dir(pub_file).unwrap(); + assert!(gen_sk(0).is_err()); + fs::remove_dir(pub_file).unwrap(); + assert_eq!(gen_sk(0).unwrap().0, public_key); + assert_eq!(fs::read_to_string(pub_file).unwrap(), public_key); #[cfg(target_os = "linux")] assert_public_key_write_failure(&public_key); let path = Path::new(pub_file); @@ -635,26 +664,19 @@ mod tests { file.write_all(pk.as_bytes()).unwrap(); file.into_temp_path() }; - std::fs::remove_file(path).unwrap(); + fs::remove_file(path).unwrap(); let pending = prepare(&public_key); gen_sk(0).unwrap(); - let inode = std::fs::metadata(path).unwrap().ino(); + let inode = fs::metadata(path).unwrap().ino(); publish_public_key(pending, path, &public_key).unwrap(); - let mismatched_key = base64::encode(sign::gen_keypair().0); assert!(publish_public_key(prepare(&mismatched_key), path, &mismatched_key).is_err()); - assert_eq!(std::fs::metadata(path).unwrap().ino(), inode); - assert_eq!(std::fs::read_to_string(path).unwrap(), public_key); - std::fs::write(pub_file, &mismatched_key).unwrap(); + assert_eq!(fs::metadata(path).unwrap().ino(), inode); + assert_eq!(fs::read_to_string(path).unwrap(), public_key); + fs::write(pub_file, &mismatched_key).unwrap(); assert!(gen_sk(0).is_err()); - assert!(std::fs::read(sk_file).unwrap() == private_key); - assert_eq!(std::fs::read_to_string(pub_file).unwrap(), mismatched_key); - std::fs::write(pub_file, &public_key).unwrap(); - let permissions = std::fs::metadata(pub_file).unwrap().permissions(); - let mut read_only = permissions.clone(); - read_only.set_readonly(true); - std::fs::set_permissions(pub_file, read_only).unwrap(); - assert_eq!(gen_sk(0).unwrap().0, public_key); - std::fs::set_permissions(pub_file, permissions).unwrap(); + assert!(fs::read(sk_file).unwrap() == private_key); + assert_eq!(fs::read_to_string(pub_file).unwrap(), mismatched_key); + fs::write(pub_file, &public_key).unwrap(); public_key } @@ -667,50 +689,47 @@ mod tests { const EXISTING_MODE: u32 = 0o644; const READ_ONLY_MODE: u32 = 0o400; const RESTRICTIVE_UMASK: hbb_common::libc::mode_t = 0o777; - let directory = - std::env::temp_dir().join(format!("rustdesk-private-key-{}", uuid::Uuid::new_v4())); - std::fs::create_dir(&directory).unwrap(); - std::env::set_current_dir(&directory).unwrap(); - let path = std::path::Path::new("id_ed25519"); - let mode = || std::fs::metadata(path).unwrap().permissions().mode() & PERMISSION_BITS; - + let directory = tempfile::tempdir().unwrap(); + std::env::set_current_dir(directory.path()).unwrap(); + let path = Path::new("id_ed25519"); + let mode = || fs::metadata(path).unwrap().permissions().mode() & PERMISSION_BITS; + symlink("key-alias", path).unwrap(); + symlink("key-target", "key-alias").unwrap(); + #[cfg(target_os = "linux")] + { + const PARTIAL_WRITE_LIMIT: hbb_common::libc::rlim_t = 16; + for limit in [0, PARTIAL_WRITE_LIMIT] { + assert!(with_file_size_limit(limit, || gen_sk(0)).is_err()); + assert!(path.is_symlink() && Path::new("key-alias").is_symlink()); + assert!(!Path::new("key-target").exists()); + assert!(!Path::new("id_ed25519.pub").exists()); + } + } let public_key = assert_public_key_recovery(); assert_eq!(mode(), NEW_FILE_MODE); - let contents = std::fs::read(path).unwrap(); - std::fs::set_permissions(path, std::fs::Permissions::from_mode(EXISTING_MODE)).unwrap(); - let file = std::fs::File::open(path).unwrap(); + let contents = fs::read(path).unwrap(); + fs::set_permissions(path, fs::Permissions::from_mode(EXISTING_MODE)).unwrap(); + let file = fs::File::open(path).unwrap(); assert!(set_private_key_permissions(&file, true).is_err()); assert_eq!(mode(), EXISTING_MODE); gen_sk(0).unwrap(); assert_eq!(mode(), NEW_FILE_MODE); - assert_eq!(std::fs::read(path).unwrap(), contents); - assert!(create_private_key_file("id_ed25519").is_err()); - assert_eq!(std::fs::read(path).unwrap(), contents); - std::fs::set_permissions(path, std::fs::Permissions::from_mode(READ_ONLY_MODE)).unwrap(); + assert_eq!(fs::read(path).unwrap(), contents); + assert!(create_private_key_file("id_ed25519", b"do not overwrite").is_err()); + assert_eq!(fs::read(path).unwrap(), contents); + fs::set_permissions(path, fs::Permissions::from_mode(READ_ONLY_MODE)).unwrap(); + fs::set_permissions("id_ed25519.pub", fs::Permissions::from_mode(READ_ONLY_MODE)).unwrap(); assert_eq!(gen_sk(0).unwrap().0, public_key); assert_eq!(mode(), READ_ONLY_MODE); - std::fs::remove_file(path).unwrap(); - symlink("missing/key", path).unwrap(); - assert!(gen_sk(0).is_err()); - assert!(std::fs::read_to_string("id_ed25519.pub").unwrap() == public_key); - std::fs::remove_file(path).unwrap(); - std::fs::remove_file("id_ed25519.pub").unwrap(); - symlink("key-alias", path).unwrap(); - symlink("key-target", "key-alias").unwrap(); - gen_sk(0).unwrap(); - assert_eq!(mode(), NEW_FILE_MODE); - assert!(!std::fs::read("key-target").unwrap().is_empty()); - - std::fs::remove_file(path).unwrap(); - std::fs::remove_file("id_ed25519.pub").unwrap(); + fs::remove_file(path).unwrap(); + fs::remove_file("id_ed25519.pub").unwrap(); unsafe { hbb_common::libc::umask(RESTRICTIVE_UMASK) }; gen_sk(0).unwrap(); assert_eq!(mode(), NEW_FILE_MODE); - let contents = std::fs::read(path).unwrap(); + let contents = fs::read(path).unwrap(); gen_sk(0).unwrap(); - assert_eq!(std::fs::read(path).unwrap(), contents); - std::env::set_current_dir(directory.parent().unwrap()).unwrap(); - std::fs::remove_dir_all(directory).unwrap(); + assert_eq!(fs::read(path).unwrap(), contents); + std::env::set_current_dir(directory.path().parent().unwrap()).unwrap(); } } From 545c7d63cfcb1896b32b833210f2dbdeff5890a0 Mon Sep 17 00:00:00 2001 From: fufesou Date: Sat, 19 Sep 2026 19:42:38 +0800 Subject: [PATCH 8/8] fix: key file perm, simple refactor Signed-off-by: fufesou --- src/common.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/common.rs b/src/common.rs index 5497ced73..82e32ac0f 100644 --- a/src/common.rs +++ b/src/common.rs @@ -202,16 +202,13 @@ const MAX_KEY_SYMLINKS: usize = 40; fn resolve_key_path(path: &Path) -> ResultType { let mut path = path.to_path_buf(); // Bound traversal even if symlinks change while they are being resolved. - for hops in 0..=MAX_KEY_SYMLINKS { + for _ in 0..MAX_KEY_SYMLINKS { match fs::symlink_metadata(&path) { Ok(metadata) if metadata.file_type().is_symlink() => {} Ok(_) => return Ok(path), Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(path), Err(err) => return Err(err.into()), } - if hops == MAX_KEY_SYMLINKS { - break; - } let target = fs::read_link(&path)?; path.pop(); path.push(target); @@ -557,7 +554,7 @@ mod tests { let directory = tempfile::tempdir().unwrap(); let target = directory.path().join("target"); let mut path = target.clone(); - for index in 0..MAX_KEY_SYMLINKS { + for index in 1..MAX_KEY_SYMLINKS { let link = directory.path().join(format!("link-{index}")); symlink(&path, &link).unwrap(); path = link;