🛡️ Sentinel: [HIGH] Fix OOM DoS and TOCTOU in JSON file adapters - #72
🛡️ Sentinel: [HIGH] Fix OOM DoS and TOCTOU in JSON file adapters#72ivangegovdve-sudo wants to merge 1 commit into
Conversation
Replaced vulnerable `path.stat().st_size` checks with secure bounded reads (`f.read(limit + 1)`) and added `path.is_file()` validation across all file-backed JSON adapters to prevent out-of-memory denial-of-service via device files like `/dev/zero`. 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. |
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 high-severity security vulnerability related to potential Out-of-Memory (OOM) Denial-of-Service (DoS) attacks and Time-of-Check to Time-of-Use (TOCTOU) race conditions in JSON file adapters. The changes fortify the application's file reading mechanisms by introducing more robust file type validation and secure, bounded memory reads, thereby preventing an attacker or misconfiguration from causing application crashes due to excessively large or malicious input files. 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 enhances the security of file-reading operations across several adapter classes by implementing a more robust mechanism to prevent Denial-of-Service (DoS) attacks. The changes involve replacing path.exists() with path.is_file() and path.stat().st_size checks with a bounded read approach (f.read(limit + 1)) to mitigate Time-of-Check to Time-of-Use (TOCTOU) vulnerabilities and safely handle device files. A new entry in .jules/sentinel.md documents this vulnerability and its prevention. The review suggests adding error handling for OSError and json.JSONDecodeError in the _read_json function for consistency and robustness, and extracting the hardcoded file size limit into a module-level constant across all affected files to improve maintainability.
| with path.open("r", encoding="utf-8") as f: | ||
| content = f.read(10 * 1024 * 1024 + 1) | ||
| if len(content) > 10 * 1024 * 1024: | ||
| raise ValueError(f"Checkpoint file {path} exceeds 10MB size limit") | ||
| parsed = json.loads(path.read_text(encoding="utf-8")) | ||
| parsed = json.loads(content) | ||
| return parsed if isinstance(parsed, dict) else {} |
There was a problem hiding this comment.
This _read_json function does not handle potential OSError or json.JSONDecodeError, which could lead to unhandled exceptions if the file is corrupted or a read error occurs. Other file adapters in this PR, such as JsonFileProgressSnapshotStore, handle these exceptions by returning a default value. For consistency and robustness, it would be beneficial to wrap the logic in a try...except (OSError, json.JSONDecodeError) block and return {} in case of an error.
| 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 used twice here. To improve maintainability and avoid magic numbers, consider defining it as a module-level constant. This would make the code cleaner and easier to update if the limit needs to change.
For example:
_MAX_FILE_SIZE_BYTES = 10 * 1024 * 1024 # 10MB
# ... inside _read_json
with path.open("r", encoding="utf-8") as f:
content = f.read(_MAX_FILE_SIZE_BYTES + 1)
if len(content) > _MAX_FILE_SIZE_BYTES:
# ...This principle applies to the other file adapters modified in this pull request as well.
| 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 and avoid magic numbers, it's recommended to define this value as a module-level constant and reuse it. This makes the code cleaner and simplifies future updates to the size limit.
For example:
_MAX_FILE_SIZE_BYTES = 10 * 1024 * 1024 # 10MB
# ... inside _load_storage
with self._file_path.open("r", encoding="utf-8") as f:
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.
The magic number 10 * 1024 * 1024 is used for the file size limit. It's better to extract this into a module-level constant to improve code clarity and make it easier to manage the value in one place.
For example:
_MAX_FILE_SIZE_BYTES = 10 * 1024 * 1024 # 10MB
# ... inside _load_storage
with self._file_path.open("r", encoding="utf-8") as f:
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.
The file size limit 10 * 1024 * 1024 is hardcoded. To enhance maintainability, this value should be defined as a constant at the module level. This avoids repetition and makes the limit's purpose clearer.
Example:
_MAX_FILE_SIZE_BYTES = 10 * 1024 * 1024 # 10MB
# ... inside _load_payload
with self._file_path.open("r", encoding="utf-8") as f:
content = f.read(_MAX_FILE_SIZE_BYTES + 1)
if len(content) > _MAX_FILE_SIZE_BYTES:
# ...1 participant
🚨 Severity: HIGH
💡 Vulnerability: The application relied on
path.stat().st_sizeto enforce a 10MB limit before callingjson.loads(path.read_text()). This is vulnerable to Time-of-Check to Time-of-Use (TOCTOU) race conditions and can be completely bypassed by character device files (e.g.,/dev/zero), which report a size of 0 but can stream infinite data, leading to unbounded memory consumption (OOM DoS).🎯 Impact: An attacker or misconfiguration pointing the application to a device file or a rapidly growing file could cause the application to crash due to out-of-memory errors, leading to denial of service.
🔧 Fix:
path.exists()withpath.is_file()to explicitly reject device files and directories.path.read_text()with a secure bounded read patterncontent = f.read(10 * 1024 * 1024 + 1).len(content) > 10 * 1024 * 1024after reading to enforce the limit safely in memory.✅ Verification: Ran
uv run pytest,uv run ruff format .,uv run ruff check ., anduv run mypy. All checks passed. Logged learning to.jules/sentinel.md.PR created automatically by Jules for task 14765083783185029734 started by @ivangegovdve-sudo