🛡️ Sentinel: [HIGH] Fix TOCTOU vulnerability in file archival move - #302
🛡️ Sentinel: [HIGH] Fix TOCTOU vulnerability in file archival move#302matta wants to merge 1 commit into
Conversation
Co-authored-by: matta <37314+matta@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. |
|
Merging to
|
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 resolves a high-severity Time-of-Check to Time-of-Use (TOCTOU) vulnerability in the file archiving mechanism. The changes introduce a new atomic file movement utility that safeguards against race conditions and potential data loss or corruption during file operations, significantly enhancing the system's robustness and security posture. 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 effectively resolves the Time-of-Check to Time-of-Use (TOCTOU) vulnerability in the file archival process by introducing a safe_move_file function. The use of fs::hard_link with a fallback to an atomic copy operation is a solid approach. The refactoring of the calling function and the addition of a file-system-based test significantly improve the code's correctness and reliability. I have one suggestion to enhance the error handling in the new safe_move_file function to prevent a potential data duplication issue under certain failure conditions.
| fn safe_move_file(src: &Path, dst: &Path) -> io::Result<()> { | ||
| match fs::hard_link(src, dst) { | ||
| Ok(_) => fs::remove_file(src), | ||
| Err(e) if e.kind() == ErrorKind::AlreadyExists => Err(e), | ||
| Err(_) => { | ||
| // Fall back to atomic file creation and copy if hard link fails (e.g., across filesystems) | ||
| let mut src_file = fs::File::open(src)?; | ||
| let mut dst_file = OpenOptions::new().write(true).create_new(true).open(dst)?; | ||
| io::copy(&mut src_file, &mut dst_file)?; | ||
| fs::remove_file(src) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of safe_move_file can lead to duplicate archived files if removing the source file fails after a successful hard link or copy.
If fs::remove_file(src) returns an error, the entire safe_move_file operation fails. The caller (run_archive) will abort, leaving a successfully created link/copy in the archive directory but also leaving the original file in place. On the next run, the archiver will process the original file again, see a conflict with the existing archived file, and create a new version with a -2 suffix, resulting in duplicates.
To make the archival process more robust, the operation should be considered successful once the file is present at the destination. A failure to clean up the source file should not cause the entire move to fail. Ideally, this cleanup failure would be logged.
fn safe_move_file(src: &Path, dst: &Path) -> io::Result<()> {
match fs::hard_link(src, dst) {
Ok(_) => {
// The link is created, so the move is logically complete.
// If removing the source fails, we proceed anyway to prevent duplicates on subsequent runs.
// Ideally, this failure would be logged.
let _ = fs::remove_file(src);
Ok(())
}
Err(e) if e.kind() == ErrorKind::AlreadyExists => Err(e),
Err(_) => {
// Fall back to atomic file creation and copy if hard link fails (e.g., across filesystems)
let mut src_file = fs::File::open(src)?;
let mut dst_file = OpenOptions::new().write(true).create_new(true).open(dst)?;
io::copy(&mut src_file, &mut dst_file)?;
// The file is now copied. We can ignore a failure to remove the source.
let _ = fs::remove_file(src);
Ok(())
}
}
}1 participant
🚨 Severity: HIGH
💡 Vulnerability: The
run_archivefunction usedfs::renameto move tasks into thearchivedirectory. It checked for existence first (exists(&new_path)) and then moved the file. This creates a Time-of-Check to Time-of-Use (TOCTOU) vulnerability where a file or symlink could be created at the destination in between the check and the move. Becausefs::renamesilently overwrites files, this could lead to unintended data loss or file clobbering.🎯 Impact: An attacker or concurrent process could cause an existing task or file to be overwritten, leading to data loss or tampering.
🔧 Fix: Replaced
fs::renamewith a robustsafe_move_filefunction. This function usesfs::hard_linkfollowed byfs::remove_fileto ensure an atomic move. If the hard link fails due to the file already existing, it bubbles up anAlreadyExistserror to the retry loop. If hard linking fails for other reasons (like crossing device boundaries), it falls back to an atomic copy usingOpenOptions::new().write(true).create_new(true). Added a test usingtempdirto verify the newmove_to_archivefunction.✅ Verification: Ran unit and integration tests successfully using
just checkandjust test. Tested fallback behavior and unique naming logic locally.PR created automatically by Jules for task 7993948144143463845 started by @matta