Commit dc9b3db8 authored by Douwe Maan's avatar Douwe Maan Committed by Rémy Coutable

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: default avatarRémy Coutable <remy@rymai.me>
parent cfb511ea
......@@ -43,6 +43,14 @@ module Gitlab
raise Projects::ImportService::Error.new("Unable to decompress #{@archive_file} into #{@shared.export_path}") unless result
remove_symlinks!
end
def remove_symlinks!
Dir["#{@shared.export_path}/**/*"].each do |path|
FileUtils.rm(path) if File.lstat(path).symlink?
end
true
end
end
......
......@@ -9,8 +9,14 @@ module Gitlab
end
def restore
json = IO.read(@path)
@tree_hash = ActiveSupport::JSON.decode(json)
begin
json = IO.read(@path)
@tree_hash = ActiveSupport::JSON.decode(json)
rescue => e
Rails.logger.error("Import/Export error: #{e.message}")
raise Gitlab::ImportExport::Error.new('Incorrect JSON format')
end
@project_members = @tree_hash.delete('project_members')
ActiveRecord::Base.no_touching do
......
......@@ -24,12 +24,19 @@ module Gitlab
end
def verify_version!(version)
if Gem::Version.new(version) != Gem::Version.new(Gitlab::ImportExport.version)
if different_version?(version)
raise Gitlab::ImportExport::Error.new("Import version mismatch: Required #{Gitlab::ImportExport.version} but was #{version}")
else
true
end
end
def different_version?(version)
Gem::Version.new(version) != Gem::Version.new(Gitlab::ImportExport.version)
rescue => e
Rails.logger.error("Import/Export error: #{e.message}")
raise Gitlab::ImportExport::Error.new('Incorrect VERSION format')
end
end
end
end
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
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
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
module ImportExport
module CommonUtil
def setup_symlink(tmpdir, symlink_name)
allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(tmpdir)
File.open("#{tmpdir}/test", 'w') { |file| file.write("test") }
FileUtils.ln_s("#{tmpdir}/test", "#{tmpdir}/#{symlink_name}")
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment