Commit fd03bf75 authored by Douwe Maan's avatar Douwe Maan Committed by Oswaldo Ferreir
Browse files

Protect Gitlab::HTTP against DNS rebinding attack

Gitlab::HTTP now resolves the hostname only once, verifies the IP is not
blocked, and then uses the same IP to perform the actual request, while
passing the original hostname in the `Host` header and SSL SNI field.
parent 6e7d675d
......@@ -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,
......
......@@ -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',
......
......@@ -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"
---
title: Protect Gitlab::HTTP against DNS rebinding attack
merge_request:
author:
type: security
......@@ -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
# 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
# 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
......@@ -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"
......@@ -225,6 +225,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 ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id", using: :btree
t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id", using: :btree
t.index ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id", using: :btree
......
require 'spec_helper'
 
describe 'Groups > Billing', :js do
include StubRequests
let!(:user) { create(:user) }
let!(:group) { create(:group) }
let!(:bronze_plan) { create(:bronze_plan) }
 
before do
stub_request(:get, %r{https://customers.gitlab.com/gitlab_plans})
stub_full_request("https://customers.gitlab.com/gitlab_plans?plan=#{plan}")
.to_return(status: 200, body: File.new(Rails.root.join('ee/spec/fixtures/gitlab_com_plans.json')))
 
stub_application_setting(check_namespace_plan: true)
......@@ -16,6 +18,8 @@ describe 'Groups > Billing', :js do
end
 
context 'with a free plan' do
let(:plan) { 'free' }
before do
create(:gitlab_subscription, namespace: group, hosted_plan: nil, seats: 15)
end
......@@ -28,6 +32,8 @@ describe 'Groups > Billing', :js do
end
 
context 'with a paid plan' do
let(:plan) { 'bronze' }
before do
create(:gitlab_subscription, namespace: group, hosted_plan: bronze_plan, seats: 15)
end
......@@ -40,6 +46,8 @@ describe 'Groups > Billing', :js do
end
 
context 'with a legacy paid plan' do
let(:plan) { 'bronze' }
before do
group.update_attribute(:plan, bronze_plan)
end
......
......@@ -52,12 +52,14 @@ describe Gitlab::Geo::Oauth::Session, :geo do
 
context 'primary is configured with relative URL' do
it 'includes primary relative URL path' do
primary_node.update(url: 'http:://localhost/relative-path/')
api_url = 'http://localhost/relative-path/'
primary_node.update(url: api_url)
 
api_response = double(parsed: true)
 
expect_any_instance_of(OAuth2::AccessToken)
.to receive(:get).with(%r{^http:://localhost/relative-path/}).and_return(api_response)
.to receive(:get).with(%r{^#{api_url}}).and_return(api_response)
 
subject.authenticate('any token')
end
......
......@@ -34,7 +34,7 @@ describe DependencyProxy::DownloadBlobService do
before do
blob_url = DependencyProxy::Registry.blob_url(image, blob_sha)
 
stub_request(:get, blob_url).to_timeout
stub_full_request(blob_url).to_timeout
end
 
it { expect(subject[:status]).to eq(:error) }
......
......@@ -34,7 +34,7 @@ describe DependencyProxy::PullManifestService do
before do
manifest_link = DependencyProxy::Registry.manifest_url(image, tag)
 
stub_request(:get, manifest_link).to_timeout
stub_full_request(manifest_link).to_timeout
end
 
it { expect(subject[:status]).to eq(:error) }
......
......@@ -42,7 +42,7 @@ describe DependencyProxy::RequestTokenService do
before do
auth_link = DependencyProxy::Registry.auth_url(image)
 
stub_request(:any, auth_link).to_timeout
stub_full_request(auth_link, method: :any).to_timeout
end
 
it { expect(subject[:status]).to eq(:error) }
......
require Rails.root.join("spec/support/helpers/stub_requests.rb")
Dir[Rails.root.join("ee/spec/support/helpers/*.rb")].each { |f| require f }
Dir[Rails.root.join("ee/spec/support/shared_contexts/*.rb")].each { |f| require f }
Dir[Rails.root.join("ee/spec/support/shared_examples/*.rb")].each { |f| require f }
......
......@@ -2,25 +2,27 @@
 
module EE
module DependencyProxyHelpers
include StubRequests
def stub_registry_auth(image, token, status = 200, body = nil)
auth_body = { 'token' => token }.to_json
auth_link = registry.auth_url(image)
 
stub_request(:get, auth_link)
stub_full_request(auth_link)
.to_return(status: status, body: body || auth_body)
end
 
def stub_manifest_download(image, tag, status = 200, body = nil)
manifest_url = registry.manifest_url(image, tag)
 
stub_request(:get, manifest_url)
stub_full_request(manifest_url)
.to_return(status: status, body: body || manifest)
end
 
def stub_blob_download(image, blob_sha, status = 200, body = '123456')
download_link = registry.blob_url(image, blob_sha)
 
stub_request(:get, download_link)
stub_full_request(download_link)
.to_return(status: status, body: body)
end
 
......
......@@ -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)
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -4609,6 +4609,9 @@ msgstr ""
msgid "Ends at (UTC)"
msgstr ""
 
msgid "Enforce DNS rebinding attack protection"
msgstr ""
msgid "Enter at least three characters to search"
msgstr ""
 
......@@ -10791,6 +10794,9 @@ msgstr ""
msgid "Resolved by %{resolvedByName}"
msgstr ""
 
msgid "Resolves IP addresses once and uses them to submit requests"
msgstr ""
msgid "Response"
msgstr ""
 
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment