fix(security): validate --upload and --output file paths#374
fix(security): validate --upload and --output file paths#374AshwinRenjith wants to merge 5 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 9eaf1b0 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 |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
googleworkspace-bot
added
area: http
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 improves the security posture of the application by implementing robust validation for user-provided file paths. It addresses a potential trust-boundary gap by ensuring that paths specified via 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 introduces important security validations for file paths provided via --upload and --output. However, the validation logic for output files is vulnerable to a bypass using broken symbolic links, which could allow an attacker to write files outside the current directory. Additionally, a critical Time-of-check to Time-of-use (TOCTOU) race condition vulnerability exists in the new validation logic, potentially allowing unintended file access. The new validation functions also perform blocking I/O within an async context, which may impact performance and stability.
|
/gemini review |
1 similar comment
|
/gemini review |
|
/gemini review |
Harden file path handling for untrusted CLI inputs by adding safe upload/output validators and enforcing them centrally in execute_method. Covers traversal, absolute paths, control characters, and symlink escapes before any file I/O. Adds regression tests and required changeset.
Address broken symlink prefix bypass in output path validation by using symlink-aware normalization and canonicalization. Also run path validators via spawn_blocking in execute_method to avoid blocking filesystem checks on async runtime threads. Adds regression coverage for broken symlink prefixes.
Disallow existing FIFO/special-file paths for --output so untrusted CLI inputs cannot redirect writes to non-regular sinks. Adds unix regression test for FIFO target rejection.
AshwinRenjith
force-pushed
the
ashwin/codex/pr1-path-validation-upload-output
branch
from
7ec989e to
6eaa5ad
Compare
|
/gemini review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces important security hardening for file path handling, which is a great improvement. The use of spawn_blocking for file I/O and the TOCTOU-aware validation on Unix platforms using openat and O_NOFOLLOW are excellent.
However, I've found critical security vulnerabilities in the non-Unix implementations of read_safe_upload_file and open_safe_output_file. They are susceptible to Time-of-check to time-of-use (TOCTOU) race conditions, which could lead to arbitrary file reads and writes, undermining the goal of this PR. Please see my detailed comments in src/validate.rs.
| std::fs::read(&canonical).map_err(|e| { | ||
| GwsError::Validation(format!( | ||
| "Failed to read upload file '{}': {}", | ||
| canonical.display(), | ||
| e | ||
| )) | ||
| }) |
There was a problem hiding this comment.
The non-unix implementation of read_safe_upload_file is vulnerable to a Time-of-check to time-of-use (TOCTOU) race condition. validate_safe_upload_file_path is called first, but std::fs::read(&canonical) is used afterwards, which follows symlinks. An attacker could replace a component of the validated path with a symlink to a sensitive file (e.g., ../../.ssh/id_rsa) between the validation check and the file read operation. This would bypass the path safety checks and could lead to arbitrary file reads. This is a critical security flaw.
To mitigate this on Windows, you would need to use the CreateFileW function from the Windows API with the FILE_FLAG_OPEN_REPARSE_POINT flag to open the symlink itself, and then verify its target. This is more complex than the std::fs API allows but is necessary for a secure implementation.
| let file = opts.open(&canonical).map_err(|e| { | ||
| GwsError::Validation(format!( | ||
| "Failed to open output file '{}': {}", | ||
| canonical.display(), | ||
| e | ||
| )) | ||
| })?; | ||
|
|
||
| let canonical_after_open = canonical.canonicalize().map_err(|e| { | ||
| GwsError::Validation(format!( | ||
| "Failed to resolve output file after open '{}': {}", | ||
| canonical.display(), | ||
| e | ||
| )) | ||
| })?; | ||
| let cwd = std::env::current_dir().map_err(|e| { | ||
| GwsError::Validation(format!("Failed to determine current directory: {e}")) | ||
| })?; | ||
| let canonical_cwd = cwd.canonicalize().map_err(|e| { | ||
| GwsError::Validation(format!("Failed to canonicalize current directory: {e}")) | ||
| })?; | ||
| if !canonical_after_open.starts_with(&canonical_cwd) { | ||
| return Err(GwsError::Validation(format!( | ||
| "--output '{}' resolves outside current directory after open: '{}'", | ||
| path, | ||
| canonical_after_open.display() | ||
| ))); | ||
| } |
There was a problem hiding this comment.
The non-unix implementation of open_safe_output_file is vulnerable to a Time-of-check to time-of-use (TOCTOU) race condition. After validate_safe_output_file_path checks the path, opts.open(&canonical) is called, which follows symlinks. An attacker could replace a path component with a symlink to a sensitive file (e.g., /etc/passwd) between validation and opening, causing your application to overwrite it.
The post-open check canonical.canonicalize() is also racy as it operates on the path, not the opened file handle, and the file is already open for writing at this point.
This is a critical security vulnerability that could lead to arbitrary file writes.
To mitigate this on Windows, you would need to use the CreateFileW function with appropriate flags (CREATE_NEW for new files, and FILE_FLAG_OPEN_REPARSE_POINT to handle existing symlinks safely).
2 participants
Harden file path handling for untrusted CLI inputs by adding safe upload/output validators and enforcing them centrally in
execute_method.Covers traversal, absolute paths, control characters, and symlink escapes before any file I/O. Adds regression tests and required changeset.
Description
This PR fixes a filesystem trust-boundary gap where untrusted CLI path inputs (
--upload,--output) could be consumed directly for file I/O.Changes:
validate_safe_upload_file_pathandvalidate_safe_output_file_pathinsrc/validate.rs.src/executor.rs::execute_method, so all command paths (including helpers) are covered..changeset/hardening-upload-output-paths.md.Dry Run Output (
--outputsafe path):{ "body": null, "dry_run": true, "is_multipart_upload": false, "method": "GET", "query_params": {}, "url": "https://www.googleapis.com/drive/v3/files" }Dry Run Output (
--uploadsafe path):{ "body": null, "dry_run": true, "is_multipart_upload": true, "method": "POST", "query_params": {}, "url": "https://www.googleapis.com/upload/drive/v3/files" }Checklist:
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.