Merge branch 'security-ssrf-kubernetes-dns-12-1' into '12-1-stable'

DNS Rebind SSRF in Kubernetes Integration

See merge request gitlab/gitlabhq!3289

5 files changed, 269 insertions, 18 deletions

diff --git a/changelogs/unreleased/security-ssrf-kubernetes-dns.yml b/changelogs/unreleased/security-ssrf-kubernetes-dns.yml
new file mode 100644
index 00000000000..4d6335e4b08
--- /dev/null
+++ b/changelogs/unreleased/security-ssrf-kubernetes-dns.yml
@@ -0,0 +1,5 @@
+---
+title: Fix SSRF via DNS rebinding in Kubernetes Integration
+merge_request:
+author:
+type: security
diff --git a/config/initializers/rest-client-hostname_override.rb b/config/initializers/rest-client-hostname_override.rb
new file mode 100644
index 00000000000..bc1b70bd73f
--- /dev/null
+++ b/config/initializers/rest-client-hostname_override.rb
@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+module RestClient
+  class Request
+    attr_accessor :hostname_override
+
+    module UrlBlocker
+      def transmit(uri, req, payload, &block)
+        begin
+          ip, hostname_override = Gitlab::UrlBlocker.validate!(uri, allow_local_network: allow_settings_local_requests?,
+                                                               allow_localhost: allow_settings_local_requests?,
+                                                               dns_rebind_protection: dns_rebind_protection?)
+
+          self.hostname_override = hostname_override
+        rescue Gitlab::UrlBlocker::BlockedUrlError => e
+          raise ArgumentError, "URL '#{uri}' is blocked: #{e.message}"
+        end
+
+        # Gitlab::UrlBlocker returns a Addressable::URI which we need to coerce
+        # to URI so that rest-client can use it to determine if it's a
+        # URI::HTTPS or not. It uses it to set `net.use_ssl` to true or not:
+        #
+        # https://github.com/rest-client/rest-client/blob/f450a0f086f1cd1049abbef2a2c66166a1a9ba71/lib/restclient/request.rb#L656
+        ip_as_uri = URI.parse(ip)
+        super(ip_as_uri, req, payload, &block)
+      end
+
+      def net_http_object(hostname, port)
+        super.tap do |http|
+          http.hostname_override = hostname_override if hostname_override
+        end
+      end
+
+      private
+
+      def dns_rebind_protection?
+        return false if Gitlab.http_proxy_env?
+
+        Gitlab::CurrentSettings.dns_rebinding_protection_enabled?
+      end
+
+      def allow_settings_local_requests?
+        Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services?
+      end
+    end
+
+    prepend UrlBlocker
+  end
+end
diff --git a/spec/initializers/rest-client-hostname_override_spec.rb b/spec/initializers/rest-client-hostname_override_spec.rb
new file mode 100644
index 00000000000..1ff82342fb5
--- /dev/null
+++ b/spec/initializers/rest-client-hostname_override_spec.rb
@@ -0,0 +1,147 @@
+require 'spec_helper'
+
+describe 'rest-client dns rebinding protection' do
+  include StubRequests
+
+  context 'when local requests are not allowed' do
+    it 'allows an external request with http' do
+      request_stub = stub_full_request('http://example.com', ip_address: '93.184.216.34')
+
+      RestClient.get('http://example.com/')
+
+      expect(request_stub).to have_been_requested
+    end
+
+    it 'allows an external request with https' do
+      request_stub = stub_full_request('https://example.com', ip_address: '93.184.216.34')
+
+      RestClient.get('https://example.com/')
+
+      expect(request_stub).to have_been_requested
+    end
+
+    it 'raises error when it is a request that resolves to a local address' do
+      stub_full_request('https://example.com', ip_address: '172.16.0.0')
+
+      expect { RestClient.get('https://example.com') }
+        .to raise_error(ArgumentError,
+                        "URL 'https://example.com' is blocked: Requests to the local network are not allowed")
+    end
+
+    it 'raises error when it is a request that resolves to a localhost address' do
+      stub_full_request('https://example.com', ip_address: '127.0.0.1')
+
+      expect { RestClient.get('https://example.com') }
+        .to raise_error(ArgumentError,
+                        "URL 'https://example.com' is blocked: Requests to localhost are not allowed")
+    end
+
+    it 'raises error when it is a request to local address' do
+      expect { RestClient.get('http://172.16.0.0') }
+        .to raise_error(ArgumentError,
+                        "URL 'http://172.16.0.0' is blocked: Requests to the local network are not allowed")
+    end
+
+    it 'raises error when it is a request to localhost address' do
+      expect { RestClient.get('http://127.0.0.1') }
+        .to raise_error(ArgumentError,
+                        "URL 'http://127.0.0.1' is blocked: Requests to localhost are not allowed")
+    end
+  end
+
+  context 'when port different from URL scheme is used' do
+    it 'allows the request' do
+      request_stub = stub_full_request('https://example.com:8080', ip_address: '93.184.216.34')
+
+      RestClient.get('https://example.com:8080/')
+
+      expect(request_stub).to have_been_requested
+    end
+
+    it 'raises error when it is a request to local address' do
+      expect { RestClient.get('https://172.16.0.0:8080') }
+        .to raise_error(ArgumentError,
+                        "URL 'https://172.16.0.0:8080' is blocked: Requests to the local network are not allowed")
+    end
+
+    it 'raises error when it is a request to localhost address' do
+      expect { RestClient.get('https://127.0.0.1:8080') }
+        .to raise_error(ArgumentError,
+                        "URL 'https://127.0.0.1:8080' is blocked: Requests to localhost are not allowed")
+    end
+  end
+
+  context 'when DNS rebinding protection is disabled' do
+    before do
+      stub_application_setting(dns_rebinding_protection_enabled: false)
+    end
+
+    it 'allows the request' do
+      request_stub = stub_request(:get, 'https://example.com')
+
+      RestClient.get('https://example.com/')
+
+      expect(request_stub).to have_been_requested
+    end
+  end
+
+  context 'when http(s) proxy environment variable is set' do
+    before do
+      stub_env('https_proxy' => 'https://my.proxy')
+    end
+
+    it 'allows the request' do
+      request_stub = stub_request(:get, 'https://example.com')
+
+      RestClient.get('https://example.com/')
+
+      expect(request_stub).to have_been_requested
+    end
+  end
+
+  context 'when local requests are allowed' do
+    before do
+      stub_application_setting(allow_local_requests_from_hooks_and_services: true)
+    end
+
+    it 'allows an external request' do
+      request_stub = stub_full_request('https://example.com', ip_address: '93.184.216.34')
+
+      RestClient.get('https://example.com/')
+
+      expect(request_stub).to have_been_requested
+    end
+
+    it 'allows an external request that resolves to a local address' do
+      request_stub = stub_full_request('https://example.com', ip_address: '172.16.0.0')
+
+      RestClient.get('https://example.com/')
+
+      expect(request_stub).to have_been_requested
+    end
+
+    it 'allows an external request that resolves to a localhost address' do
+      request_stub = stub_full_request('https://example.com', ip_address: '127.0.0.1')
+
+      RestClient.get('https://example.com/')
+
+      expect(request_stub).to have_been_requested
+    end
+
+    it 'allows a local address request' do
+      request_stub = stub_request(:get, 'http://172.16.0.0')
+
+      RestClient.get('http://172.16.0.0')
+
+      expect(request_stub).to have_been_requested
+    end
+
+    it 'allows a localhost address request' do
+      request_stub = stub_request(:get, 'http://127.0.0.1')
+
+      RestClient.get('http://127.0.0.1')
+
+      expect(request_stub).to have_been_requested
+    end
+  end
+end
diff --git a/spec/lib/gitlab/kubernetes/kube_client_spec.rb b/spec/lib/gitlab/kubernetes/kube_client_spec.rb
index 97ebb5f1554..cceb57394cf 100644
--- a/spec/lib/gitlab/kubernetes/kube_client_spec.rb
+++ b/spec/lib/gitlab/kubernetes/kube_client_spec.rb
@@ -3,6 +3,7 @@
 require 'spec_helper'
 
 describe Gitlab::Kubernetes::KubeClient do
