Commit d04f4388 authored by GitLab Bot's avatar GitLab Bot
Browse files

Add latest changes from gitlab-org/security/gitlab@12-6-stable-ee

parent 3d240b44
......@@ -9,6 +9,7 @@ module UploadsActions
 
included do
prepend_before_action :set_request_format_from_path_extension
rescue_from FileUploader::InvalidSecret, with: :render_404
end
 
def create
......
......@@ -16,6 +16,9 @@ class FileUploader < GitlabUploader
 
MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*?)\)}.freeze
DYNAMIC_PATH_PATTERN = %r{.*(?<secret>\h{32})/(?<identifier>.*)}.freeze
VALID_SECRET_PATTERN = %r{\A\h{10,32}\z}.freeze
InvalidSecret = Class.new(StandardError)
 
after :remove, :prune_store_dir
 
......@@ -153,6 +156,10 @@ class FileUploader < GitlabUploader
 
def secret
@secret ||= self.class.generate_secret
raise InvalidSecret unless @secret =~ VALID_SECRET_PATTERN
@secret
end
 
# return a new uploader with a file copy on another project
......
---
title: Prevent directory traversal through FileUploader
merge_request:
author:
type: security
......@@ -242,8 +242,10 @@ describe SnippetsController do
context 'when the snippet description contains a file' do
include FileMoverHelpers
 
let(:picture_file) { "/-/system/user/#{user.id}/secret56/picture.jpg" }
let(:text_file) { "/-/system/user/#{user.id}/secret78/text.txt" }
let(:picture_secret) { SecureRandom.hex }
let(:text_secret) { SecureRandom.hex }
let(:picture_file) { "/-/system/user/#{user.id}/#{picture_secret}/picture.jpg" }
let(:text_file) { "/-/system/user/#{user.id}/#{text_secret}/text.txt" }
let(:description) do
"Description with picture: ![picture](/uploads#{picture_file}) and "\
"text: [text.txt](/uploads#{text_file})"
......@@ -266,8 +268,8 @@ describe SnippetsController do
snippet = subject
 
expected_description = "Description with picture: "\
"![picture](/uploads/-/system/personal_snippet/#{snippet.id}/secret56/picture.jpg) and "\
"text: [text.txt](/uploads/-/system/personal_snippet/#{snippet.id}/secret78/text.txt)"
"![picture](/uploads/-/system/personal_snippet/#{snippet.id}/#{picture_secret}/picture.jpg) and "\
"text: [text.txt](/uploads/-/system/personal_snippet/#{snippet.id}/#{text_secret}/text.txt)"
 
expect(snippet.description).to eq(expected_description)
end
......
......@@ -69,13 +69,39 @@ shared_examples 'handle uploads' do
end
 
describe "GET #show" do
let(:filename) { "rails_sample.jpg" }
let(:upload_service) do
UploadService.new(model, jpg, uploader_class).execute
end
let(:show_upload) do
get :show, params: params.merge(secret: secret, filename: "rails_sample.jpg")
get :show, params: params.merge(secret: secret, filename: filename)
end
 
before do
allow(FileUploader).to receive(:generate_secret).and_return(secret)
UploadService.new(model, jpg, uploader_class).execute
upload_service
end
context 'when the secret is invalid' do
let(:secret) { "../../../../../../../../" }
let(:filename) { "Gemfile.lock" }
let(:upload_service) { nil }
it 'responds with status 404' do
show_upload
expect(response).to have_gitlab_http_status(:not_found)
end
it 'is a working exploit without the validation' do
allow_any_instance_of(FileUploader).to receive(:secret) { secret }
show_upload
expect(response).to have_gitlab_http_status(:ok)
end
end
 
context 'when accessing a specific upload via different model' do
......
......@@ -7,7 +7,7 @@ describe FileMover do
 
let(:user) { create(:user) }
let(:filename) { 'banana_sample.gif' }
let(:secret) { 'secret55' }
let(:secret) { SecureRandom.hex }
let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", secret, filename) }
 
let(:temp_description) do
......@@ -47,8 +47,8 @@ describe FileMover do
subject
 
expect(snippet.reload.description)
.to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\
"same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) ")
.to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) "\
"same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) ")
end
 
it 'updates existing upload record' do
......@@ -75,8 +75,8 @@ describe FileMover do
subject
 
expect(snippet.reload.description)
.to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\
"same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) ")
.to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) "\
"same ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) ")
end
 
it 'does not change the upload record' do
......@@ -101,8 +101,8 @@ describe FileMover do
subject
 
expect(snippet.reload.description)
.to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\
"same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) ")
.to eq("test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) "\
"same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/#{secret}/banana_sample.gif) ")
end
 
