diff --git a/.changeset/fix-security-toctou.md b/.changeset/fix-security-toctou.md new file mode 100644 index 00000000..3cf7d3cc --- /dev/null +++ b/.changeset/fix-security-toctou.md @@ -0,0 +1,11 @@ +--- +"@googleworkspace/cli": patch +--- + +Fix critical security vulnerability (TOCTOU/Symlink race) in atomic file writes. + +The atomic_write and atomic_write_async utilities now use: +- Randomized temporary filenames to prevent predictability. +- O_EXCL creation flags to prevent following pre-existing symlinks. +- Strict 0600 permissions from the moment of file creation on Unix systems. +- Redundant post-write permission calls have been removed to close race windows. diff --git a/Cargo.toml b/Cargo.toml index 24bc253b..5be656ee 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,6 +32,7 @@ path = "src/main.rs" [dependencies] +tempfile = "3" aes-gcm = "0.10" anyhow = "1" clap = { version = "4", features = ["derive", "string"] } @@ -81,4 +82,3 @@ lto = "thin" [dev-dependencies] serial_test = "3.4.0" -tempfile = "3" diff --git a/src/credential_store.rs b/src/credential_store.rs index e985b76b..075a9765 100644 --- a/src/credential_store.rs +++ b/src/credential_store.rs @@ -61,29 +61,11 @@ fn save_key_file_exclusive(path: &std::path::Path, b64_key: &str) -> std::io::Re /// Persist the base64-encoded encryption key to a local file with restrictive /// permissions (0600 file, 0700 directory). Overwrites any existing file. +/// Persist the base64-encoded encryption key to a local file with restrictive +/// permissions. Uses atomic_write to prevent TOCTOU/symlink race conditions. fn save_key_file(path: &std::path::Path, b64_key: &str) -> std::io::Result<()> { - use std::io::Write; - ensure_key_dir(path)?; - - #[cfg(unix)] - { - use std::os::unix::fs::OpenOptionsExt; - let mut options = std::fs::OpenOptions::new(); - options.write(true).create(true).truncate(true).mode(0o600); - let mut file = options.open(path)?; - file.write_all(b64_key.as_bytes())?; - file.sync_all()?; // fsync: ensure key is durable before returning - } - #[cfg(not(unix))] - { - std::fs::write(path, b64_key)?; - } - Ok(()) + crate::fs_util::atomic_write(path, b64_key.as_bytes()) } - -/// Read and decode a base64-encoded 256-bit key from a file. -/// -/// On Unix, warns if the file is world-readable (mode & 0o077 != 0). fn read_key_file(path: &std::path::Path) -> Option<[u8; 32]> { use base64::{engine::general_purpose::STANDARD, Engine as _}; @@ -400,18 +382,6 @@ pub fn save_encrypted(json: &str) -> anyhow::Result { crate::fs_util::atomic_write(&path, &encrypted) .map_err(|e| anyhow::anyhow!("Failed to write credentials: {e}"))?; - // Set permissions to 600 on Unix (contains secrets) - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - if let Err(e) = std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) { - eprintln!( - "Warning: failed to set file permissions on {}: {e}", - path.display() - ); - } - } - Ok(path) } diff --git a/src/fs_util.rs b/src/fs_util.rs index b387565e..fab255b0 100644 --- a/src/fs_util.rs +++ b/src/fs_util.rs @@ -14,51 +14,96 @@ //! File-system utilities. -use std::io; +use std::io::{self, Write}; use std::path::Path; /// Write `data` to `path` atomically. /// -/// The data is first written to a temporary file alongside the target -/// (e.g. `credentials.enc.tmp`) and then renamed into place. On POSIX -/// systems `rename(2)` is atomic with respect to crashes, so a reader of -/// `path` will always see either the old or the new content — never a -/// partially-written file. +/// This implementation uses `tempfile::NamedTempFile` to create a temporary +/// file with a random name, `O_EXCL` flags (preventing symlink attacks), +/// and secure 0600 permissions from the moment of creation. /// /// # Errors /// -/// Returns an `io::Error` if the temporary file cannot be written or if the -/// rename fails. +/// Returns an `io::Error` if the temporary file cannot be created/written or if the +/// final rename fails. pub fn atomic_write(path: &Path, data: &[u8]) -> io::Result<()> { - // Derive a sibling tmp path, e.g. `/home/user/.config/gws/credentials.enc.tmp` - let file_name = path - .file_name() - .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "path has no file name"))?; - let tmp_name = format!("{}.tmp", file_name.to_string_lossy()); - let tmp_path = path - .parent() - .map(|p| p.join(&tmp_name)) - .unwrap_or_else(|| std::path::PathBuf::from(&tmp_name)); - - std::fs::write(&tmp_path, data)?; - std::fs::rename(&tmp_path, path)?; + let parent = path.parent().ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidInput, "path has no parent directory") + })?; + + let mut tmp = tempfile::NamedTempFile::new_in(parent)?; + tmp.write_all(data)?; + tmp.as_file().sync_all()?; + tmp.persist(path) + .map_err(|e| io::Error::new(e.error.kind(), e.error))?; + Ok(()) } /// Async variant of [`atomic_write`] for use with tokio. +/// +/// This implementation uses `create_new(true)` (O_EXCL) and `mode(0o600)` to +/// prevent TOCTOU/symlink race conditions. pub async fn atomic_write_async(path: &Path, data: &[u8]) -> io::Result<()> { + use rand::Rng; + use tokio::io::AsyncWriteExt; + + let parent = path.parent().ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidInput, "path has no parent directory") + })?; let file_name = path .file_name() - .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "path has no file name"))?; - let tmp_name = format!("{}.tmp", file_name.to_string_lossy()); - let tmp_path = path - .parent() - .map(|p| p.join(&tmp_name)) - .unwrap_or_else(|| std::path::PathBuf::from(&tmp_name)); - - tokio::fs::write(&tmp_path, data).await?; - tokio::fs::rename(&tmp_path, path).await?; - Ok(()) + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "path has no file name"))? + .to_string_lossy(); + + let mut retries = 0; + let mut file: tokio::fs::File; + let mut tmp_path; + + loop { + let suffix: String = rand::thread_rng() + .sample_iter(&rand::distributions::Alphanumeric) + .take(8) + .map(char::from) + .collect(); + let tmp_name = format!("{}.tmp.{}", file_name, suffix); + tmp_path = parent.join(tmp_name); + + let mut opts = tokio::fs::OpenOptions::new(); + opts.write(true).create_new(true); + + #[cfg(unix)] + { + opts.mode(0o600); + } + + match opts.open(&tmp_path).await { + Ok(f) => { + file = f; + break; + } + Err(e) if e.kind() == io::ErrorKind::AlreadyExists && retries < 10 => { + retries += 1; + continue; + } + Err(e) => return Err(e), + } + } + + let write_result = async { + file.write_all(data).await?; + file.sync_all().await?; + drop(file); + tokio::fs::rename(&tmp_path, path).await + } + .await; + + if write_result.is_err() { + let _ = tokio::fs::remove_file(&tmp_path).await; + } + + write_result } #[cfg(test)] @@ -72,6 +117,13 @@ mod tests { let path = dir.path().join("credentials.enc"); atomic_write(&path, b"hello").unwrap(); assert_eq!(fs::read(&path).unwrap(), b"hello"); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let meta = fs::metadata(&path).unwrap(); + assert_eq!(meta.permissions().mode() & 0o777, 0o600); + } } #[test] @@ -88,8 +140,13 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("credentials.enc"); atomic_write(&path, b"data").unwrap(); - let tmp = dir.path().join("credentials.enc.tmp"); - assert!(!tmp.exists(), "tmp file should be cleaned up by rename"); + // Since we use random names, we just check that no .tmp files remain in the dir + let files: Vec<_> = fs::read_dir(dir.path()) + .unwrap() + .map(|res| res.unwrap().file_name()) + .collect(); + assert_eq!(files.len(), 1); + assert_eq!(files[0], "credentials.enc"); } #[tokio::test] @@ -98,5 +155,12 @@ mod tests { let path = dir.path().join("token_cache.json"); atomic_write_async(&path, b"async hello").await.unwrap(); assert_eq!(fs::read(&path).unwrap(), b"async hello"); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let meta = fs::metadata(&path).unwrap(); + assert_eq!(meta.permissions().mode() & 0o777, 0o600); + } } } diff --git a/src/oauth_config.rs b/src/oauth_config.rs index 02154b58..89f34535 100644 --- a/src/oauth_config.rs +++ b/src/oauth_config.rs @@ -84,13 +84,6 @@ pub fn save_client_config( crate::fs_util::atomic_write(&path, json.as_bytes()) .map_err(|e| anyhow::anyhow!("Failed to write client config: {e}"))?; - // Set file permissions to 600 on Unix (contains secrets) - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600))?; - } - Ok(path) }