diff options
author | David Wilkins <dwilkins@gitlab.com> | 2019-10-24 18:53:42 +0000 |
---|---|---|
committer | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-10-24 18:53:42 +0000 |
commit | 9a0dc3fa39ec3a7bb85d76252ee33176152b85b4 (patch) | |
tree | eb64e2f4e24ae7ee64b390270f5ae124bdd91f8a | |
parent | 3e2d0afa6e9c8bdf728f72b3a06d1ecd06930a9f (diff) | |
download | gitlab-ce-9a0dc3fa39ec3a7bb85d76252ee33176152b85b4.tar.gz |
Handle Stored XSS for Grafana URL in settings
- Extend Gitlab::UrlBlocker to allow relative urls (require_absolute
setting). The new `require_absolute` setting defaults to true,
which is the existing behavior.
- Extend AddressableUrlValidator to accept `require_abosolute` and
default to the existing behavior
- Add validation for ApplicationSetting#grafana_url to validate that
the URL does not contain XSS but can be a valid relative or absolute
url.
- In the case of existing stored URLs, validate the stored URL does
not contain XSS. If the stored URL contains stored XSS or is an
otherwise invalid URL, return the default database column value.
- Add tests for Gitlab::UrlBlocker to test require_absolute setting
- Add tests for AddressableUrlValidator
- Add tests for ApplicationSetting#grafana_url
-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 6aadbbc9d03..6cfbaf16db7 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 @@ -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 diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb index 273e15ef925..9e9ff9b0d1f 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 cc937cbb3cf..50192d97cf7 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,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) diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index 0fab890978a..0574f4a8e50 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' } @@ -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') diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 904185f4140..33c073a65a9 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) } @@ -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) diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb index b5e45f99109..f79b37bf07d 100644 --- a/spec/requests/api/commit_statuses_spec.rb +++ b/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 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 |