diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index ddd43311d9b14f25a3089c11e712fb30c06f6ea9..2f8f6f6b420e42601e63c658ccd0ad969be127a7 100644 --- a/app/models/application_setting.rb +++ b/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') diff --git a/app/models/badge.rb b/app/models/badge.rb index eb351425e66c754b8d8d26a47b83860ace58a726..3400d6d407d8339bab4c4e4e456a75824f740250 100644 --- a/app/models/badge.rb +++ b/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 diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb index 300bd01ed229bf05dc03e34a3f21a54c60d1e36a..99f503c3f0662c0f28ec5a3bab7687a742c6f232 100644 --- a/app/validators/addressable_url_validator.rb +++ b/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: -# * message - A custom error message (default is: "must be a valid URL"). +# * message - A custom error message, used when the URL is blank. (default is: "must be a valid URL"). +# * blocked_message - A custom error message, used when the URL is blocked. Default: +'is blocked: %{exception_message}'+. # * schemes - Array of URI schemes. Default: +['http', 'https']+ # * allow_localhost - Allow urls pointing to +localhost+. Default: +true+ # * allow_local_network - 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 diff --git a/app/views/admin/application_settings/_grafana.html.haml b/app/views/admin/application_settings/_grafana.html.haml index b6e02bde8952fc66cb76a47e4eeb5f680a177a62..700be7db54fed118832379e40e89e82c0d7b9088 100644 --- a/app/views/admin/application_settings/_grafana.html.haml +++ b/app/views/admin/application_settings/_grafana.html.haml @@ -1,4 +1,4 @@ -= 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 diff --git a/app/views/layouts/nav/sidebar/_admin.html.haml b/app/views/layouts/nav/sidebar/_admin.html.haml index 9f70124ba0dc42318e06e0cb32cfabe824a6ccab..78cd3f62decbb4f8e5e2edbfa79a517b49b93b99 100644 --- a/app/views/layouts/nav/sidebar/_admin.html.haml +++ b/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' diff --git a/changelogs/unreleased/security-badge-camo.yml b/changelogs/unreleased/security-badge-camo.yml new file mode 100644 index 0000000000000000000000000000000000000000..b882bffdcaa6583b0af819d42be87bb318ee00d8 --- /dev/null +++ b/changelogs/unreleased/security-badge-camo.yml @@ -0,0 +1,5 @@ +--- +title: Run project badge images through the asset proxy +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-expire-confirmation-token.yml b/changelogs/unreleased/security-expire-confirmation-token.yml new file mode 100644 index 0000000000000000000000000000000000000000..40d8063c40942d89f4e4ecfe95b04428d876d274 --- /dev/null +++ b/changelogs/unreleased/security-expire-confirmation-token.yml @@ -0,0 +1,5 @@ +--- +title: Expire account confirmation token +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-grafana-stored-xss.yml b/changelogs/unreleased/security-grafana-stored-xss.yml new file mode 100644 index 0000000000000000000000000000000000000000..5a98c6fd7ff87bc4d1524e6ec8a7e264490693d2 --- /dev/null +++ b/changelogs/unreleased/security-grafana-stored-xss.yml @@ -0,0 +1,5 @@ +--- +title: Prevent XSS in admin grafana URL setting +merge_request: +author: +type: security diff --git a/config/initializers/8_devise.rb b/config/initializers/8_devise.rb index e1c37caaafdc8371b38d99607719fc9ebc51c04a..6ed56598e15728b400925e15853ac081a0e47c80 100644 --- a/config/initializers/8_devise.rb +++ b/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 ] diff --git a/db/migrate/20200214085940_clean_grafana_url.rb b/db/migrate/20200214085940_clean_grafana_url.rb new file mode 100644 index 0000000000000000000000000000000000000000..7dff653251622f94554ab55fbf07dbbe780ea792 --- /dev/null +++ b/db/migrate/20200214085940_clean_grafana_url.rb @@ -0,0 +1,22 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index 435d994e2015c87b17dbc1a4c1debeb9f7d82c43..517177e82b93cb5d1601908b728b306769fba0b8 100644 --- a/db/schema.rb +++ b/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" diff --git a/doc/administration/monitoring/performance/grafana_configuration.md b/doc/administration/monitoring/performance/grafana_configuration.md index 2fdeeae302be33438d16e759e0227fac3777d40d..6d0fc8ad6d4e5044d009374be6d211b9e9deab69 100644 --- a/doc/administration/monitoring/performance/grafana_configuration.md +++ b/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**. diff --git a/lib/gitlab/asset_proxy.rb b/lib/gitlab/asset_proxy.rb new file mode 100644 index 0000000000000000000000000000000000000000..fd7c58ba68f3c422c5de73d6808715aeb27a2ac4 --- /dev/null +++ b/lib/gitlab/asset_proxy.rb @@ -0,0 +1,33 @@ +# 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 diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb index 7eddfc471f6a7888ff5ef5158424783c6ccb2671..3c567fad68de09ea19c7d3bf67376b7ad12a31e5 100644 --- a/lib/gitlab/utils.rb +++ b/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 diff --git a/spec/lib/gitlab/asset_proxy_spec.rb b/spec/lib/gitlab/asset_proxy_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..f5aa1819982665ce6024f9311b79ef5ade7e194b --- /dev/null +++ b/spec/lib/gitlab/asset_proxy_spec.rb @@ -0,0 +1,50 @@ +# 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 diff --git a/spec/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb index 85a536ee6ad0b566d2132f349f18a2d9b3900504..6841e7719dc7c8523cb234fb6e0a067f803743d0 100644 --- a/spec/lib/gitlab/utils_spec.rb +++ b/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 diff --git a/spec/migrations/clean_grafana_url_spec.rb b/spec/migrations/clean_grafana_url_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..9f060fbaf7d8aca1513b3056dddc6174ab033991 --- /dev/null +++ b/spec/migrations/clean_grafana_url_spec.rb @@ -0,0 +1,37 @@ +# 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 diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index bbd50f1c0ef2734585ed694c5b4fa07e7e3f79a7..abbaa22ff3e6d8c5128e7f348134db43bce17cec 100644 --- a/spec/models/application_setting_spec.rb +++ b/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 diff --git a/spec/models/badge_spec.rb b/spec/models/badge_spec.rb index 60ae579eb03fdd118e777d991a1d6b30d2c3d0c1..fba8f40e99bc0dda20fadafb45ebfc2dd60e5ecf 100644 --- a/spec/models/badge_spec.rb +++ b/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 diff --git a/spec/validators/addressable_url_validator_spec.rb b/spec/validators/addressable_url_validator_spec.rb index e8a44f7a12ab24501e8e2b2683660b1dfaf00d02..46b1bebb0744fcd0c88607e708db870508d9df4d 100644 --- a/spec/validators/addressable_url_validator_spec.rb +++ b/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) } diff --git a/vendor/gitignore/C++.gitignore b/vendor/gitignore/C++.gitignore old mode 100644 new mode 100755 diff --git a/vendor/gitignore/Java.gitignore b/vendor/gitignore/Java.gitignore old mode 100644 new mode 100755