diff options
| -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-- | 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 |
13 files changed, 179 insertions, 13 deletions
diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 971d1052824..4469118f065 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 e51619b0f9c..904d650ef96 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/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..8835dc8b7ba --- /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 bb59af540fe..9facc1c4bb2 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: 20190524062810) do +ActiveRecord::Schema.define(version: 20190529142545) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -193,6 +193,7 @@ ActiveRecord::Schema.define(version: 20190524062810) do t.integer "elasticsearch_replicas", default: 1, null: false t.text "encrypted_lets_encrypt_private_key" t.text "encrypted_lets_encrypt_private_key_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 3f107fbbf3b..ccaf06c5d6a 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}.freeze 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 @@ -66,6 +67,10 @@ module Gitlab end 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 81935f2b052..9a8df719827 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: [], schemes: [], 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 0d47c574120..b550553f303 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3775,6 +3775,9 @@ msgstr "" msgid "Ends at (UTC)" msgstr "" +msgid "Enforce DNS rebinding attack protection" +msgstr "" + msgid "Enter at least three characters to search" msgstr "" @@ -8286,6 +8289,9 @@ msgstr "" msgid "Resolved by %{resolvedByName}" 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 f9950b5b03f..c4dbe23f6b4 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -332,16 +332,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 416f8cfece9..253366e0789 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 |