Protect Gitlab::HTTP against DNS rebinding attack

Gitlab::HTTP now resolves the hostname only once, verifies the IP is not
blocked, and then uses the same IP to perform the actual request, while
passing the original hostname in the `Host` header and SSL SNI field.

10 files changed, 199 insertions, 23 deletions

diff --git a/spec/lib/gitlab/ci/config/external/file/remote_spec.rb b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb
index d8a61618e77..46d68097fff 100644
--- a/spec/lib/gitlab/ci/config/external/file/remote_spec.rb
+++ b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb
@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Config::External::File::Remote do
+  include StubRequests
+
   let(:context) { described_class::Context.new(nil, '12345', nil, Set.new) }
   let(:params) { { remote: location } }
   let(:remote_file) { described_class.new(params, context) }
@@ -46,7 +48,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
   describe "#valid?" do
     context 'when is a valid remote url' do
       before do
-        WebMock.stub_request(:get, location).to_return(body: remote_file_content)
+        stub_full_request(location).to_return(body: remote_file_content)
       end
 
       it 'returns true' do
@@ -92,7 +94,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
   describe "#content" do
     context 'with a valid remote file' do
       before do
-        WebMock.stub_request(:get, location).to_return(body: remote_file_content)
+        stub_full_request(location).to_return(body: remote_file_content)
       end
 
       it 'returns the content of the file' do
@@ -114,7 +116,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
       let(:location) { 'https://asdasdasdaj48ggerexample.com' }
 
       before do
-        WebMock.stub_request(:get, location).to_raise(SocketError.new('Some HTTP error'))
+        stub_full_request(location).to_raise(SocketError.new('Some HTTP error'))
       end
 
       it 'is nil' do
@@ -144,7 +146,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
 
     context 'when timeout error has been raised' do
       before do
-        WebMock.stub_request(:get, location).to_timeout
+        stub_full_request(location).to_timeout
       end
 
       it 'returns error message about a timeout' do
@@ -154,7 +156,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
 
     context 'when HTTP error has been raised' do
       before do
-        WebMock.stub_request(:get, location).to_raise(Gitlab::HTTP::Error)
+        stub_full_request(location).to_raise(Gitlab::HTTP::Error)
       end
 
       it 'returns error message about a HTTP error' do
@@ -164,7 +166,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
 
     context 'when response has 404 status' do
       before do
-        WebMock.stub_request(:get, location).to_return(body: remote_file_content, status: 404)
+        stub_full_request(location).to_return(body: remote_file_content, status: 404)
       end
 
       it 'returns error message about a timeout' do
diff --git a/spec/lib/gitlab/ci/config/external/mapper_spec.rb b/spec/lib/gitlab/ci/config/external/mapper_spec.rb
index 136974569de..e068b786b02 100644
--- a/spec/lib/gitlab/ci/config/external/mapper_spec.rb
+++ b/spec/lib/gitlab/ci/config/external/mapper_spec.rb
@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Config::External::Mapper do
+  include StubRequests
+
   set(:project) { create(:project, :repository) }
   set(:user) { create(:user) }
 
@@ -18,7 +20,7 @@ describe Gitlab::Ci::Config::External::Mapper do
   end
 
   before do
-    WebMock.stub_request(:get, remote_url).to_return(body: file_content)
+    stub_full_request(remote_url).to_return(body: file_content)
   end
 
   describe '#process' do
diff --git a/spec/lib/gitlab/ci/config/external/processor_spec.rb b/spec/lib/gitlab/ci/config/external/processor_spec.rb
index 0f58a4f1d44..856187371e1 100644
--- a/spec/lib/gitlab/ci/config/external/processor_spec.rb
+++ b/spec/lib/gitlab/ci/config/external/processor_spec.rb
@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Config::External::Processor do
+  include StubRequests
+
   set(:project) { create(:project, :repository) }
   set(:another_project) { create(:project, :repository) }
   set(:user) { create(:user) }
