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

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

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

changelogs/unreleased/security-xss-grafana-url-12-4.yml

+---
+title: Fix stored XSS issue for grafana_url
+merge_request:
+author:
+type: security

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)

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')
 

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)

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
 

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