summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorYorick Peterse <yorickpeterse@gmail.com>2019-01-15 12:49:50 +0000
committerYorick Peterse <yorickpeterse@gmail.com>2019-01-15 12:51:05 +0000
commitf9ef74fca85d8d91f418601d2a3fa620ca4b69ef (patch)
tree5afe3d5b90aa39534fb2bfbab893a714cf4f78d5
parenta01a29e09158647967bd808406c32bdec0114f7a (diff)
downloadgitlab-ce-f9ef74fca85d8d91f418601d2a3fa620ca4b69ef.tar.gz
Merge branch 'security-2770-verify-bundle-import-files-11-5' into 'security-11-5'
[11.5] Validate bundle files before unpacking them See merge request gitlab/gitlabhq!2775 (cherry picked from commit 28bec61b5d3c43ef896780cb0eebf09353b51995) 68433868 Validate bundle files before unpacking them
-rw-r--r--changelogs/unreleased/security-2770-verify-bundle-import-files-11-5.yml5
-rw-r--r--lib/gitlab/git/bundle_file.rb30
-rw-r--r--lib/gitlab/git/repository.rb5
-rw-r--r--spec/fixtures/malicious.bundle1
-rw-r--r--spec/lib/gitlab/git/bundle_file_spec.rb26
-rw-r--r--spec/lib/gitlab/git/repository_spec.rb17
6 files changed, 79 insertions, 5 deletions
diff --git a/changelogs/unreleased/security-2770-verify-bundle-import-files-11-5.yml b/changelogs/unreleased/security-2770-verify-bundle-import-files-11-5.yml
new file mode 100644
index 00000000000..dea40dd1ef1
--- /dev/null
+++ b/changelogs/unreleased/security-2770-verify-bundle-import-files-11-5.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 1642c4c5687..756bdb20774 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -770,6 +770,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 54291e847d8..a19b1442abe 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -1726,22 +1726,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
@@ -1749,11 +1750,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