Commit 69645389 authored by Nick Thomas's avatar Nick Thomas

Prevent a path traversal attack on global file templates

The API permits path traversal characters like '../' to be passed down
to the template finder. Detect these requests and cause them to fail
with a 500 response code.
parent 87186cbc
---
title: Prevent a path traversal attack on global file templates
merge_request:
author:
type: security
...@@ -82,7 +82,7 @@ module API ...@@ -82,7 +82,7 @@ module API
params do params do
requires :name, type: String, desc: 'The name of the template' requires :name, type: String, desc: 'The name of the template'
end 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]) finder = TemplateFinder.build(template_type, nil, name: declared(params)[:name])
new_template = finder.execute new_template = finder.execute
......
...@@ -18,6 +18,10 @@ module Gitlab ...@@ -18,6 +18,10 @@ module Gitlab
def find(key) def find(key)
file_name = "#{key}#{@extension}" 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 = select_directory(file_name)
directory ? File.join(category_directory(directory), file_name) : nil directory ? File.join(category_directory(directory), file_name) : nil
end end
......
...@@ -26,6 +26,11 @@ module Gitlab ...@@ -26,6 +26,11 @@ module Gitlab
def find(key) def find(key)
file_name = "#{key}#{@extension}" 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) directory = select_directory(file_name)
raise FileNotFoundError if directory.nil? raise FileNotFoundError if directory.nil?
......
...@@ -4,6 +4,15 @@ module Gitlab ...@@ -4,6 +4,15 @@ module Gitlab
module Utils module Utils
extend self 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. # Run system command without outputting to stdout.
# #
# @param cmd [Array<String>] # @param cmd [Array<String>]
......
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
...@@ -25,6 +25,10 @@ describe Gitlab::Template::Finders::RepoTemplateFinder do ...@@ -25,6 +25,10 @@ describe Gitlab::Template::Finders::RepoTemplateFinder do
expect(result).to eq('files/html/500.html') expect(result).to eq('files/html/500.html')
end end
it 'does not permit path traversal requests' do
expect { finder.find('../foo') }.to raise_error(/Invalid path/)
end
end end
describe '#list_files_for' do describe '#list_files_for' do
......
...@@ -2,7 +2,33 @@ require 'spec_helper' ...@@ -2,7 +2,33 @@ require 'spec_helper'
describe Gitlab::Utils do describe Gitlab::Utils do
delegate :to_boolean, :boolean_to_yes_no, :slugify, :random_string, :which, :ensure_array_from_string, 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 describe '.slugify' do
{ {
......
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