Merge branch 'security-xss-grafana-url-12-3' into '12-3-stable'

Handle Stored XSS for Grafana URL in settings

See merge request gitlab/gitlabhq!3482

8 files changed, 186 insertions, 15 deletions

diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 92526def144..a14445511a7 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -6,6 +6,13 @@ class ApplicationSetting < ApplicationRecord
   include TokenAuthenticatable
   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
   add_authentication_token_field :static_objects_external_storage_auth_token
@@ -48,6 +55,11 @@ class ApplicationSetting < ApplicationRecord
             allow_nil: false,
             qualified_domain_array: 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
@@ -303,6 +314,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 300bd01ed22..179abde17ff 100644
--- a/app/validators/addressable_url_validator.rb
+++ b/app/validators/addressable_url_validator.rb
@@ -55,7 +55,8 @@ class AddressableUrlValidator < ActiveModel::EachValidator
     ascii_only: false,
     enforce_user: false,
     enforce_sanitization: false,
-    dns_rebind_protection: false
+    dns_rebind_protection: 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 4285b2675c5..94cef820f0d 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,10 +46,11 @@ module Gitlab
           ports: ports,
           enforce_sanitization: enforce_sanitization,
           enforce_user: enforce_user,
-          ascii_only: ascii_only
+          ascii_only: ascii_only,
+          require_absolute: require_absolute
         )
 
-        address_info = get_address_info(uri, dns_rebind_protection)
+        address_info = get_address_info(uri, dns_rebind_protection) if require_absolute || uri.absolute?
         return [uri, nil] unless address_info
 
         ip_address = ip_address(address_info)
@@ -95,12 +100,14 @@ module Gitlab
         address_info.first&.ip_address
       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)
@@ -177,8 +184,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
@@ -237,9 +256,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 0e66e959b24..2bdc70205b9 100644
--- a/spec/lib/gitlab/url_blocker_spec.rb
+++ b/spec/lib/gitlab/url_blocker_spec.rb
@@ -62,6 +62,22 @@ describe Gitlab::UrlBlocker do
           expect { subject }.to raise_error(described_class::BlockedUrlError)
         end
       end
+
+      context 'when domain is too long' do
+        let(:import_url) { 'https://example' + 'a' * 1024 + '.com' }
+
+        it 'raises an error' do
+          expect { subject }.to raise_error(ArgumentError)
+        end
+      end
+
+      context 'when scheme is missing' do
+        let(:import_url) { '//example.org/path' }
+
+        it 'raises and error' do
+          expect { subject }.to raise_error(described_class::BlockedUrlError)
+        end
+      end
     end
 
     context 'when the URL hostname is an IP address' do
@@ -83,6 +99,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
       subject { described_class.validate!(import_url, dns_rebind_protection: false) }
 
@@ -608,6 +650,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 d12f9b9100a..855ce9cb4f9 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) }
@@ -52,6 +53,11 @@ describe ApplicationSetting do
     it { is_expected.to allow_value(http).for(:static_objects_external_storage_url) }
     it { is_expected.to allow_value(https).for(:static_objects_external_storage_url) }
 
+    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 1be8883bd3c..deeab82a80d 100644
--- a/spec/requests/api/commit_statuses_spec.rb
+++ b/spec/requests/api/commit_statuses_spec.rb
@@ -322,7 +322,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 6927a1f67a1..f57e8f3cfc5 100644
--- a/spec/validators/addressable_url_validator_spec.rb
+++ b/spec/validators/addressable_url_validator_spec.rb
@@ -349,4 +349,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