Merge branch '54857-fix-templates-path-traversal' into 'master'

[master]: Prevent a path traversal attack on global file templates

Closes #2745

See merge request gitlab/gitlabhq!2677

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 8dab19d50c2..51f357d9477 100644
--- a/lib/api/templates.rb
+++ b/lib/api/templates.rb
@@ -82,7 +82,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, nil, 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 76bb9eb611e..2dd4b7a4092 100644
--- a/lib/gitlab/template/finders/global_template_finder.rb
+++ b/lib/gitlab/template/finders/global_template_finder.rb
@@ -18,6 +18,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 b92cefefb8f..8e234148a63 100644
--- a/lib/gitlab/template/finders/repo_template_finder.rb
+++ b/lib/gitlab/template/finders/repo_template_finder.rb
@@ -26,6 +26,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 e0e8f598ba4..26fc56227a2 100644
--- a/lib/gitlab/utils.rb
+++ b/lib/gitlab/utils.rb
@@ -4,6 +4,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 3579ed9a759..47a5fd0bdb4 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, :append_path, to: :described_class
+   :bytes_to_megabytes, :append_path, :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
     {