diff options
author | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-10-24 18:53:35 +0000 |
---|---|---|
committer | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-10-24 18:53:35 +0000 |
commit | 8987371e54046fd6c01d7ed39bff87403be6d193 (patch) | |
tree | 691bbb53a66daf8e1c54231ccd48c1eb3e0106d5 | |
parent | 82a0d826a47d961725f1b62db8fe51849f8d87f1 (diff) | |
parent | da6a3067ec16715a580bd914f55c9d638d96c73b (diff) | |
download | gitlab-ce-8987371e54046fd6c01d7ed39bff87403be6d193.tar.gz |
Merge branch 'security-xss-grafana-url-12-2' into '12-2-stable'
Handle Stored XSS for Grafana URL in settings
See merge request gitlab/gitlabhq!3481
-rw-r--r-- | app/models/application_setting.rb | 21 | ||||
-rw-r--r-- | app/validators/addressable_url_validator.rb | 3 | ||||
-rw-r--r-- | changelogs/unreleased/security-xss-grafana-url-12-4.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/url_blocker.rb | 44 | ||||
-rw-r--r-- | spec/lib/gitlab/url_blocker_spec.rb | 60 | ||||
-rw-r--r-- | spec/models/application_setting_spec.rb | 6 | ||||
-rw-r--r-- | spec/requests/api/commit_statuses_spec.rb | 2 | ||||
-rw-r--r-- | spec/validators/addressable_url_validator_spec.rb | 63 |
8 files changed, 189 insertions, 15 deletions
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index d6caf092ed0..7b5d67556ea 100644 --- 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 @@ -55,6 +62,11 @@ class ApplicationSetting < ApplicationRecord allow_nil: false, qualified_domain_array: 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 } @@ -72,7 +84,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 @@ -303,6 +314,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 diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb index bb445499cee..1075d71869f 100644 --- 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({ diff --git a/changelogs/unreleased/security-xss-grafana-url-12-4.yml b/changelogs/unreleased/security-xss-grafana-url-12-4.yml new file mode 100644 index 00000000000..d0adff94b76 --- /dev/null +++ b/changelogs/unreleased/security-xss-grafana-url-12-4.yml @@ -0,0 +1,5 @@ +--- +title: Fix stored XSS issue for grafana_url +merge_request: +author: +type: security diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index 9c35d200dcb..cba7d4dc194 100644 --- a/lib/gitlab/url_blocker.rb +++ b/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,14 +46,15 @@ module Gitlab ports: ports, enforce_sanitization: enforce_sanitization, enforce_user: enforce_user, - ascii_only: ascii_only + ascii_only: ascii_only, + require_absolute: require_absolute ) normalized_hostname = uri.normalized_host 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 ip_address = ip_address(address_info) @@ -98,12 +103,14 @@ module Gitlab address_info.first&.ip_address 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) @@ -183,8 +190,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 @@ -243,9 +262,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) diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index 45d9022abeb..1f5bb3ae13d 100644 --- 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 @@ 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' } @@ -520,6 +565,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') diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 4f7a6d102b8..9a27f608771 100644 --- a/spec/models/application_setting_spec.rb +++ b/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) } @@ -48,6 +49,11 @@ describe ApplicationSetting do it { is_expected.not_to allow_value(nil).for(:outbound_local_requests_whitelist) } it { is_expected.to allow_value([]).for(:outbound_local_requests_whitelist) } + 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) diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb index 1be8883bd3c..deeab82a80d 100644 --- a/spec/requests/api/commit_statuses_spec.rb +++ b/spec/requests/api/commit_statuses_spec.rb @@ -322,7 +322,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 diff --git a/spec/validators/addressable_url_validator_spec.rb b/spec/validators/addressable_url_validator_spec.rb index 387e84b2d04..d9e21585629 100644 --- a/spec/validators/addressable_url_validator_spec.rb +++ b/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 |