Commit 840f80d4 authored by Francisco Javier López's avatar Francisco Javier López Committed by Douwe Maan

Add validation to webhook and service URLs to ensure they are not blocked because of SSRF

parent e206e328
......@@ -101,13 +101,19 @@ export default class IntegrationSettingsForm {
return axios.put(this.testEndPoint, formData)
.then(({ data }) => {
if (data.error) {
flash(`${data.message} ${data.service_response}`, 'alert', document, {
title: 'Save anyway',
clickHandler: (e) => {
e.preventDefault();
this.$form.submit();
},
});
let flashActions;
if (data.test_failed) {
flashActions = {
title: 'Save anyway',
clickHandler: (e) => {
e.preventDefault();
this.$form.submit();
},
};
}
flash(`${data.message} ${data.service_response}`, 'alert', document, flashActions);
} else {
this.$form.submit();
}
......
......@@ -41,13 +41,13 @@ class Projects::ServicesController < Projects::ApplicationController
if outcome[:success]
{}
else
{ error: true, message: 'Test failed.', service_response: outcome[:result].to_s }
{ error: true, message: 'Test failed.', service_response: outcome[:result].to_s, test_failed: true }
end
else
{ error: true, message: 'Validations failed.', service_response: @service.errors.full_messages.join(',') }
{ error: true, message: 'Validations failed.', service_response: @service.errors.full_messages.join(','), test_failed: false }
end
rescue Gitlab::HTTP::BlockedUrlError => e
{ error: true, message: 'Test failed.', service_response: e.message }
{ error: true, message: 'Test failed.', service_response: e.message, test_failed: true }
end
def success_message
......
......@@ -18,7 +18,7 @@ class Badge < ActiveRecord::Base
scope :order_created_at_asc, -> { reorder(created_at: :asc) }
validates :link_url, :image_url, url_placeholder: { protocols: %w(http https), placeholder_regex: PLACEHOLDERS_REGEX }
validates :link_url, :image_url, url: { protocols: %w(http https) }
validates :type, presence: true
def rendered_link_url(project = nil)
......
......@@ -32,7 +32,7 @@ class Environment < ActiveRecord::Base
validates :external_url,
length: { maximum: 255 },
allow_nil: true,
addressable_url: true
url: true
delegate :stop_action, :manual_actions, to: :last_deployment, allow_nil: true
......
class GenericCommitStatus < CommitStatus
before_validation :set_default_values
validates :target_url, addressable_url: true,
validates :target_url, url: true,
length: { maximum: 255 },
allow_nil: true
......
......@@ -11,4 +11,9 @@ class SystemHook < WebHook
default_value_for :push_events, false
default_value_for :repository_update_events, true
default_value_for :merge_requests_events, false
# Allow urls pointing localhost and the local network
def allow_local_requests?
true
end
end
......@@ -3,7 +3,9 @@ class WebHook < ActiveRecord::Base
has_many :web_hook_logs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
validates :url, presence: true, url: true
validates :url, presence: true, public_url: { allow_localhost: lambda(&:allow_local_requests?),
allow_local_network: lambda(&:allow_local_requests?) }
validates :token, format: { without: /\n/ }
def execute(data, hook_name)
......@@ -13,4 +15,9 @@ class WebHook < ActiveRecord::Base
def async_execute(data, hook_name)
WebHookService.new(self, data, hook_name).async_execute
end
# Allow urls pointing localhost and the local network
def allow_local_requests?
false
end
end
......@@ -289,8 +289,9 @@ class Project < ActiveRecord::Base
validates :namespace, presence: true
validates :name, uniqueness: { scope: :namespace_id }
validates :import_url, addressable_url: true, if: :external_import?
validates :import_url, importable_url: true, if: [:external_import?, :import_url_changed?]
validates :import_url, url: { protocols: %w(http https ssh git),
allow_localhost: false,
ports: VALID_IMPORT_PORTS }, if: [:external_import?, :import_url_changed?]
validates :star_count, numericality: { greater_than_or_equal_to: 0 }
validate :check_limit, on: :create
validate :check_repository_path_availability, on: :update, if: ->(project) { project.renamed? }
......
......@@ -3,7 +3,7 @@ class BambooService < CiService
prop_accessor :bamboo_url, :build_key, :username, :password
validates :bamboo_url, presence: true, url: true, if: :activated?
validates :bamboo_url, presence: true, public_url: true, if: :activated?
validates :build_key, presence: true, if: :activated?
validates :username,
presence: true,
......
class BugzillaService < IssueTrackerService
validates :project_url, :issues_url, :new_issue_url, presence: true, url: true, if: :activated?
validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
prop_accessor :title, :description, :project_url, :issues_url, :new_issue_url
......
......@@ -8,7 +8,7 @@ class BuildkiteService < CiService
prop_accessor :project_url, :token
boolean_accessor :enable_ssl_verification
validates :project_url, presence: true, url: true, if: :activated?
validates :project_url, presence: true, public_url: true, if: :activated?
validates :token, presence: true, if: :activated?
after_save :compose_service_hook, if: :activated?
......
......@@ -8,7 +8,7 @@ class ChatNotificationService < Service
prop_accessor :webhook, :username, :channel
boolean_accessor :notify_only_broken_pipelines, :notify_only_default_branch
validates :webhook, presence: true, url: true, if: :activated?
validates :webhook, presence: true, public_url: true, if: :activated?
def initialize_properties
# Custom serialized properties initialization
......
class CustomIssueTrackerService < IssueTrackerService
validates :project_url, :issues_url, :new_issue_url, presence: true, url: true, if: :activated?
validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
prop_accessor :title, :description, :project_url, :issues_url, :new_issue_url
......
......@@ -4,7 +4,7 @@ class DroneCiService < CiService
prop_accessor :drone_url, :token
boolean_accessor :enable_ssl_verification
validates :drone_url, presence: true, url: true, if: :activated?
validates :drone_url, presence: true, public_url: true, if: :activated?
validates :token, presence: true, if: :activated?
after_save :compose_service_hook, if: :activated?
......
class ExternalWikiService < Service
prop_accessor :external_wiki_url
validates :external_wiki_url, presence: true, url: true, if: :activated?
validates :external_wiki_url, presence: true, public_url: true, if: :activated?
def title
'External Wiki'
......
class GitlabIssueTrackerService < IssueTrackerService
include Gitlab::Routing
validates :project_url, :issues_url, :new_issue_url, presence: true, url: true, if: :activated?
validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
prop_accessor :title, :description, :project_url, :issues_url, :new_issue_url
......
......@@ -3,8 +3,8 @@ class JiraService < IssueTrackerService
include ApplicationHelper
include ActionView::Helpers::AssetUrlHelper
validates :url, url: true, presence: true, if: :activated?
validates :api_url, url: true, allow_blank: true
validates :url, public_url: true, presence: true, if: :activated?
validates :api_url, public_url: true, allow_blank: true
validates :username, presence: true, if: :activated?
validates :password, presence: true, if: :activated?
......
......@@ -24,7 +24,7 @@ class KubernetesService < DeploymentService
prop_accessor :ca_pem
with_options presence: true, if: :activated? do
validates :api_url, url: true
validates :api_url, public_url: true
validates :token
end
......
......@@ -3,7 +3,7 @@ class MockCiService < CiService
ALLOWED_STATES = %w[failed canceled running pending success success_with_warnings skipped not_found].freeze
prop_accessor :mock_service_url
validates :mock_service_url, presence: true, url: true, if: :activated?
validates :mock_service_url, presence: true, public_url: true, if: :activated?
def title
'MockCI'
......
......@@ -6,7 +6,7 @@ class PrometheusService < MonitoringService
boolean_accessor :manual_configuration
with_options presence: true, if: :manual_configuration? do
validates :api_url, url: true
validates :api_url, public_url: true
end
before_save :synchronize_service_state
......
class RedmineService < IssueTrackerService
validates :project_url, :issues_url, :new_issue_url, presence: true, url: true, if: :activated?
validates :project_url, :issues_url, :new_issue_url, presence: true, public_url: true, if: :activated?
prop_accessor :title, :description, :project_url, :issues_url, :new_issue_url
......
......@@ -3,7 +3,7 @@ class TeamcityService < CiService
prop_accessor :teamcity_url, :build_type, :username, :password
validates :teamcity_url, presence: true, url: true, if: :activated?
validates :teamcity_url, presence: true, public_url: true, if: :activated?
validates :build_type, presence: true, if: :activated?
validates :username,
presence: true,
......
......@@ -17,7 +17,6 @@ class RemoteMirror < ActiveRecord::Base
belongs_to :project, inverse_of: :remote_mirrors
validates :url, presence: true, url: { protocols: %w(ssh git http https), allow_blank: true }
validates :url, addressable_url: true, if: :url_changed?
before_save :set_new_remote_name, if: :mirror_url_changed?
......
......@@ -29,7 +29,7 @@ module Projects
def add_repository_to_project
if project.external_import? && !unknown_url?
begin
Gitlab::UrlBlocker.validate!(project.import_url, valid_ports: Project::VALID_IMPORT_PORTS)
Gitlab::UrlBlocker.validate!(project.import_url, ports: Project::VALID_IMPORT_PORTS)
rescue Gitlab::UrlBlocker::BlockedUrlError => e
raise Error, "Blocked import URL: #{e.message}"
end
......
# AddressableUrlValidator
#
# Custom validator for URLs. This is a stricter version of UrlValidator - it also checks
# for using the right protocol, but it actually parses the URL checking for any syntax errors.
# The regex is also different from `URI` as we use `Addressable::URI` here.
#
# By default, only URLs for http, https, ssh, and git protocols will be considered valid.
# Provide a `:protocols` option to configure accepted protocols.
#
# Example:
#
# class User < ActiveRecord::Base
# validates :personal_url, addressable_url: true
#
# validates :ftp_url, addressable_url: { protocols: %w(ftp) }
#
# validates :git_url, addressable_url: { protocols: %w(http https ssh git) }
# end
#
class AddressableUrlValidator < ActiveModel::EachValidator
DEFAULT_OPTIONS = { protocols: %w(http https ssh git) }.freeze
def validate_each(record, attribute, value)
unless valid_url?(value)
record.errors.add(attribute, "must be a valid URL")
end
end
private
def valid_url?(value)
return false unless value
valid_protocol?(value) && valid_uri?(value)
end
def valid_uri?(value)
Gitlab::UrlSanitizer.valid?(value)
end
def valid_protocol?(value)
options = DEFAULT_OPTIONS.merge(self.options)
value =~ /\A#{URI.regexp(options[:protocols])}\z/
end
end
# ImportableUrlValidator
#
# This validator blocks projects from using dangerous import_urls to help
# protect against Server-side Request Forgery (SSRF).
class ImportableUrlValidator < ActiveModel::EachValidator
def validate_each(record, attribute, value)
Gitlab::UrlBlocker.validate!(value, valid_ports: Project::VALID_IMPORT_PORTS)
rescue Gitlab::UrlBlocker::BlockedUrlError => e
record.errors.add(attribute, "is blocked: #{e.message}")
end
end
# PublicUrlValidator
#
# Custom validator for URLs. This validator works like UrlValidator but
# it blocks by default urls pointing to localhost or the local network.
#
# This validator accepts the same params UrlValidator does.
#
# Example:
#
# class User < ActiveRecord::Base
# validates :personal_url, public_url: true
#
# validates :ftp_url, public_url: { protocols: %w(ftp) }
#
# validates :git_url, public_url: { allow_localhost: true, allow_local_network: true}
# end
#
class PublicUrlValidator < UrlValidator
private
def default_options
# By default block all urls pointing to localhost or the local network
super.merge(allow_localhost: false,
allow_local_network: false)
end
end
# UrlValidator
#
# Custom validator for URLs.
#
# By default, only URLs for the HTTP(S) protocols will be considered valid.
# Provide a `:protocols` option to configure accepted protocols.
#
# Also, this validator can help you validate urls with placeholders inside.
# Usually, if you have a url like 'http://www.example.com/%{project_path}' the
# URI parser will reject that URL format. Provide a `:placeholder_regex` option
# to configure accepted placeholders.
#
# Example:
#
# class User < ActiveRecord::Base
# validates :personal_url, url: true
#
# validates :ftp_url, url: { protocols: %w(ftp) }
#
# validates :git_url, url: { protocols: %w(http https ssh git) }
#
# validates :placeholder_url, url: { placeholder_regex: /(project_path|project_id|default_branch)/ }
# end
#
class UrlPlaceholderValidator < UrlValidator
def validate_each(record, attribute, value)
placeholder_regex = self.options[:placeholder_regex]
value = value.gsub(/%{#{placeholder_regex}}/, 'foo') if placeholder_regex && value
super(record, attribute, value)
end
end
......@@ -15,25 +15,63 @@
# validates :git_url, url: { protocols: %w(http https ssh git) }
# end
#
# This validator can also block urls pointing to localhost or the local network to
# protect against Server-side Request Forgery (SSRF), or check for the right port.
#
# Example:
# class User < ActiveRecord::Base
# validates :personal_url, url: { allow_localhost: false, allow_local_network: false}
#
# validates :web_url, url: { ports: [80, 443] }
# end
class UrlValidator < ActiveModel::EachValidator
DEFAULT_PROTOCOLS = %w(http https).freeze
attr_reader :record
def validate_each(record, attribute, value)
unless valid_url?(value)
@record = record
if value.present?
value.strip!
else
record.errors.add(attribute, "must be a valid URL")
end
Gitlab::UrlBlocker.validate!(value, blocker_args)
rescue Gitlab::UrlBlocker::BlockedUrlError => e
record.errors.add(attribute, "is blocked: #{e.message}")
end
private
def default_options
@default_options ||= { protocols: %w(http https) }
# By default the validator doesn't block any url based on the ip address
{
protocols: DEFAULT_PROTOCOLS,
ports: [],
allow_localhost: true,
allow_local_network: true
}
end
def valid_url?(value)
return false if value.nil?
def current_options
options = self.options.map do |option, value|
[option, value.is_a?(Proc) ? value.call(record) : value]
end.to_h
default_options.merge(options)
end
options = default_options.merge(self.options)
def blocker_args
current_options.slice(:allow_localhost, :allow_local_network, :protocols, :ports).tap do |args|
if allow_setting_local_requests?
args[:allow_localhost] = args[:allow_local_network] = true
end
end
end
value.strip!
value =~ /\A#{URI.regexp(options[:protocols])}\z/
def allow_setting_local_requests?
ApplicationSetting.current&.allow_local_requests_from_hooks_and_services?
end
end
---
title: Refactoring UrlValidators to include url blocking
merge_request: 18686
author:
type: changed
......@@ -5,7 +5,7 @@ module Gitlab
BlockedUrlError = Class.new(StandardError)
class << self
def validate!(url, allow_localhost: false, allow_local_network: true, valid_ports: [])
def validate!(url, allow_localhost: false, allow_local_network: true, ports: [], protocols: [])
return true if url.nil?
begin
......@@ -18,7 +18,8 @@ module Gitlab
return true if internal?(uri)
port = uri.port || uri.default_port
validate_port!(port, valid_ports) if valid_ports.any?
validate_protocol!(uri.scheme, protocols)
validate_port!(port, ports) if ports.any?
validate_user!(uri.user)
validate_hostname!(uri.hostname)
......@@ -44,13 +45,19 @@ module Gitlab
private
def validate_port!(port, valid_ports)
def validate_port!(port, ports)
return if port.blank?
# Only ports under 1024 are restricted
return if port >= 1024
return if valid_ports.include?(port)
return if ports.include?(port)
raise BlockedUrlError, "Only allowed ports are #{valid_ports.join(', ')}, and any over 1024"
raise BlockedUrlError, "Only allowed ports are #{ports.join(', ')}, and any over 1024"
end
def validate_protocol!(protocol, protocols)
if protocol.blank? || (protocols.any? && !protocols.include?(protocol))
raise BlockedUrlError, "Only allowed protocols are #{protocols.join(', ')}"
end
end
def validate_user!(value)
......
......@@ -54,7 +54,7 @@ describe Projects::MirrorsController do
do_put(project, remote_mirrors_attributes: remote_mirror_attributes)
expect(response).to redirect_to(project_settings_repository_path(project))
expect(flash[:alert]).to match(/must be a valid URL/)
expect(flash[:alert]).to match(/Only allowed protocols are/)
end
it 'should not create a RemoteMirror object' do
......
......@@ -102,7 +102,7 @@ describe Projects::ServicesController do
expect(response.status).to eq(200)
expect(JSON.parse(response.body))
.to eq('error' => true, 'message' => 'Test failed.', 'service_response' => 'Bad test')
.to eq('error' => true, 'message' => 'Test failed.', 'service_response' => 'Bad test', 'test_failed' => true)
end
end
end
......
......@@ -143,6 +143,7 @@ describe('IntegrationSettingsForm', () => {
error: true,
message: errorMessage,
service_response: 'some error',
test_failed: true,
});
integrationSettingsForm.testSettings(formData)
......@@ -157,6 +158,27 @@ describe('IntegrationSettingsForm', () => {
.catch(done.fail);
});
it('should not show error Flash with `Save anyway` action if ajax request responds with error in validation', (done) => {
const errorMessage = 'Validations failed.';
mock.onPut(integrationSettingsForm.testEndPoint).reply(200, {
error: true,
message: errorMessage,
service_response: 'some error',
test_failed: false,
});
integrationSettingsForm.testSettings(formData)
.then(() => {
const $flashContainer = $('.flash-container');
expect($flashContainer.find('.flash-text').text().trim()).toEqual('Validations failed. some error');
expect($flashContainer.find('.flash-action')).toBeDefined();
expect($flashContainer.find('.flash-action').text().trim()).toEqual('');
done();
})
.catch(done.fail);
});
it('should submit form if ajax request responds without any error in test', (done) => {
spyOn(integrationSettingsForm.$form, 'submit');
......@@ -180,6 +202,7 @@ describe('IntegrationSettingsForm', () => {
mock.onPut(integrationSettingsForm.testEndPoint).reply(200, {
error: true,
message: errorMessage,
test_failed: true,
});
integrationSettingsForm.testSettings(formData)
......
......@@ -2,7 +2,7 @@ require 'spec_helper'
describe Gitlab::UrlBlocker do
describe '#blocked_url?' do
let(:valid_ports) { Project::VALID_IMPORT_PORTS }
let(:ports) { Project::VALID_IMPORT_PORTS }
it 'allows imports from configured web host and port' do
import_url = "http://#{Gitlab.config.gitlab.host}:#{Gitlab.config.gitlab.port}/t.git"
......@@ -19,7 +19,13 @@ describe Gitlab::UrlBlocker do
end
it 'returns true for bad port' do
expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git', valid_ports: valid_ports)).to be true
expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git', ports: ports)).to be true
end
it 'returns true for bad protocol' do
expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git', protocols: ['https'])).to be false
expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git')).to be false
expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git', protocols: ['http'])).to be true
end
it 'returns true for alternative version of 127.0.0.1 (0177.1)' do
......
......@@ -12,8 +12,8 @@ describe RemoteMirror do
context 'with an invalid URL' do
it 'should not be valid' do
remote_mirror = build(:remote_mirror, url: 'ftp://invalid.invalid')
expect(remote_mirror).not_to be_valid
expect(remote_mirror.errors[:url].size).to eq(2)
end
end
end
......
......@@ -304,7 +304,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 'must be a valid URL'
.to include 'is blocked: Only allowed protocols are http, https'
end
end
end
......
require 'spec_helper'
describe UrlPlaceholderValidator do
RSpec.shared_examples 'url validator examples' do |protocols|
let(:validator) { described_class.new(attributes: [:link_url], **options) }
let!(:badge) { build(:badge) }
let(:placeholder_url) { 'http://www.example.com/%{project_path}/%{project_id}/%{default_branch}/%{commit_sha}' }
let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
subject { validator.validate_each(badge, :link_url, badge.link_url) }
......@@ -11,12 +8,12 @@ describe UrlPlaceholderValidator do
context 'with no options' do
let(:options) { {} }
it 'allows http and https protocols by default' do
expect(validator.send(:default_options)[:protocols]).to eq %w(http https)
it "allows #{protocols.join(',')} protocols by default" do
expect(validator.send(:default_options)[:protocols]).to eq protocols
end
it 'checks that the url structure is valid' do
badge.link_url = placeholder_url
badge.link_url = "#{badge.link_url}:invalid_port"
subject
......@@ -24,16 +21,22 @@ describe UrlPlaceholderValidator do
end
end
context 'with placeholder regex' do
let(:options) { { placeholder_regex: /(project_path|project_id|commit_sha|default_branch)/ } }
it 'checks that the url is valid and obviate placeholders that match regex' do
badge.link_url = placeholder_url
context 'with protocols' do
let(:options) { { protocols: %w[http] } }
it 'allows urls with the defined protocols' do
subject
expect(badge.errors.empty?).to be true
end
it 'add error if the url protocol does not match the selected ones' do
badge.link_url = 'https://www.example.com'
subject
expect(badge.errors.empty?).to be false
end
end
end
end
require 'spec_helper'
describe PublicUrlValidator do
include_examples 'url validator examples', described_class::DEFAULT_PROTOCOLS
context 'by default' do
let(:validator) { described_class.new(attributes: [:link_url]) }
let!(:badge) { build(:badge, link_url: 'http://www.example.com') }
subject { validator.validate_each(badge, :link_url, badge.link_url) }
it 'blocks urls pointing to localhost' do
badge.link_url = 'https://127.0.0.1'
subject
expect(badge.errors.empty?).to be false
end
it 'blocks urls pointing to the local network' do
badge.link_url = 'https://192.168.1.1'
subject
expect(badge.errors.empty?).to be false
end
end
end