Skip to content

Commit 1625173

Browse files
committed
fix: disallow 'noscript' from safe lists
https://hackerone.com/reports/2509647
1 parent 5104ca9 commit 1625173

File tree

3 files changed

+49
-0
lines changed

3 files changed

+49
-0
lines changed

lib/rails/html/scrubbers.rb

+6
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,12 @@ def validate!(var, name)
134134
if var && !var.is_a?(Enumerable)
135135
raise ArgumentError, "You should pass :#{name} as an Enumerable"
136136
end
137+
138+
if var && name == :tags && var.include?("noscript")
139+
warn("WARNING: 'noscript' tags cannot be allowed by the PermitScrubber and will be scrubbed")
140+
var.delete("noscript")
141+
end
142+
137143
var
138144
end
139145

test/sanitizer_test.rb

+35
Original file line numberDiff line numberDiff line change
@@ -1026,6 +1026,24 @@ def test_should_sanitize_across_newlines
10261026
assert_equal "", sanitize_css(raw)
10271027
end
10281028

1029+
def test_should_prune_noscript
1030+
# https://hackerone.com/reports/2509647
1031+
input, tags = "<div><noscript><p id='</noscript><script>alert(1)</script>'></noscript>", ["p", "div", "noscript"]
1032+
actual = nil
1033+
assert_output(nil, /WARNING: 'noscript' tags cannot be allowed by the PermitScrubber/) do
1034+
actual = safe_list_sanitize(input, tags: tags, attributes: %w(id))
1035+
end
1036+
1037+
acceptable_results = [
1038+
# libxml2
1039+
"<div><p id=\"&lt;/noscript&gt;&lt;script&gt;alert(1)&lt;/script&gt;\"></p></div>",
1040+
# libgumbo
1041+
"<div><p id=\"</noscript><script>alert(1)</script>\"></p></div>",
1042+
]
1043+
1044+
assert_includes(acceptable_results, actual)
1045+
end
1046+
10291047
protected
10301048
def safe_list_sanitize(input, options = {})
10311049
module_under_test::SafeListSanitizer.new.sanitize(input, options)
@@ -1075,5 +1093,22 @@ class HTML4SafeListSanitizerTest < Minitest::Test
10751093
class HTML5SafeListSanitizerTest < Minitest::Test
10761094
@module_under_test = Rails::HTML5
10771095
include SafeListSanitizerTest
1096+
1097+
def test_should_not_be_vulnerable_to_noscript_attacks
1098+
# https://hackerone.com/reports/2509647
1099+
skip("browser assertion requires parse_noscript_content_as_text") unless Nokogiri::VERSION >= "1.17"
1100+
1101+
input = '<noscript><p id="</noscript><script>alert(1)</script>"></noscript>'
1102+
1103+
result = nil
1104+
assert_output(nil, /WARNING/) do
1105+
result = Rails::HTML5::SafeListSanitizer.new.sanitize(input, tags: %w(p div noscript), attributes: %w(id class style))
1106+
end
1107+
1108+
browser = Nokogiri::HTML5::Document.parse(result, parse_noscript_content_as_text: true)
1109+
xss = browser.at_xpath("//script")
1110+
1111+
assert_nil(xss)
1112+
end
10781113
end if loofah_html5_support?
10791114
end

test/scrubbers_test.rb

+8
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,14 @@ def test_leaves_only_supplied_tags_and_attributes
121121
assert_scrubbed html, '<tag></tag><tag cooler=""></tag>'
122122
end
123123

124+
def test_does_not_allow_safelisted_noscript
125+
# https://hackerone.com/reports/2509647
126+
assert_output(nil, /WARNING: 'noscript' tags cannot be allowed by the PermitScrubber/) do
127+
@scrubber.tags = ["div", "noscript", "span"]
128+
end
129+
assert_equal(["div", "span"], @scrubber.tags)
130+
end
131+
124132
def test_leaves_text
125133
assert_scrubbed("some text")
126134
end

0 commit comments

Comments
 (0)