Moved to whitelist sanitizer.

config/initializers/html_sanitizer.rb

@@ -0,0 +1,48 @@
+
+# content of this tags will also be removed
+Rails.application.config.html_sanitizer_tags_remove_content = %w(
+  style
+)
+
+# only this tags are allowed
+Rails.application.config.html_sanitizer_tags_whitelist = %w(
+  a abbr acronym address area article aside audio
+  b bdi bdo big blockquote br
+  canvas caption center cite code col colgroup command
+  datalist dd del details dfn dir div dl dt em
+  figcaption figure footer h1 h2 h3 h4 h5 h6 header hr
+  i img ins kbd label legend li map mark menu meter nav
+  ol output optgroup option p pre q
+  s samp section small span strike strong sub summary sup
+  text table tbody td tfoot th thead time tr tt u ul var video
+)
+
+# attributes allowed for tags
+Rails.application.config.html_sanitizer_attributes_whitelist = {
+  :all         => %w(class dir lang style title translate data-signature data-signature-id),
+  'a'          => %w(href hreflang name rel),
+  'abbr'       => %w(title),
+  'blockquote' => %w(cite),
+  'col'        => %w(span width),
+  'colgroup'   => %w(span width),
+  'data'       => %w(value),
+  'del'        => %w(cite datetime),
+  'dfn'        => %w(title),
+  'img'        => %w(align alt border height src srcset width),
+  'ins'        => %w(cite datetime),
+  'li'         => %w(value),
+  'ol'         => %w(reversed start type),
+  'table'      => %w(align bgcolor border cellpadding cellspacing frame rules sortable summary width),
+  'td'         => %w(abbr align axis colspan headers rowspan valign width),
+  'th'         => %w(abbr align axis colspan headers rowspan scope sorted valign width),
+  'ul'         => %w(type),
+  'q'          => %w(cite),
+  'time'       => %w(datetime pubdate),
+}
+
+# only this css properties are allowed
+Rails.application.config.html_sanitizer_css_properties_whitelist = %w(
+  width height
+  max-width min-width
+  max-height min-height
+)

lib/html_sanitizer.rb

@@ -1,48 +1,92 @@
 class HtmlSanitizer
 
   def self.strict(string)
-    remove = %w(style body head)
+
-    strip = ['script']
+    # config
+    tags_remove_content = Rails.configuration.html_sanitizer_tags_remove_content
+    tags_whitelist = Rails.configuration.html_sanitizer_tags_whitelist
+    attributes_whitelist = Rails.configuration.html_sanitizer_attributes_whitelist
+    css_properties_whitelist = Rails.configuration.html_sanitizer_css_properties_whitelist
 
     scrubber = Loofah::Scrubber.new do |node|
 
-      # strip tags
+      # remove tags with subtree
-      if strip.include?(node.name)
+      if tags_remove_content.include?(node.name)
-        node.before node.children
         node.remove
       end
 
-      # remove tags
+      # replace tags, keep subtree
-      if remove.include?(node.name)
+      if !tags_whitelist.include?(node.name)
-        node.remove
+        traversal(node, scrubber)
       end
 
       # prepare src attribute
       if node['src']
-        if node['src'].downcase.start_with?('http', 'ftp')
+        src = cleanup(node['src'])
-          node.before node.children
+        if src =~ /(javascript|livescript|vbscript):/i || src.start_with?('http', 'ftp', '//')
-          node.remove
+          traversal(node, scrubber)
         end
       end
 
+      # clean style / only use allowed style properties
+      if node['style']
+        pears = node['style'].downcase.gsub(/\t|\n|\r/, '').split(';')
+        style = ''
+        pears.each { |pear|
+          prop = pear.split(':')
+          next if !prop[0]
+          key = prop[0].strip
+          next if !css_properties_whitelist.include?(key)
+          style += "#{pear};"
+        }
+        node['style'] = style
+        if style == ''
+          node.delete('style')
+        end
+      end
+
+      # scan for invalid link content
+      %w(href style).each { |attribute_name|
+        next if !node[attribute_name]
+        href = cleanup(node[attribute_name])
+        next if href !~ /(javascript|livescript|vbscript):/i
+        node.delete(attribute_name)
+      }
+
+      # remove attributes if not whitelisted
+      node.each { |attribute, _value|
+        attribute_name = attribute.downcase
+        next if attributes_whitelist[:all].include?(attribute_name) || (attributes_whitelist[node.name] && attributes_whitelist[node.name].include?(attribute_name))
+        node.delete(attribute)
+      }
+
       # prepare links
       if node['href']
