Merge branch 'dm-http-hostname-override' into 'master'

Protect Gitlab::HTTP against DNS rebinding attack

See merge request gitlab/gitlabhq!3071

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,

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',

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"

changelogs/unreleased/dm-http-hostname-override.yml

+---
+title: Protect Gitlab::HTTP against DNS rebinding attack
+merge_request:
+author:
+type: security

config/initializers/hipchat_client_patch.rb

@@ -2,14 +2,14 @@
 # This monkey patches the HTTParty used in https://github.com/hipchat/hipchat-rb.
 module HipChat
   class Client
-    connection_adapter ::Gitlab::ProxyHTTPConnectionAdapter
+    connection_adapter ::Gitlab::HTTPConnectionAdapter
   end
   class Room
-    connection_adapter ::Gitlab::ProxyHTTPConnectionAdapter
+    connection_adapter ::Gitlab::HTTPConnectionAdapter
   end
   class User
-    connection_adapter ::Gitlab::ProxyHTTPConnectionAdapter
+    connection_adapter ::Gitlab::HTTPConnectionAdapter
   end
 end

config/initializers/http_hostname_override.rb

+# This override allows passing `@hostname_override` to the SNI protocol,
+# which is used to lookup the correct SSL certificate in the
+# request handshake process.
+#
+# Given we've forced the HTTP request to be sent to the resolved
+# IP address in a few scenarios (e.g.: `Gitlab::HTTP` through
+# `Gitlab::UrlBlocker.validate!`), we need to provide the _original_
+# hostname via SNI in order to have a clean connection setup.
+#
+# This is ultimately needed in order to avoid DNS rebinding attacks
+# through HTTP requests.
+#
+class OpenSSL::SSL::SSLContext
+  attr_accessor :hostname_override
+end
+
+class OpenSSL::SSL::SSLSocket
+  module HostnameOverride
+    # rubocop: disable Gitlab/ModuleWithInstanceVariables
+    def hostname=(hostname)
+      super(@context.hostname_override || hostname)
+    end
+
+    def post_connection_check(hostname)
+      super(@context.hostname_override || hostname)
+    end
+    # rubocop: enable Gitlab/ModuleWithInstanceVariables
+  end
+
+  prepend HostnameOverride
+end
+
+class Net::HTTP
+  attr_accessor :hostname_override
+  SSL_IVNAMES << :@hostname_override
+  SSL_ATTRIBUTES << :hostname_override
+
+  module HostnameOverride
+    def addr_port
+      return super unless hostname_override
+
+      addr = hostname_override
+      default_port = use_ssl? ? Net::HTTP.https_default_port : Net::HTTP.http_default_port
+      default_port == port ? addr : "#{addr}:#{port}"
+    end
+  end
+
+  prepend HostnameOverride
+end

db/migrate/20190529142545_add_dns_rebinding_protection_enabled_to_application_settings.rb

+# 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

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

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)

lib/gitlab/http.rb

@@ -11,7 +11,7 @@ module Gitlab
     include HTTParty # rubocop:disable Gitlab/HTTParty
-    connection_adapter ProxyHTTPConnectionAdapter
+    connection_adapter HTTPConnectionAdapter
     def self.perform_request(http_method, path, options, &block)
       super

lib/gitlab/proxy_http_connection_adapter.rb → lib/gitlab/http_connection_adapter.rb

@@ -10,17 +10,19 @@
 #
 # This option will take precedence over the global setting.
 module Gitlab
-  class ProxyHTTPConnectionAdapter < HTTParty::ConnectionAdapter
+  class HTTPConnectionAdapter < HTTParty::ConnectionAdapter
     def connection
