Commit d72b1cd0 authored by Mark Chao's avatar Mark Chao

Check snippet attached file to be moved is within designated directory

Previously one could move any temp/ sub folder around.
parent a9291f15
......@@ -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)
......
---
title: Check snippet attached file to be moved is within designated directory
merge_request:
author:
type: security
......@@ -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] }) }
......
# 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
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
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