🛡️ Sentinel: [HIGH] Fix TOCTOU and device file size limit bypass - #65
🛡️ Sentinel: [HIGH] Fix TOCTOU and device file size limit bypass#65ivangegovdve-sudo wants to merge 1 commit into
Conversation
Added `is_file()` checks to prevent size limit bypass using device files like `/dev/zero`. Also switched from `read_text()` to bounded `f.read()` to prevent TOCTOU vulnerabilities where a file grows after its size was checked. Updates all JSON file-backed adapters and records the critical learning in Sentinel's journal. Co-authored-by: ivangegovdve-sudo <225339531+ivangegovdve-sudo@users.noreply.github.com>
|
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 high-severity security vulnerability related to unbounded memory consumption. It prevents potential denial-of-service (DoS) attacks by ensuring that only regular files are processed and by implementing bounded file reading to avoid TOCTOU issues. The changes enhance the application's resilience against malicious or misconfigured file inputs. 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 effectively addresses a critical TOCTOU vulnerability and a device file bypass issue by implementing bounded file reads and checking for regular files. This is a significant security enhancement. My review includes a high-severity suggestion to handle potential crashes from empty JSON files in the checkpoint_store. Additionally, I've made several medium-severity recommendations to extract the hardcoded file size limit into a shared constant, which will improve code maintainability and consistency across the different file-based adapters.
| parsed = json.loads(content) | ||
| return parsed if isinstance(parsed, dict) else {} |
There was a problem hiding this comment.
An empty checkpoint file will cause json.loads(content) to raise a json.JSONDecodeError because content will be an empty string. This exception is not handled, which could crash the application. It's best to handle this case gracefully by returning an empty dictionary, which is consistent with how a non-existent file is handled.
| parsed = json.loads(content) | |
| return parsed if isinstance(parsed, dict) else {} | |
| try: | |
| parsed = json.loads(content) | |
| except json.JSONDecodeError: | |
| return {} | |
| return parsed if isinstance(parsed, dict) else {} |
| content = f.read(10 * 1024 * 1024 + 1) | ||
| if len(content) > 10 * 1024 * 1024: |
There was a problem hiding this comment.
The file size limit 10 * 1024 * 1024 is hardcoded. This value is duplicated across multiple files (json_file_practice_repository.py, json_file_progress_repository.py, json_file_progress_snapshot_store.py), making it difficult to maintain. Consider defining it as a constant in a shared module (e.g., a new src/python_learning_orchestrated/adapters/constants.py) and importing it where needed.
For example, in a new constants.py file:
MAX_FILE_SIZE = 10 * 1024 * 1024 # 10MBThen you can import and use this constant.
| content = f.read(10 * 1024 * 1024 + 1) | |
| if len(content) > 10 * 1024 * 1024: | |
| content = f.read(MAX_FILE_SIZE + 1) | |
| if len(content) > MAX_FILE_SIZE: |
| content = f.read(10 * 1024 * 1024 + 1) | ||
| if len(content) > 10 * 1024 * 1024: |
There was a problem hiding this comment.
The file size limit 10 * 1024 * 1024 is hardcoded here. To improve maintainability, this should be replaced with a shared constant. Please see my other comment about creating a MAX_FILE_SIZE constant.
| content = f.read(10 * 1024 * 1024 + 1) | |
| if len(content) > 10 * 1024 * 1024: | |
| content = f.read(MAX_FILE_SIZE + 1) | |
| if len(content) > MAX_FILE_SIZE: |
| content = f.read(10 * 1024 * 1024 + 1) | ||
| if len(content) > 10 * 1024 * 1024: |
There was a problem hiding this comment.
The file size limit 10 * 1024 * 1024 is hardcoded here. To improve maintainability, this should be replaced with a shared constant. Please see my other comment about creating a MAX_FILE_SIZE constant.
| content = f.read(10 * 1024 * 1024 + 1) | |
| if len(content) > 10 * 1024 * 1024: | |
| content = f.read(MAX_FILE_SIZE + 1) | |
| if len(content) > MAX_FILE_SIZE: |
| content = f.read(10 * 1024 * 1024 + 1) | ||
| if len(content) > 10 * 1024 * 1024: |
There was a problem hiding this comment.
The file size limit 10 * 1024 * 1024 is hardcoded here. To improve maintainability, this should be replaced with a shared constant. Please see my other comment about creating a MAX_FILE_SIZE constant.
| content = f.read(10 * 1024 * 1024 + 1) | |
| if len(content) > 10 * 1024 * 1024: | |
| content = f.read(MAX_FILE_SIZE + 1) | |
| if len(content) > MAX_FILE_SIZE: |
1 participant
🚨 Severity: HIGH
💡 Vulnerability: File size checks using
path.stat().st_sizecould be bypassed if the path pointed to a device file (like/dev/zero, which reports a size of 0). Additionally, there was a Time-of-Check to Time-of-Use (TOCTOU) vulnerability where a file could be modified and grow between thestat()size check and theread_text()call. Both vectors could lead to unbounded memory consumption DoS or application hangs.🎯 Impact: An attacker or a severe system misconfiguration could provide an excessively large file or a device file, causing the application to consume all available memory and crash when attempting to load JSON data.
🔧 Fix:
is_file()checks to ensure the target is a regular file, rejecting device files.read_text()to opening the file and using a bounded read (f.read(limit + 1)) to load the data safely and without TOCTOU risk.JsonFilePracticeRepository,JsonFileProgressRepository,JsonFileProgressSnapshotStore, andCheckpointStore..jules/sentinel.md.✅ Verification: Run
uv run pytestto ensure all functionality remains correct and exceptions are correctly thrown when expected. Checked thatuv run ruff check .passes without errors.PR created automatically by Jules for task 5267218339915732856 started by @ivangegovdve-sudo