Fixed SSRF in project imports with LFS

parent 7304d66b
...@@ -12,28 +12,43 @@ module Projects ...@@ -12,28 +12,43 @@ module Projects
return if LfsObject.exists?(oid: oid) return if LfsObject.exists?(oid: oid)
sanitized_uri = Gitlab::UrlSanitizer.new(url) sanitized_uri = sanitize_url!(url)
Gitlab::UrlBlocker.validate!(sanitized_uri.sanitized_url, protocols: VALID_PROTOCOLS)
with_tmp_file(oid) do |file| with_tmp_file(oid) do |file|
size = download_and_save_file(file, sanitized_uri) download_and_save_file(file, sanitized_uri)
lfs_object = LfsObject.new(oid: oid, size: size, file: file) lfs_object = LfsObject.new(oid: oid, size: file.size, file: file)
project.all_lfs_objects << lfs_object project.all_lfs_objects << lfs_object
end end
rescue Gitlab::UrlBlocker::BlockedUrlError => e
Rails.logger.error("LFS file with oid #{oid} couldn't be downloaded: #{e.message}")
rescue StandardError => e 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 end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
private 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) 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 end
def headers(sanitized_uri) def headers(sanitized_uri)
{}.tap do |headers| query_options.tap do |headers|
credentials = sanitized_uri.credentials credentials = sanitized_uri.credentials
if credentials[:user].present? || credentials[:password].present? if credentials[:user].present? || credentials[:password].present?
...@@ -43,10 +58,14 @@ module Projects ...@@ -43,10 +58,14 @@ module Projects
end end
end end
def query_options
{ stream_body: true }
end
def with_tmp_file(oid) def with_tmp_file(oid)
create_tmp_storage_dir 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 end
def create_tmp_storage_dir def create_tmp_storage_dir
......
...@@ -4,17 +4,15 @@ describe Projects::LfsPointers::LfsDownloadService do ...@@ -4,17 +4,15 @@ describe Projects::LfsPointers::LfsDownloadService do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:oid) { '9e548e25631dd9ce6b43afd6359ab76da2819d6a5b474e66118c7819e1d8b3e8' } let(:oid) { '9e548e25631dd9ce6b43afd6359ab76da2819d6a5b474e66118c7819e1d8b3e8' }
let(:download_link) { "http://gitlab.com/#{oid}" } let(:download_link) { "http://gitlab.com/#{oid}" }
let(:lfs_content) do let(:lfs_content) { SecureRandom.random_bytes(10) }
<<~HEREDOC
whatever
HEREDOC
end
subject { described_class.new(project) } subject { described_class.new(project) }
before do before do
allow(project).to receive(:lfs_enabled?).and_return(true) allow(project).to receive(:lfs_enabled?).and_return(true)
WebMock.stub_request(:get, download_link).to_return(body: lfs_content) 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 end
describe '#execute' do describe '#execute' do
...@@ -32,7 +30,7 @@ describe Projects::LfsPointers::LfsDownloadService do ...@@ -32,7 +30,7 @@ describe Projects::LfsPointers::LfsDownloadService do
it 'stores the content' do it 'stores the content' do
subject.execute(oid, download_link) 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
end end
...@@ -54,18 +52,61 @@ describe Projects::LfsPointers::LfsDownloadService do ...@@ -54,18 +52,61 @@ describe Projects::LfsPointers::LfsDownloadService do
end end
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 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 with_them do
it 'does not download the file' 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 } expect { subject.execute(oid, download_link) }.not_to change { LfsObject.count }
end end
end 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 context 'when an lfs object with the same oid already exists' do
before do before do
create(:lfs_object, oid: 'oid') 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