Merge branch 'fix/import-export-symlink-vulnerability' into 'security'

Fix symlink vulnerability in Import/Export

Replaces https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2018 made by @james

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/23822

See merge request !2022

Signed-off-by: Rémy Coutable <remy@rymai.me>

3 files changed, 71 insertions, 1 deletions

diff --git a/spec/lib/gitlab/import_export/file_importer_spec.rb b/spec/lib/gitlab/import_export/file_importer_spec.rb
new file mode 100644
index 00000000000..a88ddd17aca
--- /dev/null
+++ b/spec/lib/gitlab/import_export/file_importer_spec.rb
@@ -0,0 +1,42 @@
+require 'spec_helper'
+
+describe Gitlab::ImportExport::FileImporter, lib: true do
+  let(:shared) { Gitlab::ImportExport::Shared.new(relative_path: 'test') }
+  let(:export_path) { "#{Dir::tmpdir}/file_importer_spec" }
+  let(:valid_file) { "#{shared.export_path}/valid.json" }
+  let(:symlink_file) { "#{shared.export_path}/invalid.json" }
+  let(:subfolder_symlink_file) { "#{shared.export_path}/subfolder/invalid.json" }
+
+  before 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)
+
+    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
+
+  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
+
+  def setup_files
+    FileUtils.mkdir_p("#{shared.export_path}/subfolder/")
+    FileUtils.touch(valid_file)
+    FileUtils.ln_s(valid_file, symlink_file)
+    FileUtils.ln_s(valid_file, subfolder_symlink_file)
+  end
+end
diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
index 069ea960321..3038ab53ad8 100644
--- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
+++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb
@@ -1,4 +1,5 @@
 require 'spec_helper'
+include ImportExport::CommonUtil
 
 describe Gitlab::ImportExport::ProjectTreeRestorer, services: true do
   describe 'restore project tree' do
@@ -175,6 +176,19 @@ describe Gitlab::ImportExport::ProjectTreeRestorer, services: true do
           expect(MergeRequest.find_by_title('MR2').source_project_id).to eq(-1)
         end
       end
+
+      context 'project.json file access check' do
+        it 'does not read a symlink' do
+          Dir.mktmpdir do |tmpdir|
+            setup_symlink(tmpdir, 'project.json')
+            allow(shared).to receive(:export_path).and_call_original
+
+            restored_project_json
+
+            expect(shared.errors.first).not_to include('test')
+          end
+        end
+      end
     end
   end
 end
diff --git a/spec/lib/gitlab/import_export/version_checker_spec.rb b/spec/lib/gitlab/import_export/version_checker_spec.rb
index c680e712b59..2405ac5abfe 100644
--- a/spec/lib/gitlab/import_export/version_checker_spec.rb
+++ b/spec/lib/gitlab/import_export/version_checker_spec.rb
@@ -1,8 +1,10 @@
 require 'spec_helper'
+include ImportExport::CommonUtil
 
 describe Gitlab::ImportExport::VersionChecker, services: true do
+  let(:shared) { Gitlab::ImportExport::Shared.new(relative_path: '') }
+
   describe 'bundle a project Git repo' do
-    let(:shared) { Gitlab::ImportExport::Shared.new(relative_path: '') }
     let(:version) { Gitlab::ImportExport.version }
 
     before do
@@ -27,4 +29,16 @@ describe Gitlab::ImportExport::VersionChecker, services: true do
       end
     end
   end
+
+  describe 'version file access check' do
+    it 'does not read a symlink' do
+      Dir.mktmpdir do |tmpdir|
+        setup_symlink(tmpdir, 'VERSION')
+
+        described_class.check!(shared: shared)
+
+        expect(shared.errors.first).not_to include('test')
+      end
+    end
+  end
 end