Commit 0eba3277 authored by Cindy Pallares's avatar Cindy Pallares

Merge branch 'security-fj-crlf-injection' into 'master'

[master] Fix CRLF issue in UrlValidator

See merge request gitlab/gitlabhq!2627
parents a7ae5c0c 2327897e
---
title: Fix CRLF vulnerability in Project hooks
merge_request:
author:
type: security
...@@ -10,11 +10,8 @@ module Gitlab ...@@ -10,11 +10,8 @@ module Gitlab
def validate!(url, allow_localhost: false, allow_local_network: true, enforce_user: false, ports: [], protocols: []) def validate!(url, allow_localhost: false, allow_local_network: true, enforce_user: false, ports: [], protocols: [])
return true if url.nil? return true if url.nil?
begin # Param url can be a string, URI or Addressable::URI
uri = Addressable::URI.parse(url) uri = parse_url(url)
rescue Addressable::URI::InvalidURIError
raise BlockedUrlError, "URI is invalid"
end
# Allow imports from the GitLab instance itself but only from the configured ports # Allow imports from the GitLab instance itself but only from the configured ports
return true if internal?(uri) return true if internal?(uri)
...@@ -49,6 +46,18 @@ module Gitlab ...@@ -49,6 +46,18 @@ module Gitlab
private 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) def validate_port!(port, ports)
return if port.blank? return if port.blank?
# Only ports under 1024 are restricted # Only ports under 1024 are restricted
......
...@@ -234,76 +234,93 @@ describe Project do ...@@ -234,76 +234,93 @@ describe Project do
end end
end end
it 'does not allow an invalid URI as import_url' do describe 'import_url' do
project = build(:project, import_url: 'invalid://') it 'does not allow an invalid URI as import_url' do
project = build(:project, import_url: 'invalid://')
expect(project).not_to be_valid expect(project).not_to be_valid
end end
it 'does allow a SSH URI as import_url for persisted projects' do it 'does allow a SSH URI as import_url for persisted projects' do
project = create(:project) project = create(:project)
project.import_url = 'ssh://test@gitlab.com/project.git' project.import_url = 'ssh://test@gitlab.com/project.git'
expect(project).to be_valid expect(project).to be_valid
end end
it 'does not allow a SSH URI as import_url for new projects' do it 'does not allow a SSH URI as import_url for new projects' do
project = build(:project, import_url: 'ssh://test@gitlab.com/project.git') project = build(:project, import_url: 'ssh://test@gitlab.com/project.git')
expect(project).not_to be_valid expect(project).not_to be_valid
end end
it 'does allow a valid URI as import_url' do it 'does allow a valid URI as import_url' do
project = build(:project, import_url: 'http://gitlab.com/project.git') project = build(:project, import_url: 'http://gitlab.com/project.git')
expect(project).to be_valid expect(project).to be_valid
end end
it 'allows an empty URI' do it 'allows an empty URI' do
project = build(:project, import_url: '') project = build(:project, import_url: '')
expect(project).to be_valid expect(project).to be_valid
end end
it 'does not produce import data on an empty URI' do it 'does not produce import data on an empty URI' do
project = build(:project, import_url: '') project = build(:project, import_url: '')
expect(project.import_data).to be_nil expect(project.import_data).to be_nil
end end
it 'does not produce import data on an invalid URI' do it 'does not produce import data on an invalid URI' do
project = build(:project, import_url: 'test://') project = build(:project, import_url: 'test://')
expect(project.import_data).to be_nil expect(project.import_data).to be_nil
end end
it "does not allow import_url pointing to localhost" do it "does not allow import_url pointing to localhost" do
project = build(:project, import_url: 'http://localhost:9000/t.git') project = build(:project, import_url: 'http://localhost:9000/t.git')
expect(project).to be_invalid expect(project).to be_invalid
expect(project.errors[:import_url].first).to include('Requests to localhost are not allowed') expect(project.errors[:import_url].first).to include('Requests to localhost are not allowed')
end end
it "does not allow import_url with invalid ports for new projects" do it "does not allow import_url with invalid ports for new projects" do
project = build(:project, import_url: 'http://github.com:25/t.git') project = build(:project, import_url: 'http://github.com:25/t.git')
expect(project).to be_invalid expect(project).to be_invalid
expect(project.errors[:import_url].first).to include('Only allowed ports are 80, 443') expect(project.errors[:import_url].first).to include('Only allowed ports are 80, 443')
end end
it "does not allow import_url with invalid ports for persisted projects" do it "does not allow import_url with invalid ports for persisted projects" do
project = create(:project) project = create(:project)
project.import_url = 'http://github.com:25/t.git' project.import_url = 'http://github.com:25/t.git'
expect(project).to be_invalid expect(project).to be_invalid
expect(project.errors[:import_url].first).to include('Only allowed ports are 22, 80, 443') expect(project.errors[:import_url].first).to include('Only allowed ports are 22, 80, 443')
end end
it "does not allow import_url with invalid user" do
project = build(:project, import_url: 'http://$user:password@github.com/t.git')
expect(project).to be_invalid
expect(project.errors[:import_url].first).to include('Username needs to start with an alphanumeric character')
end
it "does not allow import_url with invalid user" do include_context 'invalid urls'
project = build(:project, import_url: 'http://$user:password@github.com/t.git')
expect(project).to be_invalid it 'does not allow urls with CR or LF characters' do
expect(project.errors[:import_url].first).to include('Username needs to start with an alphanumeric character') project = build(:project)
aggregate_failures do
urls_with_CRLF.each do |url|
project.import_url = url
expect(project).not_to be_valid
expect(project.errors.full_messages.first).to match(/is blocked: URI is invalid/)
end
end
end
end end
describe 'project pending deletion' do describe 'project pending deletion' do
......
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
# frozen_string_literal: true
require 'spec_helper' require 'spec_helper'
describe UrlValidator do describe UrlValidator do
...@@ -6,6 +8,30 @@ describe UrlValidator do ...@@ -6,6 +8,30 @@ describe UrlValidator do
include_examples 'url validator examples', described_class::DEFAULT_PROTOCOLS 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 context 'by default' do
let(:validator) { described_class.new(attributes: [:link_url]) } let(:validator) { described_class.new(attributes: [:link_url]) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment