Skip to content
代码片段 群组 项目
未验证 提交 e805a78d 编辑于 作者: David Fernandez's avatar David Fernandez 提交者： GitLab
浏览文件

Ignore new lines 

in the path traversal middleware.
Also, decompose the regex so that we can use it with or without the new line char included.
上级 c250f935
No related branches found
No related tags found
无相关合并请求
隐藏空白变更内容
行内 左右并排
lib/gitlab/middleware/path_traversal_check.rb
+ 3
− 2
浏览文件 @ e805a78d
......@@ -66,8 +66,9 @@ def measure_execution_time(&blk)
def check(request, log_params)
decoded_fullpath = CGI.unescape(request.fullpath)
::Gitlab::PathTraversal.check_path_traversal!(decoded_fullpath, skip_decoding: true)
rescue ::Gitlab::PathTraversal::PathTraversalAttackError
return unless Gitlab::PathTraversal.path_traversal?(decoded_fullpath, match_new_line: false)
log_params[:method] = request.request_method
log_params[:fullpath] = request.fullpath
log_params[:message] = PATH_TRAVERSAL_MESSAGE
......
lib/gitlab/path_traversal.rb
+ 17
− 4
浏览文件 @ e805a78d
......@@ -9,21 +9,22 @@ module PathTraversal
@logger ||= Gitlab::AppLogger
end
PATH_TRAVERSAL_REGEX = %r{(\A(\.{1,2})\z|\A\.\.[/\\]|[/\\]\.\.\z|[/\\]\.\.[/\\]|\n)}
PATH_TRAVERSAL_REGEX = %r{\A(\.{1,2})\z|\A\.\.[/\\]|[/\\]\.\.\z|[/\\]\.\.[/\\]}
PATH_TRAVERSAL_REGEX_WITH_NEWLINE = %r{(#{PATH_TRAVERSAL_REGEX}|\n)}
# Ensure that the relative path will not traverse outside the base directory
# We url decode the path to avoid passing invalid paths forward in url encoded format.
# Also see https://gitlab.com/gitlab-org/gitlab/-/merge_requests/24223#note_284122580
# It also checks for backslash '\', which is sometimes a File::ALT_SEPARATOR.
def check_path_traversal!(path, skip_decoding: false)
def check_path_traversal!(path)
return unless path
path = path.to_s if path.is_a?(Gitlab::HashedPath)
raise PathTraversalAttackError, 'Invalid path' unless path.is_a?(String)
path = ::Gitlab::Utils.decode_path(path) unless skip_decoding
path = ::Gitlab::Utils.decode_path(path)
if path.match?(PATH_TRAVERSAL_REGEX)
if path_traversal?(path)
logger.warn(message: "Potential path traversal attempt detected", path: path.to_s)
raise PathTraversalAttackError, 'Invalid path'
end
......@@ -31,6 +32,18 @@ def check_path_traversal!(path, skip_decoding: false)
path
end
def path_traversal?(decoded_path, match_new_line: true)
return false unless decoded_path
regex = if match_new_line
PATH_TRAVERSAL_REGEX_WITH_NEWLINE
else
PATH_TRAVERSAL_REGEX
end
decoded_path.match?(regex)
end
def check_allowed_absolute_path!(path, allowlist)
return unless Pathname.new(path).absolute?
return if ::Gitlab::Utils.allowlisted?(path, allowlist)
......
spec/lib/gitlab/middleware/path_traversal_check_spec.rb
+ 36
− 35
浏览文件 @ e805a78d
......@@ -13,6 +13,15 @@
describe '#call' do
let(:fullpath) { ::Rack::Request.new(env).fullpath }
let(:decoded_fullpath) { CGI.unescape(fullpath) }
let(:graphql_query) do
<<~QUERY
{
currentUser {
username
}
}
QUERY
end
let(:env) do
Rack::MockRequest.env_for(
......@@ -27,8 +36,8 @@
shared_examples 'no issue' do
it 'measures and logs the execution time' do
expect(::Gitlab::PathTraversal)
.to receive(:check_path_traversal!)
.with(decoded_fullpath, skip_decoding: true)
.to receive(:path_traversal?)
.with(decoded_fullpath, match_new_line: false)
.and_call_original
expect(::Gitlab::AppLogger)
.to receive(:warn)
......@@ -48,11 +57,10 @@
it 'does nothing' do
expect(::Gitlab::PathTraversal)
.to receive(:check_path_traversal!)
.with(decoded_fullpath, skip_decoding: true)
.to receive(:path_traversal?)
.with(decoded_fullpath, match_new_line: false)
.and_call_original
expect(::Gitlab::AppLogger)
.not_to receive(:warn)
expect(::Gitlab::AppLogger).not_to receive(:warn)
expect(subject).to eq(fake_response)
end
......@@ -61,8 +69,7 @@
shared_examples 'excluded path' do
it 'measures and logs the execution time' do
expect(::Gitlab::PathTraversal)
.not_to receive(:check_path_traversal!)
expect(::Gitlab::PathTraversal).not_to receive(:path_traversal?)
expect(::Gitlab::AppLogger)
.to receive(:warn)
.with({
......@@ -80,10 +87,8 @@
end
it 'does nothing' do
expect(::Gitlab::PathTraversal)
.not_to receive(:check_path_traversal!)
expect(::Gitlab::AppLogger)
.not_to receive(:warn)
expect(::Gitlab::PathTraversal).not_to receive(:path_traversal?)
expect(::Gitlab::AppLogger).not_to receive(:warn)
expect(subject).to eq(fake_response)
end
......@@ -93,12 +98,9 @@
shared_examples 'path traversal' do
it 'logs the problem and measures the execution time' do
expect(::Gitlab::PathTraversal)
.to receive(:check_path_traversal!)
.with(decoded_fullpath, skip_decoding: true)
.to receive(:path_traversal?)
.with(decoded_fullpath, match_new_line: false)
.and_call_original
expect(::Gitlab::AppLogger)
.to receive(:warn)
.with({ message: described_class::PATH_TRAVERSAL_MESSAGE, path: instance_of(String) })
expect(::Gitlab::AppLogger)
.to receive(:warn)
.with({
......@@ -120,12 +122,9 @@
it 'logs the problem without the execution time' do
expect(::Gitlab::PathTraversal)
.to receive(:check_path_traversal!)
.with(decoded_fullpath, skip_decoding: true)
.to receive(:path_traversal?)
.with(decoded_fullpath, match_new_line: false)
.and_call_original
expect(::Gitlab::AppLogger)
.to receive(:warn)
.with({ message: described_class::PATH_TRAVERSAL_MESSAGE, path: instance_of(String) })
expect(::Gitlab::AppLogger)
.to receive(:warn)
.with({
......@@ -149,18 +148,20 @@
let(:method) { 'get' }
where(:path, :query_params, :shared_example_name) do
'/foo/bar' | {} | 'no issue'
'/foo/../bar' | {} | 'path traversal'
'/foo%2Fbar' | {} | 'no issue'
'/foo%2F..%2Fbar' | {} | 'path traversal'
'/foo%252F..%252Fbar' | {} | 'no issue'
'/foo/bar' | { x: 'foo' } | 'no issue'
'/foo/bar' | { x: 'foo/../bar' } | 'path traversal'
'/foo/bar' | { x: 'foo%2Fbar' } | 'no issue'
'/foo/bar' | { x: 'foo%2F..%2Fbar' } | 'no issue'
'/foo/bar' | { x: 'foo%252F..%252Fbar' } | 'no issue'
'/foo%2F..%2Fbar' | { x: 'foo%252F..%252Fbar' } | 'path traversal'
'/foo/bar' | {} | 'no issue'
'/foo/../bar' | {} | 'path traversal'
'/foo%2Fbar' | {} | 'no issue'
'/foo%2F..%2Fbar' | {} | 'path traversal'
'/foo%252F..%252Fbar' | {} | 'no issue'
'/foo/bar' | { x: 'foo' } | 'no issue'
'/foo/bar' | { x: 'foo/../bar' } | 'path traversal'
'/foo/bar' | { x: 'foo%2Fbar' } | 'no issue'
'/foo/bar' | { x: 'foo%2F..%2Fbar' } | 'no issue'
'/foo/bar' | { x: 'foo%252F..%252Fbar' } | 'no issue'
'/foo%2F..%2Fbar' | { x: 'foo%252F..%252Fbar' } | 'path traversal'
'/api/graphql' | { query: CGI.escape(graphql_query) } | 'no issue'
end
with_them do
......@@ -351,7 +352,7 @@
let(:method) { http_method }
it 'does not check for path traversals' do
expect(::Gitlab::PathTraversal).not_to receive(:check_path_traversal!)
expect(::Gitlab::PathTraversal).not_to receive(:path_traversal?)
subject
end
......
spec/lib/gitlab/path_traversal_spec.rb
+ 39
− 5
浏览文件 @ e805a78d
......@@ -93,12 +93,46 @@
it 'raises for other non-strings' do
expect { check_path_traversal!(%w[/tmp /tmp/../etc/passwd]) }.to raise_error(/Invalid path/)
end
end
context 'when skip_decoding is used' do
it 'does not detect double encoded chars' do
expect(check_path_traversal!('foo%252F..%2Fbar', skip_decoding: true)).to eq('foo%252F..%2Fbar')
expect(check_path_traversal!('foo%252F%2E%2E%2Fbar', skip_decoding: true)).to eq('foo%252F%2E%2E%2Fbar')
end
describe '.path_traversal?' do
subject { described_class.path_traversal?(decoded_path, match_new_line: match_new_line) }
where(:decoded_path, :match_new_line, :result) do
nil | true | false
'.' | true | true
'..' | true | true
'../foo' | true | true
'..\\foo' | true | true
'../' | true | true
'..\\' | true | true
'/../' | true | true
'\\..\\' | true | true
'foo/../../bar' | true | true
'foo\\..\\..\\bar' | true | true
'foo/..\\bar' | true | true
'foo\\../bar' | true | true
'foo/..\\..\\..\\..\\../bar' | true | true
'foo/../' | true | true
'foo\\..\\' | true | true
'foo/..' | true | true
'foo\\..' | true | true
'./foo' | true | false
'.test/foo' | true | false
'..test/foo' | true | false
'dir/..foo.rb' | true | false
'dir/.foo.rb' | true | false
# single quotes will escape \n ('\\n') and will not get matched.
# we must use double quotes strings
"./foo\n" | true | true
"..test/foo\n" | true | true
"./foo\n" | false | false
"..test/foo\n" | false | false
end
with_them do
it { is_expected.to eq(result) }
end
end
......
0% 或 .
You are about to add 0 people to the discussion. Proceed with caution.
先完成此消息的编辑！
想要评论请 注册 或