Handle Stored XSS for Grafana URL in settings

- Extend Gitlab::UrlBlocker to allow relative urls (require_absolute
  setting).  The new `require_absolute` setting defaults to true,
  which is the existing behavior.

- Extend AddressableUrlValidator to accept `require_abosolute` and
  default to the existing behavior

- Add validation for ApplicationSetting#grafana_url to validate that
  the URL does not contain XSS but can be a valid relative or absolute
  url.

- In the case of existing stored URLs, validate the stored URL does
  not contain XSS. If the stored URL contains stored XSS or is an
  otherwise invalid URL, return the default database column value.

- Add tests for Gitlab::UrlBlocker to test require_absolute setting

- Add tests for AddressableUrlValidator

- Add tests for ApplicationSetting#grafana_url

8 files changed, 189 insertions, 15 deletions

diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 6aadbbc9d03..6cfbaf16db7 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -7,6 +7,13 @@ class ApplicationSetting < ApplicationRecord
   include IgnorableColumn
   include ChronicDurationAttribute
 
+  GRAFANA_URL_RULES = {
+    allow_localhost: true,
+    allow_local_network: true,
+    enforce_sanitization: true,
+    require_absolute: false
+  }.freeze
+
   add_authentication_token_field :runners_registration_token, encrypted: -> { Feature.enabled?(:application_settings_tokens_optional_encryption, default_enabled: true) ? :optional : :required }
   add_authentication_token_field :health_check_access_token
 
@@ -48,6 +55,11 @@ class ApplicationSetting < ApplicationRecord
 
   validates :uuid, presence: true
 
+  validates :grafana_url,
+            allow_blank: true,
+            allow_nil: true,
+            addressable_url: GRAFANA_URL_RULES
+
   validates :session_expire_delay,
             presence: true,
             numericality: { only_integer: true, greater_than_or_equal_to: 0 }
@@ -65,7 +77,6 @@ class ApplicationSetting < ApplicationRecord
   validates :after_sign_out_path,
             allow_blank: true,
             addressable_url: true
-
   validates :admin_notification_email,
             devise_email: true,
             allow_blank: true
@@ -291,6 +302,14 @@ class ApplicationSetting < ApplicationRecord
     current_without_cache
   end
 
+  def grafana_url
+    if Gitlab::UrlBlocker.blocked_url?(self[:grafana_url], GRAFANA_URL_RULES)
+      ApplicationSetting.column_defaults["grafana_url"]
+    else
+      self[:grafana_url]
+    end
+  end
+
   # By default, the backend is Rails.cache, which uses
   # ActiveSupport::Cache::RedisStore. Since loading ApplicationSetting
   # can cause a significant amount of load on Redis, let's cache it in
diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb
index 273e15ef925..9e9ff9b0d1f 100644
--- a/app/validators/addressable_url_validator.rb
+++ b/app/validators/addressable_url_validator.rb
@@ -49,7 +49,8 @@ class AddressableUrlValidator < ActiveModel::EachValidator
     allow_local_network: true,
     ascii_only: false,
     enforce_user: false,
-    enforce_sanitization: false
+    enforce_sanitization: false,
+    require_absolute: true
   }.freeze
 
   DEFAULT_OPTIONS = BLOCKER_VALIDATE_OPTIONS.merge({
diff --git a/changelogs/unreleased/security-xss-grafana-url-12-4.yml b/changelogs/unreleased/security-xss-grafana-url-12-4.yml
new file mode 100644
index 00000000000..d0adff94b76
--- /dev/null
+++ b/changelogs/unreleased/security-xss-grafana-url-12-4.yml
@@ -0,0 +1,5 @@
+---
+title: Fix stored XSS issue for grafana_url
+merge_request:
+author:
+type: security
diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb
index cc937cbb3cf..50192d97cf7 100644
--- a/lib/gitlab/url_blocker.rb
+++ b/lib/gitlab/url_blocker.rb
@@ -11,11 +11,14 @@ module Gitlab
       # Validates the given url according to the constraints specified by arguments.
       #
       # ports - Raises error if the given URL port does is not between given ports.
-      # allow_localhost - Raises error if URL resolves to a localhost IP address and argument is true.
-      # allow_local_network - Raises error if URL resolves to a link-local address and argument is true.
+      # allow_localhost - Raises error if URL resolves to a localhost IP address and argument is false.
+      # allow_local_network - Raises error if URL resolves to a link-local address and argument is false.
       # ascii_only - Raises error if URL has unicode characters and argument is true.
       # enforce_user - Raises error if URL user doesn't start with alphanumeric characters and argument is true.
       # enforce_sanitization - Raises error if URL includes any HTML/CSS/JS tags and argument is true.
+      # require_absolute - Raises error if URL is not absolute and argument is true.
+      #                    Allow relative URLs beginning with slash when argument is false
+      #                    Raises error if relative URL does not begin with slash and argument is false
       #
       # Returns an array with [<uri>, <original-hostname>].
       # rubocop:disable Metrics/ParameterLists
@@ -28,7 +31,8 @@ module Gitlab
         ascii_only: false,
         enforce_user: false,
         enforce_sanitization: false,
-        dns_rebind_protection: true)
+        dns_rebind_protection: true,
+        require_absolute: true)
         # rubocop:enable Metrics/ParameterLists
 
         return [nil, nil] if url.nil?
