From d72b1cd0b5b01d6fec6b93d9dfe84f8302083072 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Wed, 13 Feb 2019 16:24:26 +0800
Subject: [PATCH] Check snippet attached file to be moved is within designated
 directory

Previously one could move any temp/ sub folder around.
---
 app/uploaders/file_mover.rb                  |  8 ++++++
 changelogs/unreleased/security-56348.yml     |  5 ++++
 spec/controllers/snippets_controller_spec.rb |  4 +++
 spec/support/helpers/file_mover_helpers.rb   | 12 ++++++++
 spec/uploaders/file_mover_spec.rb            | 30 ++++++++++++++++++++
 5 files changed, 59 insertions(+)
 create mode 100644 changelogs/unreleased/security-56348.yml
 create mode 100644 spec/support/helpers/file_mover_helpers.rb

diff --git a/app/uploaders/file_mover.rb b/app/uploaders/file_mover.rb
index a7f8615e9ba..236b7ed2b3d 100644
--- a/app/uploaders/file_mover.rb
+++ b/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)
diff --git a/changelogs/unreleased/security-56348.yml b/changelogs/unreleased/security-56348.yml
new file mode 100644
index 00000000000..a289e4e9077
--- /dev/null
+++ b/changelogs/unreleased/security-56348.yml
@@ -0,0 +1,5 @@
+---
+title: Check snippet attached file to be moved is within designated directory
+merge_request:
+author:
+type: security
diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb
index 5c6858dc7b2..77a94f26d8c 100644
--- a/spec/controllers/snippets_controller_spec.rb
+++ b/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] }) }
diff --git a/spec/support/helpers/file_mover_helpers.rb b/spec/support/helpers/file_mover_helpers.rb
new file mode 100644
index 00000000000..1ba7cc03354
--- /dev/null
+++ b/spec/support/helpers/file_mover_helpers.rb
@@ -0,0 +1,12 @@
+# 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
diff --git a/spec/uploaders/file_mover_spec.rb b/spec/uploaders/file_mover_spec.rb
index a28d7445b1c..e474a714b10 100644
--- a/spec/uploaders/file_mover_spec.rb
+++ b/spec/uploaders/file_mover_spec.rb
@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 describe FileMover do
+  include FileMoverHelpers
+
   let(:filename) { 'banana_sample.gif' }
   let(:temp_file_path) { File.join('uploads/-/system/temp', 'secret55', filename) }
 
@@ -19,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
@@ -65,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