Commit 70f9e7fd authored by John Jarvis's avatar John Jarvis

Merge branch 'security-11-5-fix-ssrf-lfs-project-import' into 'security-11-5'

[11.5] SSRF in project imports with LFS

See merge request gitlab/gitlabhq!2726
parents 7e0f8781 4d8e8d8a
......@@ -12,28 +12,43 @@ module Projects
return if LfsObject.exists?(oid: oid)
sanitized_uri = Gitlab::UrlSanitizer.new(url)
Gitlab::UrlBlocker.validate!(sanitized_uri.sanitized_url, protocols: VALID_PROTOCOLS)
sanitized_uri = sanitize_url!(url)
with_tmp_file(oid) do |file|
size = download_and_save_file(file, sanitized_uri)
lfs_object = LfsObject.new(oid: oid, size: size, file: file)
download_and_save_file(file, sanitized_uri)
lfs_object = LfsObject.new(oid: oid, size: file.size, file: file)
project.all_lfs_objects << lfs_object
end
rescue Gitlab::UrlBlocker::BlockedUrlError => e
Rails.logger.error("LFS file with oid #{oid} couldn't be downloaded: #{e.message}")
rescue StandardError => e
Rails.logger.error("LFS file with oid #{oid} could't be downloaded from #{sanitized_uri.sanitized_url}: #{e.message}")
Rails.logger.error("LFS file with oid #{oid} couldn't be downloaded from #{sanitized_uri.sanitized_url}: #{e.message}")
end
# rubocop: enable CodeReuse/ActiveRecord
private
def sanitize_url!(url)
Gitlab::UrlSanitizer.new(url).tap do |sanitized_uri|
# Just validate that HTTP/HTTPS protocols are used. The
# subsequent Gitlab::HTTP.get call will do network checks
# based on the settings.
Gitlab::UrlBlocker.validate!(sanitized_uri.sanitized_url,
protocols: VALID_PROTOCOLS)
end
end
def download_and_save_file(file, sanitized_uri)
IO.copy_stream(open(sanitized_uri.sanitized_url, headers(sanitized_uri)), file) # rubocop:disable Security/Open
response = Gitlab::HTTP.get(sanitized_uri.sanitized_url, headers(sanitized_uri)) do |fragment|
file.write(fragment)
end
raise StandardError, "Received error code #{response.code}" unless response.success?
end
def headers(sanitized_uri)
{}.tap do |headers|
query_options.tap do |headers|
credentials = sanitized_uri.credentials
if credentials[:user].present? || credentials[:password].present?
......@@ -43,10 +58,14 @@ module Projects
end
end
def query_options
{ stream_body: true }
end
def with_tmp_file(oid)
create_tmp_storage_dir
File.open(File.join(tmp_storage_dir, oid), 'w') { |file| yield file }
File.open(File.join(tmp_storage_dir, oid), 'wb') { |file| yield file }
end
def create_tmp_storage_dir
......
......@@ -4,17 +4,15 @@ describe Projects::LfsPointers::LfsDownloadService do
let(:project) { create(:project) }
let(:oid) { '9e548e25631dd9ce6b43afd6359ab76da2819d6a5b474e66118c7819e1d8b3e8' }
let(:download_link) { "http://gitlab.com/#{oid}" }
let(:lfs_content) do
<<~HEREDOC
whatever
HEREDOC
end
let(:lfs_content) { SecureRandom.random_bytes(10) }
subject { described_class.new(project) }
before do
allow(project).to receive(:lfs_enabled?).and_return(true)
WebMock.stub_request(:get, download_link).to_return(body: lfs_content)
allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(false)
end
describe '#execute' do
......@@ -32,7 +30,7 @@ describe Projects::LfsPointers::LfsDownloadService do
it 'stores the content' do
subject.execute(oid, download_link)
expect(File.read(LfsObject.first.file.file.file)).to eq lfs_content
expect(File.binread(LfsObject.first.file.file.file)).to eq lfs_content
end
end
......@@ -54,18 +52,61 @@ describe Projects::LfsPointers::LfsDownloadService do
end
end
context 'when localhost requests are allowed' do
let(:download_link) { 'http://192.168.2.120' }
before do
allow(Gitlab::CurrentSettings).to receive(:allow_local_requests_from_hooks_and_services?).and_return(true)
end
it 'downloads the file' do
expect(subject).to receive(:download_and_save_file).and_call_original
expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.by(1)
end
end
context 'when a bad URL is used' do
where(download_link: ['/etc/passwd', 'ftp://example.com', 'http://127.0.0.2'])
where(download_link: ['/etc/passwd', 'ftp://example.com', 'http://127.0.0.2', 'http://192.168.2.120'])
with_them do
it 'does not download the file' do
expect(subject).not_to receive(:download_and_save_file)
expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count }
end
end
end
context 'when the URL points to a redirected URL' do
context 'that is blocked' do
where(redirect_link: ['ftp://example.com', 'http://127.0.0.2', 'http://192.168.2.120'])
with_them do
before do
WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link })
end
it 'does not follow the redirection' do
expect(Rails.logger).to receive(:error).with(/LFS file with oid #{oid} couldn't be downloaded/)
expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count }
end
end
end
context 'that is valid' do
let(:redirect_link) { "http://example.com/"}
before do
WebMock.stub_request(:get, download_link).to_return(status: 301, headers: { 'Location' => redirect_link })
WebMock.stub_request(:get, redirect_link).to_return(body: lfs_content)
end
it 'follows the redirection' do
expect { subject.execute(oid, download_link) }.to change { LfsObject.count }.from(0).to(1)
end
end
end
context 'when an lfs object with the same oid already exists' do
before do
create(:lfs_object, oid: 'oid')
......
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