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.
This commit is contained in:
parent
e674a9d978
commit
f5c1cd4898
|
@ -0,0 +1,5 @@
|
||||||
|
---
|
||||||
|
title: Fix Server Side Request Forgery mitigation bypass
|
||||||
|
merge_request:
|
||||||
|
author:
|
||||||
|
type: security
|
|
@ -85,9 +85,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
|
||||||
|
@ -111,6 +111,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
|
||||||
|
# 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 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
|
||||||
|
|
||||||
def validate_local_request(address_info:, allow_localhost:, allow_local_network:)
|
def validate_local_request(address_info:, allow_localhost:, allow_local_network:)
|
||||||
|
|
|
@ -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
|
||||||
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
|
||||||
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
|
||||||
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
|
||||||
expect(described_class.blocked_url?('https://gitlab.com/foo/foo.bar', ascii_only: true)).to be true
|
expect(described_class.blocked_url?('https://gitlab.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
|
||||||
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!
|
||||||
|
|
||||||
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'
|
||||||
|
|
Loading…
Reference in New Issue