Skip to content

🔒 [security] Secure atomic file creation with restrictive permissions #391

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jpoehnelt wants to merge 1 commit into
base: main
Choose a base branch
from
+65 −2
Open

🔒 [security] Secure atomic file creation with restrictive permissions #391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/security-fix-atomic-write-permissions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@googleworkspace/cli": patch
---

🔒 Fix: Use restrictive permissions (0600) for temporary files created during atomic writes in fs_util.
62 changes: 60 additions & 2 deletions src/fs_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,21 @@ pub fn atomic_write(path: &Path, data: &[u8]) -> io::Result<()> {
.map(|p| p.join(&tmp_name))
.unwrap_or_else(|| std::path::PathBuf::from(&tmp_name));

std::fs::write(&tmp_path, data)?;
{
use std::fs::OpenOptions;
use std::io::Write;
let mut opts = OpenOptions::new();
opts.write(true).create(true).truncate(true);
#[cfg(unix)]
{
use std::os::unix::fs::OpenOptionsExt;
opts.mode(0o600);
}
let mut file = opts.open(&tmp_path)?;
file.write_all(data)?;
file.sync_all()?;
}
Comment on lines +43 to +56
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The implementation of atomic writes uses a predictable temporary filename (<filename>.tmp) and opens it without the O_EXCL flag (via create_new(true)). This presents two security risks:

  1. Symlink Attack: An attacker can create a symbolic link at the predictable temporary path, causing the application to follow the link and write to an arbitrary file.
  2. Permission Bypass: If an attacker pre-creates the temporary file with permissive permissions (e.g., 0666), the application will reuse the existing file and its permissive permissions, allowing the attacker to read sensitive data. This undermines the security fix intended by this PR.

To remediate this, use a unique, non-predictable temporary filename and ensure it is created exclusively. The recommended approach is to use the tempfile crate's NamedTempFile or Builder to create a temporary file with a random suffix in the target directory. If a predictable name must be used, ensure create_new(true) is used and handle the case where the file already exists by either failing or using a different name. Alternatively, you could generate a semi-random name, for example by including a timestamp and process ID:

use std::time::{SystemTime, UNIX_EPOCH};

let pid = std::process::id();
let rand_part = SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_nanos();
let tmp_name = format!("{}.{}-{}.tmp", file_name.to_string_lossy(), pid, rand_part);

This change should be applied where tmp_name is defined.


std::fs::rename(&tmp_path, path)?;
Ok(())
}
Expand All @@ -56,7 +70,21 @@ pub async fn atomic_write_async(path: &Path, data: &[u8]) -> io::Result<()> {
.map(|p| p.join(&tmp_name))
.unwrap_or_else(|| std::path::PathBuf::from(&tmp_name));

tokio::fs::write(&tmp_path, data).await?;
{
use tokio::fs::OpenOptions;
use tokio::io::AsyncWriteExt;
let mut opts = OpenOptions::new();
opts.write(true).create(true).truncate(true);
#[cfg(unix)]
{
use tokio::os::unix::fs::OpenOptionsExt;
opts.mode(0o600);
}
let mut file = opts.open(&tmp_path).await?;
file.write_all(data).await?;
file.sync_all().await?;
}
Comment on lines +73 to +86
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The implementation of atomic writes uses a predictable temporary filename (<filename>.tmp) and opens it without the O_EXCL flag (via create_new(true)). This presents two security risks:

  1. Symlink Attack: An attacker can create a symbolic link at the predictable temporary path, causing the application to follow the link and write to an arbitrary file.
  2. Permission Bypass: If an attacker pre-creates the temporary file with permissive permissions (e.g., 0666), the application will reuse the existing file and its permissive permissions, allowing the attacker to read sensitive data. This undermines the security fix intended by this PR.

To remediate this, use a unique, non-predictable temporary filename and ensure it is created exclusively. The recommended approach is to use the tempfile crate's NamedTempFile or Builder to create a temporary file with a random suffix in the target directory. If a predictable name must be used, ensure create_new(true) is used and handle the case where the file already exists by either failing or using a different name. For asynchronous functions, while the tempfile crate is ideal, its primary API is synchronous and might require tokio::task::spawn_blocking for integration. Alternatively, you could generate a semi-random name, for example by including a timestamp and process ID:

use std::time::{SystemTime, UNIX_EPOCH};

let pid = std::process::id();
let rand_part = SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_nanos();
let tmp_name = format!("{}.{}-{}.tmp", file_name.to_string_lossy(), pid, rand_part);

This change should be applied where tmp_name is defined.


tokio::fs::rename(&tmp_path, path).await?;
Ok(())
}
Expand Down Expand Up @@ -99,4 +127,34 @@ mod tests {
atomic_write_async(&path, b"async hello").await.unwrap();
assert_eq!(fs::read(&path).unwrap(), b"async hello");
}

#[tokio::test]
async fn test_atomic_write_permissions() {
#[cfg(unix)]
{
use std::os::unix::fs::PermissionsExt;

let dir = tempfile::tempdir().unwrap();

// Sync
let path_sync = dir.path().join("sync.txt");
atomic_write(&path_sync, b"sync").unwrap();
let meta_sync = fs::metadata(&path_sync).unwrap();
assert_eq!(
meta_sync.permissions().mode() & 0o777,
0o600,
"Sync atomic_write should create file with 0600 permissions"
);

// Async
let path_async = dir.path().join("async.txt");
atomic_write_async(&path_async, b"async").await.unwrap();
let meta_async = fs::metadata(&path_async).unwrap();
assert_eq!(
meta_async.permissions().mode() & 0o777,
0o600,
"Async atomic_write_async should create file with 0600 permissions"
);
}
}
}
Loading