Merge branch 'dm-gitlab-http-blocked-url-error' into 'master'

Raise more descriptive errors when URLs are blocked

See merge request gitlab-org/gitlab-ce!18058

10 files changed, 90 insertions, 50 deletions

diff --git a/app/controllers/projects/services_controller.rb b/app/controllers/projects/services_controller.rb
index f14cb5f6a9f..a5ea9ff7ed7 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 a34024f4f80..a3828acc50b 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 3ec1594e202..612d3c71913 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 96558872a37..9aca3b0fb26 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 c70d6f4cd84..d682289b632 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 0f9f939e204..db97f65bd54 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 b0bc081a3c8..d0dadfa78da 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 2d35b026485..a3b3dc3be6d 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 96adf64bcec..fef868ac0f2 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 bf7facaec99..30c89ebd821 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