Commit 65aafb99 authored by Douwe Maan's avatar Douwe Maan Committed by DJ Mountney

Merge branch 'ssrf' into 'security'

Protect server against SSRF in project import URLs

See merge request !2068
parent c5a9d73a
...@@ -196,6 +196,7 @@ class Project < ActiveRecord::Base ...@@ -196,6 +196,7 @@ class Project < ActiveRecord::Base
validates :name, uniqueness: { scope: :namespace_id } validates :name, uniqueness: { scope: :namespace_id }
validates :path, uniqueness: { scope: :namespace_id } validates :path, uniqueness: { scope: :namespace_id }
validates :import_url, addressable_url: true, if: :external_import? validates :import_url, addressable_url: true, if: :external_import?
validates :import_url, importable_url: true, if: [:external_import?, :import_url_changed?]
validates :star_count, numericality: { greater_than_or_equal_to: 0 } validates :star_count, numericality: { greater_than_or_equal_to: 0 }
validate :check_limit, on: :create validate :check_limit, on: :create
validate :avatar_type, validate :avatar_type,
......
...@@ -33,6 +33,7 @@ module Projects ...@@ -33,6 +33,7 @@ module Projects
def import_repository def import_repository
begin begin
raise Error, "Blocked import URL." if Gitlab::UrlBlocker.blocked_url?(project.import_url)
gitlab_shell.import_repository(project.repository_storage_path, project.path_with_namespace, project.import_url) gitlab_shell.import_repository(project.repository_storage_path, project.path_with_namespace, project.import_url)
rescue => e rescue => e
# Expire cache to prevent scenarios such as: # Expire cache to prevent scenarios such as:
......
# 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)
if Gitlab::UrlBlocker.blocked_url?(value)
record.errors.add(attribute, "imports are not allowed from that URL")
end
end
end
---
title: To protect against Server-side Request Forgery project import URLs are now prohibited against localhost or the server IP except for the assigned instance URL and port. Imports are also prohibited from ports below 1024 with the exception of ports 22, 80, and 443.
merge_request:
author:
require 'resolv'
module Gitlab
class UrlBlocker
class << self
# Used to specify what hosts and port numbers should be prohibited for project
# imports.
VALID_PORTS = [22, 80, 443].freeze
def blocked_url?(url)
blocked_ips = ["127.0.0.1", "::1", "0.0.0.0"]
blocked_ips.concat(Socket.ip_address_list.map(&:ip_address))
begin
uri = Addressable::URI.parse(url)
# Allow imports from the GitLab instance itself but only from the configured ports
return false if internal?(uri)
return true if blocked_port?(uri.port)
server_ips = Resolv.getaddresses(uri.hostname)
return true if (blocked_ips & server_ips).any?
rescue Addressable::URI::InvalidURIError
return true
end
false
end
private
def blocked_port?(port)
return false if port.blank?
port < 1024 && !VALID_PORTS.include?(port)
end
def internal?(uri)
internal_web?(uri) || internal_shell?(uri)
end
def internal_web?(uri)
uri.hostname == config.gitlab.host &&
(uri.port.blank? || uri.port == config.gitlab.port)
end
def internal_shell?(uri)
uri.hostname == config.gitlab_shell.ssh_host &&
(uri.port.blank? || uri.port == config.gitlab_shell.ssh_port)
end
def config
Gitlab.config
end
end
end
end
require 'spec_helper'
describe Gitlab::UrlBlocker, lib: true do
describe '#blocked_url?' do
it 'allows imports from configured web host and port' do
import_url = "http://#{Gitlab.config.gitlab.host}:#{Gitlab.config.gitlab.port}/t.git"
expect(described_class.blocked_url?(import_url)).to be false
end
it 'allows imports from configured SSH host and port' do
import_url = "http://#{Gitlab.config.gitlab_shell.ssh_host}:#{Gitlab.config.gitlab_shell.ssh_port}/t.git"
expect(described_class.blocked_url?(import_url)).to be false
end
it 'returns true for bad localhost hostname' do
expect(described_class.blocked_url?('https://localhost:65535/foo/foo.git')).to be true
end
it 'returns true for bad port' do
expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git')).to be true
end
it 'returns true for invalid URL' do
expect(described_class.blocked_url?('http://:8080')).to be true
end
it 'returns false for legitimate URL' do
expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git')).to be false
end
end
end
...@@ -218,6 +218,20 @@ describe Project, models: true do ...@@ -218,6 +218,20 @@ describe Project, models: true do
expect(project2.import_data).to be_nil expect(project2.import_data).to be_nil
end end
it "does not allow blocked import_url localhost" do
project2 = build(:empty_project, import_url: 'http://localhost:9000/t.git')
expect(project2).to be_invalid
expect(project2.errors[:import_url]).to include('imports are not allowed from that URL')
end
it "does not allow blocked import_url port" do
project2 = build(:empty_project, import_url: 'http://github.com:25/t.git')
expect(project2).to be_invalid
expect(project2.errors[:import_url]).to include('imports are not allowed from that URL')
end
describe 'project pending deletion' do describe 'project pending deletion' do
let!(:project_pending_deletion) do let!(:project_pending_deletion) do
create(:empty_project, create(:empty_project,
......
...@@ -120,6 +120,26 @@ describe Projects::ImportService, services: true do ...@@ -120,6 +120,26 @@ describe Projects::ImportService, services: true do
end end
end end
context 'with blocked import_URL' do
it 'fails with localhost' do
project.import_url = 'https://localhost:9000/vim/vim.git'
result = described_class.new(project, user).execute
expect(result[:status]).to eq :error
expect(result[:message]).to end_with 'Blocked import URL.'
end
it 'fails with port 25' do
project.import_url = "https://github.com:25/vim/vim.git"
result = described_class.new(project, user).execute
expect(result[:status]).to eq :error
expect(result[:message]).to end_with 'Blocked import URL.'
end
end
def stub_github_omniauth_provider def stub_github_omniauth_provider
provider = OpenStruct.new( provider = OpenStruct.new(
'name' => 'github', 'name' => 'github',
......
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