Merge branch 'ee-fj-66950-avoid-dns-checkings-domain-whitelisted' into 'master'

Avoid dns rebinding checks when the domain is whitelisted

Closes #31538

See merge request gitlab-org/gitlab-ee!16529

changelogs/unreleased/fj-66950-avoid-dns-checkings-domain-whitelisted.yml

+---
+title: Avoid dns rebinding checks when the domain is whitelisted
+merge_request: 32603
+author:
+type: changed

lib/gitlab/url_blocker.rb

@@ -45,21 +45,18 @@ module Gitlab
           ascii_only: ascii_only
         )
 
-        normalized_hostname = uri.normalized_host
-        hostname = uri.hostname
-        port = get_port(uri)
-
-        address_info = get_address_info(hostname, port, dns_rebind_protection)
+        address_info = get_address_info(uri, dns_rebind_protection)
         return [uri, nil] unless address_info
 
         ip_address = ip_address(address_info)
-        protected_uri_with_hostname = enforce_uri_hostname(ip_address, uri, hostname, dns_rebind_protection)
+        return [uri, nil] if domain_whitelisted?(uri) || ip_whitelisted?(ip_address)
+
+        protected_uri_with_hostname = enforce_uri_hostname(ip_address, uri, dns_rebind_protection)
 
         # Allow url from the GitLab instance itself but only for the configured hostname and ports
         return protected_uri_with_hostname if internal?(uri)
 
         validate_local_request(
-          normalized_hostname: normalized_hostname,
           address_info: address_info,
           allow_localhost: allow_localhost,
           allow_local_network: allow_local_network
@@ -86,12 +83,12 @@ module Gitlab
       #
       # The original hostname is used to validate the SSL, given in that scenario
       # we'll be making the request to the IP address, instead of using the hostname.
-      def enforce_uri_hostname(ip_address, uri, hostname, dns_rebind_protection)
-        return [uri, nil] unless dns_rebind_protection && ip_address && ip_address != hostname
+      def enforce_uri_hostname(ip_address, uri, dns_rebind_protection)
+        return [uri, nil] unless dns_rebind_protection && ip_address && ip_address != uri.hostname
 
-        uri = uri.dup
-        uri.hostname = ip_address
-        [uri, hostname]
+        new_uri = uri.dup
+        new_uri.hostname = ip_address
+        [new_uri, uri.hostname]
       end
 
       def ip_address(address_info)
@@ -110,14 +107,14 @@ module Gitlab
         validate_unicode_restriction(uri) if ascii_only
       end
 
-      def get_address_info(hostname, port, dns_rebind_protection)
-        Addrinfo.getaddrinfo(hostname, port, nil, :STREAM).map do |addr|
+      def get_address_info(uri, dns_rebind_protection)
+        Addrinfo.getaddrinfo(uri.hostname, get_port(uri), nil, :STREAM).map do |addr|
           addr.ipv6_v4mapped? ? addr.ipv6_to_ipv4 : addr
         end
       rescue SocketError
-        # If the dns rebinding protection is not enabled, we allow
-        # urls that can't be resolved at this point.
-        return unless dns_rebind_protection
+        # If the dns rebinding protection is not enabled or the domain
+        # is whitelisted we avoid the dns rebinding checks
+        return if domain_whitelisted?(uri) || !dns_rebind_protection
 
         # 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
@@ -131,18 +128,11 @@ module Gitlab
       end
 
       def validate_local_request(
-        normalized_hostname:,
         address_info:,
         allow_localhost:,
         allow_local_network:)
         return if allow_local_network && allow_localhost
 
-        ip_whitelist, domain_whitelist =
-          Gitlab::CurrentSettings.outbound_local_requests_whitelist_arrays
-
-        return if local_domain_whitelisted?(domain_whitelist, normalized_hostname) ||
-          local_ip_whitelisted?(ip_whitelist, ip_address(address_info))
-
         unless allow_localhost
           validate_localhost(address_info)
           validate_loopback(address_info)
@@ -258,14 +248,12 @@ module Gitlab
           (uri.port.blank? || uri.port == config.gitlab_shell.ssh_port)
       end
 
-      def local_ip_whitelisted?(ip_whitelist, ip_string)
-        ip_obj = Gitlab::Utils.string_to_ip_object(ip_string)
-
-        ip_whitelist.any? { |ip| ip.include?(ip_obj) }
+      def domain_whitelisted?(uri)
+        Gitlab::UrlBlockers::UrlWhitelist.domain_whitelisted?(uri.normalized_host)
       end
 
-      def local_domain_whitelisted?(domain_whitelist, domain_string)
-        domain_whitelist.include?(domain_string)
+      def ip_whitelisted?(ip_address)
+        Gitlab::UrlBlockers::UrlWhitelist.ip_whitelisted?(ip_address)
       end
 
       def config

lib/gitlab/url_blockers/url_whitelist.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module UrlBlockers
+    class UrlWhitelist
+      class << self
+        def ip_whitelisted?(ip_string)
+          return false if ip_string.blank?
+
+          ip_whitelist, _ = outbound_local_requests_whitelist_arrays
+          ip_obj = Gitlab::Utils.string_to_ip_object(ip_string)
+
+          ip_whitelist.any? { |ip| ip.include?(ip_obj) }
+        end
+
+        def domain_whitelisted?(domain_string)
+          return false if domain_string.blank?
+
+          _, domain_whitelist = outbound_local_requests_whitelist_arrays
+
+          domain_whitelist.include?(domain_string)
+        end
+
+        private
+
+        attr_reader :ip_whitelist, :domain_whitelist
+
+        # We cannot use Gitlab::CurrentSettings as ApplicationSetting itself
+        # calls this class. This ends up in a cycle where
+        # Gitlab::CurrentSettings creates an ApplicationSetting which then
+        # calls this method.
+        #
+        # See https://gitlab.com/gitlab-org/gitlab-ee/issues/9833
+        def outbound_local_requests_whitelist_arrays
+          return [[], []] unless ApplicationSetting.current
+
+          ApplicationSetting.current.outbound_local_requests_whitelist_arrays
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/url_blocker_spec.rb

@@ -30,8 +30,12 @@ describe Gitlab::UrlBlocker do
     context 'when URI is internal' do
       let(:import_url) { 'http://localhost' }
 
+      before do
+        stub_dns(import_url, ip_address: '127.0.0.1')
+      end
+
       it_behaves_like 'validates URI and hostname' do
-        let(:expected_uri) { 'http://[::1]' }
+        let(:expected_uri) { 'http://127.0.0.1' }
         let(:expected_hostname) { 'localhost' }
       end
     end
@@ -347,6 +351,7 @@ describe Gitlab::UrlBlocker do
           end
 
           before do
+            allow(ApplicationSetting).to receive(:current).and_return(ApplicationSetting.new)
             stub_application_setting(outbound_local_requests_whitelist: whitelist)
           end
 
@@ -384,9 +389,15 @@ describe Gitlab::UrlBlocker do
             it_behaves_like 'allows local requests', { allow_localhost: false, allow_local_network: false }
 
             it 'whitelists IP when dns_rebind_protection is disabled' do
-              stub_domain_resolv('example.com', '192.168.1.1') do
-                expect(described_class).not_to be_blocked_url("http://example.com",
-                  url_blocker_attributes.merge(dns_rebind_protection: false))
+              url = "http://example.com"
+              attrs = url_blocker_attributes.merge(dns_rebind_protection: false)
+
+              stub_domain_resolv('example.com', '192.168.1.2') do
+                expect(described_class).not_to be_blocked_url(url, attrs)
+              end
+
+              stub_domain_resolv('example.com', '192.168.1.3') do
+                expect(described_class).to be_blocked_url(url, attrs)
               end
             end
           end
@@ -437,6 +448,51 @@ describe Gitlab::UrlBlocker do
                   url_blocker_attributes)
               end
             end
