diff options
author | Cindy Pallares <cindy@gitlab.com> | 2018-12-06 02:09:02 +0300 |
---|---|---|
committer | Cindy Pallares <cindy@gitlab.com> | 2018-12-06 02:27:06 +0300 |
commit | 90d09306d976fb6ca96be6fce09e4e89b9a34c97 (patch) | |
tree | 4d789d8532d94384e888bc65b46f384770e4af4b | |
parent | 9c30391cbee2ccf2906d592a29e3533bdb9e9375 (diff) |
Merge branch 'security-54857-fix-templates-path-traversal-11-3' into 'security-11-3'
[11.3] Prevent a path traversal attack on global file templates
See merge request gitlab/gitlabhq!2671
-rw-r--r-- | changelogs/unreleased/54857-fix-templates-path-traversal.yml | 5 | ||||
-rw-r--r-- | lib/api/templates.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/template/finders/global_template_finder.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/template/finders/repo_template_finder.rb | 5 | ||||
-rw-r--r-- | lib/gitlab/utils.rb | 9 | ||||
-rw-r--r-- | spec/lib/gitlab/template/finders/global_template_finder_spec.rb | 35 | ||||
-rw-r--r-- | spec/lib/gitlab/template/finders/repo_template_finders_spec.rb | 4 | ||||
-rw-r--r-- | spec/lib/gitlab/utils_spec.rb | 28 |
8 files changed, 90 insertions, 2 deletions
diff --git a/changelogs/unreleased/54857-fix-templates-path-traversal.yml b/changelogs/unreleased/54857-fix-templates-path-traversal.yml new file mode 100644 index 00000000000..0da02432c60 --- /dev/null +++ b/changelogs/unreleased/54857-fix-templates-path-traversal.yml @@ -0,0 +1,5 @@ +--- +title: Prevent a path traversal attack on global file templates +merge_request: +author: +type: security diff --git a/lib/api/templates.rb b/lib/api/templates.rb index 7bf0e0f5934..a9342153c42 100644 --- a/lib/api/templates.rb +++ b/lib/api/templates.rb @@ -81,7 +81,7 @@ module API params do requires :name, type: String, desc: 'The name of the template' end - get "templates/#{template_type}/:name" do + get "templates/#{template_type}/:name", requirements: { name: /[\w\.-]+/ } do finder = TemplateFinder.build(template_type, name: declared(params)[:name]) new_template = finder.execute diff --git a/lib/gitlab/template/finders/global_template_finder.rb b/lib/gitlab/template/finders/global_template_finder.rb index 831da45191f..1b97020e6e9 100644 --- a/lib/gitlab/template/finders/global_template_finder.rb +++ b/lib/gitlab/template/finders/global_template_finder.rb @@ -16,6 +16,10 @@ module Gitlab def find(key) file_name = "#{key}#{@extension}" + # The key is untrusted input, so ensure we can't be directed outside + # of base_dir + Gitlab::Utils.check_path_traversal!(file_name) + directory = select_directory(file_name) directory ? File.join(category_directory(directory), file_name) : nil end diff --git a/lib/gitlab/template/finders/repo_template_finder.rb b/lib/gitlab/template/finders/repo_template_finder.rb index 29bc2393ff9..6e4ffc70282 100644 --- a/lib/gitlab/template/finders/repo_template_finder.rb +++ b/lib/gitlab/template/finders/repo_template_finder.rb @@ -24,6 +24,11 @@ module Gitlab def find(key) file_name = "#{key}#{@extension}" + + # The key is untrusted input, so ensure we can't be directed outside + # of base_dir inside the repository + Gitlab::Utils.check_path_traversal!(file_name) + directory = select_directory(file_name) raise FileNotFoundError if directory.nil? diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb index aeda66763e8..6ec91b26799 100644 --- a/lib/gitlab/utils.rb +++ b/lib/gitlab/utils.rb @@ -2,6 +2,15 @@ module Gitlab module Utils extend self + # Ensure that the relative path will not traverse outside the base directory + def check_path_traversal!(path) + raise StandardError.new("Invalid path") if path.start_with?("..#{File::SEPARATOR}") || + path.include?("#{File::SEPARATOR}..#{File::SEPARATOR}") || + path.end_with?("#{File::SEPARATOR}..") + + path + end + # Run system command without outputting to stdout. # # @param cmd [Array<String>] diff --git a/spec/lib/gitlab/template/finders/global_template_finder_spec.rb b/spec/lib/gitlab/template/finders/global_template_finder_spec.rb new file mode 100644 index 00000000000..c7f58fbd2a5 --- /dev/null +++ b/spec/lib/gitlab/template/finders/global_template_finder_spec.rb @@ -0,0 +1,35 @@ +require 'spec_helper' + +describe Gitlab::Template::Finders::GlobalTemplateFinder do + let(:base_dir) { Dir.mktmpdir } + + def create_template!(name_with_category) + full_path = File.join(base_dir, name_with_category) + FileUtils.mkdir_p(File.dirname(full_path)) + FileUtils.touch(full_path) + end + + after do + FileUtils.rm_rf(base_dir) + end + + subject(:finder) { described_class.new(base_dir, '', 'Foo' => '', 'Bar' => 'bar') } + + describe '.find' do + it 'finds a template in the Foo category' do + create_template!('test-template') + + expect(finder.find('test-template')).to be_present + end + + it 'finds a template in the Bar category' do + create_template!('bar/test-template') + + expect(finder.find('test-template')).to be_present + end + + it 'does not permit path traversal requests' do + expect { finder.find('../foo') }.to raise_error(/Invalid path/) + end + end +end diff --git a/spec/lib/gitlab/template/finders/repo_template_finders_spec.rb b/spec/lib/gitlab/template/finders/repo_template_finders_spec.rb index 2eabccd5dff..e329d55d837 100644 --- a/spec/lib/gitlab/template/finders/repo_template_finders_spec.rb +++ b/spec/lib/gitlab/template/finders/repo_template_finders_spec.rb @@ -25,6 +25,10 @@ describe Gitlab::Template::Finders::RepoTemplateFinder do expect(result).to eq('files/html/500.html') end + + it 'does not permit path traversal requests' do + expect { finder.find('../foo') }.to raise_error(/Invalid path/) + end end describe '#list_files_for' do diff --git a/spec/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb index 4ba99009855..f7528f296c3 100644 --- a/spec/lib/gitlab/utils_spec.rb +++ b/spec/lib/gitlab/utils_spec.rb @@ -2,7 +2,33 @@ require 'spec_helper' describe Gitlab::Utils do delegate :to_boolean, :boolean_to_yes_no, :slugify, :random_string, :which, :ensure_array_from_string, - :bytes_to_megabytes, to: :described_class + :bytes_to_megabytes, :check_path_traversal!, to: :described_class + + describe '.check_path_traversal!' do + it 'detects path traversal at the start of the string' do + expect { check_path_traversal!('../foo') }.to raise_error(/Invalid path/) + end + + it 'detects path traversal at the start of the string, even to just the subdirectory' do + expect { check_path_traversal!('../') }.to raise_error(/Invalid path/) + end + + it 'detects path traversal in the middle of the string' do + expect { check_path_traversal!('foo/../../bar') }.to raise_error(/Invalid path/) + end + + it 'detects path traversal at the end of the string when slash-terminates' do + expect { check_path_traversal!('foo/../') }.to raise_error(/Invalid path/) + end + + it 'detects path traversal at the end of the string' do + expect { check_path_traversal!('foo/..') }.to raise_error(/Invalid path/) + end + + it 'does nothing for a safe string' do + expect(check_path_traversal!('./foo')).to eq('./foo') + end + end describe '.slugify' do { |