Fix SSRF vulnerability in the remote file download feature

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

Author: mshibuya

carrierwave.gemspec

@@ -27,6 +27,7 @@ Gem::Specification.new do |s|
   s.add_dependency "image_processing", "~> 1.1"
   s.add_dependency "mimemagic", ">= 0.3.0"
   s.add_dependency "addressable", "~> 2.6"
+  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/downloader/base.rb

@@ -1,4 +1,5 @@
 require 'open-uri'
+require 'ssrf_filter'
 require 'addressable'
 require 'carrierwave/downloader/remote_file'
 
@@ -22,12 +23,22 @@ def initialize(uploader)
       def download(url, remote_headers = {})
         headers = remote_headers.
           reverse_merge('User-Agent' => "CarrierWave/#{CarrierWave::VERSION}")
+        uri = process_uri(url.to_s)
         begin
-          file = OpenURI.open_uri(process_uri(url.to_s), headers)
+          if skip_ssrf_protection?(uri)
+            response = OpenURI.open_uri(process_uri(url.to_s), headers)
+          else
+            request = nil
+            response = SsrfFilter.get(uri, headers: headers) do |req|
+              request = req
+            end
+            response.uri = request.uri
+            response.value
+          end
         rescue StandardError => e
           raise CarrierWave::DownloadError, "could not download file: #{e.message}"
         end
-        CarrierWave::Downloader::RemoteFile.new(file)
+        CarrierWave::Downloader::RemoteFile.new(response)
       end
 
       ##
@@ -45,6 +56,28 @@ def process_uri(uri)
       rescue URI::InvalidURIError, Addressable::URI::InvalidURIError
         raise CarrierWave::DownloadError, "couldn't parse URL: #{uri}"
       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 CarrierWave::Downloader::CustomDownloader < CarrierWave::Downloader::Base
+      #       def skip_ssrf_protection?(uri)
+      #         uri.hostname == 'localhost' && uri.port == 80
+      #       end
+      #     end
+      #
+      #     my_uploader.downloader = CarrierWave::Downloader::CustomDownloader
+      #
+      def skip_ssrf_protection?(uri)
+        false
+      end
     end
   end
 end

lib/carrierwave/downloader/remote_file.rb

@@ -1,15 +1,36 @@
 module CarrierWave
   module Downloader
     class RemoteFile
-      attr_reader :file
+      attr_reader :file, :uri
 
       def initialize(file)
-        @file = file.is_a?(String) ? StringIO.new(file) : file
+        case file
+        when String
+          @file = StringIO.new(file)
+        when Net::HTTPResponse
+          @file = StringIO.new(file.body)
+          @content_type = file.content_type
+          @headers = file
+          @uri = file.uri
+        else
+          @file = file
+          @content_type = file.content_type
+          @headers = file.meta
+          @uri = file.base_uri
+        end
+      end
+
+      def content_type
+        @content_type || 'application/octet-stream'
+      end
+
+      def headers
+        @headers || {}
       end
 
       def original_filename
         filename = filename_from_header || filename_from_uri
-        mime_type = MiniMime.lookup_by_content_type(file.content_type)
+        mime_type = MiniMime.lookup_by_content_type(content_type)
         unless File.extname(filename).present? || mime_type.blank?
           filename = "#{filename}.#{mime_type.extension}"
         end
@@ -23,16 +44,16 @@ def respond_to?(*args)
       private
 
       def filename_from_header
-        return nil unless file.meta.include? 'content-disposition'
+        return nil unless headers['content-disposition']
 
-        match = file.meta['content-disposition'].match(/filename=(?:"([^"]+)"|([^";]+))/)
+        match = headers['content-disposition'].match(/filename=(?:"([^"]+)"|([^";]+))/)
         return nil unless match
 
         match[1].presence || match[2].presence
       end
 
       def filename_from_uri
-        CGI.unescape(File.basename(file.base_uri.path))
+        CGI.unescape(File.basename(uri.path))
       end
 
       def method_missing(*args, &block)

spec/downloader/base_spec.rb

@@ -26,21 +26,6 @@
     end
   end
 