@@ -42,13 +46,14 @@ module Gitlab
           ports: ports,
           enforce_sanitization: enforce_sanitization,
           enforce_user: enforce_user,
-          ascii_only: ascii_only
+          ascii_only: ascii_only,
+          require_absolute: require_absolute
         )
 
         hostname = uri.hostname
         port = get_port(uri)
 
-        address_info = get_address_info(hostname, port)
+        address_info = get_address_info(hostname, port) if require_absolute || uri.absolute?
         return [uri, nil] unless address_info
 
         protected_uri_with_hostname = enforce_uri_hostname(address_info, uri, hostname, dns_rebind_protection)
@@ -94,12 +99,14 @@ module Gitlab
         [uri, hostname]
       end
 
-      def validate_uri(uri:, schemes:, ports:, enforce_sanitization:, enforce_user:, ascii_only:)
+      def validate_uri(uri:, schemes:, ports:, enforce_sanitization:, enforce_user:, ascii_only:, require_absolute:)
         validate_html_tags(uri) if enforce_sanitization
+        validate_absolute(uri) if require_absolute
+        validate_relative(uri) unless require_absolute
 
         return if internal?(uri)
 
-        validate_scheme(uri.scheme, schemes)
+        validate_scheme(uri.scheme, schemes, require_absolute)
         validate_port(get_port(uri), ports) if ports.any?
         validate_user(uri.user) if enforce_user
         validate_hostname(uri.hostname)
@@ -169,8 +176,20 @@ module Gitlab
         raise BlockedUrlError, "Only allowed ports are #{ports.join(', ')}, and any over 1024"
       end
 
-      def validate_scheme(scheme, schemes)
-        if scheme.blank? || (schemes.any? && !schemes.include?(scheme))
+      def validate_absolute(uri)
+        return if uri.absolute?
+
+        raise BlockedUrlError, 'must be absolute'
+      end
+
+      def validate_relative(uri)
+        return if uri.absolute? || uri.path.starts_with?('/')
+
+        raise BlockedUrlError, 'relative path must begin with a / (slash)'
+      end
+
+      def validate_scheme(scheme, schemes, require_absolute)
+        if (require_absolute && scheme.blank?) || (schemes.any? && !schemes.include?(scheme))
           raise BlockedUrlError, "Only allowed schemes are #{schemes.join(', ')}"
         end
       end
@@ -229,9 +248,10 @@ module Gitlab
       end
 
       def internal_web?(uri)
-        uri.scheme == config.gitlab.protocol &&
-          uri.hostname == config.gitlab.host &&
-          (uri.port.blank? || uri.port == config.gitlab.port)
+        (uri.scheme.blank? && uri.hostname.blank? && uri.port.blank? && uri.path.starts_with?('/')) ||
+        (uri.scheme == config.gitlab.protocol &&
+         uri.hostname == config.gitlab.host &&
+         (uri.port.blank? || uri.port == config.gitlab.port))
       end
 
       def internal_shell?(uri)
diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb
index 0fab890978a..0574f4a8e50 100644
--- a/spec/lib/gitlab/url_blocker_spec.rb
+++ b/spec/lib/gitlab/url_blocker_spec.rb
@@ -2,7 +2,18 @@
 require 'spec_helper'
 
 describe Gitlab::UrlBlocker do
+  include StubRequests
+
   describe '#validate!' do
+    shared_examples 'validates URI and hostname' do
+      it 'runs the url validations' do
+        uri, hostname = subject
+
+        expect(uri).to eq(Addressable::URI.parse(expected_uri))
+        expect(hostname).to eq(expected_hostname)
+      end
+    end
+
     context 'when URI is nil' do
       let(:import_url) { nil }
 
@@ -34,6 +45,14 @@ describe Gitlab::UrlBlocker do
         expect(uri).to eq(Addressable::URI.parse('https://93.184.216.34'))
         expect(hostname).to eq('example.org')
       end
+
+      context 'when scheme is missing' do
+        let(:import_url) { '//example.org/path' }
+
+        it 'raises and error' do
+          expect { described_class.validate!(import_url) }.to raise_error(described_class::BlockedUrlError)
+        end
+      end
     end
 
     context 'when the URL hostname is an IP address' do
@@ -47,6 +66,32 @@ describe Gitlab::UrlBlocker do
       end
     end
 