@@ -42,7 +44,7 @@ describe Gitlab::Ci::Config::External::Processor do
       let(:values) { { include: remote_file, image: 'ruby:2.2' } }
 
       before do
-        WebMock.stub_request(:get, remote_file).to_raise(SocketError.new('Some HTTP error'))
+        stub_full_request(remote_file).and_raise(SocketError.new('Some HTTP error'))
       end
 
       it 'raises an error' do
@@ -75,7 +77,7 @@ describe Gitlab::Ci::Config::External::Processor do
       end
 
       before do
-        WebMock.stub_request(:get, remote_file).to_return(body: external_file_content)
+        stub_full_request(remote_file).to_return(body: external_file_content)
       end
 
       it 'appends the file to the values' do
@@ -145,7 +147,7 @@ describe Gitlab::Ci::Config::External::Processor do
         allow_any_instance_of(Gitlab::Ci::Config::External::File::Local)
           .to receive(:fetch_local_content).and_return(local_file_content)
 
-        WebMock.stub_request(:get, remote_file).to_return(body: remote_file_content)
+        stub_full_request(remote_file).to_return(body: remote_file_content)
       end
 
       it 'appends the files to the values' do
@@ -191,7 +193,8 @@ describe Gitlab::Ci::Config::External::Processor do
       end
 
       it 'takes precedence' do
-        WebMock.stub_request(:get, remote_file).to_return(body: remote_file_content)
+        stub_full_request(remote_file).to_return(body: remote_file_content)
+
         expect(processor.perform[:image]).to eq('ruby:2.2')
       end
     end
@@ -231,7 +234,8 @@ describe Gitlab::Ci::Config::External::Processor do
           HEREDOC
         end
 
-        WebMock.stub_request(:get, 'http://my.domain.com/config.yml').to_return(body: 'remote_build: { script: echo Hello World }')
+        stub_full_request('http://my.domain.com/config.yml')
+          .to_return(body: 'remote_build: { script: echo Hello World }')
       end
 
       context 'when project is public' do
@@ -273,8 +277,10 @@ describe Gitlab::Ci::Config::External::Processor do
 
     context 'when config includes an external configuration file via SSL web request' do
       before do
-        stub_request(:get, 'https://sha256.badssl.com/fake.yml').to_return(body: 'image: ruby:2.6', status: 200)
-        stub_request(:get, 'https://self-signed.badssl.com/fake.yml')
+        stub_full_request('https://sha256.badssl.com/fake.yml', ip_address: '8.8.8.8')
+          .to_return(body: 'image: ruby:2.6', status: 200)
+
+        stub_full_request('https://self-signed.badssl.com/fake.yml', ip_address: '8.8.8.9')
           .to_raise(OpenSSL::SSL::SSLError.new('SSL_connect returned=1 errno=0 state=error: certificate verify failed (self signed certificate)'))
       end
 
diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb
index 092e9f242b7..7f336ee853e 100644
--- a/spec/lib/gitlab/ci/config_spec.rb
+++ b/spec/lib/gitlab/ci/config_spec.rb
@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 describe Gitlab::Ci::Config do
+  include StubRequests
+
   set(:user) { create(:user) }
 
   let(:config) do
@@ -216,8 +218,7 @@ describe Gitlab::Ci::Config do
     end
 
     before do
-      WebMock.stub_request(:get, remote_location)
-        .to_return(body: remote_file_content)
+      stub_full_request(remote_location).to_return(body: remote_file_content)
 
       allow(project.repository)
         .to receive(:blob_data_at).and_return(local_file_content)
diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb
index 0d998d89d73..29276d5b686 100644
--- a/spec/lib/gitlab/ci/yaml_processor_spec.rb
+++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb
@@ -3,6 +3,8 @@ require 'spec_helper'
 module Gitlab
   module Ci
     describe YamlProcessor do
