Add latest changes from gitlab-org/security/gitlab@12-8-stable-ee

app/models/application_setting.rb

@@ -6,6 +6,9 @@ class ApplicationSetting < ApplicationRecord
   include TokenAuthenticatable
   include ChronicDurationAttribute
+  GRAFANA_URL_ERROR_MESSAGE = 'Please check your Grafana URL setting in ' \
+    'Admin Area > Settings > Metrics and profiling > Metrics - Grafana'
+
   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
@@ -38,6 +41,14 @@ class ApplicationSetting < ApplicationRecord
   chronic_duration_attr_writer :archive_builds_in_human_readable, :archive_builds_in_seconds
+  validates :grafana_url,
+            system_hook_url: {
+              blocked_message: "is blocked: %{exception_message}. " + GRAFANA_URL_ERROR_MESSAGE
+            },
+            if: :grafana_url_absolute?
+
+  validate :validate_grafana_url
+
   validates :uuid, presence: true
   validates :outbound_local_requests_whitelist,
@@ -357,6 +368,19 @@ class ApplicationSetting < ApplicationRecord
   end
   after_commit :expire_performance_bar_allowed_user_ids_cache, if: -> { previous_changes.key?('performance_bar_allowed_group_id') }
+  def validate_grafana_url
+    unless parsed_grafana_url
+      self.errors.add(
+        :grafana_url,
+        "must be a valid relative or absolute URL. #{GRAFANA_URL_ERROR_MESSAGE}"
+      )
+    end
+  end
+
+  def grafana_url_absolute?
+    parsed_grafana_url&.absolute?
+  end
+
   def sourcegraph_url_is_com?
     !!(sourcegraph_url =~ /\Ahttps:\/\/(www\.)?sourcegraph\.com/)
   end
@@ -381,6 +405,12 @@ class ApplicationSetting < ApplicationRecord
   def recaptcha_or_login_protection_enabled
     recaptcha_enabled || login_recaptcha_protection_enabled
   end
+
+  private
+
+  def parsed_grafana_url
+    @parsed_grafana_url ||= Gitlab::Utils.parse_url(grafana_url)
+  end
 end
 ApplicationSetting.prepend_if_ee('EE::ApplicationSetting')

app/models/badge.rb

@@ -32,7 +32,9 @@ class Badge < ApplicationRecord
   end
   def rendered_image_url(project = nil)
-    build_rendered_url(image_url, project)
+    Gitlab::AssetProxy.proxy_url(
+      build_rendered_url(image_url, project)
+    )
   end
   private

app/validators/addressable_url_validator.rb

@@ -23,7 +23,8 @@
 # protect against Server-side Request Forgery (SSRF), or check for the right port.
 #
 # Configuration options:
-# * <tt>message</tt> - A custom error message (default is: "must be a valid URL").
+# * <tt>message</tt> - A custom error message, used when the URL is blank. (default is: "must be a valid URL").
+# * <tt>blocked_message</tt> - A custom error message, used when the URL is blocked. Default: +'is blocked: %{exception_message}'+.
 # * <tt>schemes</tt> - Array of URI schemes. Default: +['http', 'https']+
 # * <tt>allow_localhost</tt> - Allow urls pointing to +localhost+. Default: +true+
 # * <tt>allow_local_network</tt> - Allow urls pointing to private network addresses. Default: +true+
@@ -59,7 +60,8 @@ class AddressableUrlValidator < ActiveModel::EachValidator
   }.freeze
   DEFAULT_OPTIONS = BLOCKER_VALIDATE_OPTIONS.merge({
-    message: 'must be a valid URL'
+    message: 'must be a valid URL',
+    blocked_message: 'is blocked: %{exception_message}'
   }).freeze
   def initialize(options)
@@ -80,7 +82,7 @@ class AddressableUrlValidator < ActiveModel::EachValidator
     Gitlab::UrlBlocker.validate!(value, blocker_args)
   rescue Gitlab::UrlBlocker::BlockedUrlError => e
-    record.errors.add(attribute, "is blocked: #{e.message}")
+    record.errors.add(attribute, options.fetch(:blocked_message) % { exception_message: e.message })
   end
   private

