Commit 112776b2 authored by GitLab Release Tools Bot's avatar GitLab Release Tools Bot

Merge branch 'security-http-hostname-override-11-10' into '11-10-stable'

Protect Gitlab::HTTP against DNS rebinding attack

See merge request gitlab/gitlabhq!3114
parents 701914e1 1de0a033
---
title: Protect Gitlab::HTTP against DNS rebinding attack
merge_request:
author:
type: security
...@@ -2,14 +2,14 @@ ...@@ -2,14 +2,14 @@
# This monkey patches the HTTParty used in https://github.com/hipchat/hipchat-rb. # This monkey patches the HTTParty used in https://github.com/hipchat/hipchat-rb.
module HipChat module HipChat
class Client class Client
connection_adapter ::Gitlab::ProxyHTTPConnectionAdapter connection_adapter ::Gitlab::HTTPConnectionAdapter
end end
class Room class Room
connection_adapter ::Gitlab::ProxyHTTPConnectionAdapter connection_adapter ::Gitlab::HTTPConnectionAdapter
end end
class User class User
connection_adapter ::Gitlab::ProxyHTTPConnectionAdapter connection_adapter ::Gitlab::HTTPConnectionAdapter
end end
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
...@@ -11,7 +11,7 @@ module Gitlab ...@@ -11,7 +11,7 @@ module Gitlab
include HTTParty # rubocop:disable Gitlab/HTTParty include HTTParty # rubocop:disable Gitlab/HTTParty
connection_adapter ProxyHTTPConnectionAdapter connection_adapter HTTPConnectionAdapter
def self.perform_request(http_method, path, options, &block) def self.perform_request(http_method, path, options, &block)
super super
......
...@@ -10,17 +10,18 @@ ...@@ -10,17 +10,18 @@
# #
# This option will take precedence over the global setting. # This option will take precedence over the global setting.
module Gitlab module Gitlab
class ProxyHTTPConnectionAdapter < HTTParty::ConnectionAdapter class HTTPConnectionAdapter < HTTParty::ConnectionAdapter
def connection def connection
unless allow_local_requests? begin
begin @uri, hostname = Gitlab::UrlBlocker.validate!(uri, allow_local_network: allow_local_requests?,
Gitlab::UrlBlocker.validate!(uri, allow_local_network: false) allow_localhost: allow_local_requests?)
rescue Gitlab::UrlBlocker::BlockedUrlError => e rescue Gitlab::UrlBlocker::BlockedUrlError => e
raise Gitlab::HTTP::BlockedUrlError, "URL '#{uri}' is blocked: #{e.message}" raise Gitlab::HTTP::BlockedUrlError, "URL '#{uri}' is blocked: #{e.message}"
end
end end
super super.tap do |http|
http.hostname_override = hostname if hostname
end
end end
private private
......
...@@ -8,38 +8,52 @@ module Gitlab ...@@ -8,38 +8,52 @@ module Gitlab
BlockedUrlError = Class.new(StandardError) BlockedUrlError = Class.new(StandardError)
class << self class << self
def validate!(url, ports: [], protocols: [], allow_localhost: false, allow_local_network: true, ascii_only: false, enforce_user: false, enforce_sanitization: false) # Validates the given url according to the constraints specified by arguments.
return true if url.nil? #
# 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>].
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
return [nil, nil] if url.nil?
# Param url can be a string, URI or Addressable::URI # Param url can be a string, URI or Addressable::URI
uri = parse_url(url) uri = parse_url(url)
validate_html_tags!(uri) if enforce_sanitization validate_html_tags!(uri) if enforce_sanitization
# Allow imports from the GitLab instance itself but only from the configured ports hostname = uri.hostname
return true if internal?(uri)
port = get_port(uri) port = get_port(uri)
validate_protocol!(uri.scheme, protocols)
validate_port!(port, ports) if ports.any? unless internal?(uri)
validate_user!(uri.user) if enforce_user validate_protocol!(uri.scheme, protocols)
validate_hostname!(uri.hostname) validate_port!(port, ports) if ports.any?
validate_unicode_restriction!(uri) if ascii_only validate_user!(uri.user) if enforce_user
validate_hostname!(hostname)
validate_unicode_restriction!(uri) if ascii_only
end
begin 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 addr.ipv6_v4mapped? ? addr.ipv6_to_ipv4 : addr
end end
rescue SocketError rescue SocketError
return true return [uri, nil]
end end
# 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)
validate_localhost!(addrs_info) unless allow_localhost validate_localhost!(addrs_info) unless allow_localhost
validate_loopback!(addrs_info) unless allow_localhost validate_loopback!(addrs_info) unless allow_localhost
validate_local_network!(addrs_info) unless allow_local_network validate_local_network!(addrs_info) unless allow_local_network
validate_link_local!(addrs_info) unless allow_local_network validate_link_local!(addrs_info) unless allow_local_network
true enforce_uri_hostname(addrs_info, uri, hostname)
end end
def blocked_url?(*args) def blocked_url?(*args)
...@@ -52,6 +66,27 @@ module Gitlab ...@@ -52,6 +66,27 @@ module Gitlab
private 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)
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
[uri, nil]
end
def get_port(uri) def get_port(uri)
uri.port || uri.default_port uri.port || uri.default_port
end end
......
require 'spec_helper' require 'spec_helper'
describe Projects::Ci::LintsController do describe Projects::Ci::LintsController do
include StubRequests
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:user) { create(:user) } let(:user) { create(:user) }
...@@ -68,7 +70,7 @@ describe Projects::Ci::LintsController do ...@@ -68,7 +70,7 @@ describe Projects::Ci::LintsController do
context 'with a valid gitlab-ci.yml' do context 'with a valid gitlab-ci.yml' do
before 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) project.add_developer(user)
post :create, params: { namespace_id: project.namespace, project_id: project, content: content } post :create, params: { namespace_id: project.namespace, project_id: project, content: content }
......
...@@ -3,6 +3,8 @@ ...@@ -3,6 +3,8 @@
require 'spec_helper' require 'spec_helper'
describe Gitlab::Ci::Config::External::File::Remote do describe Gitlab::Ci::Config::External::File::Remote do
include StubRequests
let(:context) { described_class::Context.new(nil, '12345', nil, Set.new) } let(:context) { described_class::Context.new(nil, '12345', nil, Set.new) }
let(:params) { { remote: location } } let(:params) { { remote: location } }
let(:remote_file) { described_class.new(params, context) } let(:remote_file) { described_class.new(params, context) }
...@@ -46,7 +48,7 @@ describe Gitlab::Ci::Config::External::File::Remote do ...@@ -46,7 +48,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
describe "#valid?" do describe "#valid?" do
context 'when is a valid remote url' do context 'when is a valid remote url' do
before 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 end
it 'returns true' do it 'returns true' do
...@@ -92,7 +94,7 @@ describe Gitlab::Ci::Config::External::File::Remote do ...@@ -92,7 +94,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
describe "#content" do describe "#content" do
context 'with a valid remote file' do context 'with a valid remote file' do
before 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 end
it 'returns the content of the file' do it 'returns the content of the file' do
...@@ -114,7 +116,7 @@ describe Gitlab::Ci::Config::External::File::Remote do ...@@ -114,7 +116,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
let(:location) { 'https://asdasdasdaj48ggerexample.com' } let(:location) { 'https://asdasdasdaj48ggerexample.com' }
before do 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 end
it 'is nil' do it 'is nil' do
...@@ -144,7 +146,7 @@ describe Gitlab::Ci::Config::External::File::Remote do ...@@ -144,7 +146,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
context 'when timeout error has been raised' do context 'when timeout error has been raised' do
before do before do
WebMock.stub_request(:get, location).to_timeout stub_full_request(location).to_timeout
end end
it 'returns error message about a timeout' do it 'returns error message about a timeout' do
...@@ -154,7 +156,7 @@ describe Gitlab::Ci::Config::External::File::Remote do ...@@ -154,7 +156,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
context 'when HTTP error has been raised' do context 'when HTTP error has been raised' do
before do before do
WebMock.stub_request(:get, location).to_raise(Gitlab::HTTP::Error) stub_full_request(location).to_raise(Gitlab::HTTP::Error)
end end
it 'returns error message about a HTTP error' do it 'returns error message about a HTTP error' do
...@@ -164,7 +166,7 @@ describe Gitlab::Ci::Config::External::File::Remote do ...@@ -164,7 +166,7 @@ describe Gitlab::Ci::Config::External::File::Remote do
context 'when response has 404 status' do context 'when response has 404 status' do
before 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 end
it 'returns error message about a timeout' do it 'returns error message about a timeout' do
......
...@@ -3,6 +3,8 @@ ...@@ -3,6 +3,8 @@
require 'spec_helper' require 'spec_helper'
describe Gitlab::Ci::Config::External::Mapper do describe Gitlab::Ci::Config::External::Mapper do
include StubRequests
set(:project) { create(:project, :repository) } set(:project) { create(:project, :repository) }
set(:user) { create(:user) } set(:user) { create(:user) }
...@@ -18,7 +20,7 @@ describe Gitlab::Ci::Config::External::Mapper do ...@@ -18,7 +20,7 @@ describe Gitlab::Ci::Config::External::Mapper do
end end
before do before do
WebMock.stub_request(:get, remote_url).to_return(body: file_content) stub_full_request(remote_url).to_return(body: file_content)
end end
describe '#process' do describe '#process' do
......
...@@ -3,6 +3,8 @@ ...@@ -3,6 +3,8 @@
require 'spec_helper' require 'spec_helper'
describe Gitlab::Ci::Config::External::Processor do describe Gitlab::Ci::Config::External::Processor do
include StubRequests
set(:project) { create(:project, :repository) } set(:project) { create(:project, :repository) }
set(:another_project) { create(:project, :repository) } set(:another_project) { create(:project, :repository) }
set(:user) { create(:user) } set(:user) { create(:user) }
...@@ -42,7 +44,7 @@ describe Gitlab::Ci::Config::External::Processor do ...@@ -42,7 +44,7 @@ describe Gitlab::Ci::Config::External::Processor do
let(:values) { { include: remote_file, image: 'ruby:2.2' } } let(:values) { { include: remote_file, image: 'ruby:2.2' } }
before do 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 end
it 'raises an error' do it 'raises an error' do
...@@ -75,7 +77,7 @@ describe Gitlab::Ci::Config::External::Processor do ...@@ -75,7 +77,7 @@ describe Gitlab::Ci::Config::External::Processor do
end end
before do 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 end
it 'appends the file to the values' do it 'appends the file to the values' do
...@@ -145,7 +147,7 @@ describe Gitlab::Ci::Config::External::Processor do ...@@ -145,7 +147,7 @@ describe Gitlab::Ci::Config::External::Processor do
allow_any_instance_of(Gitlab::Ci::Config::External::File::Local) allow_any_instance_of(Gitlab::Ci::Config::External::File::Local)
.to receive(:fetch_local_content).and_return(local_file_content) .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 end
it 'appends the files to the values' do it 'appends the files to the values' do
...@@ -191,7 +193,8 @@ describe Gitlab::Ci::Config::External::Processor do ...@@ -191,7 +193,8 @@ describe Gitlab::Ci::Config::External::Processor do
end end
it 'takes precedence' do 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') expect(processor.perform[:image]).to eq('ruby:2.2')
end end
end end
...@@ -231,7 +234,8 @@ describe Gitlab::Ci::Config::External::Processor do ...@@ -231,7 +234,8 @@ describe Gitlab::Ci::Config::External::Processor do
HEREDOC HEREDOC
end 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 end
context 'when project is public' do context 'when project is public' do
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::Ci::Config do describe Gitlab::Ci::Config do
include StubRequests
set(:user) { create(:user) } set(:user) { create(:user) }
let(:config) do let(:config) do
...@@ -217,8 +219,7 @@ describe Gitlab::Ci::Config do ...@@ -217,8 +219,7 @@ describe Gitlab::Ci::Config do
end end
before do before do
WebMock.stub_request(:get, remote_location) stub_full_request(remote_location).to_return(body: remote_file_content)
.to_return(body: remote_file_content)
allow(project.repository) allow(project.repository)
.to receive(:blob_data_at).and_return(local_file_content) .to receive(:blob_data_at).and_return(local_file_content)
......
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::HTTPConnectionAdapter do
describe '#connection' do
context 'when local requests are not allowed' do
it 'sets up the connection' do
uri = URI('https://example.org')
connection = described_class.new(uri).connection
expect(connection).to be_a(Net::HTTP)
expect(connection.address).to eq('93.184.216.34')
expect(connection.hostname_override).to eq('example.org')
expect(connection.addr_port).to eq('example.org')
expect(connection.port).to eq(443)
end
it 'raises error when it is a request to local address' do
uri = URI('http://172.16.0.0/12')
expect { described_class.new(uri).connection }
.to raise_error(Gitlab::HTTP::BlockedUrlError,
"URL 'http://172.16.0.0/12' is blocked: Requests to the local network are not allowed")
end
it 'raises error when it is a request to localhost address' do
uri = URI('http://127.0.0.1')
expect { described_class.new(uri).connection }
.to raise_error(Gitlab::HTTP::BlockedUrlError,
"URL 'http://127.0.0.1' is blocked: Requests to localhost are not allowed")
end
context 'when port different from URL scheme is used' do
it 'sets up the addr_port accordingly' do
uri = URI('https://example.org:8080')
connection = described_class.new(uri).connection
expect(connection.address).to eq('93.184.216.34')
expect(connection.hostname_override).to eq('example.org')
expect(connection.addr_port).to eq('example.org:8080')
expect(connection.port).to eq(8080)
end
end
end
context 'when local requests are allowed' do
it 'sets up the connection' do
uri = URI('https://example.org')
connection = described_class.new(uri, allow_local_requests: true).connection
expect(connection).to be_a(Net::HTTP)
expect(connection.address).to eq('93.184.216.34')
expect(connection.hostname_override).to eq('example.org')
expect(connection.addr_port).to eq('example.org')
expect(connection.port).to eq(443)
end
it 'sets up the connection when it is a local network' do
uri = URI('http://172.16.0.0/12')
connection = described_class.new(uri, allow_local_requests: true).connection
expect(connection).to be_a(Net::HTTP)
expect(connection.address).to eq('172.16.0.0')
expect(connection.hostname_override).to be(nil)
expect(connection.addr_port).to eq('172.16.0.0')
expect(connection.port).to eq(80)
end
it 'sets up the connection when it is localhost' do
uri = URI('http://127.0.0.1')
connection = described_class.new(uri, allow_local_requests: true).connection
expect(connection).to be_a(Net::HTTP)
expect(connection.address).to eq('127.0.0.1')
expect(connection.hostname_override).to be(nil)
expect(connection.addr_port).to eq('127.0.0.1')
expect(connection.port).to eq(80)
end
end
end
end
require 'spec_helper' require 'spec_helper'
describe Gitlab::HTTP do describe Gitlab::HTTP do
include StubRequests
context 'when allow_local_requests' do
it 'sends the request to the correct URI' do
stub_full_request('https://example.org:8080', ip_address: '8.8.8.8').to_return(status: 200)
described_class.get('https://example.org:8080', allow_local_requests: false)
expect(WebMock).to have_requested(:get, 'https://8.8.8.8:8080').once
end
end
context 'when not allow_local_requests' do
it 'sends the request to the correct URI' do
stub_full_request('https://example.org:8080')
described_class.get('https://example.org:8080', allow_local_requests: true)
expect(WebMock).to have_requested(:get, 'https://8.8.8.9:8080').once
end
end
describe 'allow_local_requests_from_hooks_and_services is' do describe 'allow_local_requests_from_hooks_and_services is' do
before do before do
WebMock.stub_request(:get, /.*/).to_return(status: 200, body: 'Success') WebMock.stub_request(:get, /.*/).to_return(status: 200, body: 'Success')
...@@ -21,6 +43,8 @@ describe Gitlab::HTTP do ...@@ -21,6 +43,8 @@ describe Gitlab::HTTP do
context 'if allow_local_requests set to true' do context 'if allow_local_requests set to true' do
it 'override the global value and allow requests to localhost or private network' do it 'override the global value and allow requests to localhost or private network' do
stub_full_request('http://localhost:3003')
expect { described_class.get('http://localhost:3003', allow_local_requests: true) }.not_to raise_error expect { described_class.get('http://localhost:3003', allow_local_requests: true) }.not_to raise_error
end end
end end
...@@ -32,6 +56,8 @@ describe Gitlab::HTTP do ...@@ -32,6 +56,8 @@ describe Gitlab::HTTP do
end end
it 'allow requests to localhost' do it 'allow requests to localhost' do
stub_full_request('http://localhost:3003')
expect { described_class.get('http://localhost:3003') }.not_to raise_error expect { described_class.get('http://localhost:3003') }.not_to raise_error
end end
...@@ -49,7 +75,7 @@ describe Gitlab::HTTP do ...@@ -49,7 +75,7 @@ describe Gitlab::HTTP do
describe 'handle redirect loops' do describe 'handle redirect loops' do
before do before do
WebMock.stub_request(:any, "http://example.org").to_raise(HTTParty::RedirectionTooDeep.new("Redirection Too Deep")) stub_full_request("http://example.org", method: :any).to_raise(HTTParty::RedirectionTooDeep.new("Redirection Too Deep"))
end end
it 'handles GET requests' do it 'handles GET requests' do
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::ImportExport::AfterExportStrategies::WebUploadStrategy do describe Gitlab::ImportExport::AfterExportStrategies::WebUploadStrategy do
include StubRequests
let(:example_url) { 'http://www.example.com' } let(:example_url) { 'http://www.example.com' }
let(:strategy) { subject.new(url: example_url, http_method: 'post') } let(:strategy) { subject.new(url: example_url, http_method: 'post') }
let!(:project) { create(:project, :with_export) } let!(:project) { create(:project, :with_export) }
...@@ -35,7 +37,7 @@ describe Gitlab::ImportExport::AfterExportStrategies::WebUploadStrategy do ...@@ -35,7 +37,7 @@ describe Gitlab::ImportExport::AfterExportStrategies::WebUploadStrategy do
context 'when upload fails' do context 'when upload fails' do
it 'stores the export error' do it 'stores the export error' do
stub_request(:post, example_url).to_return(status: [404, 'Page not found']) stub_full_request(example_url, method: :post).to_return(status: [404, 'Page not found'])
strategy.execute(user, project) strategy.execute(user, project)
......
...@@ -2,6 +2,52 @@ ...@@ -2,6 +2,52 @@
require 'spec_helper' require 'spec_helper'