diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 5995ef57e260c8df7ac1feeb36fe43e86791b31b..ce097251212f314d0b396620a788f0d9a4d0e0a3 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -160,6 +160,7 @@ module ApplicationSettingsHelper :akismet_api_key, :akismet_enabled, :allow_local_requests_from_hooks_and_services, + :dns_rebinding_protection_enabled, :archive_builds_in_human_readable, :authorized_keys_enabled, :auto_devops_enabled, diff --git a/app/models/application_setting_implementation.rb b/app/models/application_setting_implementation.rb index b413ffddb9dc67a17ad58a9b1941a8436b37ddde..ca99b1694d782a4ae5657fc8a914541a3e6f1965 100644 --- a/app/models/application_setting_implementation.rb +++ b/app/models/application_setting_implementation.rb @@ -21,6 +21,7 @@ module ApplicationSettingImplementation after_sign_up_text: nil, akismet_enabled: false, allow_local_requests_from_hooks_and_services: false, + dns_rebinding_protection_enabled: true, authorized_keys_enabled: true, # TODO default to false if the instance is configured to use AuthorizedKeysCommand container_registry_token_expire_delay: 5, default_artifacts_expire_in: '30 days', diff --git a/app/views/admin/application_settings/_outbound.html.haml b/app/views/admin/application_settings/_outbound.html.haml index f4bfb5af385701f5fa9f22b7028c0d4efc4c1d37..dd56bb99a06318ee2885dac862f5e42a76998ff0 100644 --- a/app/views/admin/application_settings/_outbound.html.haml +++ b/app/views/admin/application_settings/_outbound.html.haml @@ -8,4 +8,12 @@ = f.label :allow_local_requests_from_hooks_and_services, class: 'form-check-label' do Allow requests to the local network from hooks and services + .form-group + .form-check + = f.check_box :dns_rebinding_protection_enabled, class: 'form-check-input' + = f.label :dns_rebinding_protection_enabled, class: 'form-check-label' do + = _('Enforce DNS rebinding attack protection') + %span.form-text.text-muted + = _('Resolves IP addresses once and uses them to submit requests') + = f.submit 'Save changes', class: "btn btn-success" diff --git a/db/migrate/20190529142545_add_dns_rebinding_protection_enabled_to_application_settings.rb b/db/migrate/20190529142545_add_dns_rebinding_protection_enabled_to_application_settings.rb new file mode 100644 index 0000000000000000000000000000000000000000..8835dc8b7ba72f5e382b4ea2e565b9d410b2e635 --- /dev/null +++ b/db/migrate/20190529142545_add_dns_rebinding_protection_enabled_to_application_settings.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddDnsRebindingProtectionEnabledToApplicationSettings < ActiveRecord::Migration[5.1] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default(:application_settings, :dns_rebinding_protection_enabled, + :boolean, + default: true, + allow_null: false) + end + + def down + remove_column(:application_settings, :dns_rebinding_protection_enabled) + end +end diff --git a/db/schema.rb b/db/schema.rb index 8f13b079cf338c52c2567953b08d3268c5f4643c..bfed49655d70126ef92ae05a6394eb895e74e3fb 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20190408163745) do +ActiveRecord::Schema.define(version: 20190529142545) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -187,6 +187,7 @@ ActiveRecord::Schema.define(version: 20190408163745) do t.string "encrypted_external_auth_client_key_iv" t.string "encrypted_external_auth_client_key_pass" t.string "encrypted_external_auth_client_key_pass_iv" + t.boolean "dns_rebinding_protection_enabled", default: true, null: false t.index ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id", using: :btree end diff --git a/lib/gitlab.rb b/lib/gitlab.rb index 1204e53ee2e4fad1e74171b2f4fafdfe0b7bc6c5..7c03b9fd7cc53d8a1f7ec54069fbb74c5b832e54 100644 --- a/lib/gitlab.rb +++ b/lib/gitlab.rb @@ -40,6 +40,7 @@ module Gitlab SUBDOMAIN_REGEX = %r{\Ahttps://[a-z0-9]+\.gitlab\.com\z} VERSION = File.read(root.join("VERSION")).strip.freeze INSTALLATION_TYPE = File.read(root.join("INSTALLATION_TYPE")).strip.freeze + HTTP_PROXY_ENV_VARS = %w(http_proxy https_proxy HTTP_PROXY HTTPS_PROXY).freeze def self.com? # Check `gl_subdomain?` as well to keep parity with gitlab.com @@ -62,6 +63,10 @@ module Gitlab Object.const_defined?(:License) end + def self.http_proxy_env? + HTTP_PROXY_ENV_VARS.any? { |name| ENV[name] } + end + def self.process_name return 'sidekiq' if Sidekiq.server? return 'console' if defined?(Rails::Console) diff --git a/lib/gitlab/http_connection_adapter.rb b/lib/gitlab/http_connection_adapter.rb index 9ccf0653903d8f4b717a6de253745cfce67c3130..41eab3658bcb8cc08e971c3320c31456787718fe 100644 --- a/lib/gitlab/http_connection_adapter.rb +++ b/lib/gitlab/http_connection_adapter.rb @@ -14,7 +14,8 @@ module Gitlab def connection begin @uri, hostname = Gitlab::UrlBlocker.validate!(uri, allow_local_network: allow_local_requests?, - allow_localhost: allow_local_requests?) + allow_localhost: allow_local_requests?, + dns_rebind_protection: dns_rebind_protection?) rescue Gitlab::UrlBlocker::BlockedUrlError => e raise Gitlab::HTTP::BlockedUrlError, "URL '#{uri}' is blocked: #{e.message}" end @@ -30,6 +31,12 @@ module Gitlab options.fetch(:allow_local_requests, allow_settings_local_requests?) end + 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 diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index 622595e601262c6389a50edf783533bdbd0af2ba..5ba83f8cc98f9518322069266b1db990a61e3f58 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -18,7 +18,21 @@ module Gitlab # enforce_sanitization - Raises error if URL includes any HTML/CSS/JS tags and argument is true. # # Returns an array with [, ]. - def validate!(url, ports: [], protocols: [], allow_localhost: false, allow_local_network: true, ascii_only: false, enforce_user: false, enforce_sanitization: false) # rubocop:disable Metrics/CyclomaticComplexity + # rubocop:disable Metrics/CyclomaticComplexity + # rubocop:disable Metrics/ParameterLists + def validate!( + url, + ports: [], + schemes: [], + allow_localhost: false, + allow_local_network: true, + ascii_only: false, + enforce_user: false, + enforce_sanitization: false, + dns_rebind_protection: true) + # rubocop:enable Metrics/CyclomaticComplexity + # rubocop:enable Metrics/ParameterLists + return [nil, nil] if url.nil? # Param url can be a string, URI or Addressable::URI @@ -45,15 +59,17 @@ module Gitlab return [uri, nil] end + protected_uri_with_hostname = enforce_uri_hostname(addrs_info, uri, hostname, dns_rebind_protection) + # Allow url from the GitLab instance itself but only for the configured hostname and ports - return enforce_uri_hostname(addrs_info, uri, hostname) if internal?(uri) + return protected_uri_with_hostname if internal?(uri) validate_localhost!(addrs_info) unless allow_localhost validate_loopback!(addrs_info) unless allow_localhost validate_local_network!(addrs_info) unless allow_local_network validate_link_local!(addrs_info) unless allow_local_network - enforce_uri_hostname(addrs_info, uri, hostname) + protected_uri_with_hostname end def blocked_url?(*args) @@ -74,17 +90,15 @@ 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(addrs_info, uri, hostname) + def enforce_uri_hostname(addrs_info, uri, hostname, dns_rebind_protection) address = addrs_info.first ip_address = address&.ip_address - if ip_address && ip_address != hostname - uri = uri.dup - uri.hostname = ip_address - return [uri, hostname] - end + return [uri, nil] unless dns_rebind_protection && ip_address && ip_address != hostname - [uri, nil] + uri = uri.dup + uri.hostname = ip_address + [uri, hostname] end def get_port(uri) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 3b1b231bb8c0e8932aa2c08d488f0fdb9693ce99..bbe8b1bde3e03857aefb971a54021a2d30647d74 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3235,6 +3235,9 @@ msgstr "" msgid "Ends at (UTC)" msgstr "" +msgid "Enforce DNS rebinding attack protection" +msgstr "" + msgid "Enter at least three characters to search" msgstr "" @@ -6978,6 +6981,9 @@ msgstr "" msgid "Resolved all discussions." msgstr "" +msgid "Resolves IP addresses once and uses them to submit requests" +msgstr "" + msgid "Response metrics (AWS ELB)" msgstr "" diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index 04f39b807d7d514e75d5f936658b4433480366b1..52e586bb5f56874a0bc7f65b05c5f702a253c00b 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -325,16 +325,19 @@ describe 'Admin updates settings' do end context 'Network page' do - it 'Enable outbound requests' do + it 'Changes Outbound requests settings' do visit network_admin_application_settings_path page.within('.as-outbound') do check 'Allow requests to the local network from hooks and services' + # Enabled by default + uncheck 'Enforce DNS rebinding attack protection' click_button 'Save changes' end expect(page).to have_content "Application settings saved successfully" expect(Gitlab::CurrentSettings.allow_local_requests_from_hooks_and_services).to be true + expect(Gitlab::CurrentSettings.dns_rebinding_protection_enabled).to be false end end diff --git a/spec/lib/gitlab/http_connection_adapter_spec.rb b/spec/lib/gitlab/http_connection_adapter_spec.rb index af033be8e5bcb407d4480d937e2a69a308218e0d..930d1f62272159b31388405df3cdf3f5b0d21b75 100644 --- a/spec/lib/gitlab/http_connection_adapter_spec.rb +++ b/spec/lib/gitlab/http_connection_adapter_spec.rb @@ -47,6 +47,38 @@ describe Gitlab::HTTPConnectionAdapter do end end + context 'when DNS rebinding protection is disabled' do + it 'sets up the connection' do + stub_application_setting(dns_rebinding_protection_enabled: false) + + uri = URI('https://example.org') + + connection = described_class.new(uri).connection + + expect(connection).to be_a(Net::HTTP) + expect(connection.address).to eq('example.org') + expect(connection.hostname_override).to eq(nil) + expect(connection.addr_port).to eq('example.org') + expect(connection.port).to eq(443) + end + end + + context 'when http(s) environment variable is set' do + it 'sets up the connection' do + stub_env('https_proxy' => 'https://my.proxy') + + uri = URI('https://example.org') + + connection = described_class.new(uri).connection + + expect(connection).to be_a(Net::HTTP) + expect(connection.address).to eq('example.org') + expect(connection.hostname_override).to eq(nil) + expect(connection.addr_port).to eq('example.org') + expect(connection.port).to eq(443) + end + end + context 'when local requests are allowed' do it 'sets up the connection' do uri = URI('https://example.org') diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index 9c5c7602d05108436dc4b83c066c98483f4db655..7cb1898d5c00e87f8385403626a29a3f12246d23 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -46,6 +46,41 @@ describe Gitlab::UrlBlocker do expect(hostname).to be(nil) end end + + context 'disabled DNS rebinding protection' do + 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, dns_rebind_protection: false) + + expect(uri).to eq(Addressable::URI.parse('http://localhost')) + expect(hostname).to be(nil) + end + end + + context 'when the URL hostname is a domain' do + let(:import_url) { 'https://example.org' } + + it 'returns URI and no hostname' do + uri, hostname = described_class.validate!(import_url, dns_rebind_protection: false) + + expect(uri).to eq(Addressable::URI.parse('https://example.org')) + expect(hostname).to eq(nil) + 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, dns_rebind_protection: false) + + expect(uri).to eq(Addressable::URI.parse('https://93.184.216.34')) + expect(hostname).to be(nil) + end + end + end end describe '#blocked_url?' do diff --git a/spec/lib/gitlab_spec.rb b/spec/lib/gitlab_spec.rb index 767b5779a79c9fe4ca180f7fd55e813224bb66b9..e075904b0ccd5f5939702b6a1b9375c0c29574ac 100644 --- a/spec/lib/gitlab_spec.rb +++ b/spec/lib/gitlab_spec.rb @@ -109,4 +109,34 @@ describe Gitlab do expect(described_class.ee?).to eq(false) end end + + describe '.http_proxy_env?' do + it 'returns true when lower case https' do + stub_env('https_proxy', 'https://my.proxy') + + expect(described_class.http_proxy_env?).to eq(true) + end + + it 'returns true when upper case https' do + stub_env('HTTPS_PROXY', 'https://my.proxy') + + expect(described_class.http_proxy_env?).to eq(true) + end + + it 'returns true when lower case http' do + stub_env('http_proxy', 'http://my.proxy') + + expect(described_class.http_proxy_env?).to eq(true) + end + + it 'returns true when upper case http' do + stub_env('HTTP_PROXY', 'http://my.proxy') + + expect(described_class.http_proxy_env?).to eq(true) + end + + it 'returns false when not set' do + expect(described_class.http_proxy_env?).to eq(false) + end + end end