diff options
author | Steve Azzopardi <sazzopardi@gitlab.com> | 2018-11-26 13:26:40 +0300 |
---|---|---|
committer | Steve Azzopardi <sazzopardi@gitlab.com> | 2018-11-26 13:26:40 +0300 |
commit | f59f728f442a49c6622597c61f2215d5da49fd51 (patch) | |
tree | ee9c9ba497516d76492787e8a00ba7677b74842d | |
parent | 707d210b6a1bedc90d1b9d09ba341ccf1db9dddd (diff) | |
parent | 536b7dcad6933d53a627fd57d2c0108d219d3aea (diff) |
Merge branch 'security-11-3-fj-crlf-injection' into 'security-11-3'
[11.3] Fix CRLF issue in UrlValidator
See merge request gitlab/gitlabhq!2654
-rw-r--r-- | app/validators/url_validator.rb | 14 | ||||
-rw-r--r-- | changelogs/unreleased/security-11-3-fj-crlf-injection.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/url_blocker.rb | 19 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 85 | ||||
-rw-r--r-- | spec/support/shared_contexts/url_shared_context.rb | 17 | ||||
-rw-r--r-- | spec/validators/url_validator_spec.rb | 24 |
6 files changed, 122 insertions, 42 deletions
diff --git a/app/validators/url_validator.rb b/app/validators/url_validator.rb index faaf1283078..216acf79cbd 100644 --- a/app/validators/url_validator.rb +++ b/app/validators/url_validator.rb @@ -41,12 +41,13 @@ class UrlValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) @record = record - if value.present? - value.strip! - else + unless value.present? record.errors.add(attribute, 'must be a valid URL') + return end + value = strip_value!(record, attribute, value) + Gitlab::UrlBlocker.validate!(value, blocker_args) rescue Gitlab::UrlBlocker::BlockedUrlError => e record.errors.add(attribute, "is blocked: #{e.message}") @@ -54,6 +55,13 @@ class UrlValidator < ActiveModel::EachValidator private + def strip_value!(record, attribute, value) + new_value = value.strip + return value if new_value == value + + record.public_send("#{attribute}=", new_value) # rubocop:disable GitlabSecurity/PublicSend + end + def default_options # By default the validator doesn't block any url based on the ip address { diff --git a/changelogs/unreleased/security-11-3-fj-crlf-injection.yml b/changelogs/unreleased/security-11-3-fj-crlf-injection.yml new file mode 100644 index 00000000000..861167b8a6e --- /dev/null +++ b/changelogs/unreleased/security-11-3-fj-crlf-injection.yml @@ -0,0 +1,5 @@ +--- +title: Fix CRLF vulnerability in Project hooks +merge_request: +author: +type: security diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index 6d3c6909bee..ade5c5242ff 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -9,11 +9,8 @@ module Gitlab def validate!(url, allow_localhost: false, allow_local_network: true, enforce_user: false, ports: [], protocols: []) return true if url.nil? - begin - uri = Addressable::URI.parse(url) - rescue Addressable::URI::InvalidURIError - raise BlockedUrlError, "URI is invalid" - end + # Param url can be a string, URI or Addressable::URI + uri = parse_url(url) # Allow imports from the GitLab instance itself but only from the configured ports return true if internal?(uri) @@ -50,6 +47,18 @@ module Gitlab private + def parse_url(url) + raise Addressable::URI::InvalidURIError if multiline?(url) + + Addressable::URI.parse(url) + rescue Addressable::URI::InvalidURIError, URI::InvalidURIError + raise BlockedUrlError, 'URI is invalid' + end + + def multiline?(url) + CGI.unescape(url.to_s) =~ /\n|\r/ + end + def validate_port!(port, ports) return if port.blank? # Only ports under 1024 are restricted diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 3a285735682..a6cb1048a02 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -227,55 +227,72 @@ describe Project do end end - it 'does not allow an invalid URI as import_url' do - project2 = build(:project, import_url: 'invalid://') + describe 'import_url' do + it 'does not allow an invalid URI as import_url' do + project2 = build(:project, import_url: 'invalid://') - expect(project2).not_to be_valid - end + expect(project2).not_to be_valid + end - it 'does allow a valid URI as import_url' do - project2 = build(:project, import_url: 'ssh://test@gitlab.com/project.git') + it 'does allow a valid URI as import_url' do + project2 = build(:project, import_url: 'ssh://test@gitlab.com/project.git') - expect(project2).to be_valid - end + expect(project2).to be_valid + end - it 'allows an empty URI' do - project2 = build(:project, import_url: '') + it 'allows an empty URI' do + project2 = build(:project, import_url: '') - expect(project2).to be_valid - end + expect(project2).to be_valid + end - it 'does not produce import data on an empty URI' do - project2 = build(:project, import_url: '') + it 'does not produce import data on an empty URI' do + project2 = build(:project, import_url: '') - expect(project2.import_data).to be_nil - end + expect(project2.import_data).to be_nil + end - it 'does not produce import data on an invalid URI' do - project2 = build(:project, import_url: 'test://') + it 'does not produce import data on an invalid URI' do + project2 = build(:project, import_url: 'test://') - expect(project2.import_data).to be_nil - end + expect(project2.import_data).to be_nil + end - it "does not allow import_url pointing to localhost" do - project2 = build(:project, import_url: 'http://localhost:9000/t.git') + it "does not allow import_url pointing to localhost" do + project2 = build(:project, import_url: 'http://localhost:9000/t.git') - expect(project2).to be_invalid - expect(project2.errors[:import_url].first).to include('Requests to localhost are not allowed') - end + expect(project2).to be_invalid + expect(project2.errors[:import_url].first).to include('Requests to localhost are not allowed') + end - it "does not allow import_url with invalid ports" do - project2 = build(:project, import_url: 'http://github.com:25/t.git') + it "does not allow import_url with invalid ports" do + project2 = build(:project, import_url: 'http://github.com:25/t.git') - expect(project2).to be_invalid - expect(project2.errors[:import_url].first).to include('Only allowed ports are 22, 80, 443') - end + expect(project2).to be_invalid + expect(project2.errors[:import_url].first).to include('Only allowed ports are 22, 80, 443') + end + + it "does not allow import_url with invalid user" do + project2 = build(:project, import_url: 'http://$user:password@github.com/t.git') + + expect(project2).to be_invalid + expect(project2.errors[:import_url].first).to include('Username needs to start with an alphanumeric character') + end + + include_context 'invalid urls' + + it 'does not allow urls with CR or LF characters' do + project = build(:project) - it "does not allow import_url with invalid user" do - project2 = build(:project, import_url: 'http://$user:password@github.com/t.git') + aggregate_failures do + urls_with_CRLF.each do |url| + project.import_url = url - expect(project2).to be_invalid - expect(project2.errors[:import_url].first).to include('Username needs to start with an alphanumeric character') + expect(project).not_to be_valid + expect(project.errors.full_messages.first).to match(/is blocked: URI is invalid/) + end + end + end end describe 'project pending deletion' do diff --git a/spec/support/shared_contexts/url_shared_context.rb b/spec/support/shared_contexts/url_shared_context.rb new file mode 100644 index 00000000000..1b1f67daac3 --- /dev/null +++ b/spec/support/shared_contexts/url_shared_context.rb @@ -0,0 +1,17 @@ +shared_context 'invalid urls' do + let(:urls_with_CRLF) do + ["http://127.0.0.1:333/pa\rth", + "http://127.0.0.1:333/pa\nth", + "http://127.0a.0.1:333/pa\r\nth", + "http://127.0.0.1:333/path?param=foo\r\nbar", + "http://127.0.0.1:333/path?param=foo\rbar", + "http://127.0.0.1:333/path?param=foo\nbar", + "http://127.0.0.1:333/pa%0dth", + "http://127.0.0.1:333/pa%0ath", + "http://127.0a.0.1:333/pa%0d%0th", + "http://127.0.0.1:333/pa%0D%0Ath", + "http://127.0.0.1:333/path?param=foo%0Abar", + "http://127.0.0.1:333/path?param=foo%0Dbar", + "http://127.0.0.1:333/path?param=foo%0D%0Abar"] + end +end diff --git a/spec/validators/url_validator_spec.rb b/spec/validators/url_validator_spec.rb index 93fe013d11c..522aedac25a 100644 --- a/spec/validators/url_validator_spec.rb +++ b/spec/validators/url_validator_spec.rb @@ -6,6 +6,30 @@ describe UrlValidator do include_examples 'url validator examples', described_class::DEFAULT_PROTOCOLS + describe 'validations' do + include_context 'invalid urls' + + let(:validator) { described_class.new(attributes: [:link_url]) } + + it 'returns error when url is nil' do + expect(validator.validate_each(badge, :link_url, nil)).to be_nil + expect(badge.errors.first[1]).to eq 'must be a valid URL' + end + + it 'returns error when url is empty' do + expect(validator.validate_each(badge, :link_url, '')).to be_nil + expect(badge.errors.first[1]).to eq 'must be a valid URL' + end + + it 'does not allow urls with CR or LF characters' do + aggregate_failures do + urls_with_CRLF.each do |url| + expect(validator.validate_each(badge, :link_url, url)[0]).to eq 'is blocked: URI is invalid' + end + end + end + end + context 'by default' do let(:validator) { described_class.new(attributes: [:link_url]) } |