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

Handle Stored XSS for Grafana URL in settings See merge request gitlab/gitlabhq!3483

Merge branch 'security-xss-grafana-url-12-1' into '12-1-stable'
Handle Stored XSS for Grafana URL in settings See merge request gitlab/gitlabhq!3483
27e6daa2 · GitLab Release Tools Bot · 3e2d0afa · 9a0dc3fa · 27e6daa2 · 27e6daa2
--- 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 @@ def self.create_from_defaults
    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

--- 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({

--- a/changelogs/unreleased/security-xss-grafana-url-12-4.yml
+++ b/changelogs/unreleased/security-xss-grafana-url-12-4.yml
+---
+title: Fix stored XSS issue for grafana_url
+merge_request:
+author:
+type: security
--- a/lib/gitlab/url_blocker.rb
+++ b/lib/gitlab/url_blocker.rb
@@ -11,11 +11,14 @@ class << self
      # 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 @@ def validate!(
        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 @@ def validate!(
          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 @@ def enforce_uri_hostname(addrs_info, uri, hostname, dns_rebind_protection)
        [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 @@ def validate_port(port, ports)
        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 @@ def internal?(uri)
      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)

--- 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 @@
        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 @@
      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 @@ def unstub_domain_resolv
      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')


--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -17,6 +17,7 @@
    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 @@
    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)

--- a/spec/requests/api/commit_statuses_spec.rb
+++ b/spec/requests/api/commit_statuses_spec.rb
@@ -306,7 +306,7 @@ def create_status(commit, opts = {})
        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


--- a/spec/validators/addressable_url_validator_spec.rb
+++ b/spec/validators/addressable_url_validator_spec.rb
@@ -312,4 +312,67 @@
      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