From 3f27b7178b476bab405dc21931b04644fd72939d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Tue, 10 Sep 2019 13:34:36 +0200 Subject: Avoid dns rebinding checks if the domain is whitelisted If the domain is whitelisted we skip dns rebinding checks as well as local host checks --- ...6950-avoid-dns-checkings-domain-whitelisted.yml | 5 ++ lib/gitlab/url_blocker.rb | 49 ++++++---------- lib/gitlab/url_blockers/url_whitelist.rb | 38 +++++++++++++ spec/lib/gitlab/url_blocker_spec.rb | 64 +++++++++++++++++++-- spec/lib/gitlab/url_blockers/url_whitelist_spec.rb | 65 ++++++++++++++++++++++ 5 files changed, 185 insertions(+), 36 deletions(-) create mode 100644 changelogs/unreleased/fj-66950-avoid-dns-checkings-domain-whitelisted.yml create mode 100644 lib/gitlab/url_blockers/url_whitelist.rb create mode 100644 spec/lib/gitlab/url_blockers/url_whitelist_spec.rb diff --git a/changelogs/unreleased/fj-66950-avoid-dns-checkings-domain-whitelisted.yml b/changelogs/unreleased/fj-66950-avoid-dns-checkings-domain-whitelisted.yml new file mode 100644 index 00000000000..0a1c5065208 --- /dev/null +++ b/changelogs/unreleased/fj-66950-avoid-dns-checkings-domain-whitelisted.yml @@ -0,0 +1,5 @@ +--- +title: Avoid dns rebinding checks when the domain is whitelisted +merge_request: 32603 +author: +type: changed diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index fab504aa603..931576a4c15 100644 --- a/lib/gitlab/url_blocker.rb +++ b/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 uri_whitelisted?(uri, 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 UrlBlockers::UrlWhitelist.domain_whitelisted?(uri.normalized_host) || !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,9 @@ 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) } - end - - def local_domain_whitelisted?(domain_whitelist, domain_string) - domain_whitelist.include?(domain_string) + def uri_whitelisted?(uri, ip_address) + UrlBlockers::UrlWhitelist.domain_whitelisted?(uri.normalized_host) || + UrlBlockers::UrlWhitelist.ip_whitelisted?(ip_address) end def config diff --git a/lib/gitlab/url_blockers/url_whitelist.rb b/lib/gitlab/url_blockers/url_whitelist.rb new file mode 100644 index 00000000000..522013db676 --- /dev/null +++ b/lib/gitlab/url_blockers/url_whitelist.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module Gitlab + module UrlBlockers + class UrlWhitelist + class << self + def ip_whitelisted?(ip_string) + 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) + _, 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 diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index 6d1d7e48326..6ce002ad70e 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/url_blockers/url_whitelist_spec.rb b/spec/lib/gitlab/url_blockers/url_whitelist_spec.rb new file mode 100644 index 00000000000..0320d7b1d16 --- /dev/null +++ b/spec/lib/gitlab/url_blockers/url_whitelist_spec.rb @@ -0,0 +1,65 @@ +# coding: utf-8 +# 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 + 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 + end +end -- cgit v1.2.3