-        if node['href'].downcase.start_with?('http', 'ftp')
+        href = cleanup(node['href'])
+        next if !href.start_with?('http', 'ftp', '//')
         node.set_attribute('rel', 'nofollow')
         node.set_attribute('target', '_blank')
       end
-        if node['href'] =~ /javascript/i
-          node.delete('href')
-        end
-      end
-
-      # remove on* attributes
-      node.each { |attribute, _value|
-        next if !attribute.downcase.start_with?('on')
-        node.delete(attribute)
-      }
     end
     Loofah.fragment(string).scrub!(scrubber).to_s
   end
 
+  def self.traversal(node, scrubber)
+    node.children.each { |child|
+      if child.class == Nokogiri::XML::CDATA
+        node.before Nokogiri::XML::Text.new(node.content, node.document)
+      else
+        node.before Loofah.fragment(child.to_s).scrub!(scrubber)
+      end
+    }
+    node.remove
+  end
+
+  def self.cleanup(string)
+    string.downcase.gsub(/[[:space:]]|\t|\n|\r/, '').gsub(%r{/\*.*?\*/}, '').gsub(/<!--.*?-->/, '').gsub(/\[.+?\]/, '')
+  end
+
+  private_class_method :traversal
+  private_class_method :cleanup
+
 end

test/unit/html_sanitizer_test.rb

