fix(security): eliminate TOCTOU race condition in atomic writes (#500)

- Use tempfile::NamedTempFile for synchronous atomic_write to ensure 0600 permissions at creation.
- Use tokio::fs::OpenOptions with mode(0o600) for asynchronous atomic_write_async.
- Remove redundant set_permissions calls in oauth_config.rs and credential_store.rs.
- Ensure tempfile is a regular dependency (was dev-dependency).
- Add tests to verify file permissions on Unix systems.

Fixes #401

.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.

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"

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<PathBuf> {
     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)
 }
 

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));
+    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))?;
 
-    std::fs::write(&tmp_path, data)?;
-    std::fs::rename(&tmp_path, path)?;
     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));
+        .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "path has no file name"))?
+        .to_string_lossy();
 
-    tokio::fs::write(&tmp_path, data).await?;
-    tokio::fs::rename(&tmp_path, path).await?;
-    Ok(())
+    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);
+        }
     }
 }

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)
 }