diff options
| -rw-r--r-- | app/uploaders/file_mover.rb | 8 | ||||
| -rw-r--r-- | changelogs/unreleased/security-56348.yml | 5 | ||||
| -rw-r--r-- | spec/controllers/snippets_controller_spec.rb | 4 | ||||
| -rw-r--r-- | spec/support/helpers/file_mover_helpers.rb | 12 | ||||
| -rw-r--r-- | spec/uploaders/file_mover_spec.rb | 30 |
5 files changed, 59 insertions, 0 deletions
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 |