diff options
22 files changed, 314 insertions, 11 deletions
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index ddd43311d9b..2f8f6f6b420 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 eb351425e66..3400d6d407d 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 300bd01ed22..99f503c3f06 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: -# * <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 diff --git a/app/views/admin/application_settings/_grafana.html.haml b/app/views/admin/application_settings/_grafana.html.haml index b6e02bde895..700be7db54f 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 9f70124ba0d..78cd3f62dec 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 00000000000..b882bffdcaa --- /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 00000000000..40d8063c409 --- /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 00000000000..5a98c6fd7ff --- /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 e1c37caaafd..6ed56598e15 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 00000000000..7dff6532516 --- /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 435d994e201..517177e82b9 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 2fdeeae302b..6d0fc8ad6d4 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 00000000000..fd7c58ba68f --- /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 7eddfc471f6..3c567fad68d 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 00000000000..f5aa1819982 --- /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 85a536ee6ad..6841e7719dc 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 00000000000..9f060fbaf7d --- /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 bbd50f1c0ef..abbaa22ff3e 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 60ae579eb03..fba8f40e99b 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 e8a44f7a12a..46b1bebb074 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 index 259148fa18f..259148fa18f 100644..100755 --- a/vendor/gitignore/C++.gitignore +++ b/vendor/gitignore/C++.gitignore diff --git a/vendor/gitignore/Java.gitignore b/vendor/gitignore/Java.gitignore index a1c2a238a96..a1c2a238a96 100644..100755 --- a/vendor/gitignore/Java.gitignore +++ b/vendor/gitignore/Java.gitignore |