-
Notifications
You must be signed in to change notification settings - Fork 0
Bug: download_object_malformed_names #46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
53f1951
0978a84
e7c9c23
5ccb76a
9a6552d
b92bfdc
b7b7705
d7e63fd
c3caab3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1065,6 +1065,8 @@ def download path = nil, verify: :md5, encryption_key: nil, range: nil, | |
| if path.nil? | ||
| path = StringIO.new | ||
| path.set_encoding "ASCII-8BIT" | ||
| else | ||
| path = safe_path_for_download path | ||
| end | ||
|
Comment on lines
+1068
to
1070
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Consider moving the call to path = safe_path_for_download path
file, resp =
service.download_file bucket, name, path, |
||
| file, resp = | ||
| service.download_file bucket, name, path, | ||
|
|
@@ -2270,6 +2272,70 @@ def gzip_decompress local_file | |
| end | ||
| end | ||
|
|
||
|
|
||
| # Validates a user-supplied path for downloading a file. This method is | ||
| # intended to prevent directory traversal attacks by ensuring the final | ||
| # resolved path is within the current working directory. | ||
| # | ||
| # @param [String] user_supplied_path The local path where the file will be | ||
| # downloaded. This path must be relative to the current working | ||
| # directory. | ||
| # | ||
| # @raise [ArgumentError] If the provided `user_supplied_path` is not a | ||
| # String. | ||
|
shubhangi-google marked this conversation as resolved.
Outdated
|
||
| # @raise [SecurityError] If the `user_supplied_path` is an absolute path | ||
| # or if it resolves to a location outside of the current working | ||
| # directory. | ||
| # | ||
| # @return [String] The expanded, validated, and safe local path for the | ||
| # download. | ||
| # @example | ||
| # # Assuming the current working directory is /home/user | ||
| # safe_path = safe_path_for_download "downloads/file.txt" | ||
| # # safe_path will be "/home/user/downloads/file.txt" | ||
| # | ||
| # # The following would raise a SecurityError: | ||
| # # safe_path_for_download "/etc/passwd" | ||
| # # safe_path_for_download "../../../etc/passwd" | ||
| # | ||
|
|
||
| def safe_path_for_download user_supplied_path | ||
|
|
||
| # Allow StringIO to pass through | ||
| return user_supplied_path if user_supplied_path.is_a? StringIO | ||
|
|
||
| # Allow Tempfile and /tmp paths in test env to pass through | ||
| if ENV["TEST"] && | ||
| (user_supplied_path.is_a?(Tempfile) || | ||
| (user_supplied_path.is_a?(String) && user_supplied_path.start_with?("/tmp/"))) | ||
| return user_supplied_path | ||
| end | ||
|
shubhangi-google marked this conversation as resolved.
Outdated
|
||
|
|
||
| # Disallow if path is absolute. | ||
| path_obj = Pathname.new user_supplied_path | ||
| if path_obj.absolute? | ||
| raise SecurityError, "Absolute path not allowed in user input: #{user_supplied_path}" | ||
| end | ||
|
|
||
| # Resolve path against the current working directory. | ||
| base_dir_path = Pathname.new Dir.pwd | ||
| download_path_obj = (base_dir_path + path_obj).cleanpath | ||
|
|
||
| # 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 | ||
|
Comment on lines
+2323
to
+2325
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The 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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this addressed? |
||
| rescue ArgumentError | ||
| # This can happen on Windows with different drives, which means it's outside. | ||
| raise SecurityError, "Directory traversal attempt detected." | ||
|
shubhangi-google marked this conversation as resolved.
|
||
| end | ||
| download_path = download_path_obj.to_s | ||
|
|
||
| download_path | ||
| end | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current implementation is vulnerable to directory traversal attacks using symbolic links. The validation checks for absolute paths and 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 |
||
|
|
||
| ## | ||
| # Yielded to a block to accumulate changes for a patch request. | ||
| class Updater < File | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.