Skip to content

Commit 49dfc15

Browse files
rafaelfrancatenderlove
authored andcommitted
Do not unescape already escaped HTML entities
The full sanitizer was using Loofah's #text method that automatically escapes HTML entities. That behavior caused some problems where strings that were not escaped in the older sanitizer started to be escaped. To fix these problems we used the #text's `encode_special_chars` option as `false` that not just skipped the HTML entities escaping but unescaped already escaped entities. This introduced a security bug because an attacker can pass escaped HTML tags that will not be sanitized and will be returned as unescaped HTML tags. To fix it properly we introduced a new scrubber that will remove all tags and keep just the text nodes of these tags without changing how to escape the string. CVE-2015-7579
1 parent 297161e commit 49dfc15

File tree

4 files changed

+50
-10
lines changed

4 files changed

+50
-10
lines changed

Diff for: lib/rails/html/sanitizer.rb

+10-7
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,10 @@ def remove_xpaths(node, xpaths)
1313
node.xpath(*xpaths).remove
1414
node
1515
end
16+
17+
def properly_encode(fragment, options)
18+
fragment.xml? ? fragment.to_xml(options) : fragment.to_html(options)
19+
end
1620
end
1721

1822
# === Rails::Html::FullSanitizer
@@ -26,9 +30,12 @@ def sanitize(html, options = {})
2630
return unless html
2731
return html if html.empty?
2832

29-
Loofah.fragment(html).tap do |fragment|
30-
remove_xpaths(fragment, XPATHS_TO_REMOVE)
31-
end.text(options)
33+
loofah_fragment = Loofah.fragment(html)
34+
35+
remove_xpaths(loofah_fragment, XPATHS_TO_REMOVE)
36+
loofah_fragment.scrub!(TextOnlyScrubber.new)
37+
38+
properly_encode(loofah_fragment, encoding: 'UTF-8')
3239
end
3340
end
3441

@@ -140,10 +147,6 @@ def allowed_tags(options)
140147
def allowed_attributes(options)
141148
options[:attributes] || self.class.allowed_attributes
142149
end
143-
144-
def properly_encode(fragment, options)
145-
fragment.xml? ? fragment.to_xml(options) : fragment.to_html(options)
146-
end
147150
end
148151
end
149152
end

Diff for: lib/rails/html/scrubbers.rb

+20
Original file line numberDiff line numberDiff line change
@@ -169,5 +169,25 @@ def scrub_attribute?(name)
169169
!super
170170
end
171171
end
172+
173+
# === Rails::Html::TextOnlyScrubber
174+
#
175+
# Rails::Html::TextOnlyScrubber allows you to permit text nodes.
176+
#
177+
# Unallowed elements will be stripped, i.e. element is removed but its subtree kept.
178+
class TextOnlyScrubber < Loofah::Scrubber
179+
def initialize
180+
@direction = :bottom_up
181+
end
182+
183+
def scrub(node)
184+
if node.text?
185+
CONTINUE
186+
else
187+
node.before node.children
188+
node.remove
189+
end
190+
end
191+
end
172192
end
173193
end

Diff for: test/sanitizer_test.rb

+5-2
Original file line numberDiff line numberDiff line change
@@ -104,9 +104,12 @@ def test_strip_tags_with_frozen_string
104104
assert_equal "Frozen string with no tags", full_sanitize("Frozen string with no tags".freeze)
105105
end
106106

107-
def test_full_sanitize_allows_turning_off_encoding_special_chars
107+
def test_full_sanitize_respect_html_escaping_of_the_given_string
108+
assert_equal 'test\r\nstring', full_sanitize('test\r\nstring')
108109
assert_equal '&amp;', full_sanitize('&')
109-
assert_equal '&', full_sanitize('&', encode_special_chars: false)
110+
assert_equal '&amp;', full_sanitize('&amp;')
111+
assert_equal '&amp;amp;', full_sanitize('&amp;amp;')
112+
assert_equal 'omg &lt;script&gt;BOM&lt;/script&gt;', full_sanitize('omg &lt;script&gt;BOM&lt;/script&gt;')
110113
end
111114

112115
def test_strip_links_with_tags_in_tags

Diff for: test/scrubbers_test.rb

+15-1
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,20 @@ def test_targeting_tags_and_attributes_removes_only_them
143143
end
144144
end
145145

146+
class TextOnlyScrubberTest < ScrubberTest
147+
def setup
148+
@scrubber = Rails::Html::TextOnlyScrubber.new
149+
end
150+
151+
def test_removes_all_tags_and_keep_the_content
152+
assert_scrubbed '<tag>hello</tag>', 'hello'
153+
end
154+
155+
def test_skips_text_nodes
156+
assert_node_skipped('some text')
157+
end
158+
end
159+
146160
class ReturningStopFromScrubNodeTest < ScrubberTest
147161
class ScrubStopper < Rails::Html::PermitScrubber
148162
def scrub_node(node)
@@ -157,4 +171,4 @@ def setup
157171
def test_returns_stop_from_scrub_if_scrub_node_does
158172
assert_scrub_stopped '<script>remove me</script>'
159173
end
160-
end
174+
end

0 commit comments

Comments
 (0)