diff --git a/app/controllers/projects/services_controller.rb b/app/controllers/projects/services_controller.rb index f14cb5f6a9fc6c0cd44fcccad746fb29e602374c..a5ea9ff7ed7b767289128b1c36656e1dfa936c1e 100644 --- a/app/controllers/projects/services_controller.rb +++ b/app/controllers/projects/services_controller.rb @@ -46,6 +46,8 @@ class Projects::ServicesController < Projects::ApplicationController else { error: true, message: 'Validations failed.', service_response: @service.errors.full_messages.join(',') } end + rescue Gitlab::HTTP::BlockedUrlError => e + { error: true, message: 'Test failed.', service_response: e.message } end def success_message diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb index a34024f4f807b309692e7a6f4e27be891e811346..a3828acc50b408995ccb6986ff07e01f6801f61c 100644 --- a/app/services/projects/import_service.rb +++ b/app/services/projects/import_service.rb @@ -28,7 +28,11 @@ module Projects def add_repository_to_project if project.external_import? && !unknown_url? - raise Error, 'Blocked import URL.' if Gitlab::UrlBlocker.blocked_url?(project.import_url, valid_ports: Project::VALID_IMPORT_PORTS) + begin + Gitlab::UrlBlocker.validate!(project.import_url, valid_ports: Project::VALID_IMPORT_PORTS) + rescue Gitlab::UrlBlocker::BlockedUrlError => e + raise Error, "Blocked import URL: #{e.message}" + end end # We should skip the repository for a GitHub import or GitLab project import, diff --git a/app/validators/importable_url_validator.rb b/app/validators/importable_url_validator.rb index 3ec1594e20217aacc01e31d3209478cc098f7ed5..612d3c71913d6497e27075d82436209b87c9bc81 100644 --- a/app/validators/importable_url_validator.rb +++ b/app/validators/importable_url_validator.rb @@ -4,8 +4,8 @@ # protect against Server-side Request Forgery (SSRF). class ImportableUrlValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) - if Gitlab::UrlBlocker.blocked_url?(value, valid_ports: Project::VALID_IMPORT_PORTS) - record.errors.add(attribute, "imports are not allowed from that URL") - end + Gitlab::UrlBlocker.validate!(value, valid_ports: Project::VALID_IMPORT_PORTS) + rescue Gitlab::UrlBlocker::BlockedUrlError => e + record.errors.add(attribute, "is blocked: #{e.message}") end end diff --git a/lib/gitlab/http.rb b/lib/gitlab/http.rb index 96558872a378db463aa8eccb40beec218a0b9851..9aca3b0fb264e007f5a5c8235fdb62a88d16bf8f 100644 --- a/lib/gitlab/http.rb +++ b/lib/gitlab/http.rb @@ -4,6 +4,8 @@ # calling internal IP or services. module Gitlab class HTTP + BlockedUrlError = Class.new(StandardError) + include HTTParty # rubocop:disable Gitlab/HTTParty connection_adapter ProxyHTTPConnectionAdapter diff --git a/lib/gitlab/proxy_http_connection_adapter.rb b/lib/gitlab/proxy_http_connection_adapter.rb index c70d6f4cd8443b3c081f81de84c502e223097678..d682289b632cd4ff121ef1fd40a0d9e058306b22 100644 --- a/lib/gitlab/proxy_http_connection_adapter.rb +++ b/lib/gitlab/proxy_http_connection_adapter.rb @@ -10,8 +10,12 @@ module Gitlab class ProxyHTTPConnectionAdapter < HTTParty::ConnectionAdapter def connection - if !allow_local_requests? && blocked_url? - raise URI::InvalidURIError + 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 end super @@ -19,10 +23,6 @@ module Gitlab private - def blocked_url? - Gitlab::UrlBlocker.blocked_url?(uri, allow_private_networks: false) - end - def allow_local_requests? options.fetch(:allow_local_requests, allow_settings_local_requests?) end diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index 0f9f939e20443bc30b79f4f4d7b5ef4608f9a388..db97f65bd546ccfcf23f2552480ddcf4be0da450 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -2,48 +2,84 @@ require 'resolv' module Gitlab class UrlBlocker - class << self - def blocked_url?(url, allow_private_networks: true, valid_ports: []) - return false if url.nil? + BlockedUrlError = Class.new(StandardError) - blocked_ips = ["127.0.0.1", "::1", "0.0.0.0"] - blocked_ips.concat(Socket.ip_address_list.map(&:ip_address)) + class << self + def validate!(url, allow_localhost: false, allow_local_network: true, valid_ports: []) + return true if url.nil? begin uri = Addressable::URI.parse(url) - # Allow imports from the GitLab instance itself but only from the configured ports - return false if internal?(uri) + rescue Addressable::URI::InvalidURIError + raise BlockedUrlError, "URI is invalid" + end - return true if blocked_port?(uri.port, valid_ports) - return true if blocked_user_or_hostname?(uri.user) - return true if blocked_user_or_hostname?(uri.hostname) + # Allow imports from the GitLab instance itself but only from the configured ports + return true if internal?(uri) - addrs_info = Addrinfo.getaddrinfo(uri.hostname, 80, nil, :STREAM) - server_ips = addrs_info.map(&:ip_address) + port = uri.port || uri.default_port + validate_port!(port, valid_ports) if valid_ports.any? + validate_user!(uri.user) + validate_hostname!(uri.hostname) - return true if (blocked_ips & server_ips).any? - return true if !allow_private_networks && private_network?(addrs_info) - rescue Addressable::URI::InvalidURIError - return true + begin + addrs_info = Addrinfo.getaddrinfo(uri.hostname, port, nil, :STREAM) rescue SocketError - return false + return true end + validate_localhost!(addrs_info) unless allow_localhost + validate_local_network!(addrs_info) unless allow_local_network + + true + end + + def blocked_url?(*args) + validate!(*args) + false + rescue BlockedUrlError + true end private - def blocked_port?(port, valid_ports) - return false if port.blank? || valid_ports.blank? + def validate_port!(port, valid_ports) + return if port.blank? + # Only ports under 1024 are restricted + return if port >= 1024 + return if valid_ports.include?(port) - port < 1024 && !valid_ports.include?(port) + raise BlockedUrlError, "Only allowed ports are #{valid_ports.join(', ')}, and any over 1024" end - def blocked_user_or_hostname?(value) - return false if value.blank? + def validate_user!(value) + return if value.blank? + return if value =~ /\A\p{Alnum}/ - value !~ /\A\p{Alnum}/ + raise BlockedUrlError, "Username needs to start with an alphanumeric character" + end + + def validate_hostname!(value) + return if value.blank? + return if value =~ /\A\p{Alnum}/ + + raise BlockedUrlError, "Hostname needs to start with an alphanumeric character" + end + + def validate_localhost!(addrs_info) + local_ips = ["127.0.0.1", "::1", "0.0.0.0"] + local_ips.concat(Socket.ip_address_list.map(&:ip_address)) + + return if (local_ips & addrs_info.map(&:ip_address)).empty? + + raise BlockedUrlError, "Requests to localhost are not allowed" + end + + def validate_local_network!(addrs_info) + return unless addrs_info.any? { |addr| addr.ipv4_private? || addr.ipv6_sitelocal? } + + raise BlockedUrlError, "Requests to the local network are not allowed" end def internal?(uri) @@ -60,10 +96,6 @@ module Gitlab (uri.port.blank? || uri.port == config.gitlab_shell.ssh_port) end - def private_network?(addrs_info) - addrs_info.any? { |addr| addr.ipv4_private? || addr.ipv6_sitelocal? } - end - def config Gitlab.config end diff --git a/spec/lib/gitlab/http_spec.rb b/spec/lib/gitlab/http_spec.rb index b0bc081a3c81207443340e6fec7a8040466fb443..d0dadfa78da2ba678a0001f4ca2da16012e90cb6 100644 --- a/spec/lib/gitlab/http_spec.rb +++ b/spec/lib/gitlab/http_spec.rb @@ -12,11 +12,11 @@ describe Gitlab::HTTP do end it 'deny requests to localhost' do - expect { described_class.get('http://localhost:3003') }.to raise_error(URI::InvalidURIError) + expect { described_class.get('http://localhost:3003') }.to raise_error(Gitlab::HTTP::BlockedUrlError) end it 'deny requests to private network' do - expect { described_class.get('http://192.168.1.2:3003') }.to raise_error(URI::InvalidURIError) + expect { described_class.get('http://192.168.1.2:3003') }.to raise_error(Gitlab::HTTP::BlockedUrlError) end context 'if allow_local_requests set to true' do @@ -41,7 +41,7 @@ describe Gitlab::HTTP do context 'if allow_local_requests set to false' do it 'override the global value and ban requests to localhost or private network' do - expect { described_class.get('http://localhost:3003', allow_local_requests: false) }.to raise_error(URI::InvalidURIError) + expect { described_class.get('http://localhost:3003', allow_local_requests: false) }.to raise_error(Gitlab::HTTP::BlockedUrlError) end end end diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index 2d35b02648564337219339e157327d51aad9b148..a3b3dc3be6dd4984bc83f9c4923cc62cada2eb93 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -74,13 +74,13 @@ describe Gitlab::UrlBlocker do expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git')).to be false end - context 'when allow_private_networks is' do - let(:private_networks) { ['192.168.1.2', '10.0.0.2', '172.16.0.2'] } + context 'when allow_local_network is' do + let(:local_ips) { ['192.168.1.2', '10.0.0.2', '172.16.0.2'] } let(:fake_domain) { 'www.fakedomain.fake' } context 'true (default)' do it 'does not block urls from private networks' do - private_networks.each do |ip| + local_ips.each do |ip| stub_domain_resolv(fake_domain, ip) expect(described_class).not_to be_blocked_url("http://#{fake_domain}") @@ -94,14 +94,14 @@ describe Gitlab::UrlBlocker do context 'false' do it 'blocks urls from private networks' do - private_networks.each do |ip| + local_ips.each do |ip| stub_domain_resolv(fake_domain, ip) - expect(described_class).to be_blocked_url("http://#{fake_domain}", allow_private_networks: false) + expect(described_class).to be_blocked_url("http://#{fake_domain}", allow_local_network: false) unstub_domain_resolv - expect(described_class).to be_blocked_url("http://#{ip}", allow_private_networks: false) + expect(described_class).to be_blocked_url("http://#{ip}", allow_local_network: false) end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 96adf64bcec2ac0e858ba282c99b5ae71e82503d..fef868ac0f2b737259a7d44d0d4161eba7063db0 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -224,14 +224,14 @@ describe Project do project2 = build(:project, import_url: 'http://localhost:9000/t.git') expect(project2).to be_invalid - expect(project2.errors[:import_url]).to include('imports are not allowed from that URL') + expect(project2.errors[:import_url].first).to include('Requests to localhost are not allowed') end it "does not allow blocked import_url port" do project2 = build(:project, import_url: 'http://github.com:25/t.git') expect(project2).to be_invalid - expect(project2.errors[:import_url]).to include('imports are not allowed from that URL') + expect(project2.errors[:import_url].first).to include('Only allowed ports are 22, 80, 443') end describe 'project pending deletion' do diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb index bf7facaec99fda214341314a3fed0096d7c3c516..30c89ebd82159adc22e4a1e41174b57d0de21ebb 100644 --- a/spec/services/projects/import_service_spec.rb +++ b/spec/services/projects/import_service_spec.rb @@ -156,7 +156,7 @@ describe Projects::ImportService do result = described_class.new(project, user).execute expect(result[:status]).to eq :error - expect(result[:message]).to end_with 'Blocked import URL.' + expect(result[:message]).to include('Requests to localhost are not allowed') end it 'fails with port 25' do @@ -165,7 +165,7 @@ describe Projects::ImportService do result = described_class.new(project, user).execute expect(result[:status]).to eq :error - expect(result[:message]).to end_with 'Blocked import URL.' + expect(result[:message]).to include('Only allowed ports are 22, 80, 443') end end