diff options
author | Yorick Peterse <yorickpeterse@gmail.com> | 2019-01-15 12:49:47 +0000 |
---|---|---|
committer | Yorick Peterse <yorickpeterse@gmail.com> | 2019-01-15 12:50:50 +0000 |
commit | 56c0f733d55fda4a0ac46687702f1b2ab39115bc (patch) | |
tree | 255e938853fb3695dc11f6332ba5ced05bbce596 | |
parent | ee33bcba81b3d908e999c414860d02f3a867aed6 (diff) | |
download | gitlab-ce-56c0f733d55fda4a0ac46687702f1b2ab39115bc.tar.gz |
Merge branch 'security-2770-verify-bundle-import-files-11-4' into 'security-11-4'
[11.4] Validate bundle files before unpacking them
See merge request gitlab/gitlabhq!2776
(cherry picked from commit 6176b02aa6577079986410719884bd253dc5e7be)
e5e5e77e Validate bundle files before unpacking them
-rw-r--r-- | changelogs/unreleased/security-2770-verify-bundle-import-files-11-4.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/git/bundle_file.rb | 30 | ||||
-rw-r--r-- | lib/gitlab/git/repository.rb | 5 | ||||
-rw-r--r-- | spec/fixtures/malicious.bundle | 1 | ||||
-rw-r--r-- | spec/lib/gitlab/git/bundle_file_spec.rb | 26 | ||||
-rw-r--r-- | spec/lib/gitlab/git/repository_spec.rb | 17 |
6 files changed, 79 insertions, 5 deletions
diff --git a/changelogs/unreleased/security-2770-verify-bundle-import-files-11-4.yml b/changelogs/unreleased/security-2770-verify-bundle-import-files-11-4.yml new file mode 100644 index 00000000000..dea40dd1ef1 --- /dev/null +++ b/changelogs/unreleased/security-2770-verify-bundle-import-files-11-4.yml @@ -0,0 +1,5 @@ +--- +title: Validate bundle files before unpacking them +merge_request: +author: +type: security diff --git a/lib/gitlab/git/bundle_file.rb b/lib/gitlab/git/bundle_file.rb new file mode 100644 index 00000000000..8384a436fcc --- /dev/null +++ b/lib/gitlab/git/bundle_file.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module Gitlab + module Git + class BundleFile + # All git bundle files start with this string + # + # https://github.com/git/git/blob/v2.20.1/bundle.c#L15 + MAGIC = "# v2 git bundle\n" + + InvalidBundleError = Class.new(StandardError) + + attr_reader :filename + + def self.check!(filename) + new(filename).check! + end + + def initialize(filename) + @filename = filename + end + + def check! + data = File.open(filename, 'r') { |f| f.read(MAGIC.size) } + + raise InvalidBundleError, 'Invalid bundle file' unless data == MAGIC + end + end + end +end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 7732049b69b..77d7f40228c 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -739,6 +739,11 @@ module Gitlab end def create_from_bundle(bundle_path) + # It's important to check that the linked-to file is actually a valid + # .bundle file as it is passed to `git clone`, which may otherwise + # interpret it as a pointer to another repository + ::Gitlab::Git::BundleFile.check!(bundle_path) + gitaly_repository_client.create_from_bundle(bundle_path) end diff --git a/spec/fixtures/malicious.bundle b/spec/fixtures/malicious.bundle new file mode 100644 index 00000000000..7ba47932906 --- /dev/null +++ b/spec/fixtures/malicious.bundle @@ -0,0 +1 @@ +gitdir: foo.git diff --git a/spec/lib/gitlab/git/bundle_file_spec.rb b/spec/lib/gitlab/git/bundle_file_spec.rb new file mode 100644 index 00000000000..ff7c981dadd --- /dev/null +++ b/spec/lib/gitlab/git/bundle_file_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' + +describe Gitlab::Git::BundleFile do + describe '.check!' do + let(:valid_bundle) { Tempfile.new } + let(:valid_bundle_path) { valid_bundle.path } + let(:invalid_bundle_path) { Rails.root.join('spec/fixtures/malicious.bundle') } + + after do + valid_bundle.close! + end + + it 'returns nil for a valid bundle' do + valid_bundle.write("# v2 git bundle\nfoo bar baz\n") + valid_bundle.close + + expect(described_class.check!(valid_bundle_path)).to be_nil + end + + it 'raises an exception for an invalid bundle' do + expect do + described_class.check!(invalid_bundle_path) + end.to raise_error(described_class::InvalidBundleError) + end + end +end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 51eb997a325..87bdf5e3ce0 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -1677,22 +1677,23 @@ describe Gitlab::Git::Repository, :seed_helper do end describe '#create_from_bundle' do - let(:bundle_path) { File.join(Dir.tmpdir, "repo-#{SecureRandom.hex}.bundle") } + let(:valid_bundle_path) { File.join(Dir.tmpdir, "repo-#{SecureRandom.hex}.bundle") } + let(:malicious_bundle_path) { Rails.root.join('spec/fixtures/malicious.bundle') } let(:project) { create(:project) } let(:imported_repo) { project.repository.raw } before do - expect(repository.bundle_to_disk(bundle_path)).to be_truthy + expect(repository.bundle_to_disk(valid_bundle_path)).to be_truthy end after do - FileUtils.rm_rf(bundle_path) + FileUtils.rm_rf(valid_bundle_path) end it 'creates a repo from a bundle file' do expect(imported_repo).not_to exist - result = imported_repo.create_from_bundle(bundle_path) + result = imported_repo.create_from_bundle(valid_bundle_path) expect(result).to be_truthy expect(imported_repo).to exist @@ -1700,11 +1701,17 @@ describe Gitlab::Git::Repository, :seed_helper do end it 'creates a symlink to the global hooks dir' do - imported_repo.create_from_bundle(bundle_path) + imported_repo.create_from_bundle(valid_bundle_path) hooks_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access { File.join(imported_repo.path, 'hooks') } expect(File.readlink(hooks_path)).to eq(Gitlab.config.gitlab_shell.hooks_path) end + + it 'raises an error if the bundle is an attempted malicious payload' do + expect do + imported_repo.create_from_bundle(malicious_bundle_path) + end.to raise_error(::Gitlab::Git::BundleFile::InvalidBundleError) + end end describe '#checksum' do |