Fix SSRF vulnerability in the remote file download feature

Closes #2509, Refs. GHSA-fwcm-636p-68r5

Author: mshibuya

carrierwave.gemspec

@@ -24,6 +24,7 @@ Gem::Specification.new do |s|
   s.add_dependency "activesupport", ">= 4.0.0"
   s.add_dependency "activemodel", ">= 4.0.0"
   s.add_dependency "mime-types", ">= 1.16"
+  s.add_dependency "ssrf_filter", "~> 1.0"
   if RUBY_ENGINE == 'jruby'
     s.add_development_dependency 'activerecord-jdbcpostgresql-adapter'
   else

features/step_definitions/download_steps.rb

@@ -1,6 +1,6 @@
 When /^I download the file '([^']+)'/ do |url|
   unless ENV['REMOTE'] == 'true'
-    stub_request(:get, "s3.amazonaws.com/Monkey/testfile.txt").
+    stub_request(:get, %r{/Monkey/testfile.txt}).
       to_return(body: "S3 Remote File", headers: { "Content-Type" => "text/plain" })
   end
 

lib/carrierwave/uploader/download.rb

@@ -1,4 +1,5 @@
 require 'open-uri'
+require 'ssrf_filter'
 
 module CarrierWave
   module Uploader
@@ -10,15 +11,18 @@ module Download
       include CarrierWave::Uploader::Cache
 
       class RemoteFile
-        def initialize(uri, remote_headers = {})
+        attr_reader :uri
+
+        def initialize(uri, remote_headers = {}, skip_ssrf_protection: false)
           @uri = uri
-          @remote_headers = remote_headers
-          @file = nil
+          @remote_headers = remote_headers.reverse_merge('User-Agent' => "CarrierWave/#{CarrierWave::VERSION}")
+          @file, @content_type, @headers = nil
+          @skip_ssrf_protection = skip_ssrf_protection
         end
 
         def original_filename
           filename = filename_from_header || filename_from_uri
-          mime_type = MIME::Types[file.content_type].first
+          mime_type = MIME::Types[content_type].first
           unless File.extname(filename).present? || mime_type.blank?
             filename = "#{filename}.#{mime_type.extensions.first}"
           end
@@ -33,15 +37,35 @@ def http?
           @uri.scheme =~ /^https?$/
         end
 
-      private
+        def content_type
+          @content_type || 'application/octet-stream'
+        end
+
+        def headers
+          @headers || {}
+        end
+
+        private
 
         def file
           if @file.blank?
-            headers = @remote_headers.
-              reverse_merge('User-Agent' => "CarrierWave/#{CarrierWave::VERSION}")
-
-            @file = (URI.respond_to?(:open) ? URI : Kernel).open(@uri.to_s, headers)
-            @file = @file.is_a?(String) ? StringIO.new(@file) : @file
+            if @skip_ssrf_protection
+              @file = (URI.respond_to?(:open) ? URI : Kernel).open(@uri.to_s, @remote_headers)
+              @file = @file.is_a?(String) ? StringIO.new(@file) : @file
+              @content_type = @file.content_type
+              @headers = @file.meta
+              @uri = @file.base_uri
+            else
+              request = nil
+              response = SsrfFilter.get(@uri, headers: @remote_headers) do |req|
+                request = req
+              end
+              response.value
+              @file = StringIO.new(response.body)
+              @content_type = response.content_type
+              @headers = response
+              @uri = request.uri
+            end
           end
           @file
 
@@ -50,14 +74,14 @@ def file
         end
 
         def filename_from_header
-          if file.meta.include? 'content-disposition'
-            match = file.meta['content-disposition'].match(/filename="?([^"]+)/)
+          if headers['content-disposition']
+            match = headers['content-disposition'].match(/filename="?([^"]+)/)
             return match[1] unless match.nil? || match[1].empty?
           end
         end
 
         def filename_from_uri
-          URI::DEFAULT_PARSER.unescape(File.basename(file.base_uri.path))
+          URI::DEFAULT_PARSER.unescape(File.basename(@uri.path))
         end
 
         def method_missing(*args, &block)
@@ -75,7 +99,7 @@ def method_missing(*args, &block)
       #
       def download!(uri, remote_headers = {})
         processed_uri = process_uri(uri)