app/views/admin/application_settings/_grafana.html.haml

-= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-grafana-settings'), html: { class: 'fieldset-form' } do |f|
+= form_for @application_setting, url: metrics_and_profiling_admin_application_settings_path(anchor: 'js-grafana-settings'), html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
   %fieldset

app/views/layouts/nav/sidebar/_admin.html.haml

@@ -83,7 +83,7 @@
                 = _('Requests Profiles')
           - if Gitlab::CurrentSettings.current_application_settings.grafana_enabled?
             = nav_link do
-              = link_to Gitlab::CurrentSettings.current_application_settings.grafana_url, target: '_blank', title: _('Metrics Dashboard') do
+              = link_to Gitlab::CurrentSettings.current_application_settings.grafana_url, target: '_blank', title: _('Metrics Dashboard'), rel: 'noopener noreferrer' do
                 %span
                   = _('Metrics Dashboard')
           = render_if_exists 'layouts/nav/ee/admin/new_monitoring_sidebar'

changelogs/unreleased/security-badge-camo.yml

+---
+title: Run project badge images through the asset proxy
+merge_request:
+author:
+type: security

changelogs/unreleased/security-expire-confirmation-token.yml

+---
+title: Expire account confirmation token
+merge_request:
+author:
+type: security

changelogs/unreleased/security-grafana-stored-xss.yml

+---
+title: Prevent XSS in admin grafana URL setting
+merge_request:
+author:
+type: security

config/initializers/8_devise.rb

@@ -80,8 +80,16 @@ Devise.setup do |config|
   # When allow_unconfirmed_access_for is zero, the user won't be able to sign in without confirming.
   # You can use this to let your user access some features of your application
   # without confirming the account, but blocking it after a certain period
-  # (ie 2 days).
-  config.allow_unconfirmed_access_for = 30.days
+  # (e.g. 3 days).
+  config.allow_unconfirmed_access_for = 3.days
+
+  # A period that the user is allowed to confirm their account before their
+  # token becomes invalid. For example, if set to 1.day, the user can confirm
+  # their account within 1 days after the mail was sent, but on the second day
+  # their account can't be confirmed with the token any more.
+  # Default is nil, meaning there is no restriction on how long a user can take
+  # before confirming their account.
+  config.confirm_within = 1.day
   # Defines which key will be used when confirming an account
   # config.confirmation_keys = [ :email ]

db/migrate/20200214085940_clean_grafana_url.rb

+# frozen_string_literal: true
+
+class CleanGrafanaUrl < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def up
+    execute(
+      <<-SQL
+      UPDATE
+        application_settings
+      SET
+        grafana_url = default
+      WHERE
+        position('javascript:' IN btrim(application_settings.grafana_url)) = 1
+      SQL
+    )
+  end
+
+  def down
+    # no-op
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema.define(version: 2020_02_13_220211) do
+ActiveRecord::Schema.define(version: 2020_02_14_085940) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"

doc/administration/monitoring/performance/grafana_configuration.md

@@ -117,8 +117,9 @@ If you have set up Grafana, you can enable a link to access it easily from the s
 1. Expand **Metrics - Grafana**.
 1. Check the "Enable access to Grafana" checkbox.
 1. If Grafana is enabled through Omnibus GitLab and on the same server,
-   leave "Grafana URL" unchanged. In any other case, enter the full URL
-   path of the Grafana instance.
+   leave **Grafana URL** unchanged. It should be `/-/grafana`.
+
+   In any other case, enter the full URL of the Grafana instance.
 1. Click **Save changes**.
 1. The new link will be available in the **Admin Area > Monitoring > Metrics Dashboard**.

lib/gitlab/asset_proxy.rb

