Skip to content
Snippets Groups Projects
Commit 7a9669a3 authored by Robert May's avatar Robert May Committed by GitLab Release Tools Bot
Browse files

Prevent default branches from storing paths

Merge branch 'security-383709' into 'master'

See merge request gitlab-org/security/gitlab!2999

Changelog: security
parent 0a7225bd
No related branches found
No related tags found
No related merge requests found
......@@ -45,6 +45,15 @@ def sanitizes!(*attrs)
unless input.to_s == CGI.unescapeHTML(input.to_s)
record.errors.add(attr, 'cannot contain escaped HTML entities')
end
# This method raises an exception on failure so perform this
# last if multiple errors should be returned.
Gitlab::Utils.check_path_traversal!(input.to_s)
rescue Gitlab::Utils::DoubleEncodingError
record.errors.add(attr, 'cannot contain escaped components')
rescue Gitlab::Utils::PathTraversalAttackError
record.errors.add(attr, "cannot contain a path traversal component")
end
end
end
......
......@@ -14,10 +14,11 @@ class NamespaceSetting < ApplicationRecord
validates :enabled_git_access_protocol, inclusion: { in: enabled_git_access_protocols.keys }
validate :default_branch_name_content
validate :allow_mfa_for_group
validate :allow_resource_access_token_creation_for_group
sanitizes! :default_branch_name
before_validation :normalize_default_branch_name
chronic_duration_attr :runner_token_expiration_interval_human_readable, :runner_token_expiration_interval
......@@ -45,8 +46,6 @@ def self.allowed_namespace_settings_params
NAMESPACE_SETTINGS_PARAMS
end
sanitizes! :default_branch_name
def prevent_sharing_groups_outside_hierarchy
return super if namespace.root?
......@@ -85,14 +84,6 @@ def normalize_default_branch_name
self.default_branch_name = default_branch_name.presence
end
def default_branch_name_content
return if default_branch_name.nil?
if default_branch_name.blank?
errors.add(:default_branch_name, "can not be an empty string")
end
end
def allow_mfa_for_group
if namespace&.subgroup? && allow_mfa_for_subgroups == false
errors.add(:allow_mfa_for_subgroups, _('is not allowed since the group is not top-level group.'))
......
......@@ -4,6 +4,7 @@ module Gitlab
module Utils
extend self
PathTraversalAttackError ||= Class.new(StandardError)
DoubleEncodingError ||= Class.new(StandardError)
private_class_method def logger
@logger ||= Gitlab::AppLogger
......@@ -55,7 +56,7 @@ def check_allowed_absolute_path_and_path_traversal!(path, path_allowlist)
def decode_path(encoded_path)
decoded = CGI.unescape(encoded_path)
if decoded != CGI.unescape(decoded)
raise StandardError, "path #{encoded_path} is not allowed"
raise DoubleEncodingError, "path #{encoded_path} is not allowed"
end
decoded
......
......@@ -75,7 +75,58 @@ def self.model_name
it 'is not valid', :aggregate_failures do
expect(record).not_to be_valid
expect(record.errors.full_messages).to include('Name cannot contain escaped HTML entities')
expect(record.errors.full_messages).to contain_exactly(
'Name cannot contain escaped HTML entities',
'Description cannot contain escaped HTML entities'
)
end
end
context 'when input contains double-escaped data' do
let_it_be(:input) do
'%2526lt%253Bscript%2526gt%253Balert%25281%2529%2526lt%253B%252Fscript%2526gt%253B'
end
it_behaves_like 'noop'
it 'is not valid', :aggregate_failures do
expect(record).not_to be_valid
expect(record.errors.full_messages).to contain_exactly(
'Name cannot contain escaped components',
'Description cannot contain escaped components'
)
end
end
context 'when input contains a path traversal attempt' do
let_it_be(:input) { 'main../../../../../../api/v4/projects/1/import_project_members/2' }
it_behaves_like 'noop'
it 'is not valid', :aggregate_failures do
expect(record).not_to be_valid
expect(record.errors.full_messages).to contain_exactly(
'Name cannot contain a path traversal component',
'Description cannot contain a path traversal component'
)
end
end
context 'when input contains both path traversal attempt and pre-escaped entities' do
let_it_be(:input) do
'main../../../../../../api/v4/projects/1/import_project_members/2&lt;script&gt;alert(1)&lt;/script&gt;'
end
it_behaves_like 'noop'
it 'is not valid', :aggregate_failures do
expect(record).not_to be_valid
expect(record.errors.full_messages).to contain_exactly(
'Name cannot contain a path traversal component',
'Name cannot contain escaped HTML entities',
'Description cannot contain a path traversal component',
'Description cannot contain escaped HTML entities'
)
end
end
end
......
......@@ -18,7 +18,7 @@
describe "#default_branch_name_content" do
let_it_be(:group) { create(:group) }
let(:namespace_settings) { group.namespace_settings }
subject(:namespace_settings) { group.namespace_settings }
shared_examples "doesn't return an error" do
it "doesn't return an error" do
......@@ -28,6 +28,10 @@
end
context "when not set" do
before do
namespace_settings.default_branch_name = nil
end
it_behaves_like "doesn't return an error"
end
......
......@@ -32,8 +32,25 @@
subject { build(factory, attributes) }
it 'is not valid', :aggregate_failures do
error = 'cannot contain escaped HTML entities'
expect(subject).not_to be_valid
expect(subject.errors.details[field].flat_map(&:values)).to contain_exactly(error)
end
end
context 'when it contains a path component' do
let_it_be(:input) do
'main../../../../../../api/v4/projects/1/import_project_members/2'
end
subject { build(factory, attributes) }
it 'is not valid', :aggregate_failures do
error = 'cannot contain a path traversal component'
expect(subject).not_to be_valid
expect(subject.errors.details[field].flat_map(&:values)).to include('cannot contain escaped HTML entities')
expect(subject.errors.details[field].flat_map(&:values)).to contain_exactly(error)
end
end
end
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment