diff options
author | Robert Speicher <rspeicher@gmail.com> | 2017-08-07 17:04:54 -0400 |
---|---|---|
committer | Robert Speicher <rspeicher@gmail.com> | 2017-08-14 16:36:16 -0400 |
commit | c2a98b0342ebd9839b35a42cae637074fbca4020 (patch) | |
tree | 75d842fdd6e61c8dc2993e459a571dd98532bf51 | |
parent | 4efa558f2dc1fa333f3adbd6920db1a58c476482 (diff) | |
download | gitlab-ce-c2a98b0342ebd9839b35a42cae637074fbca4020.tar.gz |
Limit `style` attribute on `th` and `td` elements to specific properties
Previously we whitelisted the entire `style` attribute on `th` and `td`
elements, in order to allow Markdown table alignment to work. But this
opened us up to a potential exploit by allowing a malicious user to
define properties besides `text-align` in the attribute.
We now remove everything except `text-align: (center|left|right)`.
-rw-r--r-- | changelogs/unreleased/rs-issue-36098.yml | 4 | ||||
-rw-r--r-- | lib/banzai/filter/sanitization_filter.rb | 22 | ||||
-rw-r--r-- | spec/lib/banzai/filter/sanitization_filter_spec.rb | 20 |
3 files changed, 42 insertions, 4 deletions
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 a8a0aa6d395..c19bd49ff00 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, lib: true 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, lib: true 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, lib: true 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 |