diff --git a/google-cloud-storage/acceptance/storage/file_test.rb b/google-cloud-storage/acceptance/storage/file_test.rb index 248b724e5a8e..2abb743229a6 100644 --- a/google-cloud-storage/acceptance/storage/file_test.rb +++ b/google-cloud-storage/acceptance/storage/file_test.rb @@ -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 diff --git a/google-cloud-storage/lib/google/cloud/storage/file.rb b/google-cloud-storage/lib/google/cloud/storage/file.rb index 8df9f7b13a5a..edfaacc9341d 100644 --- a/google-cloud-storage/lib/google/cloud/storage/file.rb +++ b/google-cloud-storage/lib/google/cloud/storage/file.rb @@ -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 file, resp = service.download_file bucket, name, path, @@ -2270,6 +2272,64 @@ 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 [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 + temp_regex = /\A#{Regexp.escape Dir.tmpdir}/ + + # 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 user_supplied_path.is_a?(Tempfile) || user_supplied_path =~ temp_regex + return user_supplied_path + end + + # 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 + rescue ArgumentError + # This can happen on Windows with different drives, which means it's outside. + raise SecurityError, "Directory traversal attempt detected." + end + download_path_obj.to_s + end + ## # Yielded to a block to accumulate changes for a patch request. class Updater < File diff --git a/google-cloud-storage/test/google/cloud/storage/file_test.rb b/google-cloud-storage/test/google/cloud/storage/file_test.rb index 4b52e9954b82..1bb4b44ed187 100644 --- a/google-cloud-storage/test/google/cloud/storage/file_test.rb +++ b/google-cloud-storage/test/google/cloud/storage/file_test.rb @@ -653,6 +653,28 @@ def file.crc32c; "crc32c="; end mock.verify end end + + describe "restricting downloading files to current working directory" do + + def assert_security_error(path, expected_message) + error = expect do + file.download path + end.must_raise SecurityError + _(error.message).must_match expected_message + 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 describe "File#copy" do