@@ -0,0 +1,74 @@
+# encoding: utf-8
+require 'test_helper'
+
+class HtmlSanitizerTest < ActiveSupport::TestCase
+
+  test 'xss' do
+    assert_equal(HtmlSanitizer.strict('<b>123</b>'), '<b>123</b>')
+    assert_equal(HtmlSanitizer.strict('<script><b>123</b></script>'), '&lt;b&gt;123&lt;/b&gt;')
+    assert_equal(HtmlSanitizer.strict('<script><style><b>123</b></style></script>'), '&lt;style&gt;&lt;b&gt;123&lt;/b&gt;&lt;/style&gt;')
+    assert_equal(HtmlSanitizer.strict('<abc><i><b>123</b><bbb>123</bbb></i></abc>'), '<i><b>123</b>123</i>')
+    assert_equal(HtmlSanitizer.strict('<abc><i><b>123</b><bbb>123<i><ccc>abc</ccc></i></bbb></i></abc>'), '<i><b>123</b>123<i>abc</i></i>')
+    assert_equal(HtmlSanitizer.strict('<not_existing>123</not_existing>'), '123')
+    assert_equal(HtmlSanitizer.strict('<script type="text/javascript">alert("XSS!");</script>'), 'alert("XSS!");')
+    assert_equal(HtmlSanitizer.strict('<SCRIPT SRC=http://xss.rocks/xss.js></SCRIPT>'), '')
+    assert_equal(HtmlSanitizer.strict('<IMG SRC="javascript:alert(\'XSS\');">'), '')
+    assert_equal(HtmlSanitizer.strict('<IMG SRC=javascript:alert(\'XSS\')>'), '')
+    assert_equal(HtmlSanitizer.strict('<IMG SRC=JaVaScRiPt:alert(\'XSS\')>'), '')
+    assert_equal(HtmlSanitizer.strict('<IMG SRC=`javascript:alert("RSnake says, \'XSS\'")`>'), '')
+    assert_equal(HtmlSanitizer.strict('<IMG """><SCRIPT>alert("XSS")</SCRIPT>">'), '<img>alert("XSS")"&gt;')
+    assert_equal(HtmlSanitizer.strict('<IMG SRC=# onmouseover="alert(\'xxs\')">'), '<img src="#">')
+    assert_equal(HtmlSanitizer.strict('<IMG SRC="jav  ascript:alert(\'XSS\');">'), '')
+    assert_equal(HtmlSanitizer.strict('<IMG SRC="jav&#x09;ascript:alert(\'XSS\');">'), '')
+    assert_equal(HtmlSanitizer.strict('<IMG SRC="jav&#x0A;ascript:alert(\'XSS\');">'), '')
+    assert_equal(HtmlSanitizer.strict('<IMG SRC="jav&#x0D;ascript:alert(\'XSS\');">'), '')
+    assert_equal(HtmlSanitizer.strict('<IMG SRC=" &#14;  javascript:alert(\'XSS\');">'), '<img src="">')
+    assert_equal(HtmlSanitizer.strict('<SCRIPT/XSS SRC="http://xss.rocks/xss.js"></SCRIPT>'), '')
+    assert_equal(HtmlSanitizer.strict('<BODY onload!#$%&()*~+-_.,:;?@[/|\]^`=alert("XSS")>'), '')
+    assert_equal(HtmlSanitizer.strict('<SCRIPT/SRC="http://xss.rocks/xss.js"></SCRIPT>'), '')
+    assert_equal(HtmlSanitizer.strict('<<SCRIPT>alert("XSS");//<</SCRIPT>'), '&lt;alert("XSS");//&lt;')
+    assert_equal(HtmlSanitizer.strict('<SCRIPT SRC=http://xss.rocks/xss.js?< B >'), '')
+    assert_equal(HtmlSanitizer.strict('<SCRIPT SRC=//xss.rocks/.j>'), '')
+    assert_equal(HtmlSanitizer.strict('<IMG SRC="javascript:alert(\'XSS\')"'), '')
+    assert_equal(HtmlSanitizer.strict('<IMG SRC="javascript:alert(\'XSS\')" abc<b>123</b>'), '123')
+    assert_equal(HtmlSanitizer.strict('<iframe src=http://xss.rocks/scriptlet.html <'), '')
+    assert_equal(HtmlSanitizer.strict('</script><script>alert(\'XSS\');</script>'), 'alert(\'XSS\');')
+    assert_equal(HtmlSanitizer.strict('<STYLE>li {list-style-image: url("javascript:alert(\'XSS\')");}</STYLE><UL><LI>XSS</br>'), '<ul><li>XSS</li></ul>')
+    assert_equal(HtmlSanitizer.strict('<IMG SRC=\'vbscript:msgbox("XSS")\'>'), '')
+    assert_equal(HtmlSanitizer.strict('<IMG SRC="livescript:[code]">'), '')
+    assert_equal(HtmlSanitizer.strict('<svg/onload=alert(\'XSS\')>'), '')
+    assert_equal(HtmlSanitizer.strict('<BODY ONLOAD=alert(\'XSS\')>'), '')
+    assert_equal(HtmlSanitizer.strict('<LINK REL="stylesheet" HREF="javascript:alert(\'XSS\');">'), '')
+    assert_equal(HtmlSanitizer.strict('<STYLE>@import\'http://xss.rocks/xss.css\';</STYLE>'), '')
+    assert_equal(HtmlSanitizer.strict('<META HTTP-EQUIV="Link" Content="<http://xss.rocks/xss.css>; REL=stylesheet">'), '')
+    assert_equal(HtmlSanitizer.strict('<IMG STYLE="java/*XSS*/script:(alert(\'XSS\'), \'\')">'), '<img>')
+    assert_equal(HtmlSanitizer.strict('<IMG src="java/*XSS*/script:(alert(\'XSS\'), \'\')">'), '')
+    assert_equal(HtmlSanitizer.strict('<IFRAME SRC="javascript:alert(\'XSS\');"></IFRAME>'), '')
+    assert_equal(HtmlSanitizer.strict('<TABLE><TD BACKGROUND="javascript:alert(\'XSS\')">'), '<table><td></td></table>')
+    assert_equal(HtmlSanitizer.strict('<DIV STYLE="background-image: url(javascript:alert(\'XSS\'), \'\')">'), '<div></div>')
+    assert_equal(HtmlSanitizer.strict('<a href="/some/path">test</a>'), '<a href="/some/path">test</a>')
+    assert_equal(HtmlSanitizer.strict('<a href="https://some/path">test</a>'), '<a href="https://some/path" rel="nofollow" target="_blank">test</a>')
+    assert_equal(HtmlSanitizer.strict('<XML ID="xss"><I><B><IMG SRC="javas<!-- -->cript:alert(\'XSS\')"></B></I></XML>'), '<i><b></b></i>')
+    assert_equal(HtmlSanitizer.strict('<IMG SRC="javas<!-- -->cript:alert(\'XSS\')">'), '')
+    assert_equal(HtmlSanitizer.strict(' <HEAD><META HTTP-EQUIV="CONTENT-TYPE" CONTENT="text/html; charset=UTF-7"> </HEAD>+ADw-SCRIPT+AD4-alert(\'XSS\');+ADw-/SCRIPT+AD4-'), '  +ADw-SCRIPT+AD4-alert(\'XSS\');+ADw-/SCRIPT+AD4-')
+    assert_equal(HtmlSanitizer.strict('<SCRIPT a=">" SRC="httx://xss.rocks/xss.js"></SCRIPT>'), '')
+    assert_equal(HtmlSanitizer.strict('<A HREF="h
+tt  p://6 6.000146.0x7.147/">XSS</A>'), '<a href="h%0Att%20%20p://6%206.000146.0x7.147/" rel="nofollow" target="_blank">XSS</a>')
+    assert_equal(HtmlSanitizer.strict('<A HREF="//www.google.com/">XSS</A>'), '<a href="//www.google.com/" rel="nofollow" target="_blank">XSS</a>')
+    assert_equal(HtmlSanitizer.strict('<form id="test"></form><button form="test" formaction="javascript:alert(1)">X</button>'), 'X')
+    assert_equal(HtmlSanitizer.strict('<maction actiontype="statusline#http://google.com" xlink:href="javascript:alert(2)">CLICKME</maction>'), 'CLICKME')
+    assert_equal(HtmlSanitizer.strict('<a xlink:href="javascript:alert(2)">CLICKME</a>'), '<a>CLICKME</a>')
+    assert_equal(HtmlSanitizer.strict('<!--<img src="--><img src=x onerror=alert(1)//">'), '<img src="x">')
+    assert_equal(HtmlSanitizer.strict('<![><img src="]><img src=x onerror=alert(1)//">'), '<img src="%5D&gt;&lt;img%20src=x%20onerror=alert(1)//">')
+    assert_equal(HtmlSanitizer.strict('<svg><![CDATA[><image xlink:href="]]><img src=xx:x onerror=alert(2)//"></svg>'), '')
+    assert_equal(HtmlSanitizer.strict('<abc><img src="</abc><img src=x onerror=alert(1)//">'), '<img src="&lt;/abc&gt;&lt;img%20src=x%20onerror=alert(1)//">')
+    assert_equal(HtmlSanitizer.strict('<object data="data:text/html;base64,PHNjcmlwdD5hbGVydCgxKTwvc2NyaXB0Pg=="></object>'), '')
+    assert_equal(HtmlSanitizer.strict('<embed src="data:text/html;base64,PHNjcmlwdD5hbGVydCgxKTwvc2NyaXB0Pg=="></embed>'), '')
+    assert_equal(HtmlSanitizer.strict('<img[a][b]src=x[d]onerror[c]=[e]"alert(1)">'), '<img>')
+    assert_equal(HtmlSanitizer.strict('<a href="[a]java[b]script[c]:alert(1)">XXX</a>'), '<a>XXX</a>')
+    assert_equal(HtmlSanitizer.strict('<svg xmlns="http://www.w3.org/2000/svg"><script>alert(1)</script></svg>'), 'alert(1)')
+    assert_equal(HtmlSanitizer.strict('<a style="position:fixed;top:0;left:0;width: 260px;height:100vh;background-color:red;display: block;" href="http://example.com"></a>'), '<a style="width: 260px;height:100vh;" href="http://example.com" rel="nofollow" target="_blank"></a>')
+
+  end
+
+end

test/unit/ticket_xss_test.rb

@@ -65,9 +65,8 @@ class TicketXssTest < ActiveSupport::TestCase
       created_by_id: 1,
     )
     assert_equal("please tell me this doesn't work: <table>ada<tr></tr>
-</table><div class=\"adasd\" id=\"123\" data-abc=\"123\"></div><div>
+</table><div class=\"adasd\"></div><div>
-<a>LINK</a><a href=\"http://lalal.de\" rel=\"nofollow\" target=\"_blank\">aa</a><some_not_existing>ABC</some_not_existing>
+<a>LINK</a><a href=\"http://lalal.de\" rel=\"nofollow\" target=\"_blank\">aa</a>ABC</div>", article3.body, 'article3.body verify - inbound')
-</div>", article3.body, 'article3.body verify - inbound')
 
     article4 = Ticket::Article.create(
       ticket_id: ticket.id,
@@ -83,7 +82,7 @@ class TicketXssTest < ActiveSupport::TestCase
       updated_by_id: 1,
       created_by_id: 1,
     )
-    assert_equal("please tell me this doesn't work: <video>some video</video><foo>alal</foo>", article4.body, 'article4.body verify - inbound')
+    assert_equal("please tell me this doesn't work: <video>some video</video>alal", article4.body, 'article4.body verify - inbound')
 
     article5 = Ticket::Article.create(
       ticket_id: ticket.id,
@@ -92,14 +91,14 @@ class TicketXssTest < ActiveSupport::TestCase
       subject: 'some subject <script type="text/javascript">alert("XSS!");</script>',
       message_id: 'some@id',
       content_type: 'text/plain',
-      body: 'please tell me this doesn\'t work: <table>ada<tr></tr></table><div class="adasd" id="123" data-abc="123"></div><div><a href="javascript:someFunction()">LINK</a><a href="http://lalal.de">aa</a><some_not_existing>ABC</some_not_existing>',
+      body: 'please tell me this doesn\'t work: <table>ada<tr></tr></table><div class="adasd" id="123" data-signature-id="123"></div><div><a href="javascript:someFunction()">LINK</a><a href="http://lalal.de">aa</a><some_not_existing>ABC</some_not_existing>',
       internal: false,
       sender: Ticket::Article::Sender.find_by(name: 'Customer'),
       type: Ticket::Article::Type.find_by(name: 'email'),
       updated_by_id: 1,
       created_by_id: 1,
     )
-    assert_equal('please tell me this doesn\'t work: <table>ada<tr></tr></table><div class="adasd" id="123" data-abc="123"></div><div><a href="javascript:someFunction()">LINK</a><a href="http://lalal.de">aa</a><some_not_existing>ABC</some_not_existing>', article5.body, 'article5.body verify - inbound')
+    assert_equal('please tell me this doesn\'t work: <table>ada<tr></tr></table><div class="adasd" id="123" data-signature-id="123"></div><div><a href="javascript:someFunction()">LINK</a><a href="http://lalal.de">aa</a><some_not_existing>ABC</some_not_existing>', article5.body, 'article5.body verify - inbound')
 
     article6 = Ticket::Article.create(
       ticket_id: ticket.id,
@@ -116,7 +115,7 @@ class TicketXssTest < ActiveSupport::TestCase
       created_by_id: 1,
     )
     assert_equal('some message article helper test1 <div>
-<img style="width: 85.5px; height: 49.5px" src="cid:15.274327094.140938@zammad.example.com">asdasd<img src="cid:15.274327094.140939@zammad.example.com"><br>
+<img style="width: 85.5px; height: 49.5px;" src="cid:15.274327094.140938@zammad.example.com">asdasd<img src="cid:15.274327094.140939@zammad.example.com"><br>
 </div>', article6.body, 'article6.body verify - inbound')
 
     article7 = Ticket::Article.create(
@@ -134,7 +133,7 @@ class TicketXssTest < ActiveSupport::TestCase
       created_by_id: 1,
     )
     assert_equal('some message article helper test1 <div>
-<img style="width: 85.5px; height: 49.5px" src="api/v1/ticket_attachment/123/123/123">asdasd<img src="api/v1/ticket_attachment/123/123/123"><br>
+<img style="width: 85.5px; height: 49.5px;" src="api/v1/ticket_attachment/123/123/123">asdasd<img src="api/v1/ticket_attachment/123/123/123"><br>
 </div>', article7.body, 'article7.body verify - inbound')
 
     article8 = Ticket::Article.create(