Conversation
This commit addresses the issues raised regarding the Time-of-Check to Time-of-Use (TOCTOU) race conditions during file creation, while restoring the necessary defense-in-depth permissions logic...
🦋 Changeset detectedLatest commit: e0ab14b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
googleworkspace-bot
added
area: distribution
area: core
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security of file writing operations by addressing a critical Time-of-Check to Time-of-Use (TOCTOU) race condition in the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a TOCTOU race condition in fs_util::atomic_write and fs_util::atomic_write_async to improve security when writing files atomically. The changes for the synchronous atomic_write function correctly leverage the tempfile crate. However, the manual implementation for the asynchronous atomic_write_async introduces a blocking I/O call in a Drop implementation, which can cause issues in an async runtime. Furthermore, a build log file (check.txt) containing local user paths seems to have been committed by mistake and should be removed.
src/fs_util.rs
Outdated
| impl Drop for CleanupTmp { | ||
| fn drop(&mut self) { | ||
| let _ = std::fs::remove_file(&self.0); | ||
| } |
There was a problem hiding this comment.
Using a blocking file system operation (std::fs::remove_file) within a Drop implementation in an async context is problematic. It can block the executor's thread, leading to performance issues or even deadlocks in the async runtime. While file deletion is often fast, it's not guaranteed, especially on networked or busy filesystems.
Please consider using an async-aware cleanup mechanism. One option is to refactor atomic_write_async to explicitly handle cleanup on error paths using tokio::fs::remove_file, even if it's more verbose. Another option could be to investigate async-native tempfile crates.
Using a blocking Drop implementation in an async context (for temporary file cleanup) stalls the tokio executor thread. This commit refactors the function to explicitly handle error paths using tokio::fs::remove_file which properly awaits file unlinking.
|
/gemini review |
1 similar comment
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a Time-of-check to time-of-use (TOCTOU) vulnerability in atomic_write and atomic_write_async by ensuring temporary files are created with secure permissions (0600) and unpredictable names. The synchronous version atomic_write is correctly updated using the tempfile crate.
However, the asynchronous version atomic_write_async introduces a couple of issues. It performs blocking file I/O for cleanup in an async context, and it uses a very inefficient method for generating random filenames. My review includes a suggestion to refactor atomic_write_async to resolve these issues by using non-blocking cleanup and a more efficient random string generation method.
src/fs_util.rs
Outdated
|
|
||
| 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 tmp_name = format!("{}.{}.tmp", file_name.to_string_lossy(), suffix); | ||
| let tmp_path = parent.join(&tmp_name); | ||
|
|
||
| tokio::fs::write(&tmp_path, data).await?; | ||
| tokio::fs::rename(&tmp_path, path).await?; | ||
| Ok(()) | ||
| let write_result = async { | ||
| use tokio::io::AsyncWriteExt; | ||
| let mut opts = tokio::fs::OpenOptions::new(); | ||
| opts.write(true).create_new(true); // O_EXCL to prevent TOCTOU | ||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::OpenOptionsExt; | ||
| opts.mode(0o600); // Only applies to new files (which this is, due to create_new) | ||
| } | ||
| let mut file = opts.open(&tmp_path).await?; | ||
| file.write_all(data).await?; | ||
| file.sync_all().await?; | ||
| Ok::<(), io::Error>(()) | ||
| }.await; | ||
|
|
||
| if write_result.is_err() { | ||
| let _ = tokio::fs::remove_file(&tmp_path).await; | ||
| return write_result; | ||
| } | ||
|
|
||
| match tokio::fs::rename(&tmp_path, path).await { | ||
| Ok(()) => Ok(()), | ||
| Err(e) => { | ||
| let _ = tokio::fs::remove_file(&tmp_path).await; | ||
| Err(e) | ||
| } | ||
| } |
There was a problem hiding this comment.
This implementation of atomic_write_async has a couple of issues:
-
Blocking I/O in async context: The
CleanupTmpstruct'sDropimplementation usesstd::fs::remove_file, a blocking call. In an async function, this can block the executor's thread, causing performance problems. Cleanup should be done asynchronously usingtokio::fs::remove_file. -
Inefficient random string generation: The method used to generate a random suffix is very inefficient.
rand::random::<char>()is very unlikely to produce an ASCII alphanumeric character, causing the.filter()to loop many times. Usingrand::distributions::Alphanumericis much more efficient.
I suggest refactoring the function to address both points. This will also make the CleanupTmp struct (lines 48-54) unnecessary, so it can be removed.
pub async fn atomic_write_async(path: &Path, data: &[u8]) -> io::Result<()> {
let parent = path.parent().unwrap_or_else(|| std::path::Path::new(""));
// Generate a random suffix for the temporary file
use rand::Rng;
let suffix: String = rand::thread_rng()
.sample_iter(&rand::distributions::Alphanumeric)
.take(8)
.map(char::from)
.collect();
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(), suffix);
let tmp_path = parent.join(&tmp_name);
let write_and_rename = async {
use tokio::io::AsyncWriteExt;
let mut opts = tokio::fs::OpenOptions::new();
opts.write(true).create_new(true); // O_EXCL to prevent TOCTOU
#[cfg(unix)]
{
use std::os::unix::fs::OpenOptionsExt;
opts.mode(0o600); // Only applies to new files (which this is, due to create_new)
}
let mut file = opts.open(&tmp_path).await?;
file.write_all(data).await?;
file.sync_all().await?;
tokio::fs::rename(&tmp_path, path).await
};
match write_and_rename.await {
Ok(_) => Ok(()),
Err(e) => {
// On any error, try to clean up the temp file.
let _ = tokio::fs::remove_file(&tmp_path).await;
Err(e)
}
}
}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a Time-of-check to time-of-use (TOCTOU) vulnerability in the atomic_write and atomic_write_async functions. The use of the tempfile crate for the synchronous version is a solid approach. For the asynchronous version, the manual implementation correctly uses create_new(true) and sets file permissions upon creation to prevent race conditions.
Using rand::random::<char>() over thousands of scalar iterations to find 8 ASCII alphanumeric characters is heavily inefficient. This commit refactors the atomic_write_async string generator to use rand::distributions::Alphanumeric directly for optimal sampling.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a Time-of-check to time-of-use (TOCTOU) race condition in atomic_write and atomic_write_async to ensure file permissions are set securely on creation. The synchronous atomic_write function is correctly updated to use the tempfile crate, which is a great improvement. For the asynchronous atomic_write_async function, I've suggested a refactoring to reduce complexity and reuse the logic from the synchronous version, improving maintainability.
Additionally, the change to using randomized temporary file names in atomic_write will likely cause the test_atomic_write_leaves_no_tmp_file test to fail or pass for the wrong reasons. Please update the tests to correctly verify that no temporary files are left behind by the new implementation.
src/fs_util.rs
Outdated
| let parent = path.parent().unwrap_or_else(|| std::path::Path::new("")); | ||
|
|
||
| // Generate a random suffix for the temporary file | ||
| use rand::Rng; | ||
| let suffix: String = rand::thread_rng() | ||
| .sample_iter(&rand::distributions::Alphanumeric) | ||
| .take(8) | ||
| .map(char::from) | ||
| .collect(); | ||
|
|
||
| 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 tmp_name = format!("{}.{}.tmp", file_name.to_string_lossy(), suffix); | ||
| let tmp_path = parent.join(&tmp_name); | ||
|
|
||
| tokio::fs::write(&tmp_path, data).await?; | ||
| tokio::fs::rename(&tmp_path, path).await?; | ||
| Ok(()) | ||
| let write_result = async { | ||
| use tokio::io::AsyncWriteExt; | ||
| let mut opts = tokio::fs::OpenOptions::new(); | ||
| opts.write(true).create_new(true); // O_EXCL to prevent TOCTOU | ||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::OpenOptionsExt; | ||
| opts.mode(0o600); // Only applies to new files (which this is, due to create_new) | ||
| } | ||
| let mut file = opts.open(&tmp_path).await?; | ||
| file.write_all(data).await?; | ||
| file.sync_all().await?; | ||
| Ok::<(), io::Error>(()) | ||
| }.await; | ||
|
|
||
| if write_result.is_err() { | ||
| let _ = tokio::fs::remove_file(&tmp_path).await; | ||
| return write_result; | ||
| } | ||
|
|
||
| match tokio::fs::rename(&tmp_path, path).await { | ||
| Ok(()) => Ok(()), | ||
| Err(e) => { | ||
| let _ = tokio::fs::remove_file(&tmp_path).await; | ||
| Err(e) | ||
| } | ||
| } |
There was a problem hiding this comment.
This manual implementation for atomic_write_async is quite complex and duplicates the logic for secure atomic writes that is already handled by the tempfile crate in atomic_write. This increases the maintenance burden and potential for bugs in security-sensitive code.
A simpler and more robust approach is to reuse the synchronous atomic_write function within a tokio::task::spawn_blocking call. This keeps the core logic in one place, reduces code complexity, and ensures both sync and async versions behave identically.
| let parent = path.parent().unwrap_or_else(|| std::path::Path::new("")); | |
| // Generate a random suffix for the temporary file | |
| use rand::Rng; | |
| let suffix: String = rand::thread_rng() | |
| .sample_iter(&rand::distributions::Alphanumeric) | |
| .take(8) | |
| .map(char::from) | |
| .collect(); | |
| 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 tmp_name = format!("{}.{}.tmp", file_name.to_string_lossy(), suffix); | |
| let tmp_path = parent.join(&tmp_name); | |
| tokio::fs::write(&tmp_path, data).await?; | |
| tokio::fs::rename(&tmp_path, path).await?; | |
| Ok(()) | |
| let write_result = async { | |
| use tokio::io::AsyncWriteExt; | |
| let mut opts = tokio::fs::OpenOptions::new(); | |
| opts.write(true).create_new(true); // O_EXCL to prevent TOCTOU | |
| #[cfg(unix)] | |
| { | |
| use std::os::unix::fs::OpenOptionsExt; | |
| opts.mode(0o600); // Only applies to new files (which this is, due to create_new) | |
| } | |
| let mut file = opts.open(&tmp_path).await?; | |
| file.write_all(data).await?; | |
| file.sync_all().await?; | |
| Ok::<(), io::Error>(()) | |
| }.await; | |
| if write_result.is_err() { | |
| let _ = tokio::fs::remove_file(&tmp_path).await; | |
| return write_result; | |
| } | |
| match tokio::fs::rename(&tmp_path, path).await { | |
| Ok(()) => Ok(()), | |
| Err(e) => { | |
| let _ = tokio::fs::remove_file(&tmp_path).await; | |
| Err(e) | |
| } | |
| } | |
| let path = path.to_path_buf(); | |
| let data = data.to_vec(); | |
| tokio::task::spawn_blocking(move || atomic_write(&path, &data)) | |
| .await | |
| .map_err(|e| io::Error::new(io::ErrorKind::Other, e))? |
This refactors atomic_write_async to match the cleaner structure suggested by the Code Assist bot, wrapping the operations in an async block and evaluating the result.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully addresses a Time-of-check to time-of-use (TOCTOU) vulnerability in atomic_write and atomic_write_async by using secure methods for temporary file creation and atomic renaming. The synchronous version correctly leverages the tempfile crate, while the async version implements the necessary security measures manually.
This addresses the bot's suggestion to eliminate duplicate security logic. Rather than manually implementing secure tempfile generation and random naming twice, atomic_write_async now neatly wraps the battle-tested synchronous atomic_write inside tokio::task::spawn_blocking. This reduces cognitive overhead and centralizes security.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a Time-of-Check to Time-of-Use (TOCTOU) race condition in atomic_write and atomic_write_async by leveraging the tempfile crate. This ensures temporary files are created with secure permissions, fixing a potential vulnerability. The synchronous implementation is solid.
alysajad
left a comment
alysajad
left a comment
There was a problem hiding this comment.
fix TOCTOU Vulnerability
This PR explicitly addresses the comments from the repo owner on the rejected PR, fixing the remaining TOCTOU vulnerability in fs_util::atomic_write and restoring defense-in-depth safety nets.
Proposed Changes
Cargo.toml
Move tempfile from [dev-dependencies] to [dependencies]. tempfile::NamedTempFile provides industry standard, secure, and atomic temporary file creation without TOCTOU vulnerabilities by using O_EXCL and randomized filenames under the hood.
src/fs_util.rs
atomic_write
: Refactor to use tempfile::NamedTempFile::new_in. The temporary file will be created in the same directory as the target, written to, and then atomically persisted with persist().
atomic_write_async
: To avoid the missing import issue noted by the maintainer, avoiding complicated async unix extensions, and correctly using random names, generate a random 8-character alphanumeric string for the
tmp
suffix (e.g., credentials.enc.<random_string>.tmp), and use tokio::fs::OpenOptions with .create_new(true). We will also add use std::os::unix::fs::OpenOptionsExt or properly use spawn_blocking with NamedTempFile to ensure permissions compile correctly. (Note: Using spawn_blocking with synchronous NamedTempFile is the safest, most deterministic cross-platform solution).
src/credential_store.rs
Restore the set_permissions safety net to 0o600 on Unix after the atomic write. This was removed in the previous PR which decreased defense-in-depth.
src/oauth_config.rs
Restore the set_permissions safety net to 0o600 on Unix after the atomic write.
.changeset/resolute-toctou.md
Change the package name from "gws" to "@googleworkspace/cli" as requested by the maintainer.
Verification Plan
Automated Tests
Run cargo check and cargo test --all to verify that refactoring fs_util::atomic_write doesn't break the build and existing tempfile tests pass.
2 participants
Description
Please include a summary of the change and which issue is fixed. If adding a new feature or command, please include the output of running it with
--dry-runto prove the JSON request body matches the Discovery Document schema.Dry Run Output:
// Paste --dry-run output here if applicableChecklist:
AGENTS.mdguidelines (no generatedgoogle-*crates).cargo fmt --allto format the code perfectly.cargo clippy -- -D warningsand resolved all warnings.pnpx changeset) to document my changes.