Merge branch 'security-56348-11-6' into '11-6-stable'

Check snippet attached file to be moved is within designated directory

See merge request gitlab/gitlabhq!2943

app/uploaders/file_mover.rb

@@ -11,6 +11,8 @@ class FileMover
   end
   def execute
+    return unless valid?
+
     move
     if update_markdown
@@ -21,6 +23,12 @@ class FileMover
   private
+  def valid?
+    Pathname.new(temp_file_path).realpath.to_path.start_with?(
+      (Pathname(temp_file_uploader.root) + temp_file_uploader.base_dir).to_path
+    )
+  end
+
   def move
     FileUtils.mkdir_p(File.dirname(file_path))
     FileUtils.move(temp_file_path, file_path)

changelogs/unreleased/security-56348.yml

+---
+title: Check snippet attached file to be moved is within designated directory
+merge_request:
+author:
+type: security

spec/controllers/snippets_controller_spec.rb

@@ -205,6 +205,8 @@ describe SnippetsController do
     end
     context 'when the snippet description contains a file' do
+      include FileMoverHelpers
+
       let(:picture_file) { '/-/system/temp/secret56/picture.jpg' }
       let(:text_file) { '/-/system/temp/secret78/text.txt' }
       let(:description) do
@@ -215,6 +217,8 @@ describe SnippetsController do
       before do
         allow(FileUtils).to receive(:mkdir_p)
         allow(FileUtils).to receive(:move)
+        stub_file_mover(text_file)
+        stub_file_mover(picture_file)
       end
       subject { create_snippet({ description: description }, { files: [picture_file, text_file] }) }

spec/support/helpers/file_mover_helpers.rb

+# frozen_string_literal: true
+
+module FileMoverHelpers
+  def stub_file_mover(file_path, stub_real_path: nil)
+    file_name = File.basename(file_path)
+    allow(Pathname).to receive(:new).and_call_original
+
+    expect_next_instance_of(Pathname, a_string_including(file_name)) do |pathname|
+      allow(pathname).to receive(:realpath) { stub_real_path || pathname.cleanpath }
+    end
+  end
+end

spec/uploaders/file_mover_spec.rb

 require 'spec_helper'
 describe FileMover do
+  include FileMoverHelpers
+
   let(:filename) { 'banana_sample.gif' }
-  let(:file) { fixture_file_upload(File.join('spec', 'fixtures', filename)) }
   let(:temp_file_path) { File.join('uploads/-/system/temp', 'secret55', filename) }
   let(:temp_description) do
@@ -12,7 +13,7 @@ describe FileMover do
   let(:file_path) { File.join('uploads/-/system/personal_snippet', snippet.id.to_s, 'secret55', filename) }
   let(:snippet) { create(:personal_snippet, description: temp_description) }
-  subject { described_class.new(file_path, snippet).execute }
+  subject { described_class.new(temp_file_path, snippet).execute }
   describe '#execute' do
     before do
@@ -20,6 +21,8 @@ describe FileMover do
       expect(FileUtils).to receive(:move).with(a_string_including(temp_file_path), a_string_including(file_path))
       allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:exists?).and_return(true)
       allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:size).and_return(10)
+
+      stub_file_mover(temp_file_path)
     end
     context 'when move and field update successful' do
@@ -66,4 +69,30 @@ describe FileMover do
       end
     end
   end
+
+  context 'security' do
+    context 'when relative path is involved' do
+      let(:temp_file_path) { File.join('uploads/-/system/temp', '..', 'another_subdir_of_temp') }
+
+      it 'does not trigger move if path is outside designated directory' do
+        stub_file_mover('uploads/-/system/another_subdir_of_temp')
+        expect(FileUtils).not_to receive(:move)
+
+        subject
+
+        expect(snippet.reload.description).to eq(temp_description)
+      end
+    end
+
+    context 'when symlink is involved' do
+      it 'does not trigger move if path is outside designated directory' do
+        stub_file_mover(temp_file_path, stub_real_path: Pathname('/etc'))
+        expect(FileUtils).not_to receive(:move)
+
+        subject
+
+        expect(snippet.reload.description).to eq(temp_description)
+      end
+    end
+  end
 end