Fix path injection in addons#5667
Conversation
Summary of ChangesHello @Pikabooboo, 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 introduces critical security improvements by adding comprehensive validation and normalization to the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 aims to fix a path injection vulnerability. However, the current implementation is flawed as it uses the re module without importing it, which will cause a runtime error and a Denial of Service for the addon, preventing the security validation from being executed. Additionally, the path validation is not strict enough to prevent path traversal attacks.
| print(f"Error: target_path must be absolute: {target_path}") | ||
| exit(1) | ||
|
|
||
| if not re.match(r'^[/a-zA-Z0-9._-]+$', target_path): |
There was a problem hiding this comment.
The re module is used here (e.g., re.match() for target_path validation) but is not imported. This will cause a NameError: name 're' is not defined at runtime, leading to a crash and a Denial of Service (DoS) for the addon. Consequently, the intended security validation is never performed. Please add import re at the beginning of the file.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5667 +/- ##
=======================================
Coverage 61.29% 61.29%
=======================================
Files 444 444
Lines 30540 30540
=======================================
Hits 18721 18721
Misses 10276 10276
Partials 1543 1543 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pikabooboo
force-pushed
the
io_fix
branch
2 times, most recently
from
c6caeb2 to
0be335e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances security by adding validation for target_path to prevent path injection vulnerabilities. The checks for an absolute path, against path traversal components ('..'), and for allowed characters are good security practices.
However, I've identified a potential issue where the target_path could resolve to the root directory ('/') after normalization, which could lead to mounting on the root filesystem. I've added a review comment with a suggestion to prevent this. This is a high-severity issue that should be addressed.
| exit(1) | ||
| # Normalize to prevent traversal attacks | ||
| target_path = os.path.normpath(target_path) | ||
| os.makedirs(target_path, exist_ok=True) |
There was a problem hiding this comment.
The normalized target_path could become the root directory / (for example, if the input was /. or //). Mounting on the root directory is dangerous and should be explicitly prevented. Please add a check for this case before creating the directory.
if target_path == '/':
print("Error: target_path resolves to the root directory '/' and is not allowed.")
exit(1)
os.makedirs(target_path, exist_ok=True)
There was a problem hiding this comment.
Pull request overview
This pull request adds path validation to the GlusterFS addon's entrypoint script to prevent path injection vulnerabilities. The validation checks that the target_path from the configuration JSON is an absolute path, doesn't contain path traversal sequences (..), contains only allowed characters, and is normalized before use.
Changes:
- Added regex import for path validation
- Implemented three-stage validation: absolute path check, path traversal prevention, and character whitelist validation
- Added path normalization before directory creation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
The order of validation operations is incorrect. The regex check on line 20 is performed before os.path.normpath on line 24, which means paths like "//path" or "/path//subdir" would fail the regex check even though they normalize to valid paths. Additionally, os.path.normpath should be called before any validation checks to ensure all subsequent checks operate on the normalized path. Move the normpath call to immediately after line 9, before all validation checks.
| if '..' in target_path.split('/'): | ||
| print(f"Error: Path traversal using '..' is not allowed in target_path: {target_path}") | ||
| exit(1) |
There was a problem hiding this comment.
The path traversal check for '..' is performed before normalization. This creates a gap where paths like '/path/./subdir' or paths with multiple slashes would pass this check but might represent the same file system location as a path with '..'. While not directly a '..' bypass, checking before normalization means you're not validating the canonical form of the path. If normalization is moved to occur first (as suggested in another comment), this check can serve as a post-normalization safety check to catch any remaining '..' components, which should not exist in a properly normalized absolute path.
There was a problem hiding this comment.
After normalization on line 24, there should be an additional check to ensure the normalized path still starts with '/' and is an absolute path. The os.path.normpath function could potentially reduce a path like '/../../../etc/passwd' to '/etc/passwd' (depending on the input), and while the initial check on line 12 prevents this specific case, having a post-normalization validation ensures the path remains absolute and within expected boundaries.
| target_path = os.path.normpath(target_path) | |
| target_path = os.path.normpath(target_path) | |
| # Ensure the normalized path is still absolute and starts with '/' | |
| if not os.path.isabs(target_path) or not target_path.startswith('/'): | |
| print(f"Error: Normalized target_path is not an absolute POSIX path: {target_path}") | |
| exit(1) |
Signed-off-by: pikabooboo <pikabooboo777@gmail.com>
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to fix a path injection vulnerability in the glusterfs addon by adding validation for the target_path. However, critical vulnerabilities remain. The current validation can be bypassed, specifically through the use of os.path.normpath preserving double leading slashes (//), which allows bypassing the root directory check, and the regex validation using the $ anchor, which can be bypassed by a trailing newline. Additionally, the current checks do not prevent mounting volumes over sensitive system directories by specifying arbitrary absolute paths. It is recommended to enforce that all mounts occur within a designated safe base directory, use re.fullmatch, and apply additional path normalization to fully mitigate these risks.
| target_path = os.path.normpath(target_path) | ||
|
|
||
| # Validate that the normalized path is an absolute POSIX path | ||
| if not os.path.isabs(target_path) or not target_path.startswith('/'): | ||
| print(f"Error: target_path must be absolute: {target_path}") | ||
| exit(1) | ||
|
|
||
| # Safety check: ensure no '..' components remain after normalization | ||
| if '..' in target_path.split('/'): | ||
| print(f"Error: Path traversal using '..' is not allowed in target_path: {target_path}") | ||
| exit(1) | ||
|
|
||
| # Validate that the path contains only safe characters | ||
| if not re.match(r'^[/a-zA-Z0-9._-]+$', target_path): | ||
| print(f"Error: target_path contains invalid characters: {target_path}") | ||
| exit(1) | ||
|
|
||
| # Prevent mounting on the root directory | ||
| if target_path == '/': | ||
| print("Error: target_path resolves to the root directory '/' and is not allowed.") | ||
| exit(1) |
There was a problem hiding this comment.
This section has critical vulnerabilities. The current root directory check can be bypassed using paths with double leading slashes (e.g., //), as os.path.normpath preserves these, allowing // to resolve to root despite not matching /. Additionally, the regex validation using the $ anchor can be bypassed by a trailing newline. Beyond these bypasses, the current checks do not prevent users from specifying arbitrary absolute paths like /etc or /usr/bin for target_path, which could lead to mounting volumes over sensitive system directories and potential container escapes. It is crucial to implement robust normalization, use re.fullmatch, and enforce that target_path is restricted to a designated safe base directory.
target_path = os.path.normpath(target_path)
if target_path.startswith('//') and not target_path.startswith('///'):
target_path = '/' + target_path.lstrip('/')
# Validate that the normalized path is an absolute POSIX path
if not os.path.isabs(target_path) or not target_path.startswith('/'):
print(f"Error: target_path must be absolute: {target_path}")
exit(1)
# Safety check: ensure no '..' components remain after normalization
if '..' in target_path.split('/'):
print(f"Error: Path traversal using '..' is not allowed in target_path: {target_path}")
exit(1)
# Validate that the path contains only safe characters
if not re.fullmatch(r'^[/a-zA-Z0-9._-]+$', target_path):
print(f"Error: target_path contains invalid characters: {target_path}")
exit(1)
# Prevent mounting on the root directory
if target_path == '/':
print("Error: target_path resolves to the root directory '/' and is not allowed.")
exit(1)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RongGu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Ⅰ. Describe what this PR does
Fix path injection in addons
Ⅱ. Does this pull request fix one issue?
fixes #XXXX
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
CI pipeline
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews