diff options
author | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-05-30 10:44:45 +0000 |
---|---|---|
committer | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-05-30 10:44:45 +0000 |
commit | 5f1aaa8bf9d1c6f16d56496a7ffc151c1bfc204d (patch) | |
tree | f8c292fdafbf07e1c03cb3170c6a8e3b1b17bfb9 | |
parent | 086dd6e2bd52e7e107af7fe430cd35e9abe6b92f (diff) | |
parent | 0a9cb08389d8f71833b3a89c682fd9de3e21886c (diff) | |
download | gitlab-ce-5f1aaa8bf9d1c6f16d56496a7ffc151c1bfc204d.tar.gz |
Merge branch 'osw-disable-dns-rebind-protection-settings-11-10' into '11-10-stable'
Add DNS rebinding protection settings
See merge request gitlab/gitlabhq!3131
-rw-r--r-- | app/helpers/application_settings_helper.rb | 1 | ||||
-rw-r--r-- | app/models/application_setting_implementation.rb | 1 | ||||
-rw-r--r-- | app/views/admin/application_settings/_outbound.html.haml | 8 | ||||
-rw-r--r-- | changelogs/unreleased/osw-disable-dns-rebind-protection-settings-11-11.yml | 5 | ||||
-rw-r--r-- | db/migrate/20190529142545_add_dns_rebinding_protection_enabled_to_application_settings.rb | 23 | ||||
-rw-r--r-- | db/schema.rb | 3 | ||||
-rw-r--r-- | lib/gitlab.rb | 5 | ||||
-rw-r--r-- | lib/gitlab/http_connection_adapter.rb | 9 | ||||
-rw-r--r-- | lib/gitlab/url_blocker.rb | 34 | ||||
-rw-r--r-- | locale/gitlab.pot | 6 | ||||
-rw-r--r-- | spec/features/admin/admin_settings_spec.rb | 5 | ||||
-rw-r--r-- | spec/lib/gitlab/http_connection_adapter_spec.rb | 32 | ||||
-rw-r--r-- | spec/lib/gitlab/url_blocker_spec.rb | 35 | ||||
-rw-r--r-- | spec/lib/gitlab_spec.rb | 30 |
14 files changed, 184 insertions, 13 deletions
diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 5995ef57e26..ce097251212 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 b413ffddb9d..ca99b1694d7 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 f4bfb5af385..dd56bb99a06 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/changelogs/unreleased/osw-disable-dns-rebind-protection-settings-11-11.yml b/changelogs/unreleased/osw-disable-dns-rebind-protection-settings-11-11.yml new file mode 100644 index 00000000000..fc9a8bb8025 --- /dev/null +++ b/changelogs/unreleased/osw-disable-dns-rebind-protection-settings-11-11.yml @@ -0,0 +1,5 @@ +--- +title: Add DNS rebinding protection settings +merge_request: +author: +type: security 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 00000000000..eeb4b69b603 --- /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.0] + 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 8f13b079cf3..bfed49655d7 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 1204e53ee2e..7c03b9fd7cc 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 9ccf0653903..41eab3658bc 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 622595e6012..68cfd9fae87 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 [<uri>, <original-hostname>]. - 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: [], + protocols: [], + 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 3b1b231bb8c..bbe8b1bde3e 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 04f39b807d7..52e586bb5f5 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 af033be8e5b..930d1f62272 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 9c5c7602d05..7cb1898d5c0 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 767b5779a79..e075904b0cc 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 |