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.

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 3bd2408dc72..9a320637a2f 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'