Add latest changes from gitlab-org/security/gitlab@15-3-stable-ee

8 files changed, 111 insertions, 42 deletions

diff --git a/app/models/hooks/web_hook_log.rb b/app/models/hooks/web_hook_log.rb
index 24e5f193a32..82ae71213da 100644
--- a/app/models/hooks/web_hook_log.rb
+++ b/app/models/hooks/web_hook_log.rb
@@ -22,7 +22,7 @@ class WebHookLog < ApplicationRecord
   validates :web_hook, presence: true
 
   before_save :obfuscate_basic_auth
-  before_save :redact_author_email
+  before_save :redact_user_emails
 
   def self.recent
     where('created_at >= ?', 2.days.ago.beginning_of_day)
@@ -54,9 +54,9 @@ class WebHookLog < ApplicationRecord
     self.url = safe_url
   end
 
-  def redact_author_email
-    return unless self.request_data.dig('commit', 'author', 'email').present?
-
-    self.request_data['commit']['author']['email'] = _('[REDACTED]')
+  def redact_user_emails
+    self.request_data.deep_transform_values! do |value|
+      value =~ URI::MailTo::EMAIL_REGEXP ? _('[REDACTED]') : value
+    end
   end
 end
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/ci/ansi2json/line.rb b/lib/gitlab/ci/ansi2json/line.rb
index e48080993ab..abe2f272ca7 100644
--- a/lib/gitlab/ci/ansi2json/line.rb
+++ b/lib/gitlab/ci/ansi2json/line.rb
@@ -80,7 +80,8 @@ module Gitlab
         end
 
         def set_section_duration(duration_in_seconds)
-          duration = ActiveSupport::Duration.build(duration_in_seconds.to_i)
+          normalized_duration_in_seconds = duration_in_seconds.to_i.clamp(0, 1.year)
+          duration = ActiveSupport::Duration.build(normalized_duration_in_seconds)
           hours = duration.in_hours.floor
           hours = hours > 0 ? "%02d" % hours : nil
           minutes = "%02d" % duration.parts[:minutes].to_i
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/ci/ansi2json/line_spec.rb b/spec/lib/gitlab/ci/ansi2json/line_spec.rb
index d16750d19f1..b8563bb1d1c 100644
--- a/spec/lib/gitlab/ci/ansi2json/line_spec.rb
+++ b/spec/lib/gitlab/ci/ansi2json/line_spec.rb
@@ -87,6 +87,8 @@ RSpec.describe Gitlab::Ci::Ansi2json::Line do
       1.minute + 15.seconds                       | '01:15'
       13.hours + 14.minutes + 15.seconds          | '13:14:15'
       1.day + 13.hours + 14.minutes + 15.seconds  | '37:14:15'
+      Float::MAX                                  | '8765:00:00'
+      10**10000                                   | '8765:00:00'
     end
 
     with_them do
diff --git a/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb b/spec/lib/gitlab/gfm/uploads_rewriter_spec.rb
index 763e6f1b5f4..5ce7afa6200 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/models/hooks/web_hook_log_spec.rb b/spec/models/hooks/web_hook_log_spec.rb
index 8ff8a1c3865..3441dfda7d6 100644
--- a/spec/models/hooks/web_hook_log_spec.rb
+++ b/spec/models/hooks/web_hook_log_spec.rb
@@ -44,26 +44,49 @@ RSpec.describe WebHookLog do
       end
     end
 
-    context 'with author email' do
+    context "with users' emails" do
       let(:author) { create(:user) }
+      let(:user) { create(:user) }
       let(:web_hook_log) { create(:web_hook_log, request_data: data) }
       let(:data) do
         {
-          commit: {
-            author: {
-              name: author.name,
-              email: author.email
+          user: {
+            name: user.name,
+            email: user.email
+          },
+          commits: [
+            {
+              user: {
+                name: author.name,
+                email: author.email
+              }
+            },
+            {
+              user: {
+                name: user.name,
+                email: user.email
+              }
             }
-          }
+          ]
         }.deep_stringify_keys
       end
 
-      it "redacts author's email" do
-        expect(web_hook_log.request_data['commit']).to match a_hash_including(
-          'author' => {
-            'name' => author.name,
-            'email' => _('[REDACTED]')
-          }
+      it "redacts users' emails" do
+        expect(web_hook_log.request_data['user']).to match a_hash_including(
+          'name' => user.name,
+          'email' => _('[REDACTED]')
+        )
+        expect(web_hook_log.request_data['commits'].pluck('user')).to match_array(
+          [
+            {
+              'name' => author.name,
+              'email' => _('[REDACTED]')
+            },
+            {
+              'name' => user.name,
+              'email' => _('[REDACTED]')
+            }
+          ]
         )
       end
     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