🛡️ Sentinel: [CRITICAL] Fix TOCTOU vulnerability and unbound memory DoS in file reads - #62
Conversation
…oS in file reads Replaces naive `path.stat().st_size` and `path.exists()` checks with `path.is_file()` combined with a strict bounded read (`f.read(limit + 1)`) in JSON file-backed adapters. This prevents attackers from bypassing size limits using device files (e.g. `/dev/zero`) and completely mitigates out-of-memory DoS vulnerabilities caused by streaming large amounts of unconstrained data into memory via `read_text()`. Updated `.jules/sentinel.md` journal with details on this specific pattern. Co-authored-by: ivangegovdve-sudo <225339531+ivangegovdve-sudo@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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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 critical TOCTOU vulnerability and prevents potential Denial-of-Service attacks related to unbounded file reads in several JSON file-backed adapters. It enhances the security and stability of the application by ensuring that only regular files are processed with a strict size limit, mitigating the risk of out-of-memory errors and application crashes. Highlights
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. 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 provides a critical fix for a TOCTOU vulnerability that could lead to a Denial-of-Service attack. The approach of switching from path.exists() to path.is_file() and implementing a secure bounded read is excellent and effectively mitigates the risk. The changes are consistently applied across all four affected adapters.
I've added a few suggestions to improve maintainability by replacing the hardcoded file size limit with a constant. This is a minor point, but it will make the code cleaner and easier to manage in the long run. Overall, this is a great and important security enhancement.
| content = f.read(10 * 1024 * 1024 + 1) | ||
| if len(content) > 10 * 1024 * 1024: |
There was a problem hiding this comment.
To improve maintainability and avoid magic numbers, it's a good practice to define the file size limit as a module-level constant. This makes it easier to find and update the value if needed, and ensures consistency across the codebase.
You could define _MAX_FILE_SIZE_BYTES = 10 * 1024 * 1024 at the top of the file and use it here.
| content = f.read(10 * 1024 * 1024 + 1) | |
| if len(content) > 10 * 1024 * 1024: | |
| content = f.read(_MAX_FILE_SIZE_BYTES + 1) | |
| if len(content) > _MAX_FILE_SIZE_BYTES: |
| content = f.read(10 * 1024 * 1024 + 1) | ||
| if len(content) > 10 * 1024 * 1024: |
There was a problem hiding this comment.
Similar to the other adapters, let's replace the magic number 10 * 1024 * 1024 with a shared constant like _MAX_FILE_SIZE_BYTES for better maintainability. This will centralize the configuration for the file size limit.
| content = f.read(10 * 1024 * 1024 + 1) | |
| if len(content) > 10 * 1024 * 1024: | |
| content = f.read(_MAX_FILE_SIZE_BYTES + 1) | |
| if len(content) > _MAX_FILE_SIZE_BYTES: |
| content = f.read(10 * 1024 * 1024 + 1) | ||
| if len(content) > 10 * 1024 * 1024: |
There was a problem hiding this comment.
To maintain consistency and avoid magic numbers, please consider defining 10 * 1024 * 1024 as a module-level constant (e.g., _MAX_FILE_SIZE_BYTES) and using it here. This improves readability and makes the limit explicit.
| content = f.read(10 * 1024 * 1024 + 1) | |
| if len(content) > 10 * 1024 * 1024: | |
| content = f.read(_MAX_FILE_SIZE_BYTES + 1) | |
| if len(content) > _MAX_FILE_SIZE_BYTES: |
| content = f.read(10 * 1024 * 1024 + 1) | ||
| if len(content) > 10 * 1024 * 1024: |
There was a problem hiding this comment.
Let's replace the hardcoded size limit 10 * 1024 * 1024 with a module-level constant (e.g., _MAX_FILE_SIZE_BYTES) to improve code clarity and make future changes easier. This should be applied consistently across all modified adapters.
| content = f.read(10 * 1024 * 1024 + 1) | |
| if len(content) > 10 * 1024 * 1024: | |
| content = f.read(_MAX_FILE_SIZE_BYTES + 1) | |
| if len(content) > _MAX_FILE_SIZE_BYTES: |
1 participant
🚨 Severity
CRITICAL
💡 Vulnerability
A Time-of-Check to Time-of-Use (TOCTOU) vulnerability existed across four JSON file-backed adapters (
CheckpointStore,JsonFilePracticeRepository,JsonFileProgressRepository,JsonFileProgressSnapshotStore). Before reading file contents, the codebase checked file size usingpath.stat().st_sizeand existence usingpath.exists(), followed by an unboundedread_text()operation. This is insecure becausepath.exists()allows non-regular device files (like/dev/zero) which incorrectly report 0 bytes, bypassing the size check entirely. Once the size check is bypassed, the subsequent unboundedread_text()continuously pulls data into memory, creating a critical Denial-of-Service (DoS) and out-of-memory vulnerability.🎯 Impact
An attacker (or misconfigured process) could point a repository path to a continuous data stream or device file. Because the size check would pass trivially (returning 0 bytes), the application would attempt to read the entire infinite file into memory via
read_text(), resulting in process crashes, unbounded memory consumption, and a full application Denial of Service.🔧 Fix
path.exists()withpath.is_file()to explicitly restrict reads to regular files, completely rejecting device files.st_sizepre-check.content = f.read(10 * 1024 * 1024 + 1)within a context manager.len(content) > 10 * 1024 * 1024post-read..jules/sentinel.mdjournal to avoid future occurrences.✅ Verification
uv run pytestuv run ruff check ./uv run ruff format ./uv run mypyPR created automatically by Jules for task 9459008440490358241 started by @ivangegovdve-sudo