Merge branch 'security-fix-markdown-xss-12-2' into '12-2-stable'

Re-escape the whole HTML content when finding HTML references

See merge request gitlab/gitlabhq!3370

4 files changed, 35 insertions, 13 deletions

diff --git a/lib/banzai/filter/abstract_reference_filter.rb b/lib/banzai/filter/abstract_reference_filter.rb
index 52af28ce8ec..a0439089879 100644
--- a/lib/banzai/filter/abstract_reference_filter.rb
+++ b/lib/banzai/filter/abstract_reference_filter.rb
@@ -7,6 +7,14 @@ module Banzai
     class AbstractReferenceFilter < ReferenceFilter
       include CrossProjectReference
 
+      # REFERENCE_PLACEHOLDER is used for re-escaping HTML text except found
+      # reference (which we replace with placeholder during re-scaping).  The
+      # random number helps ensure it's pretty close to unique. Since it's a
+      # transitory value (it never gets saved) we can initialize once, and it
+      # doesn't matter if it changes on a restart.
+      REFERENCE_PLACEHOLDER = "_reference_#{SecureRandom.hex(16)}_"
+      REFERENCE_PLACEHOLDER_PATTERN = %r{#{REFERENCE_PLACEHOLDER}(\d+)}.freeze
+
       def self.object_class
         # Implement in child class
         # Example: MergeRequest
@@ -389,6 +397,14 @@ module Banzai
       def escape_html_entities(text)
         CGI.escapeHTML(text.to_s)
       end
+
+      def escape_with_placeholders(text, placeholder_data)
+        escaped = escape_html_entities(text)
+
+        escaped.gsub(REFERENCE_PLACEHOLDER_PATTERN) do |match|
+          placeholder_data[$1.to_i]
+        end
+      end
     end
   end
 end
diff --git a/lib/banzai/filter/label_reference_filter.rb b/lib/banzai/filter/label_reference_filter.rb
index 4892668fc22..a0789b7ca06 100644
--- a/lib/banzai/filter/label_reference_filter.rb
+++ b/lib/banzai/filter/label_reference_filter.rb
@@ -14,24 +14,24 @@ module Banzai
         find_labels(parent_object).find(id)
       end
 
-      def self.references_in(text, pattern = Label.reference_pattern)
-        unescape_html_entities(text).gsub(pattern) do |match|
-          yield match, $~[:label_id].to_i, $~[:label_name], $~[:project], $~[:namespace], $~
-        end
-      end
-
       def references_in(text, pattern = Label.reference_pattern)
-        unescape_html_entities(text).gsub(pattern) do |match|
+        labels = {}
+        unescaped_html = unescape_html_entities(text).gsub(pattern) do |match|
           namespace, project = $~[:namespace], $~[:project]
           project_path = full_project_path(namespace, project)
           label = find_label(project_path, $~[:label_id], $~[:label_name])
 
           if label
-            yield match, label.id, project, namespace, $~
+            labels[label.id] = yield match, label.id, project, namespace, $~
+            "#{REFERENCE_PLACEHOLDER}#{label.id}"
           else
-            escape_html_entities(match)
+            match
           end
         end
+
+        return text if labels.empty?
+
+        escape_with_placeholders(unescaped_html, labels)
       end
 
       def find_label(parent_ref, label_id, label_name)
diff --git a/lib/banzai/filter/milestone_reference_filter.rb b/lib/banzai/filter/milestone_reference_filter.rb
index 08969753d75..4c47ee4dba1 100644
--- a/lib/banzai/filter/milestone_reference_filter.rb
+++ b/lib/banzai/filter/milestone_reference_filter.rb
@@ -51,15 +51,21 @@ module Banzai
         # default implementation.
         return super(text, pattern) if pattern != Milestone.reference_pattern
 
-        unescape_html_entities(text).gsub(pattern) do |match|
+        milestones = {}
+        unescaped_html = unescape_html_entities(text).gsub(pattern) do |match|
           milestone = find_milestone($~[:project], $~[:namespace], $~[:milestone_iid], $~[:milestone_name])
 
           if milestone
-            yield match, milestone.id, $~[:project], $~[:namespace], $~
+            milestones[milestone.id] = yield match, milestone.id, $~[:project], $~[:namespace], $~
+            "#{REFERENCE_PLACEHOLDER}#{milestone.id}"
           else
-            escape_html_entities(match)
+            match
           end
         end
+
+        return text if milestones.empty?
+
+        escape_with_placeholders(unescaped_html, milestones)
       end
 
       def find_milestone(project_ref, namespace_ref, milestone_id, milestone_name)
diff --git a/lib/gitlab/markdown_cache.rb b/lib/gitlab/markdown_cache.rb
index 0354c710a3f..03a2f62cbd9 100644
--- a/lib/gitlab/markdown_cache.rb
+++ b/lib/gitlab/markdown_cache.rb
@@ -3,8 +3,8 @@
 module Gitlab
   module MarkdownCache
     # Increment this number every time the renderer changes its output
+    CACHE_COMMONMARK_VERSION        = 17
     CACHE_COMMONMARK_VERSION_START  = 10
-    CACHE_COMMONMARK_VERSION        = 16
 
     BaseError = Class.new(StandardError)
     UnsupportedClassError = Class.new(BaseError)