Commit 9e4a9cda authored by Yorick Peterse's avatar Yorick Peterse

Merge branch 'security-56348' into 'master'

Check snippet attached file to be moved is within designated directory

Closes #2806

See merge request gitlab/gitlabhq!2903
parents fa2f136f d72b1cd0
...@@ -11,6 +11,8 @@ class FileMover ...@@ -11,6 +11,8 @@ class FileMover
end end
def execute def execute
return unless valid?
move move
if update_markdown if update_markdown
...@@ -21,6 +23,12 @@ class FileMover ...@@ -21,6 +23,12 @@ class FileMover
private 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 def move
FileUtils.mkdir_p(File.dirname(file_path)) FileUtils.mkdir_p(File.dirname(file_path))
FileUtils.move(temp_file_path, 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 ...@@ -205,6 +205,8 @@ describe SnippetsController do
end end
context 'when the snippet description contains a file' do context 'when the snippet description contains a file' do
include FileMoverHelpers
let(:picture_file) { '/-/system/temp/secret56/picture.jpg' } let(:picture_file) { '/-/system/temp/secret56/picture.jpg' }
let(:text_file) { '/-/system/temp/secret78/text.txt' } let(:text_file) { '/-/system/temp/secret78/text.txt' }
let(:description) do let(:description) do
...@@ -215,6 +217,8 @@ describe SnippetsController do ...@@ -215,6 +217,8 @@ describe SnippetsController do
before do before do
allow(FileUtils).to receive(:mkdir_p) allow(FileUtils).to receive(:mkdir_p)
allow(FileUtils).to receive(:move) allow(FileUtils).to receive(:move)
stub_file_mover(text_file)
stub_file_mover(picture_file)
end end
subject { create_snippet({ description: description }, { files: [picture_file, text_file] }) } 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' require 'spec_helper'
describe FileMover do describe FileMover do
include FileMoverHelpers
let(:filename) { 'banana_sample.gif' } 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_file_path) { File.join('uploads/-/system/temp', 'secret55', filename) }
let(:temp_description) do let(:temp_description) do
...@@ -12,7 +13,7 @@ describe FileMover do ...@@ -12,7 +13,7 @@ describe FileMover do
let(:file_path) { File.join('uploads/-/system/personal_snippet', snippet.id.to_s, 'secret55', filename) } let(:file_path) { File.join('uploads/-/system/personal_snippet', snippet.id.to_s, 'secret55', filename) }
let(:snippet) { create(:personal_snippet, description: temp_description) } 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 describe '#execute' do
before do before do
...@@ -20,6 +21,8 @@ describe FileMover 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)) 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(:exists?).and_return(true)
allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:size).and_return(10) allow_any_instance_of(CarrierWave::SanitizedFile).to receive(:size).and_return(10)
stub_file_mover(temp_file_path)
end end
context 'when move and field update successful' do context 'when move and field update successful' do
...@@ -66,4 +69,30 @@ describe FileMover do ...@@ -66,4 +69,30 @@ describe FileMover do
end end
end 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 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