it 'creates new target upload record an delete the old upload' do
......@@ -121,8 +121,8 @@ describe FileMover do
subject
 
expect(snippet.reload.description)
.to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) "\
"same ![banana_sample](/uploads/-/system/user/#{user.id}/secret55/banana_sample.gif) ")
.to eq("test ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) "\
"same ![banana_sample](/uploads/-/system/user/#{user.id}/#{secret}/banana_sample.gif) ")
end
 
it 'does not change the upload record' do
......@@ -138,12 +138,8 @@ describe FileMover do
let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", '..', 'another_subdir_of_temp') }
 
it 'does not trigger move if path is outside designated directory' do
stub_file_mover("uploads/-/system/user/#{user.id}/another_subdir_of_temp")
expect(FileUtils).not_to receive(:move)
subject
expect(snippet.reload.description).to eq(temp_description)
expect { subject }.to raise_error(FileUploader::InvalidSecret)
end
end
 
......
......@@ -6,7 +6,8 @@ describe FileUploader do
let(:group) { create(:group, name: 'awesome') }
let(:project) { create(:project, :legacy_storage, namespace: group, name: 'project') }
let(:uploader) { described_class.new(project, :avatar) }
let(:upload) { double(model: project, path: 'secret/foo.jpg') }
let(:upload) { double(model: project, path: "#{secret}/foo.jpg") }
let(:secret) { "55dc16aa0edd05693fd98b5051e83321" } # this would be nicer as SecureRandom.hex, but the shared_examples breaks
 
subject { uploader }
 
......@@ -14,7 +15,7 @@ describe FileUploader do
include_examples 'builds correct paths',
store_dir: %r{awesome/project/\h+},
upload_path: %r{\h+/<filename>},
absolute_path: %r{#{described_class.root}/awesome/project/secret/foo.jpg}
absolute_path: %r{#{described_class.root}/awesome/project/55dc16aa0edd05693fd98b5051e83321/foo.jpg}
end
 
context 'legacy storage' do
......@@ -51,11 +52,11 @@ describe FileUploader do
end
 
describe 'initialize' do
let(:uploader) { described_class.new(double, secret: 'secret') }
let(:uploader) { described_class.new(double, secret: secret) }
 
it 'accepts a secret parameter' do
expect(described_class).not_to receive(:generate_secret)
expect(uploader.secret).to eq('secret')
expect(uploader.secret).to eq(secret)
end
end
 
......@@ -154,8 +155,39 @@ describe FileUploader do
 
describe '#secret' do
it 'generates a secret if none is provided' do
expect(described_class).to receive(:generate_secret).and_return('secret')
expect(uploader.secret).to eq('secret')
expect(described_class).to receive(:generate_secret).and_return(secret)
expect(uploader.secret).to eq(secret)
expect(uploader.secret.size).to eq(32)
end
context "validation" do
before do
uploader.instance_variable_set(:@secret, secret)
end
context "32-byte hexadecimal" do
let(:secret) { SecureRandom.hex }
it "returns the secret" do
expect(uploader.secret).to eq(secret)
end
end
context "10-byte hexadecimal" do
let(:secret) { SecureRandom.hex(10) }
it "returns the secret" do
expect(uploader.secret).to eq(secret)
end
end
context "invalid secret supplied" do
let(:secret) { "%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2Fgrafana%2Fconf%2F" }
it "raises an exception" do
expect { uploader.secret }.to raise_error(described_class::InvalidSecret)
end
end
end
end
 
......
......@@ -6,12 +6,13 @@ describe PersonalFileUploader do
let(:model) { create(:personal_snippet) }
let(:uploader) { described_class.new(model) }
let(:upload) { create(:upload, :personal_snippet_upload) }
let(:secret) { SecureRandom.hex }
 
subject { uploader }
 
shared_examples '#base_dir' do
before do
subject.instance_variable_set(:@secret, 'secret')
subject.instance_variable_set(:@secret, secret)
end
 
it 'is prefixed with uploads/-/system' do
......@@ -23,12 +24,12 @@ describe PersonalFileUploader do
 
shared_examples '#to_h' do
before do
subject.instance_variable_set(:@secret, 'secret')
subject.instance_variable_set(:@secret, secret)
end
 
it 'is correct' do
allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name'))
expected_url = "/uploads/-/system/personal_snippet/#{model.id}/secret/file_name"
expected_url = "/uploads/-/system/personal_snippet/#{model.id}/#{secret}/file_name"
 
expect(uploader.to_h).to eq(
alt: 'file_name',
......
File mode changed from 100644 to 100755
File mode changed from 100644 to 100755
Supports Markdown
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