Merge branch 'fix/import-rce-10-3' into 'security-10-3'

[10.3] Fix RCE via project import mechanism

See merge request gitlab/gitlabhq!2294

changelogs/unreleased/fix-import-rce.yml

+---
+title: Fix RCE via project import mechanism
+merge_request:
+author:
+type: security

lib/gitlab/import_export/file_importer.rb

@@ -17,12 +17,16 @@ module Gitlab
       def import
         mkdir_p(@shared.export_path)
 
+        remove_symlinks!
+
         wait_for_archived_file do
           decompress_archive
         end
       rescue => e
         @shared.error(e)
         false
+      ensure
+        remove_symlinks!
       end
 
       private
@@ -43,7 +47,7 @@ module Gitlab
 
         raise Projects::ImportService::Error.new("Unable to decompress #{@archive_file} into #{@shared.export_path}") unless result
 
-        remove_symlinks!
+        result
       end
 
       def remove_symlinks!

lib/gitlab/import_export/saver.rb

@@ -37,7 +37,7 @@ module Gitlab
       end
 
       def archive_file
-        @archive_file ||= File.join(@shared.export_path, '..', Gitlab::ImportExport.export_filename(project: @project))
+        @archive_file ||= File.join(@shared.archive_path, Gitlab::ImportExport.export_filename(project: @project))
       end
     end
   end

lib/gitlab/import_export/shared.rb

@@ -9,7 +9,11 @@ module Gitlab
       end
 
       def export_path
-        @export_path ||= Gitlab::ImportExport.export_path(relative_path: opts[:relative_path])
+        @export_path ||= Gitlab::ImportExport.export_path(relative_path: relative_path)
+      end
+
+      def archive_path
+        @archive_path ||= Gitlab::ImportExport.export_path(relative_path: relative_archive_path)
       end
 
       def error(error)
@@ -21,6 +25,14 @@ module Gitlab
 
       private
 
+      def relative_path
+        File.join(opts[:relative_path], SecureRandom.hex)
+      end
+
+      def relative_archive_path
+        File.join(opts[:relative_path], '..')
+      end
+
       def error_out(message, caller)
         Rails.logger.error("Import/Export error raised on #{caller}: #{message}")
       end

spec/lib/gitlab/import_export/file_importer_spec.rb

@@ -12,30 +12,61 @@ describe Gitlab::ImportExport::FileImporter do
     stub_const('Gitlab::ImportExport::FileImporter::MAX_RETRIES', 0)
     allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path)
     allow_any_instance_of(Gitlab::ImportExport::CommandLineUtil).to receive(:untar_zxf).and_return(true)
-
+    allow(SecureRandom).to receive(:hex).and_return('abcd')
     setup_files
-
-    described_class.import(archive_file: '', shared: shared)
   end
 
   after do
     FileUtils.rm_rf(export_path)
   end
 
-  it 'removes symlinks in root folder' do
-    expect(File.exist?(symlink_file)).to be false
-  end
+  context 'normal run' do
+    before do
+      described_class.import(archive_file: '', shared: shared)
+    end
 
-  it 'removes hidden symlinks in root folder' do
-    expect(File.exist?(hidden_symlink_file)).to be false
-  end
+    it 'removes symlinks in root folder' do
+      expect(File.exist?(symlink_file)).to be false
+    end
+
+    it 'removes hidden symlinks in root folder' do
+      expect(File.exist?(hidden_symlink_file)).to be false
+    end
+
+    it 'removes symlinks in subfolders' do
+      expect(File.exist?(subfolder_symlink_file)).to be false
+    end
 
-  it 'removes symlinks in subfolders' do
-    expect(File.exist?(subfolder_symlink_file)).to be false
+    it 'does not remove a valid file' do
+      expect(File.exist?(valid_file)).to be true
+    end
+
+    it 'creates the file in the right subfolder' do
+      expect(shared.export_path).to include('test/abcd')
+    end
   end
 
-  it 'does not remove a valid file' do
-    expect(File.exist?(valid_file)).to be true
+  context 'error' do
+    before do
+      allow_any_instance_of(described_class).to receive(:wait_for_archived_file).and_raise(StandardError)
+      described_class.import(archive_file: '', shared: shared)
+    end
+
+    it 'removes symlinks in root folder' do
+      expect(File.exist?(symlink_file)).to be false
+    end
+
+    it 'removes hidden symlinks in root folder' do
+      expect(File.exist?(hidden_symlink_file)).to be false
+    end
+
+    it 'removes symlinks in subfolders' do
+      expect(File.exist?(subfolder_symlink_file)).to be false
+    end
+
+    it 'does not remove a valid file' do
+      expect(File.exist?(valid_file)).to be true
+    end
   end
 
   def setup_files