Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions google-cloud-storage/acceptance/storage/file_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1040,4 +1040,35 @@
expect { uploaded_file.retention = retention }.must_raise Google::Cloud::PermissionDeniedError
end
end

describe "restricting downloading files to current working directory" do
let(:original) { File.new(files[:logo][:path]) }

def upload_logo
bucket.create_file(original, "CloudLogo.png")
end

def assert_security_error(path, expected_message)
uploaded = upload_logo

error = expect do
uploaded.download(path)
end.must_raise SecurityError

_(error.message).must_match expected_message
uploaded.delete
end

it "raises error when downloading to an absolute path" do
assert_security_error "/absolute/path/to/file.png", /Absolute path not allowed in user input/
end

it "raises error when downloading to a path with parent directory traversal" do
assert_security_error "../parent/directory/traversal/file.png", /Directory traversal attempt detected./
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./
end
end
end
66 changes: 66 additions & 0 deletions google-cloud-storage/lib/google/cloud/storage/file.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 thread
shubhangi-google marked this conversation as resolved.
Comment on lines +1068 to 1070
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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,

file, resp =
service.download_file bucket, name, path,
Expand Down Expand Up @@ -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.
Comment thread
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
Comment thread
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
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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."
Comment thread
shubhangi-google marked this conversation as resolved.
end
download_path = download_path_obj.to_s

download_path
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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


##
# Yielded to a block to accumulate changes for a patch request.
class Updater < File
Expand Down