diff options
author | Francisco Javier López <fjlopez@gitlab.com> | 2019-07-02 19:38:23 +0300 |
---|---|---|
committer | Francisco Javier López <fjlopez@gitlab.com> | 2019-07-15 10:21:20 +0300 |
commit | f5c1cd489834e824c83f2ae909cd0dd41fb95dab (patch) | |
tree | bfbf073fae7b2a5dedb63118cc3865036a92c847 | |
parent | e674a9d97822553c930062b9c5f4d4a349d39c11 (diff) |
Fix Server Side Request Forgery mitigation bypass
When we can't resolve the hostname or it is invalid, we shouldn't
even perform the request. This fix also fixes the problem the
SSRF rebinding attack.
We can't stub feature flags outside example blocks. Nevertheless,
there are some actions that calls the UrlBlocker, that are performed
outside example blocks, ie: `set` instruction.
That's why we have to use some signalign mechanism outside the scope
of the specs.
-rw-r--r-- | changelogs/unreleased/security-dns-ssrf-bypass.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/url_blocker.rb | 13 | ||||
-rw-r--r-- | spec/lib/gitlab/url_blocker_spec.rb | 44 | ||||
-rw-r--r-- | spec/spec_helper.rb | 1 |
4 files changed, 49 insertions, 14 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 f6b2e2acf16..cc937cbb3cf 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -85,9 +85,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 @@ -111,6 +111,15 @@ module Gitlab addr.ipv6_v4mapped? ? addr.ipv6_to_ipv4 : addr end rescue SocketError + # 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 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 def validate_local_request(address_info:, allow_localhost:, allow_local_network:) diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index f8b0cbfb6f6..0fab890978a 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://gitlab.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 95e0d8858b9..089dbc09aa3 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' |