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

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