Fix Server Side Request Forgery mitigation bypass

When we can't resolve the hostname or it is invalid, we shouldn't
even perform the request. This fix also fixes the problem the
SSRF rebinding attack.

We can't stub feature flags outside example blocks. Nevertheless,
there are some actions that calls the UrlBlocker, that are performed
outside example blocks, ie: `set` instruction.

That's why we have to use some signalign mechanism outside the scope
of the specs.
parent b85e6215
---
title: Fix Server Side Request Forgery mitigation bypass
merge_request:
author:
type: security
...@@ -20,6 +20,7 @@ module Gitlab ...@@ -20,6 +20,7 @@ module Gitlab
# Returns an array with [<uri>, <original-hostname>]. # Returns an array with [<uri>, <original-hostname>].
# rubocop:disable Metrics/CyclomaticComplexity # rubocop:disable Metrics/CyclomaticComplexity
# rubocop:disable Metrics/ParameterLists # rubocop:disable Metrics/ParameterLists
# rubocop:disable Metrics/PerceivedComplexity
def validate!( def validate!(
url, url,
ports: [], ports: [],
...@@ -32,6 +33,7 @@ module Gitlab ...@@ -32,6 +33,7 @@ module Gitlab
dns_rebind_protection: true) dns_rebind_protection: true)
# rubocop:enable Metrics/CyclomaticComplexity # rubocop:enable Metrics/CyclomaticComplexity
# rubocop:enable Metrics/ParameterLists # rubocop:enable Metrics/ParameterLists
# rubocop:enable Metrics/PerceivedComplexity
return [nil, nil] if url.nil? return [nil, nil] if url.nil?
...@@ -56,7 +58,15 @@ module Gitlab ...@@ -56,7 +58,15 @@ module Gitlab
addr.ipv6_v4mapped? ? addr.ipv6_to_ipv4 : addr addr.ipv6_v4mapped? ? addr.ipv6_to_ipv4 : addr
end end
rescue SocketError rescue SocketError
return [uri, nil] # In the test suite we use a lot of mocked urls that are either invalid or
# don't exist. In order to avoid modifying a ton of tests and factories
# we allow invalid urls unless the environment variable RSPEC_ALLOW_INVALID_URLS
# is not true
return [uri, nil] if Rails.env.test? && ENV['RSPEC_ALLOW_INVALID_URLS'] == 'true'
# If the addr can't be resolved or the url is invalid (i.e http://1.1.1.1.1)
# we block the url
raise BlockedUrlError, "Host cannot be resolved or invalid"
end end
protected_uri_with_hostname = enforce_uri_hostname(addrs_info, uri, hostname, dns_rebind_protection) protected_uri_with_hostname = enforce_uri_hostname(addrs_info, uri, hostname, dns_rebind_protection)
...@@ -92,9 +102,9 @@ module Gitlab ...@@ -92,9 +102,9 @@ module Gitlab
# we'll be making the request to the IP address, instead of using the hostname. # 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) def enforce_uri_hostname(addrs_info, uri, hostname, dns_rebind_protection)
address = addrs_info.first address = addrs_info.first
ip_address = address&.ip_address ip_address = address.ip_address
return [uri, nil] unless dns_rebind_protection && ip_address && ip_address != hostname return [uri, nil] unless dns_rebind_protection && ip_address != hostname
uri = uri.dup uri = uri.dup
uri.hostname = ip_address uri.hostname = ip_address
......
...@@ -68,6 +68,16 @@ describe Gitlab::UrlBlocker do ...@@ -68,6 +68,16 @@ describe Gitlab::UrlBlocker do
expect(uri).to eq(Addressable::URI.parse('https://example.org')) expect(uri).to eq(Addressable::URI.parse('https://example.org'))
expect(hostname).to eq(nil) expect(hostname).to eq(nil)
end end
context 'when it cannot be resolved' do
let(:import_url) { 'http://foobar.x' }
it 'raises error' do
stub_env('RSPEC_ALLOW_INVALID_URLS', 'false')
expect { described_class.validate!(import_url) }.to raise_error(described_class::BlockedUrlError)
end
end
end end
context 'when the URL hostname is an IP address' do context 'when the URL hostname is an IP address' do
...@@ -79,6 +89,16 @@ describe Gitlab::UrlBlocker do ...@@ -79,6 +89,16 @@ describe Gitlab::UrlBlocker do
expect(uri).to eq(Addressable::URI.parse('https://93.184.216.34')) expect(uri).to eq(Addressable::URI.parse('https://93.184.216.34'))
expect(hostname).to be(nil) expect(hostname).to be(nil)
end end
context 'when it is invalid' do
let(:import_url) { 'http://1.1.1.1.1' }
it 'raises an error' do
stub_env('RSPEC_ALLOW_INVALID_URLS', 'false')
expect { described_class.validate!(import_url) }.to raise_error(described_class::BlockedUrlError)
end
end
end end
end end
end end
...@@ -180,8 +200,6 @@ describe Gitlab::UrlBlocker do ...@@ -180,8 +200,6 @@ describe Gitlab::UrlBlocker do
end end
it 'returns true for a non-alphanumeric hostname' do it 'returns true for a non-alphanumeric hostname' do
stub_resolv
aggregate_failures do aggregate_failures do
expect(described_class).to be_blocked_url('ssh://-oProxyCommand=whoami/a') expect(described_class).to be_blocked_url('ssh://-oProxyCommand=whoami/a')
...@@ -300,10 +318,6 @@ describe Gitlab::UrlBlocker do ...@@ -300,10 +318,6 @@ describe Gitlab::UrlBlocker do
end end
context 'when enforce_user is' do context 'when enforce_user is' do
before do
stub_resolv
end
context 'false (default)' do context 'false (default)' do
it 'does not block urls with a non-alphanumeric username' do it 'does not block urls with a non-alphanumeric username' do
expect(described_class).not_to be_blocked_url('ssh://-oProxyCommand=whoami@example.com/a') expect(described_class).not_to be_blocked_url('ssh://-oProxyCommand=whoami@example.com/a')
...@@ -351,6 +365,18 @@ describe Gitlab::UrlBlocker do ...@@ -351,6 +365,18 @@ describe Gitlab::UrlBlocker do
expect(described_class.blocked_url?('https://git‌lab.com/foo/foo.bar', ascii_only: true)).to be true expect(described_class.blocked_url?('https://git‌lab.com/foo/foo.bar', ascii_only: true)).to be true
end end
end end
it 'blocks urls with invalid ip address' do
stub_env('RSPEC_ALLOW_INVALID_URLS', 'false')
expect(described_class).to be_blocked_url('http://8.8.8.8.8')
end
it 'blocks urls whose hostname cannot be resolved' do
stub_env('RSPEC_ALLOW_INVALID_URLS', 'false')
expect(described_class).to be_blocked_url('http://foobar.x')
end
end end
describe '#validate_hostname!' do describe '#validate_hostname!' do
...@@ -382,10 +408,4 @@ describe Gitlab::UrlBlocker do ...@@ -382,10 +408,4 @@ describe Gitlab::UrlBlocker do
end end
end end
end end
# Resolv does not support resolving UTF-8 domain names
# See https://bugs.ruby-lang.org/issues/4270
def stub_resolv
allow(Resolv).to receive(:getaddresses).and_return([])
end
end end
...@@ -3,6 +3,7 @@ SimpleCovEnv.start! ...@@ -3,6 +3,7 @@ SimpleCovEnv.start!
ENV["RAILS_ENV"] = 'test' ENV["RAILS_ENV"] = 'test'
ENV["IN_MEMORY_APPLICATION_SETTINGS"] = 'true' ENV["IN_MEMORY_APPLICATION_SETTINGS"] = 'true'
ENV["RSPEC_ALLOW_INVALID_URLS"] = 'true'
require File.expand_path('../config/environment', __dir__) require File.expand_path('../config/environment', __dir__)
require 'rspec/rails' require 'rspec/rails'
......
Markdown is supported
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