Merge branch 'ssrf' into 'security'

Protect server against SSRF in project import URLs

See merge request !2068
Conflicts:
	spec/models/project_spec.rb

8 files changed, 160 insertions, 1 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 0ad477ba65e..d38421a0fee 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -188,6 +188,7 @@ class Project < ActiveRecord::Base
   validates_uniqueness_of :name, scope: :namespace_id
   validates_uniqueness_of :path, scope: :namespace_id
   validates :import_url, addressable_url: true, if: :external_import?
+  validates :import_url, importable_url: true, if: [:external_import?, :import_url_changed?]
   validates :star_count, numericality: { greater_than_or_equal_to: 0 }
   validate :check_limit, on: :create
   validate :avatar_type,
diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb
index cd230528743..f060405630e 100644
--- a/app/services/projects/import_service.rb
+++ b/app/services/projects/import_service.rb
@@ -33,6 +33,7 @@ module Projects
 
     def import_repository
       begin
+        raise Error, "Blocked import URL." if Gitlab::UrlBlocker.blocked_url?(project.import_url)
         gitlab_shell.import_repository(project.repository_storage_path, project.path_with_namespace, project.import_url)
       rescue => e
         # Expire cache to prevent scenarios such as:
@@ -40,7 +41,7 @@ module Projects
         # 2. Retried import, repo is broken or not imported but +exists?+ still returns true
         project.repository.before_import if project.repository_exists?
 
-        raise Error,  "Error importing repository #{project.import_url} into #{project.path_with_namespace} - #{e.message}"
+        raise Error, "Error importing repository #{project.import_url} into #{project.path_with_namespace} - #{e.message}"
       end
     end
 
diff --git a/app/validators/importable_url_validator.rb b/app/validators/importable_url_validator.rb
new file mode 100644
index 00000000000..37a314adee6
--- /dev/null
+++ b/app/validators/importable_url_validator.rb
@@ -0,0 +1,11 @@
+# ImportableUrlValidator
+#
+# This validator blocks projects from using dangerous import_urls to help
+# protect against Server-side Request Forgery (SSRF).
+class ImportableUrlValidator < ActiveModel::EachValidator
+  def validate_each(record, attribute, value)
+    if Gitlab::UrlBlocker.blocked_url?(value)
+      record.errors.add(attribute, "imports are not allowed from that URL")
+    end
+  end
+end
diff --git a/changelogs/unreleased/ssrf-protections.yml b/changelogs/unreleased/ssrf-protections.yml
new file mode 100644
index 00000000000..8d803738009
--- /dev/null
+++ b/changelogs/unreleased/ssrf-protections.yml
@@ -0,0 +1,4 @@
+---
+title: To protect against Server-side Request Forgery project import URLs are now prohibited against localhost or the server IP except for the assigned instance URL and port. Imports are also prohibited from ports below 1024 with the exception of ports 22, 80, and 443.
+merge_request:
+author:
diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb
new file mode 100644
index 00000000000..bb2f4edc1a0
--- /dev/null
+++ b/lib/gitlab/url_blocker.rb
@@ -0,0 +1,57 @@
+require 'resolv'
+
+module Gitlab
+  class UrlBlocker
+    class << self
+      # Used to specify what hosts and port numbers should be prohibited for project
+      # imports.
+      VALID_PORTS = [22, 80, 443].freeze
+
+      def blocked_url?(url)
+        blocked_ips = ["127.0.0.1", "::1", "0.0.0.0"]
+        blocked_ips.concat(Socket.ip_address_list.map(&:ip_address))
+
+        begin
+          uri = Addressable::URI.parse(url)
+          # Allow imports from the GitLab instance itself but only from the configured ports
+          return false if internal?(uri)
+
+          return true if blocked_port?(uri.port)
+
+          server_ips = Resolv.getaddresses(uri.hostname)
+          return true if (blocked_ips & server_ips).any?
+        rescue Addressable::URI::InvalidURIError
+          return true
+        end
+
+        false
+      end
+
+      private
+
+      def blocked_port?(port)
+        return false if port.blank?
+
+        port < 1024 && !VALID_PORTS.include?(port)
+      end
+
+      def internal?(uri)
+        internal_web?(uri) || internal_shell?(uri)
+      end
+
+      def internal_web?(uri)
+        uri.hostname == config.gitlab.host &&
+          (uri.port.blank? || uri.port == config.gitlab.port)
+      end
+
+      def internal_shell?(uri)
+        uri.hostname == config.gitlab_shell.ssh_host &&
+          (uri.port.blank? || uri.port == config.gitlab_shell.ssh_port)
+      end
+
+      def config
+        Gitlab.config
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb
new file mode 100644
index 00000000000..a504d299307
--- /dev/null
+++ b/spec/lib/gitlab/url_blocker_spec.rb
@@ -0,0 +1,31 @@
+require 'spec_helper'
+
+describe Gitlab::UrlBlocker, lib: true do
+  describe '#blocked_url?' do
+    it 'allows imports from configured web host and port' do
+      import_url = "http://#{Gitlab.config.gitlab.host}:#{Gitlab.config.gitlab.port}/t.git"
+      expect(described_class.blocked_url?(import_url)).to be false
+    end
+
+    it 'allows imports from configured SSH host and port' do
+      import_url = "http://#{Gitlab.config.gitlab_shell.ssh_host}:#{Gitlab.config.gitlab_shell.ssh_port}/t.git"
+      expect(described_class.blocked_url?(import_url)).to be false
+    end
+
+    it 'returns true for bad localhost hostname' do
+      expect(described_class.blocked_url?('https://localhost:65535/foo/foo.git')).to be true
+    end
+
+    it 'returns true for bad port' do
+      expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git')).to be true
+    end
+
+    it 'returns true for invalid URL' do
+      expect(described_class.blocked_url?('http://:8080')).to be true
+    end
+
+    it 'returns false for legitimate URL' do
+      expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git')).to be false
+    end
+  end
+end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 88d5d14f855..a975d560b09 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -217,6 +217,40 @@ describe Project, models: true do
 
       expect(project2.import_data).to be_nil
     end
