Commit 759c5296 authored by Alessio Caiazza's avatar Alessio Caiazza

Validate URI scheme also for internal URI

Gitlab::UrlBlocker ignores scheme when validating URI matching either
config.gitlab or config.gitlab_shell

This patch enforces matching config.gitlab.protocol for internal web and
ssh for internal shell.
parent bfe95643
---
title: Fix stored XSS for Environments
merge_request:
author:
type: security
...@@ -111,12 +111,14 @@ module Gitlab ...@@ -111,12 +111,14 @@ module Gitlab
end end
def internal_web?(uri) def internal_web?(uri)
uri.hostname == config.gitlab.host && uri.scheme == config.gitlab.protocol &&
uri.hostname == config.gitlab.host &&
(uri.port.blank? || uri.port == config.gitlab.port) (uri.port.blank? || uri.port == config.gitlab.port)
end end
def internal_shell?(uri) def internal_shell?(uri)
uri.hostname == config.gitlab_shell.ssh_host && uri.scheme == 'ssh' &&
uri.hostname == config.gitlab_shell.ssh_host &&
(uri.port.blank? || uri.port == config.gitlab_shell.ssh_port) (uri.port.blank? || uri.port == config.gitlab_shell.ssh_port)
end end
......
...@@ -308,7 +308,7 @@ FactoryBot.define do ...@@ -308,7 +308,7 @@ FactoryBot.define do
trait :with_runner_session do trait :with_runner_session do
after(:build) do |build| after(:build) do |build|
build.build_runner_session(url: 'ws://localhost') build.build_runner_session(url: 'https://localhost')
end end
end end
end end
......
...@@ -10,8 +10,8 @@ describe Gitlab::UrlBlocker do ...@@ -10,8 +10,8 @@ describe Gitlab::UrlBlocker do
expect(described_class.blocked_url?(import_url)).to be false expect(described_class.blocked_url?(import_url)).to be false
end end
it 'allows imports from configured SSH host and port' do it 'allows mirroring from configured SSH host and port' do
import_url = "http://#{Gitlab.config.gitlab_shell.ssh_host}:#{Gitlab.config.gitlab_shell.ssh_port}/t.git" import_url = "ssh://#{Gitlab.config.gitlab_shell.ssh_host}:#{Gitlab.config.gitlab_shell.ssh_port}/t.git"
expect(described_class.blocked_url?(import_url)).to be false expect(described_class.blocked_url?(import_url)).to be false
end end
...@@ -29,6 +29,14 @@ describe Gitlab::UrlBlocker do ...@@ -29,6 +29,14 @@ describe Gitlab::UrlBlocker do
expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git', protocols: ['http'])).to be true expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git', protocols: ['http'])).to be true
end end
it 'returns true for bad protocol on configured web/SSH host and ports' do
web_url = "javascript://#{Gitlab.config.gitlab.host}:#{Gitlab.config.gitlab.port}/t.git%0aalert(1)"
expect(described_class.blocked_url?(web_url)).to be true
ssh_url = "javascript://#{Gitlab.config.gitlab_shell.ssh_host}:#{Gitlab.config.gitlab_shell.ssh_port}/t.git%0aalert(1)"
expect(described_class.blocked_url?(ssh_url)).to be true
end
it 'returns true for localhost IPs' do it 'returns true for localhost IPs' do
expect(described_class.blocked_url?('https://0.0.0.0/foo/foo.git')).to be true expect(described_class.blocked_url?('https://0.0.0.0/foo/foo.git')).to be true
expect(described_class.blocked_url?('https://[::1]/foo/foo.git')).to be true expect(described_class.blocked_url?('https://[::1]/foo/foo.git')).to be true
......
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