From 8629d5822a1a7af5708ebb785982b25e0d2400bf Mon Sep 17 00:00:00 2001
From: Robert Speicher <robert@gitlab.com>
Date: Thu, 31 Aug 2017 15:27:34 +0000
Subject: Merge branch 'rs-issue-36098' into 'security-9-5'

[9.5] Limit `style` attribute on `th` and `td` elements to specific properties

See merge request gitlab/gitlabhq!2155
---
 changelogs/unreleased/rs-issue-36098.yml           |  4 ++++
 lib/banzai/filter/sanitization_filter.rb           | 22 +++++++++++++++++++++-
 spec/lib/banzai/filter/sanitization_filter_spec.rb | 20 +++++++++++++++++---
 3 files changed, 42 insertions(+), 4 deletions(-)
 create mode 100644 changelogs/unreleased/rs-issue-36098.yml

diff --git a/changelogs/unreleased/rs-issue-36098.yml b/changelogs/unreleased/rs-issue-36098.yml
new file mode 100644
index 00000000000..56b92705a80
--- /dev/null
+++ b/changelogs/unreleased/rs-issue-36098.yml
@@ -0,0 +1,4 @@
+---
+title: Disallow arbitrary properties in `th` and `td` `style` attributes
+merge_request:
+author:
diff --git a/lib/banzai/filter/sanitization_filter.rb b/lib/banzai/filter/sanitization_filter.rb
index 2d6e8ffc90f..768baa4e227 100644
--- a/lib/banzai/filter/sanitization_filter.rb
+++ b/lib/banzai/filter/sanitization_filter.rb
@@ -5,6 +5,7 @@ module Banzai
     # Extends HTML::Pipeline::SanitizationFilter with a custom whitelist.
     class SanitizationFilter < HTML::Pipeline::SanitizationFilter
       UNSAFE_PROTOCOLS = %w(data javascript vbscript).freeze
+      TABLE_ALIGNMENT_PATTERN = /text-align: (?<alignment>center|left|right)/
 
       def whitelist
         whitelist = super
@@ -24,7 +25,8 @@ module Banzai
         # Only push these customizations once
         return if customized?(whitelist[:transformers])
 
-        # Allow table alignment
+        # Allow table alignment; we whitelist specific style properties in a
+        # transformer below
         whitelist[:attributes]['th'] = %w(style)
         whitelist[:attributes]['td'] = %w(style)
 
@@ -52,6 +54,9 @@ module Banzai
         # Remove `rel` attribute from `a` elements
         whitelist[:transformers].push(self.class.remove_rel)
 
+        # Remove any `style` properties not required for table alignment
+        whitelist[:transformers].push(self.class.remove_unsafe_table_style)
+
         whitelist
       end
 
@@ -81,6 +86,21 @@ module Banzai
             end
           end
         end
+
+        def remove_unsafe_table_style
+          lambda do |env|
+            node = env[:node]
+
+            return unless node.name == 'th' || node.name == 'td'
+            return unless node.has_attribute?('style')
+
+            if node['style'] =~ TABLE_ALIGNMENT_PATTERN
+              node['style'] = "text-align: #{$~[:alignment]}"
+            else
+              node.remove_attribute('style')
+            end
+          end
+        end
       end
     end
   end
diff --git a/spec/lib/banzai/filter/sanitization_filter_spec.rb b/spec/lib/banzai/filter/sanitization_filter_spec.rb
index 35a32a46eff..659b4460fc3 100644
--- a/spec/lib/banzai/filter/sanitization_filter_spec.rb
+++ b/spec/lib/banzai/filter/sanitization_filter_spec.rb
@@ -49,7 +49,7 @@ describe Banzai::Filter::SanitizationFilter do
       instance = described_class.new('Foo')
       3.times { instance.whitelist }
 
-      expect(instance.whitelist[:transformers].size).to eq 4
+      expect(instance.whitelist[:transformers].size).to eq 5
     end
 
     it 'sanitizes `class` attribute from all elements' do
@@ -63,8 +63,8 @@ describe Banzai::Filter::SanitizationFilter do
       expect(filter(act).to_html).to eq %q{<span>def</span>}
     end
 
-    it 'allows `style` attribute on table elements' do
-      html = <<-HTML.strip_heredoc
+    it 'allows `text-align` property in `style` attribute on table elements' do
+      html = <<~HTML
       <table>
         <tr><th style="text-align: center">Head</th></tr>
         <tr><td style="text-align: right">Body</th></tr>
@@ -77,6 +77,20 @@ describe Banzai::Filter::SanitizationFilter do
       expect(doc.at_css('td')['style']).to eq 'text-align: right'
     end
 
+    it 'disallows other properties in `style` attribute on table elements' do
+      html = <<~HTML
+        <table>
+          <tr><th style="text-align: foo">Head</th></tr>
+          <tr><td style="position: fixed; height: 50px; width: 50px; background: red; z-index: 999; font-size: 36px; text-align: center">Body</th></tr>
+        </table>
+      HTML
+
+      doc = filter(html)
+
+      expect(doc.at_css('th')['style']).to be_nil
+      expect(doc.at_css('td')['style']).to eq 'text-align: center'
+    end
+
     it 'allows `span` elements' do
       exp = act = %q{<span>Hello</span>}
       expect(filter(act).to_html).to eq exp
-- 
cgit v1.2.1