-        file = RemoteFile.new(processed_uri, remote_headers)
+        file = RemoteFile.new(processed_uri, remote_headers, skip_ssrf_protection: skip_ssrf_protection?(processed_uri))
         raise CarrierWave::DownloadError, "trying to download a file which is not served over HTTP" unless file.http?
         cache!(file)
       end
@@ -97,6 +121,25 @@ def process_uri(uri)
         URI.parse(encoded_uri) rescue raise CarrierWave::DownloadError, "couldn't parse URL"
       end
 
+      ##
+      # If this returns true, SSRF protection will be bypassed.
+      # You can override this if you want to allow accessing specific local URIs that are not SSRF exploitable.
+      #
+      # === Parameters
+      #
+      # [uri (URI)] The URI where the remote file is stored
+      #
+      # === Examples
+      #
+      #     class MyUploader < CarrierWave::Uploader::Base
+      #       def skip_ssrf_protection?(uri)
+      #         uri.hostname == 'localhost' && uri.port == 80
+      #       end
+      #     end
+      #
+      def skip_ssrf_protection?(uri)
+        false
+      end
     end # Download
   end # Uploader
 end # CarrierWave

spec/mount_multiple_spec.rb

@@ -374,7 +374,7 @@ def monkey
     describe "#remote_images_urls" do
       subject { instance.remote_images_urls }
 
-      before { stub_request(:get, "www.example.com/#{test_file_name}").to_return(body: File.read(test_file_stub)) }
+      before { stub_request(:get, "http://www.example.com/#{test_file_name}").to_return(body: File.read(test_file_stub)) }
 
       context "returns nil" do
         it { is_expected.to be_nil }
@@ -391,7 +391,7 @@ def monkey
       subject(:images) { instance.images }
 
       before do
-        stub_request(:get, "www.example.com/#{test_file_name}").to_return(body: File.read(test_file_stub))
+        stub_request(:get, "http://www.example.com/#{test_file_name}").to_return(body: File.read(test_file_stub))
         instance.remote_images_urls = remote_images_url
       end
 
@@ -571,7 +571,7 @@ def extension_whitelist
 
         context "when file was downloaded" do
           before do
-            stub_request(:get, "www.example.com/#{test_file_name}").to_return(body: File.read(test_file_stub))
+            stub_request(:get, "http://www.example.com/#{test_file_name}").to_return(body: File.read(test_file_stub))
             instance.remote_images_urls = ["http://www.example.com/#{test_file_name}"]
           end
 
@@ -628,7 +628,7 @@ def monkey
 
         context "when file was downloaded" do
           before do
-            stub_request(:get, "www.example.com/#{test_file_name}").to_return(body: File.read(test_file_stub))
+            stub_request(:get, "http://www.example.com/#{test_file_name}").to_return(body: File.read(test_file_stub))
             instance.remote_images_urls = ["http://www.example.com/#{test_file_name}"]
           end
 
@@ -641,8 +641,8 @@ def monkey
       subject(:images_download_error) { instance.images_download_error }
 
       before do
-        stub_request(:get, "www.example.com/#{test_file_name}").to_return(body: File.read(test_file_stub))
-        stub_request(:get, "www.example.com/missing.jpg").to_return(status: 404)
+        stub_request(:get, "http://www.example.com/#{test_file_name}").to_return(body: File.read(test_file_stub))
+        stub_request(:get, "http://www.example.com/missing.jpg").to_return(status: 404)
       end
 
       describe "default behaviour" do
@@ -812,7 +812,7 @@ def extension_whitelist
 
     context "when a downloaded image fails an integity check" do
       before do
-        stub_request(:get, "www.example.com/#{test_file_name}").to_return(body: test_file_stub)
+        stub_request(:get, "http://www.example.com/#{test_file_name}").to_return(body: test_file_stub)
       end
 
       it { expect(running {instance.remote_images_urls = ["http://www.example.com/#{test_file_name}"]}).to raise_error(CarrierWave::IntegrityError) }
@@ -844,7 +844,7 @@ def monkey
 
     context "when a downloaded image fails an integity check" do
       before do
-        stub_request(:get, "www.example.com/#{test_file_name}").to_return(body: test_file_stub)
+        stub_request(:get, "http://www.example.com/#{test_file_name}").to_return(body: test_file_stub)
       end
 
       it { expect(running {instance.remote_images_urls = ["http://www.example.com/#{test_file_name}"]}).to raise_error(CarrierWave::ProcessingError) }

