[11.3] Fix CRLF issue in UrlValidator

6 files changed, 122 insertions, 42 deletions

diff --git a/app/validators/url_validator.rb b/app/validators/url_validator.rb
index faaf1283078..216acf79cbd 100644
--- a/app/validators/url_validator.rb
+++ b/app/validators/url_validator.rb
@@ -41,12 +41,13 @@ class UrlValidator < ActiveModel::EachValidator
   def validate_each(record, attribute, value)
     @record = record
 
-    if value.present?
-      value.strip!
-    else
+    unless value.present?
       record.errors.add(attribute, 'must be a valid URL')
+      return
     end
 
+    value = strip_value!(record, attribute, value)
+
     Gitlab::UrlBlocker.validate!(value, blocker_args)
   rescue Gitlab::UrlBlocker::BlockedUrlError => e
     record.errors.add(attribute, "is blocked: #{e.message}")
@@ -54,6 +55,13 @@ class UrlValidator < ActiveModel::EachValidator
 
   private
 
+  def strip_value!(record, attribute, value)
+    new_value = value.strip
+    return value if new_value == value
+
+    record.public_send("#{attribute}=", new_value) # rubocop:disable GitlabSecurity/PublicSend
+  end
+
   def default_options
     # By default the validator doesn't block any url based on the ip address
     {
diff --git a/changelogs/unreleased/security-11-3-fj-crlf-injection.yml b/changelogs/unreleased/security-11-3-fj-crlf-injection.yml
new file mode 100644
index 00000000000..861167b8a6e
--- /dev/null
+++ b/changelogs/unreleased/security-11-3-fj-crlf-injection.yml
@@ -0,0 +1,5 @@
+---
+title: Fix CRLF vulnerability in Project hooks
+merge_request:
+author:
+type: security
diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb
index 6d3c6909bee..ade5c5242ff 100644
--- a/lib/gitlab/url_blocker.rb
+++ b/lib/gitlab/url_blocker.rb
@@ -9,11 +9,8 @@ module Gitlab
       def validate!(url, allow_localhost: false, allow_local_network: true, enforce_user: false, ports: [], protocols: [])
         return true if url.nil?
 
-        begin
-          uri = Addressable::URI.parse(url)
-        rescue Addressable::URI::InvalidURIError
-          raise BlockedUrlError, "URI is invalid"
-        end
+        # Param url can be a string, URI or Addressable::URI
+        uri = parse_url(url)
 
         # Allow imports from the GitLab instance itself but only from the configured ports
         return true if internal?(uri)
@@ -50,6 +47,18 @@ module Gitlab
 
       private
 
+      def parse_url(url)
+        raise Addressable::URI::InvalidURIError if multiline?(url)
+
+        Addressable::URI.parse(url)
+      rescue Addressable::URI::InvalidURIError, URI::InvalidURIError
+        raise BlockedUrlError, 'URI is invalid'
+      end
+
+      def multiline?(url)
+        CGI.unescape(url.to_s) =~ /\n|\r/
+      end
+
       def validate_port!(port, ports)
         return if port.blank?
         # Only ports under 1024 are restricted
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 3a285735682..a6cb1048a02 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -227,55 +227,72 @@ describe Project do
       end
     end
 
-    it 'does not allow an invalid URI as import_url' do
-      project2 = build(:project, import_url: 'invalid://')
+    describe 'import_url' do
+      it 'does not allow an invalid URI as import_url' do
+        project2 = build(:project, import_url: 'invalid://')
 
-      expect(project2).not_to be_valid
-    end
+        expect(project2).not_to be_valid
+      end
 
-    it 'does allow a valid URI as import_url' do
-      project2 = build(:project, import_url: 'ssh://test@gitlab.com/project.git')
+      it 'does allow a valid URI as import_url' do
+        project2 = build(:project, import_url: 'ssh://test@gitlab.com/project.git')
 
-      expect(project2).to be_valid
-    end
+        expect(project2).to be_valid
+      end
 
-    it 'allows an empty URI' do
-      project2 = build(:project, import_url: '')
+      it 'allows an empty URI' do
+        project2 = build(:project, import_url: '')
 
-      expect(project2).to be_valid
-    end
+        expect(project2).to be_valid
+      end
 
-    it 'does not produce import data on an empty URI' do
-      project2 = build(:project, import_url: '')
+      it 'does not produce import data on an empty URI' do
+        project2 = build(:project, import_url: '')
 
-      expect(project2.import_data).to be_nil
-    end
+        expect(project2.import_data).to be_nil
+      end
 
-    it 'does not produce import data on an invalid URI' do
-      project2 = build(:project, import_url: 'test://')
+      it 'does not produce import data on an invalid URI' do
+        project2 = build(:project, import_url: 'test://')
 
-      expect(project2.import_data).to be_nil
-    end
+        expect(project2.import_data).to be_nil
+      end
 
-    it "does not allow import_url pointing to localhost" do
-      project2 = build(:project, import_url: 'http://localhost:9000/t.git')
+      it "does not allow import_url pointing to localhost" do
+        project2 = build(:project, import_url: 'http://localhost:9000/t.git')
 
-      expect(project2).to be_invalid
-      expect(project2.errors[:import_url].first).to include('Requests to localhost are not allowed')
-    end
+        expect(project2).to be_invalid
+        expect(project2.errors[:import_url].first).to include('Requests to localhost are not allowed')
+      end
 
-    it "does not allow import_url with invalid ports" do
-      project2 = build(:project, import_url: 'http://github.com:25/t.git')
+      it "does not allow import_url with invalid ports" do
+        project2 = build(:project, import_url: 'http://github.com:25/t.git')
 
-      expect(project2).to be_invalid
-      expect(project2.errors[:import_url].first).to include('Only allowed ports are 22, 80, 443')
-    end
+        expect(project2).to be_invalid
+        expect(project2.errors[:import_url].first).to include('Only allowed ports are 22, 80, 443')
+      end
+
+      it "does not allow import_url with invalid user" do
+        project2 = build(:project, import_url: 'http://$user:password@github.com/t.git')
+
+        expect(project2).to be_invalid
+        expect(project2.errors[:import_url].first).to include('Username needs to start with an alphanumeric character')
+      end
+
+      include_context 'invalid urls'
+
+      it 'does not allow urls with CR or LF characters' do
+        project = build(:project)
 
-    it "does not allow import_url with invalid user" do
-      project2 = build(:project, import_url: 'http://$user:password@github.com/t.git')
+        aggregate_failures do
+          urls_with_CRLF.each do |url|
+            project.import_url = url
 
-      expect(project2).to be_invalid
-      expect(project2.errors[:import_url].first).to include('Username needs to start with an alphanumeric character')
+            expect(project).not_to be_valid
+            expect(project.errors.full_messages.first).to match(/is blocked: URI is invalid/)
+          end
+        end
+      end
     end
 
     describe 'project pending deletion' do
diff --git a/spec/support/shared_contexts/url_shared_context.rb b/spec/support/shared_contexts/url_shared_context.rb
new file mode 100644
index 00000000000..1b1f67daac3
--- /dev/null
+++ b/spec/support/shared_contexts/url_shared_context.rb
@@ -0,0 +1,17 @@
+shared_context 'invalid urls' do
+  let(:urls_with_CRLF) do
+    ["http://127.0.0.1:333/pa\rth",
+     "http://127.0.0.1:333/pa\nth",
+     "http://127.0a.0.1:333/pa\r\nth",
+     "http://127.0.0.1:333/path?param=foo\r\nbar",
+     "http://127.0.0.1:333/path?param=foo\rbar",
+     "http://127.0.0.1:333/path?param=foo\nbar",
+     "http://127.0.0.1:333/pa%0dth",
+     "http://127.0.0.1:333/pa%0ath",
+     "http://127.0a.0.1:333/pa%0d%0th",
+     "http://127.0.0.1:333/pa%0D%0Ath",
+     "http://127.0.0.1:333/path?param=foo%0Abar",
+     "http://127.0.0.1:333/path?param=foo%0Dbar",
+     "http://127.0.0.1:333/path?param=foo%0D%0Abar"]
+  end
+end
diff --git a/spec/validators/url_validator_spec.rb b/spec/validators/url_validator_spec.rb
index 93fe013d11c..522aedac25a 100644
--- a/spec/validators/url_validator_spec.rb
+++ b/spec/validators/url_validator_spec.rb
@@ -6,6 +6,30 @@ describe UrlValidator do
 
   include_examples 'url validator examples', described_class::DEFAULT_PROTOCOLS
 
+  describe 'validations' do
+    include_context 'invalid urls'
+
+    let(:validator) { described_class.new(attributes: [:link_url]) }
+
+    it 'returns error when url is nil' do
+      expect(validator.validate_each(badge, :link_url, nil)).to be_nil
+      expect(badge.errors.first[1]).to eq 'must be a valid URL'
+    end
+
+    it 'returns error when url is empty' do
+      expect(validator.validate_each(badge, :link_url, '')).to be_nil
+      expect(badge.errors.first[1]).to eq 'must be a valid URL'
+    end
+
+    it 'does not allow urls with CR or LF characters' do
+      aggregate_failures do
+        urls_with_CRLF.each do |url|
+          expect(validator.validate_each(badge, :link_url, url)[0]).to eq 'is blocked: URI is invalid'
+        end
+      end
+    end
+  end
+
   context 'by default' do
     let(:validator) { described_class.new(attributes: [:link_url]) }