+# frozen_string_literal: true
+
+# This is based on https://github.com/jch/html-pipeline/blob/v2.12.2/lib/html/pipeline/camo_filter.rb
+# and Banzai::Filter::AssetProxyFilter which we use to proxy images in Markdown
+
+module Gitlab
+  module AssetProxy
+    class << self
+      def proxy_url(url)
+        return url unless Gitlab.config.asset_proxy.enabled
+        return url if asset_host_whitelisted?(url)
+
+        "#{Gitlab.config.asset_proxy.url}/#{asset_url_hash(url)}/#{hexencode(url)}"
+      end
+
+      private
+
+      def asset_host_whitelisted?(url)
+        parsed_url = URI.parse(url)
+
+        Gitlab.config.asset_proxy.domain_regexp&.match?(parsed_url.host)
+      end
+
+      def asset_url_hash(url)
+        OpenSSL::HMAC.hexdigest('sha1', Gitlab.config.asset_proxy.secret_key, url)
+      end
+
+      def hexencode(str)
+        str.unpack1('H*')
+      end
+    end
+  end
+end

lib/gitlab/utils.rb

@@ -136,5 +136,14 @@ module Gitlab
       IPAddr.new(str)
     rescue IPAddr::InvalidAddressError
     end
+
+    # Converts a string to an Addressable::URI object.
+    # If the string is not a valid URI, it returns nil.
+    # Param uri_string should be a String object.
+    # This method returns an Addressable::URI object or nil.
+    def parse_url(uri_string)
+      Addressable::URI.parse(uri_string)
+    rescue Addressable::URI::InvalidURIError, TypeError
+    end
   end
 end

spec/lib/gitlab/asset_proxy_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::AssetProxy do
+  context 'when asset proxy is disabled' do
+    before do
+      stub_asset_proxy_setting(enabled: false)
+    end
+
+    it 'returns the original URL' do
+      url = 'http://example.com/test.png'
+
+      expect(described_class.proxy_url(url)).to eq(url)
+    end
+  end
+
+  context 'when asset proxy is enabled' do
+    before do
+      stub_asset_proxy_setting(whitelist: %w(gitlab.com *.mydomain.com))
+      stub_asset_proxy_setting(
+        enabled: true,
+        url: 'https://assets.example.com',
+        secret_key: 'shared-secret',
+        domain_regexp: Banzai::Filter::AssetProxyFilter.compile_whitelist(Gitlab.config.asset_proxy.whitelist)
+      )
+    end
+
+    it 'returns a proxied URL' do
+      url = 'http://example.com/test.png'
+      proxied_url = 'https://assets.example.com/08df250eeeef1a8cf2c761475ac74c5065105612/687474703a2f2f6578616d706c652e636f6d2f746573742e706e67'
+
+      expect(described_class.proxy_url(url)).to eq(proxied_url)
+    end
+
+    context 'whitelisted domain' do
+      it 'returns original URL for single domain whitelist' do
+        url = 'http://gitlab.com/test.png'
+
+        expect(described_class.proxy_url(url)).to eq(url)
+      end
+
+      it 'returns original URL for wildcard subdomain whitelist' do
+        url = 'http://test.mydomain.com/test.png'
+
+        expect(described_class.proxy_url(url)).to eq(url)
+      end
+    end
+  end
+end

spec/lib/gitlab/utils_spec.rb

@@ -283,4 +283,18 @@ describe Gitlab::Utils do
       expect(described_class.string_to_ip_object('1:0:0:0:0:0:0:0/124')).to eq(IPAddr.new('1:0:0:0:0:0:0:0/124'))
     end
   end
+
+  describe '.parse_url' do
+    it 'returns Addressable::URI object' do
+      expect(described_class.parse_url('http://gitlab.com')).to be_instance_of(Addressable::URI)
+    end
+
+    it 'returns nil when URI cannot be parsed' do
+      expect(described_class.parse_url('://gitlab.com')).to be nil
+    end
+
+    it 'returns nil with invalid parameter' do
+      expect(described_class.parse_url(1)).to be nil
+    end
+  end
 end

spec/migrations/clean_grafana_url_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20200214085940_clean_grafana_url.rb')
+
+describe CleanGrafanaUrl, :migration do
+  let(:application_settings_table) { table(:application_settings) }
+
+  [
+    'javascript:alert(window.opener.document.location)',
+    '  javascript:alert(window.opener.document.location)'
+  ].each do |grafana_url|
+    it "sets grafana_url back to its default value when grafana_url is '#{grafana_url}'" do
+      application_settings = application_settings_table.create!(grafana_url: grafana_url)
+
+      migrate!
+
+      expect(application_settings.reload.grafana_url).to eq('/-/grafana')
+    end
+  end
+
+  ['/-/grafana', '/some/relative/url', 'http://localhost:9000'].each do |grafana_url|
+    it "does not modify grafana_url when grafana_url is '#{grafana_url}'" do
+      application_settings = application_settings_table.create!(grafana_url: grafana_url)
+
+      migrate!
+
+      expect(application_settings.reload.grafana_url).to eq(grafana_url)
+    end
+  end
+
+  context 'when application_settings table has no rows' do
+    it 'does not fail' do
+      migrate!
+    end
+  end
+end

