diff options
| -rw-r--r-- | changelogs/unreleased/security-dns-ssrf-bypass.yml | 5 | ||||
| -rw-r--r-- | lib/gitlab/url_blocker.rb | 16 | ||||
| -rw-r--r-- | spec/lib/gitlab/url_blocker_spec.rb | 44 | ||||
| -rw-r--r-- | spec/spec_helper.rb | 1 |
4 files changed, 51 insertions, 15 deletions
diff --git a/changelogs/unreleased/security-dns-ssrf-bypass.yml b/changelogs/unreleased/security-dns-ssrf-bypass.yml new file mode 100644 index 00000000000..e48696ce5bd --- /dev/null +++ b/changelogs/unreleased/security-dns-ssrf-bypass.yml @@ -0,0 +1,5 @@ +--- +title: Fix Server Side Request Forgery mitigation bypass +merge_request: +author: +type: security diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index 9a8df719827..d9070ce5a09 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -20,6 +20,7 @@ module Gitlab # Returns an array with [<uri>, <original-hostname>]. # rubocop:disable Metrics/CyclomaticComplexity # rubocop:disable Metrics/ParameterLists + # rubocop:disable Metrics/PerceivedComplexity def validate!( url, ports: [], @@ -32,6 +33,7 @@ module Gitlab dns_rebind_protection: true) # rubocop:enable Metrics/CyclomaticComplexity # rubocop:enable Metrics/ParameterLists + # rubocop:enable Metrics/PerceivedComplexity return [nil, nil] if url.nil? @@ -56,7 +58,15 @@ module Gitlab addr.ipv6_v4mapped? ? addr.ipv6_to_ipv4 : addr end rescue SocketError - return [uri, nil] + # In the test suite we use a lot of mocked urls that are either invalid or + # don't exist. In order to avoid modifying a ton of tests and factories + # we allow invalid urls unless the environment variable RSPEC_ALLOW_INVALID_URLS + # is not true + return [uri, nil] if Rails.env.test? && ENV['RSPEC_ALLOW_INVALID_URLS'] == 'true' + + # If the addr can't be resolved or the url is invalid (i.e http://1.1.1.1.1) + # we block the url + raise BlockedUrlError, "Host cannot be resolved or invalid" end protected_uri_with_hostname = enforce_uri_hostname(addrs_info, uri, hostname, dns_rebind_protection) @@ -92,9 +102,9 @@ module Gitlab # we'll be making the request to the IP address, instead of using the hostname. def enforce_uri_hostname(addrs_info, uri, hostname, dns_rebind_protection) address = addrs_info.first - ip_address = address&.ip_address + ip_address = address.ip_address - return [uri, nil] unless dns_rebind_protection && ip_address && ip_address != hostname + return [uri, nil] unless dns_rebind_protection && ip_address != hostname uri = uri.dup uri.hostname = ip_address diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index 253366e0789..0d88a1c11a6 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -68,6 +68,16 @@ describe Gitlab::UrlBlocker do expect(uri).to eq(Addressable::URI.parse('https://example.org')) expect(hostname).to eq(nil) end + + context 'when it cannot be resolved' do + let(:import_url) { 'http://foobar.x' } + + it 'raises error' do + stub_env('RSPEC_ALLOW_INVALID_URLS', 'false') + + expect { described_class.validate!(import_url) }.to raise_error(described_class::BlockedUrlError) + end + end end context 'when the URL hostname is an IP address' do @@ -79,6 +89,16 @@ describe Gitlab::UrlBlocker do expect(uri).to eq(Addressable::URI.parse('https://93.184.216.34')) expect(hostname).to be(nil) end + + context 'when it is invalid' do + let(:import_url) { 'http://1.1.1.1.1' } + + it 'raises an error' do + stub_env('RSPEC_ALLOW_INVALID_URLS', 'false') + + expect { described_class.validate!(import_url) }.to raise_error(described_class::BlockedUrlError) + end + end end end end @@ -180,8 +200,6 @@ describe Gitlab::UrlBlocker do end it 'returns true for a non-alphanumeric hostname' do - stub_resolv - aggregate_failures do expect(described_class).to be_blocked_url('ssh://-oProxyCommand=whoami/a') @@ -300,10 +318,6 @@ describe Gitlab::UrlBlocker do end context 'when enforce_user is' do - before do - stub_resolv - end - context 'false (default)' do it 'does not block urls with a non-alphanumeric username' do expect(described_class).not_to be_blocked_url('ssh://-oProxyCommand=whoami@example.com/a') @@ -351,6 +365,18 @@ describe Gitlab::UrlBlocker do expect(described_class.blocked_url?('https://git‌lab.com/foo/foo.bar', ascii_only: true)).to be true end end + + it 'blocks urls with invalid ip address' do + stub_env('RSPEC_ALLOW_INVALID_URLS', 'false') + + expect(described_class).to be_blocked_url('http://8.8.8.8.8') + end + + it 'blocks urls whose hostname cannot be resolved' do + stub_env('RSPEC_ALLOW_INVALID_URLS', 'false') + + expect(described_class).to be_blocked_url('http://foobar.x') + end end describe '#validate_hostname!' do @@ -382,10 +408,4 @@ describe Gitlab::UrlBlocker do end end end - - # Resolv does not support resolving UTF-8 domain names - # See https://bugs.ruby-lang.org/issues/4270 - def stub_resolv - allow(Resolv).to receive(:getaddresses).and_return([]) - end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index ec17bee640d..72cbc9ebc5a 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -3,6 +3,7 @@ SimpleCovEnv.start! ENV["RAILS_ENV"] = 'test' ENV["IN_MEMORY_APPLICATION_SETTINGS"] = 'true' +ENV["RSPEC_ALLOW_INVALID_URLS"] = 'true' require File.expand_path('../config/environment', __dir__) require 'rspec/rails' |