+  include StubRequests
   include KubernetesHelpers
 
   let(:api_url) { 'https://kubernetes.example.com/prefix' }
@@ -14,6 +15,17 @@ describe Gitlab::Kubernetes::KubeClient do
     stub_kubeclient_discover(api_url)
   end
 
+  def method_call(client, method_name)
+    case method_name
+    when /\A(get_|delete_)/
+      client.public_send(method_name)
+    when /\A(create_|update_)/
+      client.public_send(method_name, {})
+    else
+      raise "Unknown method name #{method_name}"
+    end
+  end
+
   shared_examples 'a Kubeclient' do
     it 'is a Kubeclient::Client' do
       is_expected.to be_an_instance_of Kubeclient::Client
@@ -25,28 +37,30 @@ describe Gitlab::Kubernetes::KubeClient do
   end
 
   shared_examples 'redirection not allowed' do |method_name|
-    before do
-      redirect_url = 'https://not-under-our-control.example.com/api/v1/pods'
+    context 'api_url is redirected' do
+      before do
+        redirect_url = 'https://not-under-our-control.example.com/api/v1/pods'
 
-      stub_request(:get, %r{\A#{api_url}/})
-        .to_return(status: 302, headers: { location: redirect_url })
+        stub_request(:get, %r{\A#{api_url}/})
+          .to_return(status: 302, headers: { location: redirect_url })
 
-      stub_request(:get, redirect_url)
-        .to_return(status: 200, body: '{}')
-    end
+        stub_request(:get, redirect_url)
+          .to_return(status: 200, body: '{}')
+      end
 
-    it 'does not follow redirects' do
-      method_call = -> do
-        case method_name
-        when /\A(get_|delete_)/
-          client.public_send(method_name)
-        when /\A(create_|update_)/
-          client.public_send(method_name, {})
-        else
-          raise "Unknown method name #{method_name}"
-        end
+      it 'does not follow redirects' do
+        expect { method_call(client, method_name) }.to raise_error(Kubeclient::HttpError)
       end
-      expect { method_call.call }.to raise_error(Kubeclient::HttpError)
+    end
+  end
+
+  shared_examples 'dns rebinding not allowed' do |method_name|
+    it 'does not allow DNS rebinding' do
+      stub_dns(api_url, ip_address: '8.8.8.8')
+      client
+
+      stub_dns(api_url, ip_address: '192.168.2.120')
+      expect { method_call(client, method_name) }.to raise_error(ArgumentError, /is blocked/)
     end
   end
 
@@ -160,6 +174,7 @@ describe Gitlab::Kubernetes::KubeClient do
     ].each do |method|
       describe "##{method}" do
         include_examples 'redirection not allowed', method
+        include_examples 'dns rebinding not allowed', method
 
         it 'delegates to the core client' do
           expect(client).to delegate_method(method).to(:core_client)
@@ -185,6 +200,7 @@ describe Gitlab::Kubernetes::KubeClient do
     ].each do |method|
       describe "##{method}" do
         include_examples 'redirection not allowed', method
+        include_examples 'dns rebinding not allowed', method
 
         it 'delegates to the rbac client' do
           expect(client).to delegate_method(method).to(:rbac_client)
@@ -203,6 +219,7 @@ describe Gitlab::Kubernetes::KubeClient do
 
     describe '#get_deployments' do
       include_examples 'redirection not allowed', 'get_deployments'
+      include_examples 'dns rebinding not allowed', 'get_deployments'
 
       it 'delegates to the extensions client' do
         expect(client).to delegate_method(:get_deployments).to(:extensions_client)
diff --git a/spec/models/project_services/discord_service_spec.rb b/spec/models/project_services/discord_service_spec.rb
index be82f223478..96ac532dcd1 100644
--- a/spec/models/project_services/discord_service_spec.rb
+++ b/spec/models/project_services/discord_service_spec.rb
@@ -8,4 +8,37 @@ describe DiscordService do
     let(:client_arguments) { { url: webhook_url } }
     let(:content_key) { :content }
   end
+
+  describe '#execute' do
+    include StubRequests
+
+    let(:user) { create(:user) }
+    let(:project) { create(:project, :repository) }
+    let(:webhook_url) { "https://example.gitlab.com/" }
+
+    let(:sample_data) do
+      Gitlab::DataBuilder::Push.build_sample(project, user)
+    end
+
+    before do
+      allow(subject).to receive_messages(
+        project: project,
+        project_id: project.id,
+        service_hook: true,
+        webhook: webhook_url
+      )
+
+      WebMock.stub_request(:post, webhook_url)
+    end
+
+    context 'DNS rebind to local address' do
+      before do
+        stub_dns(webhook_url, ip_address: '192.168.2.120')
+      end
+
+      it 'does not allow DNS rebinding' do
+        expect { subject.execute(sample_data) }.to raise_error(ArgumentError, /is blocked/)
+      end
+    end
+  end
 end