+
+    it "does not allow blocked import_url localhost" do
+      project2 = build(:empty_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')
+    end
+
+    it "does not allow blocked import_url port" do
+      project2 = build(:empty_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')
+    end
+
+    describe 'project pending deletion' do
+      let!(:project_pending_deletion) do
+        create(:empty_project,
+               pending_delete: true)
+      end
+      let(:new_project) do
+        build(:empty_project,
+              name: project_pending_deletion.name,
+              namespace: project_pending_deletion.namespace)
+      end
+
+      before do
+        new_project.validate
+      end
+
+      it 'contains errors related to the project being deleted' do
+        expect(new_project.errors.full_messages.first).to eq('The project is still being deleted. Please try again later.')
+      end
+    end
   end
 
   describe 'default_scope' do
diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb
index ab6e8f537ba..e5917bb0b7a 100644
--- a/spec/services/projects/import_service_spec.rb
+++ b/spec/services/projects/import_service_spec.rb
@@ -120,6 +120,26 @@ describe Projects::ImportService, services: true do
       end
     end
 
+    context 'with blocked import_URL' do
+      it 'fails with localhost' do
+        project.import_url = 'https://localhost:9000/vim/vim.git'
+
+        result = described_class.new(project, user).execute
+
+        expect(result[:status]).to eq :error
+        expect(result[:message]).to end_with 'Blocked import URL.'
+      end
+
+      it 'fails with port 25' do
+        project.import_url = "https://github.com:25/vim/vim.git"
+
+        result = described_class.new(project, user).execute
+
+        expect(result[:status]).to eq :error
+        expect(result[:message]).to end_with 'Blocked import URL.'
+      end
+    end
+
     def stub_github_omniauth_provider
       provider = OpenStruct.new(
         'name' => 'github',