From 63c48f73803cf1c68d6c9af408f877ea61781118 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= <fjlopez@gitlab.com>
Date: Mon, 10 Dec 2018 17:23:52 +0100
Subject: [PATCH] Replaced UrlValidator with PublicUrlValidator for import_url
 and remote mirror urls

---
 app/models/project.rb                              |  7 +++----
 app/models/remote_mirror.rb                        |  2 +-
 .../security-fix-ssrf-import-url-remote-mirror.yml |  5 +++++
 spec/models/project_spec.rb                        |  7 +++++++
 spec/models/remote_mirror_spec.rb                  | 14 ++++++++++++++
 5 files changed, 30 insertions(+), 5 deletions(-)
 create mode 100644 changelogs/unreleased/security-fix-ssrf-import-url-remote-mirror.yml

diff --git a/app/models/project.rb b/app/models/project.rb
index 075c07f5c8e..f91ac015475 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -324,10 +324,9 @@ class Project < ActiveRecord::Base
 
   validates :namespace, presence: true
   validates :name, uniqueness: { scope: :namespace_id }
-  validates :import_url, url: { protocols: ->(project) { project.persisted? ? VALID_MIRROR_PROTOCOLS : VALID_IMPORT_PROTOCOLS },
-                                ports: ->(project) { project.persisted? ? VALID_MIRROR_PORTS : VALID_IMPORT_PORTS },
-                                allow_localhost: false,
-                                enforce_user: true }, if: [:external_import?, :import_url_changed?]
+  validates :import_url, public_url: { protocols: ->(project) { project.persisted? ? VALID_MIRROR_PROTOCOLS : VALID_IMPORT_PROTOCOLS },
+                                       ports: ->(project) { project.persisted? ? VALID_MIRROR_PORTS : VALID_IMPORT_PORTS },
+                                       enforce_user: true }, if: [:external_import?, :import_url_changed?]
   validates :star_count, numericality: { greater_than_or_equal_to: 0 }
   validate :check_limit, on: :create
   validate :check_repository_path_availability, on: :update, if: ->(project) { project.renamed? }
diff --git a/app/models/remote_mirror.rb b/app/models/remote_mirror.rb
index b7b4d0f1be9..327c6e7c7a3 100644
--- a/app/models/remote_mirror.rb
+++ b/app/models/remote_mirror.rb
@@ -17,7 +17,7 @@ class RemoteMirror < ActiveRecord::Base
 
   belongs_to :project, inverse_of: :remote_mirrors
 
-  validates :url, presence: true, url: { protocols: %w(ssh git http https), allow_blank: true, enforce_user: true }
+  validates :url, presence: true, public_url: { protocols: %w(ssh git http https), allow_blank: true, enforce_user: true }
 
   before_save :set_new_remote_name, if: :mirror_url_changed?
 
diff --git a/changelogs/unreleased/security-fix-ssrf-import-url-remote-mirror.yml b/changelogs/unreleased/security-fix-ssrf-import-url-remote-mirror.yml
new file mode 100644
index 00000000000..7ba7aa21090
--- /dev/null
+++ b/changelogs/unreleased/security-fix-ssrf-import-url-remote-mirror.yml
@@ -0,0 +1,5 @@
+---
+title: Fix SSRF with import_url and remote mirror url
+merge_request:
+author:
+type: security
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 9e5b06b745a..a9624b6a00e 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -314,6 +314,13 @@ describe Project do
         expect(project.errors[:import_url].first).to include('Requests to localhost are not allowed')
       end
 
+      it 'does not allow import_url pointing to the local network' do
+        project = build(:project, import_url: 'https://192.168.1.1')
+
+        expect(project).to be_invalid
+        expect(project.errors[:import_url].first).to include('Requests to the local network are not allowed')
+      end
+
       it "does not allow import_url with invalid ports for new projects" do
         project = build(:project, import_url: 'http://github.com:25/t.git')
 
diff --git a/spec/models/remote_mirror_spec.rb b/spec/models/remote_mirror_spec.rb
index b12ca79847c..66a25ccb410 100644
--- a/spec/models/remote_mirror_spec.rb
+++ b/spec/models/remote_mirror_spec.rb
@@ -24,6 +24,20 @@ describe RemoteMirror do
         expect(remote_mirror).to be_invalid
         expect(remote_mirror.errors[:url].first).to include('Username needs to start with an alphanumeric character')
       end
+
+      it 'does not allow url pointing to localhost' do
+        remote_mirror = build(:remote_mirror, url: 'http://127.0.0.2/t.git')
+
+        expect(remote_mirror).to be_invalid
+        expect(remote_mirror.errors[:url].first).to include('Requests to loopback addresses are not allowed')
+      end
+
+      it 'does not allow url pointing to the local network' do
+        remote_mirror = build(:remote_mirror, url: 'https://192.168.1.1')
+
+        expect(remote_mirror).to be_invalid
+        expect(remote_mirror.errors[:url].first).to include('Requests to the local network are not allowed')
+      end
     end
   end