+
+            shared_examples 'dns rebinding checks' do
+              shared_examples 'whitelists the domain' do
+                let(:whitelist) { [domain] }
+                let(:url) { "http://#{domain}" }
+
+                before do
+                  stub_env('RSPEC_ALLOW_INVALID_URLS', 'false')
+                end
+
+                it do
+                  expect(described_class).not_to be_blocked_url(url, dns_rebind_protection: dns_rebind_value)
+                end
+              end
+
+              context 'when dns_rebinding_setting is' do
+                context 'enabled' do
+                  let(:dns_rebind_value) { true }
+
+                  it_behaves_like 'whitelists the domain'
+                end
+
+                context 'disabled' do
+                  let(:dns_rebind_value) { false }
+
+                  it_behaves_like 'whitelists the domain'
+                end
+              end
+            end
+
+            context 'when the domain cannot be resolved' do
+              let(:domain) { 'foobar.x' }
+
+              it_behaves_like 'dns rebinding checks'
+            end
+
+            context 'when the domain can be resolved' do
+              let(:domain) { 'example.com' }
+
+              before do
+                stub_dns(url, ip_address: '93.184.216.34')
+              end
+
+              it_behaves_like 'dns rebinding checks'
+            end
           end
 
           context 'with ip ranges in whitelist' do

spec/lib/gitlab/url_blockers/url_whitelist_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::UrlBlockers::UrlWhitelist do
+  include StubRequests
+
+  let(:whitelist) { [] }
+
+  before do
+    allow(ApplicationSetting).to receive(:current).and_return(ApplicationSetting.new)
+    stub_application_setting(outbound_local_requests_whitelist: whitelist)
+  end
+
+  describe '#domain_whitelisted?' do
+    let(:whitelist) do
+      [
+        'www.example.com',
+        'example.com'
+      ]
+    end
+
+    it 'returns true if domains present in whitelist' do
+      aggregate_failures do
+        whitelist.each do |domain|
+          expect(described_class).to be_domain_whitelisted(domain)
+        end
+
+        ['subdomain.example.com', 'example.org'].each do |domain|
+          expect(described_class).not_to be_domain_whitelisted(domain)
+        end
+      end
+    end
+
+    it 'returns false when domain is blank' do
+      expect(described_class).not_to be_domain_whitelisted(nil)
+    end
+  end
+
+  describe '#ip_whitelisted?' do
+    let(:whitelist) do
+      [
+        '0.0.0.0',
+        '127.0.0.1',
+        '192.168.1.1',
+        '0:0:0:0:0:ffff:192.168.1.2',
+        '::ffff:c0a8:102',
+        'fc00:bf8b:e62c:abcd:abcd:aaaa:aaaa:aaaa',
+        '0:0:0:0:0:ffff:169.254.169.254',
+        '::ffff:a9fe:a9fe',
+        '::ffff:a9fe:a864',
+        'fe80::c800:eff:fe74:8'
+      ]
+    end
+
+    it 'returns true if ips present in whitelist' do
+      aggregate_failures do
+        whitelist.each do |ip_address|
+          expect(described_class).to be_ip_whitelisted(ip_address)
+        end
+
+        ['172.16.2.2', '127.0.0.2', 'fe80::c800:eff:fe74:9'].each do |ip_address|
+          expect(described_class).not_to be_ip_whitelisted(ip_address)
+        end
+      end
+    end
+
+    it 'returns false when ip is blank' do
+      expect(described_class).not_to be_ip_whitelisted(nil)
+    end
+  end
+end