+    context 'disabled require_absolute' do
+      subject { described_class.validate!(import_url, require_absolute: false) }
+
+      context 'with scheme and hostname' do
+        let(:import_url) { 'https://example.org/path' }
+
+        before do
+          stub_dns(import_url, ip_address: '93.184.216.34')
+        end
+
+        it_behaves_like 'validates URI and hostname' do
+          let(:expected_uri) { 'https://93.184.216.34/path' }
+          let(:expected_hostname) { 'example.org' }
+        end
+      end
+
+      context 'without scheme' do
+        let(:import_url) { '//93.184.216.34/path' }
+
+        it_behaves_like 'validates URI and hostname' do
+          let(:expected_uri) { import_url }
+          let(:expected_hostname) { nil }
+        end
+      end
+    end
+
     context 'disabled DNS rebinding protection' do
       context 'when URI is internal' do
         let(:import_url) { 'http://localhost' }
@@ -366,6 +411,21 @@ describe Gitlab::UrlBlocker do
       end
     end
 
+    context 'when require_absolute is false' do
+      it 'allows paths' do
+        expect(described_class.blocked_url?('/foo/foo.bar', require_absolute: false)).to be false
+      end
+
+      it 'allows absolute urls without paths' do
+        expect(described_class.blocked_url?('http://example.com', require_absolute: false)).to be false
+      end
+
+      it 'paths must begin with a slash' do
+        expect(described_class.blocked_url?('foo/foo.bar', require_absolute: false)).to be true
+        expect(described_class.blocked_url?('', require_absolute: false)).to be true
+      end
+    end
+
     it 'blocks urls with invalid ip address' do
       stub_env('RSPEC_ALLOW_INVALID_URLS', 'false')
 
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index 904185f4140..33c073a65a9 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -17,6 +17,7 @@ describe ApplicationSetting do
     let(:http)  { 'http://example.com' }
     let(:https) { 'https://example.com' }
     let(:ftp)   { 'ftp://example.com' }
+    let(:javascript) { 'javascript:alert(window.opener.document.location)' }
 
     it { is_expected.to allow_value(nil).for(:home_page_url) }
     it { is_expected.to allow_value(http).for(:home_page_url) }
@@ -37,6 +38,11 @@ describe ApplicationSetting do
     it { is_expected.not_to allow_value("myemail@example.com").for(:lets_encrypt_notification_email) }
     it { is_expected.to allow_value("myemail@test.example.com").for(:lets_encrypt_notification_email) }
 
+    it { is_expected.to allow_value(http).for(:grafana_url) }
+    it { is_expected.to allow_value(https).for(:grafana_url) }
+    it { is_expected.not_to allow_value(ftp).for(:grafana_url) }
+    it { is_expected.not_to allow_value(javascript).for(:grafana_url) }
+
     context "when user accepted let's encrypt terms of service" do
       before do
         setting.update(lets_encrypt_terms_of_service_accepted: true)
diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb
index b5e45f99109..f79b37bf07d 100644
--- a/spec/requests/api/commit_statuses_spec.rb
+++ b/spec/requests/api/commit_statuses_spec.rb
@@ -306,7 +306,7 @@ describe API::CommitStatuses do
         it 'responds with bad request status and validation errors' do
           expect(response).to have_gitlab_http_status(400)
           expect(json_response['message']['target_url'])
-            .to include 'is blocked: Only allowed schemes are http, https'
+            .to include 'is blocked: must be absolute'
         end
       end
 
diff --git a/spec/validators/addressable_url_validator_spec.rb b/spec/validators/addressable_url_validator_spec.rb
index 387e84b2d04..d9e21585629 100644
--- a/spec/validators/addressable_url_validator_spec.rb
+++ b/spec/validators/addressable_url_validator_spec.rb
@@ -312,4 +312,67 @@ describe AddressableUrlValidator do
       end
     end
   end
+
+  context 'when require_absolute is' do
+    let(:validator) { described_class.new(attributes: [:link_url], require_absolute: require_absolute) }
+    let(:valid_relative_url) { '/relative/path' }
+    let(:invalid_relative_url) { 'relative/path' }
+    let(:absolute_url) { 'https://example.com' }
+
+    context 'true' do
+      let(:require_absolute) { true }
+
+      it 'prevents valid relative urls' do
+        badge.link_url = valid_relative_url
+
+        subject
+
+        expect(badge.errors).to be_present
+      end
+
+      it 'prevents invalid relative urls' do
+        badge.link_url = invalid_relative_url
+
+        subject
+
+        expect(badge.errors).to be_present
+      end
+
+      it 'allows absolute urls' do
+        badge.link_url = absolute_url
+
+        subject
+
+        expect(badge.errors).to be_empty
+      end
+    end
+
+    context 'false' do
+      let(:require_absolute) { false }
+
+      it 'allows valid relative urls' do
+        badge.link_url = valid_relative_url
+
+        subject
+
+        expect(badge.errors).to be_empty
+      end
+
+      it 'prevents invalid relative urls' do
+        badge.link_url = invalid_relative_url
+
+        subject
+
+        expect(badge.errors).to be_present
+      end
+
+      it 'allows absolute urls' do
+        badge.link_url = absolute_url
+
+        subject
+
+        expect(badge.errors).to be_empty
+      end
+    end
+  end
 end