diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-03-04 09:52:59 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-03-04 09:52:59 +0000 |
commit | d63102281bfdb73429f23730d652027e03dfac26 (patch) | |
tree | 678001d81cf726bc44321ed1aec06e99f1bfee5e | |
parent | 89b713ed471c6a9236f9fa819b5d9f78ebae5274 (diff) | |
download | gitlab-ce-d63102281bfdb73429f23730d652027e03dfac26.tar.gz |
Add latest changes from gitlab-org/security/gitlab@12-8-stable-ee
-rw-r--r-- | app/controllers/concerns/uploads_actions.rb | 1 | ||||
-rw-r--r-- | app/uploaders/file_uploader.rb | 7 | ||||
-rw-r--r-- | changelogs/unreleased/security-709-secret-traversal.yml | 5 | ||||
-rw-r--r-- | spec/controllers/snippets_controller_spec.rb | 10 | ||||
-rw-r--r-- | spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb | 30 | ||||
-rw-r--r-- | spec/uploaders/file_mover_spec.rb | 24 | ||||
-rw-r--r-- | spec/uploaders/file_uploader_spec.rb | 44 | ||||
-rw-r--r-- | spec/uploaders/personal_file_uploader_spec.rb | 7 | ||||
-rwxr-xr-x[-rw-r--r--] | vendor/gitignore/C++.gitignore | 0 | ||||
-rwxr-xr-x[-rw-r--r--] | vendor/gitignore/Java.gitignore | 0 |
10 files changed, 99 insertions, 29 deletions
diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index 655575e0944..549a443b1a8 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/app/controllers/concerns/uploads_actions.rb @@ -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 diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 0fc71d2e3f3..505b51c2006 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -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 diff --git a/changelogs/unreleased/security-709-secret-traversal.yml b/changelogs/unreleased/security-709-secret-traversal.yml new file mode 100644 index 00000000000..33944712a20 --- /dev/null +++ b/changelogs/unreleased/security-709-secret-traversal.yml @@ -0,0 +1,5 @@ +--- +title: Prevent directory traversal through FileUploader +merge_request: +author: +type: security diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index daa560649f0..66fb1ef8129 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -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 diff --git a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb index 73087befad2..662c64647d6 100644 --- a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb @@ -69,13 +69,39 @@ RSpec.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 diff --git a/spec/uploaders/file_mover_spec.rb b/spec/uploaders/file_mover_spec.rb index 9f053f03b0e..474515b537c 100644 --- a/spec/uploaders/file_mover_spec.rb +++ b/spec/uploaders/file_mover_spec.rb @@ -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 diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index beb7aa3cf2c..efdbea85d4a 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -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 diff --git a/spec/uploaders/personal_file_uploader_spec.rb b/spec/uploaders/personal_file_uploader_spec.rb index ec652af222d..c211ec3607c 100644 --- a/spec/uploaders/personal_file_uploader_spec.rb +++ b/spec/uploaders/personal_file_uploader_spec.rb @@ -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', diff --git a/vendor/gitignore/C++.gitignore b/vendor/gitignore/C++.gitignore index 259148fa18f..259148fa18f 100644..100755 --- a/vendor/gitignore/C++.gitignore +++ b/vendor/gitignore/C++.gitignore diff --git a/vendor/gitignore/Java.gitignore b/vendor/gitignore/Java.gitignore index a1c2a238a96..a1c2a238a96 100644..100755 --- a/vendor/gitignore/Java.gitignore +++ b/vendor/gitignore/Java.gitignore |