-  context "with a URL with internationalized domain name" do
-    let(:uri) { "http://ドメイン名例.jp/#{CGI.escape(filename)}" }
-    before do
-      stub_request(:get, 'http://xn--eckwd4c7cu47r2wf.jp/test.jpg').to_return(body: file)
-    end
-
-    it "converts to Punycode URI" do
-      expect(subject.process_uri(uri).to_s).to eq 'http://xn--eckwd4c7cu47r2wf.jp/test.jpg'
-    end
-
-    it "downloads a file" do
-      expect(subject.download(uri).file.read).to eq file
-    end
-  end
-
   context 'with request headers' do
     let(:authentication_headers) do
       {
@@ -61,10 +46,6 @@
     end
   end
 
-  it "raises an error when trying to download a local file" do
-    expect { subject.download('/etc/passwd') }.to raise_error(CarrierWave::DownloadError)
-  end
-
   context "with missing file" do
     before do
       stub_request(:get, uri).to_return(status: 404)
@@ -79,6 +60,20 @@
     end
   end
 
+  context "with server error" do
+    before do
+      stub_request(:get, uri).to_return(status: 500)
+    end
+
+    it "raises an error when trying to download" do
+      expect{ subject.download(uri) }.to raise_error(CarrierWave::DownloadError)
+    end
+
+    it "doesn't obscure original exception message" do
+      expect { subject.download(uri) }.to raise_error(CarrierWave::DownloadError, /could not download file: 500/)
+    end
+  end
+
   context "with a url that contains space" do
     let(:filename) { "my test.jpg" }
     before do
@@ -95,7 +90,7 @@
     before do
       stub_request(:get, uri).
         to_return(status: 301, body: "Redirecting", headers: { "Location" => another_uri })
-      stub_request(:get, another_uri).to_return(body: file)
+      stub_request(:get, /redirected.jpg/).to_return(body: file)
     end
 
     it "retrieves redirected file" do
@@ -107,7 +102,31 @@
     end
   end
 
+  context "with SSRF prevention" do
+    before do
+      stub_request(:get, 'http://169.254.169.254/').to_return(body: file)
+      stub_request(:get, 'http://127.0.0.1/').to_return(body: file)
+    end
+
+    it "prevents accessing local files" do
+      expect { subject.download('/etc/passwd') }.to raise_error(CarrierWave::DownloadError)
+      expect { subject.download('file:///etc/passwd') }.to raise_error(CarrierWave::DownloadError)
+    end
+
+    it "prevents accessing internal addresses" do
+      expect { uploader.download!("http://169.254.169.254/") }.to raise_error CarrierWave::DownloadError
+      expect { uploader.download!("http://lvh.me/") }.to raise_error CarrierWave::DownloadError
+    end
+  end
+
   describe '#process_uri' do
+    it "converts a URL with internationalized domain name to Punycode URI" do
+      uri = "http://ドメイン名例.jp/#{CGI.escape(filename)}"
+      processed = subject.process_uri(uri)
+      expect(processed.class).to eq(URI::HTTP)
+      expect(processed.to_s).to eq 'http://xn--eckwd4c7cu47r2wf.jp/test.jpg'
+    end
+
     it "parses but not escape already escaped uris" do
       uri = 'http://example.com/%5B.jpg'
       processed = subject.process_uri(uri)
@@ -141,4 +160,16 @@
       expect { subject.process_uri(uri) }.to raise_error(CarrierWave::DownloadError)
     end
   end
+
+  describe "#skip_ssrf_protection?" do
+    let(:uri) { 'http://localhost/test.jpg' }
+    before do
+      WebMock.stub_request(:get, uri).to_return(body: file)
+      allow(subject).to receive(:skip_ssrf_protection?).and_return(true)
+    end
+
+    it "allows local request to be made" do
+      expect(subject.download(uri).read).to eq 'this is stuff'
+    end
+  end
 end

spec/downloader/remote_file_spec.rb

@@ -1,23 +1,61 @@
 require 'spec_helper'
 
 describe CarrierWave::Downloader::RemoteFile do
+  subject { CarrierWave::Downloader::RemoteFile.new(file) }
   let(:file) do
-    File.open(file_path("test.jpg")).tap { |f| OpenURI::Meta.init(f) }
+    Net::HTTPSuccess.new('1.0', '200', "").tap do |response|
+      response.body = File.read(file_path("test.jpg"))
+      response.instance_variable_set(:@read, true)
+      response.uri = URI.parse 'http://example.com/test'
+      response['content-type'] = 'image/jpeg'
+      response['vary'] = 'Accept-Encoding'
+    end
   end
-  subject { CarrierWave::Downloader::RemoteFile.new(file) }
 
-  before do
-    subject.base_uri = URI.parse 'http://example.com/test'
-    subject.meta_add_field 'content-type', 'image/jpeg'
+  context 'with Net::HTTPResponse instance' do
+    it 'returns content type' do
+      expect(subject.content_type).to eq 'image/jpeg'
+    end
+
+    it 'returns header' do
+      expect(subject.headers['vary']).to eq 'Accept-Encoding'
+    end
+
+    it 'returns URI' do
+      expect(subject.uri.to_s).to eq 'http://example.com/test'
+    end
   end
 
-  it 'sets file extension based on content-type if missing' do
-    expect(subject.original_filename).to eq "test.jpeg"
+  context 'with OpenURI::Meta instance' do
+    let(:file) do
+      File.open(file_path("test.jpg")).tap { |f| OpenURI::Meta.init(f) }.tap do |file|
+        file.base_uri = URI.parse 'http://example.com/test'
+        file.meta_add_field 'content-type', 'image/jpeg'
+        file.meta_add_field 'vary', 'Accept-Encoding'
+      end
+    end
+    it 'returns content type' do
+      expect(subject.content_type).to eq 'image/jpeg'
+    end
+
+    it 'returns header' do
+      expect(subject.headers['vary']).to eq 'Accept-Encoding'
+    end
+
+    it 'returns URI' do
+      expect(subject.uri.to_s).to eq 'http://example.com/test'
+    end
   end
 
-  describe 'with content-disposition' do
+
+  describe '#original_filename' do
+    let(:content_disposition){ nil }
     before do
-      subject.meta_add_field 'content-disposition', content_disposition
+      file['content-disposition'] = content_disposition if content_disposition
+    end
+
+    it 'sets file extension based on content-type if missing' do
+      expect(subject.original_filename).to eq "test.jpeg"
     end
 
     context 'when filename is quoted' do

spec/mount_multiple_spec.rb

@@ -504,7 +504,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 }
@@ -521,7 +521,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))
         stub_request(:get, "http://www.example.com/test.txt").to_return(status: 404)
         instance.remote_images_urls = remote_images_url
       end
@@ -733,7 +733,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
 
@@ -790,7 +790,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
 
@@ -803,8 +803,8 @@ def monkey
       subject(:images_download_errors) { instance.images_download_errors }
 
       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
@@ -978,7 +978,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) }
@@ -1010,7 +1010,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) }