🔒 [security] Secure atomic file creation with restrictive permissions#391
🔒 [security] Secure atomic file creation with restrictive permissions#391
Conversation
Use `0o600` permissions (read/write only by the owner) when creating temporary files in `atomic_write` and `atomic_write_async`. This prevents a race condition on Unix systems where a sensitive file (like credentials or tokens) could be created with default permissions (e.g., world-readable) before its permissions are corrected. The fix uses `OpenOptions::mode(0o600)` on Unix and also adds `sync_all()` for better data durability. A new test `test_atomic_write_permissions` verifies the fix on Unix. A changeset file is also included. Co-authored-by: jpoehnelt <3392975+jpoehnelt@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
🦋 Changeset detectedLatest commit: 9b49bd6 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
the
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 addresses a security vulnerability by enhancing the atomic_write and atomic_write_async functions. The primary goal is to prevent a window of opportunity for local attackers to read sensitive data from temporary files by ensuring these files are created with restrictive 0600 permissions from the outset. Additionally, data durability is improved by explicitly synchronizing file contents to disk before finalization. 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
|
googleworkspace-bot
added
the
cla: yes
![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 correctly addresses a security vulnerability by setting restrictive permissions on temporary files during atomic writes. However, a critical security vulnerability has been identified: the current implementation uses predictable temporary filenames and does not ensure exclusive file creation. This leaves the application vulnerable to symlink attacks and potential permission bypasses if an attacker pre-creates the temporary file. To fully address these concerns, it is recommended to use unique, non-predictable filenames and ensure exclusive file creation, for example, by using the O_EXCL flag (via create_new(true)). The implementation for both synchronous and asynchronous functions is otherwise sound, and the accompanying test effectively verifies the initial fix.
| { | ||
| 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()?; | ||
| } |
There was a problem hiding this comment.
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:
- 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.
- 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.
| { | ||
| 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?; | ||
| } |
There was a problem hiding this comment.
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:
- 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.
- 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.
github-actions
bot
added
the
gemini: reviewed
2 participants
🎯 What: The vulnerability fixed
Secure atomic file creation by using restrictive
0600permissions (owner-only read/write) for temporary files during the write process.On Unix-like systems, if the user's
umaskis permissive (e.g.,0002or0022), temporary files created for sensitive data (like OAuth tokens or encrypted credentials) would be world-readable or group-readable until the final permission correction. This creates a window of opportunity for local attackers to read secrets.🛡️ Solution: How the fix addresses the vulnerability
The
atomic_writeandatomic_write_asyncfunctions insrc/fs_util.rswere modified to useOpenOptionsinstead offs::write. On Unix platforms, themode(0o600)extension is called to ensure that the file is created with the correct restrictive permissions from the start. Additionally,sync_all()(orsync_all().await) is called to ensure data is durable on disk before renaming the temporary file to its final destination.Includes a new unit test on Unix to verify that the files are indeed created with
0o600permissions.PR created automatically by Jules for task 4706751054827797349 started by @jpoehnelt