fix: race condition in private key creation (#4673)

Author: LesnyRumcajs

Cargo.lock

@@ -88,7 +88,6 @@ jsonrpsee = { version = "0.24", features = ["server", "ws-client", "http-client"
 jsonwebtoken = "9"
 keccak-hash = "0.10"
 kubert-prometheus-process = "0.1"
-libc = "0.2"
 libipld = { version = "0.16", default-features = false, features = ["dag-cbor", "dag-json", "derive", "serde-codec"] }
 libipld-core = { version = "0.16", features = ['arb', 'serde-codec'] }
 libipld-macro = "0.16"

Cargo.toml

@@ -3,12 +3,15 @@
 
 use std::{
     fmt::Display,
-    fs::{self, create_dir, File},
+    fs::{create_dir, File},
     io::{BufReader, BufWriter, ErrorKind, Read, Write},
     path::{Path, PathBuf},
 };
 
-use crate::{shim::crypto::SignatureType, utils::encoding::from_slice_with_fallback};
+use crate::{
+    shim::crypto::SignatureType,
+    utils::{encoding::from_slice_with_fallback, io::create_new_sensitive_file},
+};
 use ahash::{HashMap, HashMapExt};
 use argon2::{
     password_hash::SaltString, Argon2, ParamsBuilder, PasswordHasher, RECOMMENDED_SALT_LEN,
@@ -269,16 +272,7 @@ impl KeyStore {
     pub fn flush(&self) -> anyhow::Result<()> {
         match &self.persistence {
             Some(persistent_keystore) => {
-                let dir = persistent_keystore
-                    .file_path
-                    .parent()
-                    .ok_or_else(|| Error::Other("Invalid Path".to_string()))?;
-                fs::create_dir_all(dir)?;
-                let file = File::create(&persistent_keystore.file_path)?;
-
-                // Restrict permissions on files containing private keys
-                #[cfg(unix)]
-                crate::utils::io::set_user_perm(&file)?;
+                let file = create_new_sensitive_file(&persistent_keystore.file_path)?;
 
                 let mut writer = BufWriter::new(file);
 

src/key_management/keystore.rs

@@ -3,7 +3,7 @@
 
 use tracing::{debug, info, trace};
 
-use crate::{libp2p::Keypair, utils::io::write_to_file};
+use crate::{libp2p::Keypair, utils::io::write_new_sensitive_file};
 use std::{fs, path::Path};
 
 const KEYPAIR_FILE: &str = "keypair";
@@ -29,12 +29,10 @@ fn create_and_save_keypair(path: &Path) -> anyhow::Result<Keypair> {
         backup_path.set_extension("bak");
 
         info!("Backing up existing keypair to {}", backup_path.display());
-        fs::rename(keypair_path, &backup_path)?;
+        fs::rename(&keypair_path, &backup_path)?;
     }
 
-    let file = write_to_file(&gen_keypair.to_bytes(), path, KEYPAIR_FILE)?;
-    // Restrict permissions on files containing private keys
-    crate::utils::io::set_user_perm(&file)?;
+    write_new_sensitive_file(&gen_keypair.to_bytes(), &keypair_path)?;
 
     Ok(gen_keypair.into())
 }

src/libp2p/keypair.rs

@@ -6,45 +6,41 @@ pub mod progress_log;
 mod writer_checksum;
 
 use std::{
-    fs::{create_dir_all, File},
-    io::{prelude::*, Result},
+    fs::File,
+    io::{self, prelude::*, Result},
+    os::unix::fs::OpenOptionsExt,
     path::Path,
 };
 
 pub use mmap::EitherMmapOrRandomAccessFile;
 pub use progress_log::{WithProgress, WithProgressRaw};
 pub use writer_checksum::*;
 
-/// Restricts permissions on a file to user-only: 0600
-#[cfg(unix)]
-pub fn set_user_perm(file: &File) -> Result<()> {
-    use std::os::unix::fs::PermissionsExt;
-
-    use tracing::info;
-
-    let mut perm = file.metadata()?.permissions();
-    #[allow(clippy::useless_conversion)] // Otherwise it does not build on macos
-    perm.set_mode((libc::S_IWUSR | libc::S_IRUSR).into());
-    file.set_permissions(perm)?;
-
-    info!("Permissions set to 0600 on {:?}", file);
-
-    Ok(())
+/// Writes bytes to a specified file. Creates the desired path if it does not
+/// exist.
+/// Note: The file is created with permissions 0600.
+/// Note: The file is truncated if it already exists.
+pub fn write_new_sensitive_file(message: &[u8], path: &Path) -> Result<()> {
+    create_new_sensitive_file(path)?.write_all(message)
 }
 
-#[cfg(not(unix))]
-pub fn set_user_perm(file: &File) -> Result<()> {
-    Ok(())
-}
+/// Creates a new file with the specified path. The file is created
+/// with permissions 0600 and is truncated if it already exists.
+pub fn create_new_sensitive_file(path: &Path) -> Result<File> {
+    std::fs::create_dir_all(
+        path.parent()
+            .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, "Parent directory not found"))?,
+    )?;
 
-/// Writes a string to a specified file. Creates the desired path if it does not
-/// exist. Note: `path` and `filename` are appended to produce the resulting
-/// file path.
-pub fn write_to_file(message: &[u8], path: &Path, file_name: &str) -> Result<File> {
-    // Create path if it doesn't exist
-    create_dir_all(path)?;
-    let mut file = File::create(path.join(file_name))?;
-    file.write_all(message)?;
+    let file = std::fs::OpenOptions::new()
+        .mode(0o600)
+        .write(true)
+        .create(true)
+        .truncate(true)
+        .open(path)?;
+
+    use std::os::unix::fs::PermissionsExt;
+    file.set_permissions(std::fs::Permissions::from_mode(0o600))?;
     Ok(file)
 }
 

src/utils/io/mod.rs

@@ -6,7 +6,7 @@ use std::{
     path::{Path, PathBuf},
 };
 
-use crate::utils::io::{read_toml, write_to_file};
+use crate::utils::io::{read_toml, write_new_sensitive_file};
 use serde::Deserialize;
 
 // Please use with caution, remove_dir_all will completely delete a directory
@@ -20,7 +20,7 @@ fn write_to_file_to_path() {
     let path = PathBuf::from("./test-write");
     let file = "test.txt";
 
-    match write_to_file(msg, &path, file) {
+    match write_new_sensitive_file(msg, &path.join(file)) {
         Ok(_) => cleanup_file(&path),
         Err(e) => {
             cleanup_file(&path);
@@ -34,7 +34,7 @@ fn write_to_file_nested_dir() {
     let msg = "Hello World!".as_bytes();
     let root = PathBuf::from("./test_missing");
 
-    match write_to_file(msg, &root.join("test_write_string"), "test-file") {
+    match write_new_sensitive_file(msg, &root.join("test_write_string").join("test-file")) {
         Ok(_) => cleanup_file(&root),
         Err(e) => {
             cleanup_file(&root);
@@ -48,7 +48,7 @@ fn read_from_file_vec() {
     let msg = "Hello World!".as_bytes();
     let path = PathBuf::from("./test_read_file");
     let file_name = "out.keystore";
-    write_to_file(msg, &path, file_name).unwrap();
+    write_new_sensitive_file(msg, &path.join(file_name)).unwrap();
 
     match std::fs::read(path.join(file_name)) {
         Ok(contents) => {
@@ -68,7 +68,7 @@ fn read_from_file_string() {
     let path = PathBuf::from("./test_string_read_file");
     let file_name = "out.keystore";
 
-    write_to_file(msg.as_bytes(), &path, file_name).unwrap();
+    write_new_sensitive_file(msg.as_bytes(), &path.join(file_name)).unwrap();
     match std::fs::read_to_string(path.join(file_name)) {
         Ok(contents) => {
             cleanup_file(&path);