Validate LFS hrefs before downloading them

3 files changed, 20 insertions, 0 deletions

diff --git a/app/services/projects/lfs_pointers/lfs_download_service.rb b/app/services/projects/lfs_pointers/lfs_download_service.rb
index 1c4a8d05be6..f9b9781ad5f 100644
--- a/app/services/projects/lfs_pointers/lfs_download_service.rb
+++ b/app/services/projects/lfs_pointers/lfs_download_service.rb
@@ -4,6 +4,8 @@
 module Projects
   module LfsPointers
     class LfsDownloadService < BaseService
+      VALID_PROTOCOLS = %w[http https].freeze
+
       # rubocop: disable CodeReuse/ActiveRecord
       def execute(oid, url)
         return unless project&.lfs_enabled? && oid.present? && url.present?
@@ -11,6 +13,7 @@ module Projects
         return if LfsObject.exists?(oid: oid)
 
         sanitized_uri = Gitlab::UrlSanitizer.new(url)
+        Gitlab::UrlBlocker.validate!(sanitized_uri.sanitized_url, protocols: VALID_PROTOCOLS)
 
         with_tmp_file(oid) do |file|
           size = download_and_save_file(file, sanitized_uri)
diff --git a/changelogs/unreleased/security-2754-fix-lfs-import.yml b/changelogs/unreleased/security-2754-fix-lfs-import.yml
new file mode 100644
index 00000000000..e8e74c9c3f6
--- /dev/null
+++ b/changelogs/unreleased/security-2754-fix-lfs-import.yml
@@ -0,0 +1,5 @@
+---
+title: Validate LFS hrefs before downloading them
+merge_request:
+author:
+type: security
diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb
index 6af5bfc7689..d7d7f1874eb 100644
--- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb
+++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb
@@ -54,6 +54,18 @@ describe Projects::LfsPointers::LfsDownloadService do
       end
     end
 
+    context 'when a bad URL is used' do
+      where(download_link: ['/etc/passwd', 'ftp://example.com', 'http://127.0.0.2'])
+
+      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 an lfs object with the same oid already exists'  do
       before do
         create(:lfs_object, oid: 'oid')