+      include StubRequests
+
       subject { described_class.new(config, user: nil) }
 
       describe '#build_attributes' do
@@ -648,7 +650,7 @@ module Gitlab
             end
 
             before do
-              WebMock.stub_request(:get, 'https://gitlab.com/awesome-project/raw/master/.before-script-template.yml')
+              stub_full_request('https://gitlab.com/awesome-project/raw/master/.before-script-template.yml')
                 .to_return(
                   status: 200,
                   headers: { 'Content-Type' => 'application/json' },
diff --git a/spec/lib/gitlab/http_connection_adapter_spec.rb b/spec/lib/gitlab/http_connection_adapter_spec.rb
new file mode 100644
index 00000000000..af033be8e5b
--- /dev/null
+++ b/spec/lib/gitlab/http_connection_adapter_spec.rb
@@ -0,0 +1,88 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::HTTPConnectionAdapter do
+  describe '#connection' do
+    context 'when local requests are not allowed' do
+      it 'sets up the connection' do
+        uri = URI('https://example.org')
+
+        connection = described_class.new(uri).connection
+
+        expect(connection).to be_a(Net::HTTP)
+        expect(connection.address).to eq('93.184.216.34')
+        expect(connection.hostname_override).to eq('example.org')
+        expect(connection.addr_port).to eq('example.org')
+        expect(connection.port).to eq(443)
+      end
+
+      it 'raises error when it is a request to local address' do
+        uri = URI('http://172.16.0.0/12')
+
+        expect { described_class.new(uri).connection }
+          .to raise_error(Gitlab::HTTP::BlockedUrlError,
+                          "URL 'http://172.16.0.0/12' is blocked: Requests to the local network are not allowed")
+      end
+
+      it 'raises error when it is a request to localhost address' do
+        uri = URI('http://127.0.0.1')
+
+        expect { described_class.new(uri).connection }
+          .to raise_error(Gitlab::HTTP::BlockedUrlError,
+                          "URL 'http://127.0.0.1' is blocked: Requests to localhost are not allowed")
+      end
+
+      context 'when port different from URL scheme is used' do
+        it 'sets up the addr_port accordingly' do
+          uri = URI('https://example.org:8080')
+
+          connection = described_class.new(uri).connection
+
+          expect(connection.address).to eq('93.184.216.34')
+          expect(connection.hostname_override).to eq('example.org')
+          expect(connection.addr_port).to eq('example.org:8080')
+          expect(connection.port).to eq(8080)
+        end
+      end
+    end
+
+    context 'when local requests are allowed' do
+      it 'sets up the connection' do
+        uri = URI('https://example.org')
+
+        connection = described_class.new(uri, allow_local_requests: true).connection
+
+        expect(connection).to be_a(Net::HTTP)
+        expect(connection.address).to eq('93.184.216.34')
+        expect(connection.hostname_override).to eq('example.org')
+        expect(connection.addr_port).to eq('example.org')
+        expect(connection.port).to eq(443)
+      end
+
+      it 'sets up the connection when it is a local network' do
+        uri = URI('http://172.16.0.0/12')
+
+        connection = described_class.new(uri, allow_local_requests: true).connection
+
+        expect(connection).to be_a(Net::HTTP)
+        expect(connection.address).to eq('172.16.0.0')
+        expect(connection.hostname_override).to be(nil)
+        expect(connection.addr_port).to eq('172.16.0.0')
+        expect(connection.port).to eq(80)
+      end
+
+      it 'sets up the connection when it is localhost' do
+        uri = URI('http://127.0.0.1')
+
+        connection = described_class.new(uri, allow_local_requests: true).connection
+
+        expect(connection).to be_a(Net::HTTP)
+        expect(connection.address).to eq('127.0.0.1')
+        expect(connection.hostname_override).to be(nil)
+        expect(connection.addr_port).to eq('127.0.0.1')
+        expect(connection.port).to eq(80)
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/http_spec.rb b/spec/lib/gitlab/http_spec.rb
index 6c37c157f5d..158f77cab2c 100644
--- a/spec/lib/gitlab/http_spec.rb
+++ b/spec/lib/gitlab/http_spec.rb
@@ -1,6 +1,28 @@
 require 'spec_helper'
 
 describe Gitlab::HTTP do
+  include StubRequests
+
+  context 'when allow_local_requests' do
+    it 'sends the request to the correct URI' do
+      stub_full_request('https://example.org:8080', ip_address: '8.8.8.8').to_return(status: 200)
+
+      described_class.get('https://example.org:8080', allow_local_requests: false)
+
+      expect(WebMock).to have_requested(:get, 'https://8.8.8.8:8080').once
+    end
+  end
+
+  context 'when not allow_local_requests' do
+    it 'sends the request to the correct URI' do
+      stub_full_request('https://example.org:8080')
+
+      described_class.get('https://example.org:8080', allow_local_requests: true)
+
+      expect(WebMock).to have_requested(:get, 'https://8.8.8.9:8080').once
+    end
+  end
+
   describe 'allow_local_requests_from_hooks_and_services is' do
     before do
       WebMock.stub_request(:get, /.*/).to_return(status: 200, body: 'Success')
@@ -21,6 +43,8 @@ describe Gitlab::HTTP do
 
       context 'if allow_local_requests set to true' do
         it 'override the global value and allow requests to localhost or private network' do
+          stub_full_request('http://localhost:3003')
+
           expect { described_class.get('http://localhost:3003', allow_local_requests: true) }.not_to raise_error
         end
       end
@@ -32,6 +56,8 @@ describe Gitlab::HTTP do
       end
 
       it 'allow requests to localhost' do
+        stub_full_request('http://localhost:3003')
+
         expect { described_class.get('http://localhost:3003') }.not_to raise_error
       end
 
@@ -49,7 +75,7 @@ describe Gitlab::HTTP do
 
   describe 'handle redirect loops' do
     before do
-      WebMock.stub_request(:any, "http://example.org").to_raise(HTTParty::RedirectionTooDeep.new("Redirection Too Deep"))
+      stub_full_request("http://example.org", method: :any).to_raise(HTTParty::RedirectionTooDeep.new("Redirection Too Deep"))
     end
 
     it 'handles GET requests' do
diff --git a/spec/lib/gitlab/import_export/after_export_strategies/web_upload_strategy_spec.rb b/spec/lib/gitlab/import_export/after_export_strategies/web_upload_strategy_spec.rb
index 7c4ac62790e..21a227335cd 100644
--- a/spec/lib/gitlab/import_export/after_export_strategies/web_upload_strategy_spec.rb
+++ b/spec/lib/gitlab/import_export/after_export_strategies/web_upload_strategy_spec.rb
@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 describe Gitlab::ImportExport::AfterExportStrategies::WebUploadStrategy do
+  include StubRequests
+
   let(:example_url) { 'http://www.example.com' }
   let(:strategy) { subject.new(url: example_url, http_method: 'post') }
   let!(:project) { create(:project, :with_export) }
@@ -35,7 +37,7 @@ describe Gitlab::ImportExport::AfterExportStrategies::WebUploadStrategy do
 
     context 'when upload fails' do
       it 'stores the export error' do
-        stub_request(:post, example_url).to_return(status: [404, 'Page not found'])
+        stub_full_request(example_url, method: :post).to_return(status: [404, 'Page not found'])
 
         strategy.execute(user, project)
 
diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb
index 445a56ab0d8..416f8cfece9 100644
--- a/spec/lib/gitlab/url_blocker_spec.rb
+++ b/spec/lib/gitlab/url_blocker_spec.rb
@@ -2,6 +2,52 @@
 require 'spec_helper'
 
 describe Gitlab::UrlBlocker do
+  describe '#validate!' do
+    context 'when URI is nil' do
+      let(:import_url) { nil }
+
+      it 'returns no URI and hostname' do
+        uri, hostname = described_class.validate!(import_url)
+
+        expect(uri).to be(nil)
+        expect(hostname).to be(nil)
+      end
+    end
+
+    context 'when URI is internal' do
+      let(:import_url) { 'http://localhost' }
+
+      it 'returns URI and no hostname' do
+        uri, hostname = described_class.validate!(import_url)
+
+        expect(uri).to eq(Addressable::URI.parse('http://[::1]'))
+        expect(hostname).to eq('localhost')
+      end
+    end
+
+    context 'when the URL hostname is a domain' do
+      let(:import_url) { 'https://example.org' }
+
+      it 'returns URI and hostname' do
+        uri, hostname = described_class.validate!(import_url)
+
+        expect(uri).to eq(Addressable::URI.parse('https://93.184.216.34'))
+        expect(hostname).to eq('example.org')
+      end
+    end
+
+    context 'when the URL hostname is an IP address' do
+      let(:import_url) { 'https://93.184.216.34' }
+
+      it 'returns URI and no hostname' do
+        uri, hostname = described_class.validate!(import_url)
+
+        expect(uri).to eq(Addressable::URI.parse('https://93.184.216.34'))
+        expect(hostname).to be(nil)
+      end
+    end
+  end
+
   describe '#blocked_url?' do
     let(:ports) { Project::VALID_IMPORT_PORTS }
 
@@ -208,7 +254,7 @@ describe Gitlab::UrlBlocker do
       end
 
       def stub_domain_resolv(domain, ip)
-        address = double(ip_address: ip, ipv4_private?: true, ipv6_link_local?: false, ipv4_loopback?: false, ipv6_loopback?: false)
+        address = double(ip_address: ip, ipv4_private?: true, ipv6_link_local?: false, ipv4_loopback?: false, ipv6_loopback?: false, ipv4?: false)
         allow(Addrinfo).to receive(:getaddrinfo).with(domain, any_args).and_return([address])
         allow(address).to receive(:ipv6_v4mapped?).and_return(false)
       end
diff --git a/spec/lib/mattermost/session_spec.rb b/spec/lib/mattermost/session_spec.rb
index 77fea5b2d24..346455067a7 100644
--- a/spec/lib/mattermost/session_spec.rb
+++ b/spec/lib/mattermost/session_spec.rb
@@ -2,6 +2,7 @@ require 'spec_helper'
 
 describe Mattermost::Session, type: :request do
   include ExclusiveLeaseHelpers
+  include StubRequests
 
   let(:user) { create(:user) }
 
@@ -24,7 +25,7 @@ describe Mattermost::Session, type: :request do
     let(:location) { 'http://location.tld' }
     let(:cookie_header) {'MMOAUTH=taskik8az7rq8k6rkpuas7htia; Path=/;'}
     let!(:stub) do
-      WebMock.stub_request(:get, "#{mattermost_url}/oauth/gitlab/login")
+      stub_full_request("#{mattermost_url}/oauth/gitlab/login")
         .to_return(headers: { 'location' => location, 'Set-Cookie' => cookie_header }, status: 302)
     end
 
@@ -63,7 +64,7 @@ describe Mattermost::Session, type: :request do
         end
 
         before do
-          WebMock.stub_request(:get, "#{mattermost_url}/signup/gitlab/complete")
+          stub_full_request("#{mattermost_url}/signup/gitlab/complete")
             .with(query: hash_including({ 'state' => state }))
             .to_return do |request|
               post "/oauth/token",
@@ -80,7 +81,7 @@ describe Mattermost::Session, type: :request do
               end
             end
 
-          WebMock.stub_request(:post, "#{mattermost_url}/api/v4/users/logout")
+          stub_full_request("#{mattermost_url}/api/v4/users/logout", method: :post)
             .to_return(headers: { Authorization: 'token thisworksnow' }, status: 200)
         end