spec/models/application_setting_spec.rb

@@ -19,6 +19,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) }
@@ -81,6 +82,53 @@ describe ApplicationSetting do
     it { is_expected.not_to allow_value('abc').for(:minimum_password_length) }
     it { is_expected.to allow_value(10).for(:minimum_password_length) }
+    context 'grafana_url validations' do
+      before do
+        subject.instance_variable_set(:@parsed_grafana_url, nil)
+      end
+
+      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) }
+      it { is_expected.to allow_value('/-/grafana').for(:grafana_url) }
+      it { is_expected.to allow_value('http://localhost:9000').for(:grafana_url) }
+
+      context 'when local URLs are not allowed in system hooks' do
+        before do
+          stub_application_setting(allow_local_requests_from_system_hooks: false)
+        end
+
+        it { is_expected.not_to allow_value('http://localhost:9000').for(:grafana_url) }
+      end
+
+      context 'with invalid grafana URL' do
+        it 'adds an error' do
+          subject.grafana_url = ' ' + http
+          expect(subject.save).to be false
+
+          expect(subject.errors[:grafana_url]).to eq([
+            'must be a valid relative or absolute URL. ' \
+            'Please check your Grafana URL setting in ' \
+            'Admin Area > Settings > Metrics and profiling > Metrics - Grafana'
+          ])
+        end
+      end
+
+      context 'with blocked grafana URL' do
+        it 'adds an error' do
+          subject.grafana_url = javascript
+          expect(subject.save).to be false
+
+          expect(subject.errors[:grafana_url]).to eq([
+            'is blocked: Only allowed schemes are http, https. Please check your ' \
+            'Grafana URL setting in ' \
+            'Admin Area > Settings > Metrics and profiling > Metrics - Grafana'
+          ])
+        end
+      end
+    end
+
     context 'when snowplow is enabled' do
       before do
         setting.snowplow_enabled = true

spec/models/badge_spec.rb

@@ -91,6 +91,22 @@ describe Badge do
       let(:method) { :image_url }
       it_behaves_like 'rendered_links'
+
+      context 'when asset proxy is enabled' do
+        let(:placeholder_url) { 'http://www.example.com/image' }
+
+        before do
+          stub_asset_proxy_setting(
+            enabled: true,
+            url: 'https://assets.example.com',
+            secret_key: 'shared-secret'
+          )
+        end
+
+        it 'returns a proxied URL' do
+          expect(badge.rendered_image_url).to start_with('https://assets.example.com')
+        end
+      end
     end
   end
 end

spec/validators/addressable_url_validator_spec.rb

@@ -5,6 +5,9 @@ require 'spec_helper'
 describe AddressableUrlValidator do
   let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
+  let(:validator) { described_class.new(validator_options.reverse_merge(attributes: [:link_url])) }
+  let(:validator_options) { {} }
+
   subject { validator.validate(badge) }
   include_examples 'url validator examples', described_class::DEFAULT_OPTIONS[:schemes]
@@ -114,6 +117,19 @@ describe AddressableUrlValidator do
     end
   end
+  context 'when blocked_message is set' do
+    let(:message) { 'is not allowed due to: %{exception_message}' }
+    let(:validator_options) { { blocked_message: message } }
+
+    it 'blocks url with provided error message' do
+      badge.link_url = 'javascript:alert(window.opener.document.location)'
+
+      subject
+
+      expect(badge.errors.first[1]).to eq 'is not allowed due to: Only allowed schemes are http, https'
+    end
+  end
+
   context 'when allow_nil is set to true' do
     let(:validator) { described_class.new(attributes: [:link_url], allow_nil: true) }