spec/mount_single_spec.rb

@@ -292,7 +292,7 @@ def default_url
 
     describe "#remote_image_url" do
       before do
-        stub_request(:get, "www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
+        stub_request(:get, "http://www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
       end
 
       it "returns nil" do
@@ -327,7 +327,7 @@ def default_url
 
     describe "#remote_image_url=" do
       before do
-        stub_request(:get, "www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
+        stub_request(:get, "http://www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
       end
 
       it "does nothing when nil is assigned" do
@@ -471,7 +471,7 @@ def extension_whitelist
         end
 
         it "should be an error instance if file was downloaded" do
-          stub_request(:get, "www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
+          stub_request(:get, "http://www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
           @instance.remote_image_url = "http://www.example.com/test.jpg"
           e = @instance.image_integrity_error
 
@@ -516,7 +516,7 @@ def monkey
         end
 
         it "should be an error instance if file was downloaded" do
-          stub_request(:get, "www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
+          stub_request(:get, "http://www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
           @instance.remote_image_url = "http://www.example.com/test.jpg"
 
           expect(@instance.image_processing_error).to be_an_instance_of(CarrierWave::ProcessingError)
@@ -526,8 +526,8 @@ def monkey
 
     describe '#image_download_error' do
       before do
-        stub_request(:get, "www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
-        stub_request(:get, "www.example.com/missing.jpg").to_return(status: 404)
+        stub_request(:get, "http://www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
+        stub_request(:get, "http://www.example.com/missing.jpg").to_return(status: 404)
       end
 
       it "should be nil by default" do
@@ -547,8 +547,8 @@ def monkey
 
     describe '#image_download_error' do
       before do
-        stub_request(:get, "www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
-        stub_request(:get, "www.example.com/missing.jpg").to_return(status: 404)
+        stub_request(:get, "http://www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
+        stub_request(:get, "http://www.example.com/missing.jpg").to_return(status: 404)
       end
 
       it "should be nil by default" do
@@ -702,7 +702,7 @@ def extension_whitelist
     end
 
     it "should raise an error if the image fails an integrity check when downloaded" do
-      stub_request(:get, "www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
+      stub_request(:get, "http://www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
 
       expect(running {
         @instance.remote_image_url = "http://www.example.com/test.jpg"
@@ -736,7 +736,7 @@ def monkey
     end
 
     it "should raise an error if the image fails to be processed when downloaded" do
-      stub_request(:get, "www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
+      stub_request(:get, "http://www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
 
       expect(running {
         @instance.remote_image_url = "http://www.example.com/test.jpg"

spec/orm/activerecord_spec.rb

@@ -496,7 +496,7 @@ def monkey
 
     describe "#remote_image_url=" do
       before do
-        stub_request(:get, "www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
+        stub_request(:get, "http://www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
       end
 
       # FIXME ideally image_changed? and remote_image_url_changed? would return true
@@ -1215,7 +1215,7 @@ def monkey
 
     describe "#remote_images_urls=" do
       before do
-        stub_request(:get, "www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
+        stub_request(:get, "http://www.example.com/test.jpg").to_return(body: File.read(file_path("test.jpg")))
       end
 
       # FIXME ideally images_changed? and remote_images_urls_changed? would return true

spec/spec_helper.rb

@@ -109,6 +109,14 @@ def color_of_pixel(path, x, y)
         image.run_command("convert", "#{image.path}[1x1+#{x}+#{y}]", "-depth", "8", "txt:").split("\n")[1]
       end
     end
+
+    module SsrfProtectionAwareWebMock
+      def stub_request(method, uri)
+        uri = URI.parse(uri) if uri.is_a?(String)
+        uri.hostname = Resolv.getaddress(uri.hostname) if uri.is_a?(URI)
+        super
+      end
+    end
   end
 end
 
@@ -118,6 +126,7 @@ def color_of_pixel(path, x, y)
   config.include CarrierWave::Test::MockStorage
   config.include CarrierWave::Test::I18nHelpers
   config.include CarrierWave::Test::ManipulationHelpers
+  config.prepend CarrierWave::Test::SsrfProtectionAwareWebMock
   if RUBY_ENGINE == 'jruby'
     config.filter_run_excluding :rmagick => true
   end