From 334915d50884e54ed8034b4b8820f285b14837c5 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Tue, 8 Aug 2017 21:18:02 +0000 Subject: Merge branch 'import-symlinks-9-3' into 'security-9-3' Fix file disclosure via hidden symlinks using the project import (9.3) See merge request !2164 --- changelogs/unreleased/fix-import-symbolink-links.yml | 4 ++++ lib/gitlab/import_export/file_importer.rb | 6 +++++- spec/lib/gitlab/import_export/file_importer_spec.rb | 5 +++++ 3 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/fix-import-symbolink-links.yml diff --git a/changelogs/unreleased/fix-import-symbolink-links.yml b/changelogs/unreleased/fix-import-symbolink-links.yml new file mode 100644 index 00000000000..36e73821bdc --- /dev/null +++ b/changelogs/unreleased/fix-import-symbolink-links.yml @@ -0,0 +1,4 @@ +--- +title: Remove hidden symlinks from project import files +merge_request: +author: diff --git a/lib/gitlab/import_export/file_importer.rb b/lib/gitlab/import_export/file_importer.rb index ffd17118c91..989342389bc 100644 --- a/lib/gitlab/import_export/file_importer.rb +++ b/lib/gitlab/import_export/file_importer.rb @@ -47,12 +47,16 @@ module Gitlab end def remove_symlinks! - Dir["#{@shared.export_path}/**/*"].each do |path| + extracted_files.each do |path| FileUtils.rm(path) if File.lstat(path).symlink? end true end + + def extracted_files + Dir.glob("#{@shared.export_path}/**/*", File::FNM_DOTMATCH).reject { |f| f =~ /.*\/\.{1,2}$/ } + end end end end diff --git a/spec/lib/gitlab/import_export/file_importer_spec.rb b/spec/lib/gitlab/import_export/file_importer_spec.rb index 690c7625c52..162b776e107 100644 --- a/spec/lib/gitlab/import_export/file_importer_spec.rb +++ b/spec/lib/gitlab/import_export/file_importer_spec.rb @@ -5,6 +5,7 @@ describe Gitlab::ImportExport::FileImporter do let(:export_path) { "#{Dir.tmpdir}/file_importer_spec" } let(:valid_file) { "#{shared.export_path}/valid.json" } let(:symlink_file) { "#{shared.export_path}/invalid.json" } + let(:hidden_symlink_file) { "#{shared.export_path}/.hidden" } let(:subfolder_symlink_file) { "#{shared.export_path}/subfolder/invalid.json" } before do @@ -25,6 +26,10 @@ describe Gitlab::ImportExport::FileImporter do expect(File.exist?(symlink_file)).to be false end + it 'removes hidden symlinks in root folder' do + expect(File.exist?(hidden_symlink_file)).to be false + end + it 'removes symlinks in subfolders' do expect(File.exist?(subfolder_symlink_file)).to be false end -- cgit v1.2.3 From b29692168184cef044c6a1b244f791c56c10fb1c Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Tue, 8 Aug 2017 17:36:24 +0000 Subject: Merge branch 'rs-alphanumeric-ssh-params' into 'security-9-4' Ensure user and hostnames begin with an alnum character in UrlBlocker See merge request !2138 --- .../unreleased/rs-alphanumeric-ssh-params.yml | 5 ++++ lib/gitlab/url_blocker.rb | 8 +++++ spec/lib/gitlab/url_blocker_spec.rb | 34 ++++++++++++++++++++++ 3 files changed, 47 insertions(+) create mode 100644 changelogs/unreleased/rs-alphanumeric-ssh-params.yml diff --git a/changelogs/unreleased/rs-alphanumeric-ssh-params.yml b/changelogs/unreleased/rs-alphanumeric-ssh-params.yml new file mode 100644 index 00000000000..426b01cafad --- /dev/null +++ b/changelogs/unreleased/rs-alphanumeric-ssh-params.yml @@ -0,0 +1,5 @@ +--- +title: Disallow Git URLs that include a username or hostname beginning with a non-alphanumeric + character +merge_request: +author: diff --git a/lib/gitlab/url_blocker.rb b/lib/gitlab/url_blocker.rb index 7e14a566696..fee1a127fd7 100644 --- a/lib/gitlab/url_blocker.rb +++ b/lib/gitlab/url_blocker.rb @@ -19,6 +19,8 @@ module Gitlab return false if internal?(uri) return true if blocked_port?(uri.port) + return true if blocked_user_or_hostname?(uri.user) + return true if blocked_user_or_hostname?(uri.hostname) server_ips = Resolv.getaddresses(uri.hostname) return true if (blocked_ips & server_ips).any? @@ -37,6 +39,12 @@ module Gitlab port < 1024 && !VALID_PORTS.include?(port) end + def blocked_user_or_hostname?(value) + return false if value.blank? + + value !~ /\A\p{Alnum}/ + end + def internal?(uri) internal_web?(uri) || internal_shell?(uri) end diff --git a/spec/lib/gitlab/url_blocker_spec.rb b/spec/lib/gitlab/url_blocker_spec.rb index f5b4882815f..f18823b61ef 100644 --- a/spec/lib/gitlab/url_blocker_spec.rb +++ b/spec/lib/gitlab/url_blocker_spec.rb @@ -20,6 +20,34 @@ describe Gitlab::UrlBlocker do expect(described_class.blocked_url?('https://gitlab.com:25/foo/foo.git')).to be true end + it 'returns true for a non-alphanumeric hostname' do + stub_resolv + + aggregate_failures do + expect(described_class).to be_blocked_url('ssh://-oProxyCommand=whoami/a') + + # The leading character here is a Unicode "soft hyphen" + expect(described_class).to be_blocked_url('ssh://­oProxyCommand=whoami/a') + + # Unicode alphanumerics are allowed + expect(described_class).not_to be_blocked_url('ssh://ğitlab.com/a') + end + end + + it 'returns true for a non-alphanumeric username' do + stub_resolv + + aggregate_failures do + expect(described_class).to be_blocked_url('ssh://-oProxyCommand=whoami@example.com/a') + + # The leading character here is a Unicode "soft hyphen" + expect(described_class).to be_blocked_url('ssh://­oProxyCommand=whoami@example.com/a') + + # Unicode alphanumerics are allowed + expect(described_class).not_to be_blocked_url('ssh://ğitlab@example.com/a') + end + end + it 'returns true for invalid URL' do expect(described_class.blocked_url?('http://:8080')).to be true end @@ -28,4 +56,10 @@ describe Gitlab::UrlBlocker do expect(described_class.blocked_url?('https://gitlab.com/foo/foo.git')).to be false end end + + # Resolv does not support resolving UTF-8 domain names + # See https://bugs.ruby-lang.org/issues/4270 + def stub_resolv + allow(Resolv).to receive(:getaddresses).and_return([]) + end end -- cgit v1.2.3