Merge branch 'security-fix-ssrf-lfs-project-import' into 'master'

[master] SSRF in project imports with LFS

See merge request gitlab/gitlabhq!2720

app/services/projects/lfs_pointers/lfs_download_service.rb

@@ -12,28 +12,43 @@ module Projects
 
         return if LfsObject.exists?(oid: oid)
 
-        sanitized_uri = Gitlab::UrlSanitizer.new(url)
-        Gitlab::UrlBlocker.validate!(sanitized_uri.sanitized_url, protocols: VALID_PROTOCOLS)
+        sanitized_uri = sanitize_url!(url)
 
         with_tmp_file(oid) do |file|
-          size = download_and_save_file(file, sanitized_uri)
-          lfs_object = LfsObject.new(oid: oid, size: size, file: file)
+          download_and_save_file(file, sanitized_uri)
+          lfs_object = LfsObject.new(oid: oid, size: file.size, file: file)
 
           project.all_lfs_objects << lfs_object
         end
+      rescue Gitlab::UrlBlocker::BlockedUrlError => e
+        Rails.logger.error("LFS file with oid #{oid} couldn't be downloaded: #{e.message}")
       rescue StandardError => e
-        Rails.logger.error("LFS file with oid #{oid} could't be downloaded from #{sanitized_uri.sanitized_url}: #{e.message}")
+        Rails.logger.error("LFS file with oid #{oid} couldn't be downloaded from #{sanitized_uri.sanitized_url}: #{e.message}")
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
       private
 
+      def sanitize_url!(url)
+        Gitlab::UrlSanitizer.new(url).tap do |sanitized_uri|
+          # Just validate that HTTP/HTTPS protocols are used. The
+          # subsequent Gitlab::HTTP.get call will do network checks
+          # based on the settings.
+          Gitlab::UrlBlocker.validate!(sanitized_uri.sanitized_url,
+                                       protocols: VALID_PROTOCOLS)
+        end
+      end
+
       def download_and_save_file(file, sanitized_uri)
-        IO.copy_stream(open(sanitized_uri.sanitized_url, headers(sanitized_uri)), file) # rubocop:disable Security/Open
+        response = Gitlab::HTTP.get(sanitized_uri.sanitized_url, headers(sanitized_uri)) do |fragment|
+          file.write(fragment)
+        end
+
+        raise StandardError, "Received error code #{response.code}" unless response.success?
       end
 
       def headers(sanitized_uri)
-        {}.tap do |headers|
+        query_options.tap do |headers|
           credentials = sanitized_uri.credentials
 
           if credentials[:user].present? || credentials[:password].present?
@@ -43,10 +58,14 @@ module Projects
         end
       end
 
+      def query_options
+        { stream_body: true }
+      end
+
       def with_tmp_file(oid)
         create_tmp_storage_dir
 
-        File.open(File.join(tmp_storage_dir, oid), 'w') { |file| yield file }
+        File.open(File.join(tmp_storage_dir, oid), 'wb') { |file| yield file }
       end
 
       def create_tmp_storage_dir

spec/services/projects/lfs_pointers/lfs_download_service_spec.rb

@@ -4,17 +4,15 @@ describe Projects::LfsPointers::LfsDownloadService do
   let(:project) { create(:project) }
   let(:oid) { '9e548e25631dd9ce6b43afd6359ab76da2819d6a5b474e66118c7819e1d8b3e8' }
   let(:download_link) { "http://gitlab.com/#{oid}" }
-  let(:lfs_content) do
-    <<~HEREDOC
-    whatever
-    HEREDOC
-  end
+  let(:lfs_content) { SecureRandom.random_bytes(10) }
 
   subject { described_class.new(project) }
 
   before do
     allow(project).to receive(:lfs_enabled?).and_return(true)
     WebMock.stub_request(:get, download_link).to_return(body: lfs_content)
+
+    allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false)
   end
 
   describe '#execute' do
@@ -32,7 +30,7 @@ describe Projects::LfsPointers::LfsDownloadService do
       it 'stores the content' do
         subject.execute(oid, download_link)
 
-        expect(File.read(LfsObject.first.file.file.file)).to eq lfs_content
+        expect(File.binread(LfsObject.first.file.file.file)).to eq lfs_content
       end
     end
 
@@ -54,18 +52,61 @@ describe Projects::LfsPointers::LfsDownloadService do
       end
     end
 
+    context 'when localhost requests are allowed' do
+      let(:download_link) { 'http://192.168.2.120' }
+
+      before do
+        allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(true)
+      end
+
+      it 'downloads the file' do
+        expect(subject).to receive(:download_and_save_file).and_call_original
+
+        expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.by(1)
+      end
+    end
+
     context 'when a bad URL is used' do
-      where(download_link: ['/etc/passwd', 'ftp://example.com', 'http://127.0.0.2'])
+      where(download_link: ['/etc/passwd', 'ftp://example.com', 'http://127.0.0.2', 'http://192.168.2.120'])
 
       with_them do
         it 'does not download the file' do
-          expect(subject).not_to receive(:download_and_save_file)
-
           expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count }
         end
       end
     end
 
+    context 'when the URL points to a redirected URL' do
+      context 'that is blocked' do
+        where(redirect_link: ['ftp://example.com', 'http://127.0.0.2', 'http://192.168.2.120'])
+
+        with_them do
+          before do
+            WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link })
+          end
+
+          it 'does not follow the redirection' do
+            expect(Rails.logger).to receive(:error).with(/LFS file with oid #{oid} couldn't be downloaded/)
+
+            expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count }
+          end
+        end
+      end
+
+      context 'that is valid' do
+        let(:redirect_link) { "http://example.com/"}
+
+        before do
+          WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link })
+          WebMock.stub_request(:get, redirect_link).to_return(body: lfs_content)
+        end
+
+        it 'follows the redirection' do
+          expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.from(0).to(1)
+        end
+      end
+    end
+
     context 'when an lfs object with the same oid already exists'  do
       before do
         create(:lfs_object, oid: 'oid')