diff options
| -rw-r--r-- | app/uploaders/file_uploader.rb | 7 | ||||
| -rw-r--r-- | lib/gitlab/gfm/uploads_rewriter.rb | 49 | ||||
| -rw-r--r-- | spec/lib/gitlab/gfm/uploads_rewriter_spec.rb | 29 | ||||
| -rw-r--r-- | spec/services/notes/copy_service_spec.rb | 6 |
4 files changed, 67 insertions, 24 deletions
diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index bf5be708060..7250ce5c0b0 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -14,7 +14,12 @@ class FileUploader < GitlabUploader include ObjectStorage::Concern prepend ObjectStorage::Extension::RecordsUploads - MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*?)\)}.freeze + # This pattern is vulnerable to malicious inputs, so use Gitlab::UntrustedRegexp + # to place bounds on execution time + MARKDOWN_PATTERN = Gitlab::UntrustedRegexp.new( + '!?\[.*?\]\(/uploads/(?P<secret>[0-9a-f]{32})/(?P<file>.*?)\)' + ) + DYNAMIC_PATH_PATTERN = %r{.*(?<secret>\b(\h{10}|\h{32}))\/(?<identifier>.*)}.freeze VALID_SECRET_PATTERN = %r{\A\h{10,32}\z}.freeze diff --git a/lib/gitlab/gfm/uploads_rewriter.rb b/lib/gitlab/gfm/uploads_rewriter.rb index b0bf68f4204..58b46a85aae 100644 --- a/lib/gitlab/gfm/uploads_rewriter.rb +++ b/lib/gitlab/gfm/uploads_rewriter.rb @@ -23,33 +23,24 @@ module Gitlab def rewrite(target_parent) return @text unless needs_rewrite? - @text.gsub!(@pattern) do |markdown| - file = find_file($~[:secret], $~[:file]) - # No file will be returned for a path traversal - next if file.nil? + @target_parent = target_parent - break markdown unless file.try(:exists?) - - klass = target_parent.is_a?(Namespace) ? NamespaceFileUploader : FileUploader - moved = klass.copy_to(file, target_parent) - - moved_markdown = moved.markdown_link - - # Prevents rewrite of plain links as embedded - if was_embedded?(markdown) - moved_markdown - else - moved_markdown.delete_prefix('!') - end + rewritten_text = Gitlab::StringRegexMarker.new(@text).mark(@pattern) do |markdown, left:, right:, mode:| + transform_markdown(markdown) end + + # MarkdownContentRewriterService relies on the text being changed _in place_. + @text.gsub!(@text, rewritten_text) end def needs_rewrite? strong_memoize(:needs_rewrite) do - FileUploader::MARKDOWN_PATTERN.match?(@text) + @pattern.match?(@text) end end + private + def was_embedded?(markdown) markdown.starts_with?("!") end @@ -57,6 +48,28 @@ module Gitlab def find_file(secret, file_name) UploaderFinder.new(@source_project, secret, file_name).execute end + + def transform_markdown(markdown) + match = @pattern.match(markdown) + file = find_file(match[:secret], match[:file]) + + # No file will be returned for a path traversal + return '' if file.nil? + + return markdown unless file.try(:exists?) + + klass = @target_parent.is_a?(Namespace) ? NamespaceFileUploader : FileUploader + moved = klass.copy_to(file, @target_parent) + + moved_markdown = moved.markdown_link + + # Prevents rewrite of plain links as embedded + if was_embedded?(markdown) + moved_markdown + else + moved_markdown.delete_prefix('!') + end + end end end end diff --git a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb index a16f96a7d11..b1bff242f33 100644 --- a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb +++ b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb @@ -23,8 +23,9 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do end def referenced_files(text, project) - referenced_files = text.scan(FileUploader::MARKDOWN_PATTERN).map do - UploaderFinder.new(project, $~[:secret], $~[:file]).execute + scanner = FileUploader::MARKDOWN_PATTERN.scan(text) + referenced_files = scanner.map do |match| + UploaderFinder.new(project, match[0], match[1]).execute end referenced_files.compact.select(&:exists?) @@ -32,7 +33,9 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do shared_examples "files are accessible" do describe '#rewrite' do - let!(:new_text) { rewriter.rewrite(new_project) } + subject(:rewrite) { new_text } + + let(:new_text) { rewriter.rewrite(new_project) } let(:old_files) { [image_uploader, zip_uploader] } let(:new_files) do @@ -43,11 +46,15 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do let(:new_paths) { new_files.map(&:path) } it 'rewrites content' do + rewrite + expect(new_text).not_to eq text expect(new_text.length).to eq text.length end it 'copies files' do + rewrite + expect(new_files).to all(exist) expect(old_paths).not_to match_array new_paths expect(old_paths).to all(include(old_project.disk_path)) @@ -55,10 +62,14 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do end it 'does not remove old files' do + rewrite + expect(old_files).to all(exist) end it 'generates a new secret for each file' do + rewrite + expect(new_paths).not_to include image_uploader.secret expect(new_paths).not_to include zip_uploader.secret end @@ -68,6 +79,8 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do allow(finder).to receive(:execute).and_return(nil) end + rewrite + expect(new_files).to be_empty end end @@ -84,6 +97,16 @@ RSpec.describe Gitlab::Gfm::UploadsRewriter do expect(moved_text.scan(/\A\[.*?\]/).count).to eq(1) end + it 'does not casue a timeout on pathological text' do + text = '[!l' * 30000 + + Timeout.timeout(3) do + moved_text = described_class.new(text, nil, old_project, user).rewrite(new_project) + + expect(moved_text).to eq(text) + end + end + context "file are stored locally" do include_examples "files are accessible" end diff --git a/spec/services/notes/copy_service_spec.rb b/spec/services/notes/copy_service_spec.rb index f146a49e929..2fa9a462bb9 100644 --- a/spec/services/notes/copy_service_spec.rb +++ b/spec/services/notes/copy_service_spec.rb @@ -146,8 +146,10 @@ RSpec.describe Notes::CopyService do new_note = to_noteable.notes.first aggregate_failures do - expect(note.note).to match(/Simple text with image: #{FileUploader::MARKDOWN_PATTERN}/o) - expect(new_note.note).to match(/Simple text with image: #{FileUploader::MARKDOWN_PATTERN}/o) + expect(note.note).to match(/Simple text with image:/o) + expect(FileUploader::MARKDOWN_PATTERN.match(note.note)).not_to be_nil + expect(new_note.note).to match(/Simple text with image:/o) + expect(FileUploader::MARKDOWN_PATTERN.match(new_note.note)).not_to be_nil expect(note.note).not_to eq(new_note.note) expect(note.note_html).not_to eq(new_note.note_html) end |