-      unless allow_local_requests?
-        begin
-          Gitlab::UrlBlocker.validate!(uri, allow_local_network: false)
-        rescue Gitlab::UrlBlocker::BlockedUrlError => e
-          raise Gitlab::HTTP::BlockedUrlError, "URL '#{uri}' is blocked: #{e.message}"
-        end
+      begin
+        @uri, hostname = Gitlab::UrlBlocker.validate!(uri, allow_local_network: 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
-      super
+      super.tap do |http|
+        http.hostname_override = hostname if hostname
+      end
     end
     private
@@ -29,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

lib/gitlab/url_blocker.rb

@@ -8,38 +8,68 @@ module Gitlab
     BlockedUrlError = Class.new(StandardError)
     class << self
-      def validate!(url, ports: [], schemes: [], allow_localhost: false, allow_local_network: true, ascii_only: false, enforce_user: false, enforce_sanitization: false)
-        return true if url.nil?
+      # Validates the given url according to the constraints specified by arguments.
+      #
+      # ports - Raises error if the given URL port does is not between given ports.
+      # allow_localhost - Raises error if URL resolves to a localhost IP address and argument is true.
+      # allow_local_network - Raises error if URL resolves to a link-local address and argument is true.
+      # ascii_only - Raises error if URL has unicode characters and argument is true.
+      # enforce_user - Raises error if URL user doesn't start with alphanumeric characters and argument is true.
+      # enforce_sanitization - Raises error if URL includes any HTML/CSS/JS tags and argument is true.
+      #
+      # Returns an array with [<uri>, <original-hostname>].
+      # 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
         uri = parse_url(url)
         validate_html_tags!(uri) if enforce_sanitization
-        # Allow imports from the GitLab instance itself but only from the configured ports
-        return true if internal?(uri)
-
+        hostname = uri.hostname
         port = get_port(uri)
-        validate_scheme!(uri.scheme, schemes)
-        validate_port!(port, ports) if ports.any?
-        validate_user!(uri.user) if enforce_user
-        validate_hostname!(uri.hostname)
-        validate_unicode_restriction!(uri) if ascii_only
+
+        unless internal?(uri)
+          validate_scheme!(uri.scheme, schemes)
+          validate_port!(port, ports) if ports.any?
+          validate_user!(uri.user) if enforce_user
+          validate_hostname!(hostname)
+          validate_unicode_restriction!(uri) if ascii_only
+        end
         begin
-          addrs_info = Addrinfo.getaddrinfo(uri.hostname, port, nil, :STREAM).map do |addr|
+          addrs_info = Addrinfo.getaddrinfo(hostname, port, nil, :STREAM).map do |addr|
             addr.ipv6_v4mapped? ? addr.ipv6_to_ipv4 : addr
           end
         rescue SocketError
-          return true
+          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 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
-        true
+        protected_uri_with_hostname
       end
       def blocked_url?(*args)
@@ -52,6 +82,25 @@ module Gitlab
       private
+      # Returns the given URI with IP address as hostname and the original hostname respectively
+      # in an Array.
+      #
+      # It checks whether the resolved IP address matches with the hostname. If not, it changes
+      # the hostname to the resolved IP address.
+      #
+      # 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, dns_rebind_protection)
+        address = addrs_info.first
+        ip_address = address&.ip_address
+
+        return [uri, nil] unless dns_rebind_protection && ip_address && ip_address != hostname
+
+        uri = uri.dup
+        uri.hostname = ip_address
+        [uri, hostname]
+      end
+
       def get_port(uri)
         uri.port || uri.default_port
       end

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 ""

spec/controllers/projects/ci/lints_controller_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 describe Projects::Ci::LintsController do
+  include StubRequests
+
   let(:project) { create(:project, :repository) }
   let(:user) { create(:user) }
@@ -70,7 +72,7 @@ describe Projects::Ci::LintsController do
     context 'with a valid gitlab-ci.yml' do
       before do
-        WebMock.stub_request(:get, remote_file_path).to_return(body: remote_file_content)
+        stub_full_request(remote_file_path).to_return(body: remote_file_content)
         project.add_developer(user)
         post :create, params: { namespace_id: project.namespace, project_id: project, content: content }

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

spec/lib/gitlab/ci/config/external/file/remote_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 describe Gitlab::Ci::Config::External::File::Remote do
+  include StubRequests
+
   let(:context) { described_class::Context.new(nil, '12345', nil, Set.new) }
   let(:params) { { remote: location } }
   let(:remote_file) { described_class.new(params, context) }
@@ -46,7 +48,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
   describe "#valid?" do
     context 'when is a valid remote url' do
       before do
-        WebMock.stub_request(:get, location).to_return(body: remote_file_content)
+        stub_full_request(location).to_return(body: remote_file_content)
       end
       it 'returns true' do
@@ -92,7 +94,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
   describe "#content" do
     context 'with a valid remote file' do
       before do
-        WebMock.stub_request(:get, location).to_return(body: remote_file_content)
+        stub_full_request(location).to_return(body: remote_file_content)
       end
       it 'returns the content of the file' do
@@ -114,7 +116,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
       let(:location) { 'https://asdasdasdaj48ggerexample.com' }
       before do
-        WebMock.stub_request(:get, location).to_raise(SocketError.new('Some HTTP error'))
+        stub_full_request(location).to_raise(SocketError.new('Some HTTP error'))
       end
       it 'is nil' do
@@ -144,7 +146,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
     context 'when timeout error has been raised' do
       before do
-        WebMock.stub_request(:get, location).to_timeout
+        stub_full_request(location).to_timeout
       end
       it 'returns error message about a timeout' do
@@ -154,7 +156,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
     context 'when HTTP error has been raised' do
       before do
-        WebMock.stub_request(:get, location).to_raise(Gitlab::HTTP::Error)
+        stub_full_request(location).to_raise(Gitlab::HTTP::Error)
       end
       it 'returns error message about a HTTP error' do
@@ -164,7 +166,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
     context 'when response has 404 status' do
       before do
-        WebMock.stub_request(:get, location).to_return(body: remote_file_content, status: 404)
+        stub_full_request(location).to_return(body: remote_file_content, status: 404)
       end
       it 'returns error message about a timeout' do

spec/lib/gitlab/ci/config/external/mapper_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 describe Gitlab::Ci::Config::External::Mapper do
+  include StubRequests
+
   set(:project) { create(:project, :repository) }
   set(:user) { create(:user) }
@@ -18,7 +20,7 @@ describe Gitlab::Ci::Config::External::Mapper do
   end
   before do
-    WebMock.stub_request(:get, remote_url).to_return(body: file_content)
+    stub_full_request(remote_url).to_return(body: file_content)
   end
   describe '#process' do

spec/lib/gitlab/ci/config/external/processor_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 describe Gitlab::Ci::Config::External::Processor do
+  include StubRequests
+
   set(:project) { create(:project, :repository) }
   set(:another_project) { create(:project, :repository) }
   set(:user) { create(:user) }
@@ -42,7 +44,7 @@ describe Gitlab::Ci::Config::External::Processor do
       let(:values) { { include: remote_file, image: 'ruby:2.2' } }
       before do
-        WebMock.stub_request(:get, remote_file).to_raise(SocketError.new('Some HTTP error'))
+        stub_full_request(remote_file).and_raise(SocketError.new('Some HTTP error'))
       end
       it 'raises an error' do
@@ -75,7 +77,7 @@ describe Gitlab::Ci::Config::External::Processor do
       end
       before do
-        WebMock.stub_request(:get, remote_file).to_return(body: external_file_content)
+        stub_full_request(remote_file).to_return(body: external_file_content)
       end
       it 'appends the file to the values' do
@@ -145,7 +147,7 @@ describe Gitlab::Ci::Config::External::Processor do
         allow_any_instance_of(Gitlab::Ci::Config::External::File::Local)
           .to receive(:fetch_local_content).and_return(local_file_content)
-        WebMock.stub_request(:get, remote_file).to_return(body: remote_file_content)
+        stub_full_request(remote_file).to_return(body: remote_file_content)
       end
       it 'appends the files to the values' do
@@ -191,7 +193,8 @@ describe Gitlab::Ci::Config::External::Processor do
       end
       it 'takes precedence' do
-        WebMock.stub_request(:get, remote_file).to_return(body: remote_file_content)
+        stub_full_request(remote_file).to_return(body: remote_file_content)
+
         expect(processor.perform[:image]).to eq('ruby:2.2')
       end
     end
@@ -231,7 +234,8 @@ describe Gitlab::Ci::Config::External::Processor do
           HEREDOC
         end
-        WebMock.stub_request(:get, 'http://my.domain.com/config.yml').to_return(body: 'remote_build: { script: echo Hello World }')
+        stub_full_request('http://my.domain.com/config.yml')
+          .to_return(body: 'remote_build: { script: echo Hello World }')
       end
       context 'when project is public' do
@@ -273,8 +277,10 @@ describe Gitlab::Ci::Config::External::Processor do
     context 'when config includes an external configuration file via SSL web request' do
       before do
-        stub_request(:get, 'https://sha256.badssl.com/fake.yml').to_return(body: 'image: ruby:2.6', status: 200)
-        stub_request(:get, 'https://self-signed.badssl.com/fake.yml')
+        stub_full_request('https://sha256.badssl.com/fake.yml', ip_address: '8.8.8.8')
+          .to_return(body: 'image: ruby:2.6', status: 200)
+
+        stub_full_request('https://self-signed.badssl.com/fake.yml', ip_address: '8.8.8.9')
           .to_raise(OpenSSL::SSL::SSLError.new('SSL_connect returned=1 errno=0 state=error: certificate verify failed (self signed certificate)'))
       end

spec/lib/gitlab/ci/config_spec.rb

 require 'spec_helper'
 describe Gitlab::Ci::Config do
+  include StubRequests
+
   set(:user) { create(:user) }
   let(:config) do
@@ -216,8 +218,7 @@ describe Gitlab::Ci::Config do
     end
     before do
-      WebMock.stub_request(:get, remote_location)
-        .to_return(body: remote_file_content)
+      stub_full_request(remote_location).to_return(body: remote_file_content)
       allow(project.repository)
         .to receive(:blob_data_at).and_return(local_file_content)

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -3,6 +3,8 @@ require 'spec_helper'
 module Gitlab
   module Ci
     describe YamlProcessor do
+      include StubRequests
+
       subject { described_class.new(config, user: nil) }
       describe '#build_attributes' do
@@ -648,7 +650,7 @@ module Gitlab
             end
             before do
-              WebMock.stub_request(:get, 'https://gitlab.com/awesome-project/raw/master/.before-script-template.yml')
+              stub_full_request('https://gitlab.com/awesome-project/raw/master/.before-script-template.yml')
                 .to_return(
                   status: 200,
                   headers: { 'Content-Type' => 'application/json' },