diff options
author | John Jarvis <jarv@gitlab.com> | 2018-12-27 08:30:25 +0000 |
---|---|---|
committer | John Jarvis <jarv@gitlab.com> | 2018-12-27 08:30:25 +0000 |
commit | 1522ffd7d8c3a15f53b2a24cadae20da84eb060e (patch) | |
tree | ac4473b5b4cf52747c88977d269197d55a108a74 | |
parent | aba4b6afbedb5c7f0fa8d742b090d1471f1739b7 (diff) | |
parent | b52f3d235b7f9617f65c398b4085cbea1982c1f3 (diff) | |
download | gitlab-ce-1522ffd7d8c3a15f53b2a24cadae20da84eb060e.tar.gz |
Merge branch 'security-11-6-fix-ssrf-import-url-remote-mirror' into 'security-11-6'
[11.6] SSRF - Scan Internal Ports and GCP/AWS endpoints
See merge request gitlab/gitlabhq!2708
-rw-r--r-- | app/models/project.rb | 7 | ||||
-rw-r--r-- | app/models/remote_mirror.rb | 2 | ||||
-rw-r--r-- | changelogs/unreleased/security-fix-ssrf-import-url-remote-mirror.yml | 5 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 7 | ||||
-rw-r--r-- | spec/models/remote_mirror_spec.rb | 14 |
5 files changed, 30 insertions, 5 deletions
diff --git a/app/models/project.rb b/app/models/project.rb index 9e736a3b03c..03bcbbb5489 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 50920d9d1fc..5ee0f4ce4f2 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 |