Bug: download_object_malformed_names #46
Conversation
Summary of ChangesHello @shubhangi-google, 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 potential security vulnerability in the file download mechanism by implementing stringent path validation. The primary goal is to prevent malicious users from specifying paths that could lead to directory traversal attacks, ensuring that downloaded files are always written to safe, intended locations relative to the current working directory. This enhancement significantly improves the security posture of the file download feature. 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. 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
|
shubhangi-google
requested review from
gurusai-voleti,
mahendra-google and
thiyaguk09
![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 introduces a security fix to prevent directory traversal when downloading files by adding validation for the download path, along with corresponding tests. While the intent is good, the implementation has a critical flaw that renders the security check ineffective. Additionally, the validation logic itself contains a high-severity vulnerability that would allow directory traversal in some cases. I've also noted some medium-severity issues related to leftover debugging code and test-specific logic in the production codebase. My suggestions aim to make the security fix robust and effective.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a security measure to prevent directory traversal attacks during file downloads by validating the destination path. While the added checks for absolute paths and parent directory traversals (..) are a good step, my review found a critical vulnerability that remains: the code does not account for symbolic links, which can still be exploited for directory traversal. I have provided a critical-severity comment with a suggested fix to mitigate this symlink vulnerability. Additionally, I've included a couple of medium-severity comments to improve documentation accuracy and code readability.
| # Prevent directory traversal outside the base directory | ||
| begin | ||
| relative = download_path_obj.relative_path_from base_dir_path | ||
| if relative.to_s.start_with?("..") | ||
| raise SecurityError, "Directory traversal attempt detected." | ||
| end | ||
| rescue ArgumentError | ||
| # This can happen on Windows with different drives, which means it's outside. | ||
| raise SecurityError, "Directory traversal attempt detected." | ||
| end | ||
| download_path = download_path_obj.to_s | ||
|
|
||
| download_path | ||
| end |
There was a problem hiding this comment.
The current implementation is vulnerable to directory traversal attacks using symbolic links. The validation checks for absolute paths and .. components, but it doesn't handle symlinks. A malicious user could provide a path containing a symlink that points to a location outside the intended download directory, allowing them to overwrite arbitrary files.
To mitigate this, you should check for symlinks in the resolved download path. I recommend adding a check that iterates up from the target path to the base directory and ensures no component is a symlink. You should also add a new test case to cover this symlink scenario.
# Prevent directory traversal outside the base directory
begin
relative = download_path_obj.relative_path_from base_dir_path
if relative.to_s.start_with?("..")
raise SecurityError, "Directory traversal attempt detected."
end
rescue ArgumentError
# This can happen on Windows with different drives, which means it's outside.
raise SecurityError, "Directory traversal attempt detected."
end
# Prevent writing through symlinks in the path.
path_to_check = download_path_obj
while path_to_check.to_s.length > base_dir_path.to_s.length && path_to_check.parent != path_to_check
if File.symlink? path_to_check.to_s
raise SecurityError, "Path contains a symlink, which is not allowed: #{path_to_check}"
end
path_to_check = path_to_check.parent
end
download_path_obj.to_s
end
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a security enhancement to prevent directory traversal attacks when downloading files. It adds a safe_path_for_download method to validate user-supplied paths and raises a SecurityError if the path is absolute or attempts to traverse outside the current working directory. The changes include modifications to google-cloud-storage/lib/google/cloud/storage/file.rb to incorporate the new validation logic and corresponding tests in both google-cloud-storage/test/google/cloud/storage/file_test.rb and google-cloud-storage/acceptance/storage/file_test.rb to ensure the security measures function as expected.
| else | ||
| path = safe_path_for_download path | ||
| end |
There was a problem hiding this comment.
The safe_path_for_download method should be called before any file operations are performed to prevent potential security vulnerabilities. This ensures that the path is validated before being used.
Consider moving the call to safe_path_for_download outside the else block to ensure it's always called when a path is provided.
path = safe_path_for_download path
file, resp =
service.download_file bucket, name, path,| end | ||
|
|
||
| it "raises error when downloading to a nested path with parent directory traversal" do | ||
| assert_security_error "test/../../parent/directory/traversal/file.png", /Directory traversal attempt detected./ |
There was a problem hiding this comment.
This test case is duplicated. The test on line 674-676 and 678-680 are identical. Remove the duplicate test case to avoid redundancy.
it "raises error when downloading to a nested path with parent directory traversal" do
assert_security_error "test/../../parent/directory/traversal/file.png", /Directory traversal attempt detected./
end| return user_supplied_path if user_supplied_path.is_a? StringIO | ||
|
|
||
| # Allow Tempfile and /tmp paths in test env to pass through | ||
| if user_supplied_path.is_a?(Tempfile) || user_supplied_path =~ temp_regex |
There was a problem hiding this comment.
Consider adding a comment explaining why Tempfile and /tmp paths are allowed to pass through. This will help future developers understand the reasoning behind this exception and prevent accidental removal of this check.
# Allow Tempfile and /tmp paths in test env to pass through
# These are generally safe locations for temporary files.
if user_supplied_path.is_a?(Tempfile) || user_supplied_path =~ temp_regex| if relative.to_s.start_with? ".." | ||
| raise SecurityError, "Directory traversal attempt detected." | ||
| end |
There was a problem hiding this comment.
The start_with?("..") check might not be sufficient to prevent all directory traversal attempts, especially on systems where symbolic links are allowed. Consider using File.realdirpath to resolve the actual path and then compare it to the base directory to ensure it's within the allowed scope.
real_base_dir = Pathname.new(File.realdirpath(base_dir_path))
real_download_path = Pathname.new(File.realdirpath(download_path_obj))
unless real_download_path.to_s.start_with?(real_base_dir.to_s)
raise SecurityError, "Directory traversal attempt detected."
end
There was a problem hiding this comment.
is this addressed?
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
bundle exec rake ciin the gem subdirectory.closes: #<issue_number_goes_here>