Skip to content

Commit

Permalink
Make sure we address CVE-2018-8048
Browse files Browse the repository at this point in the history
Even that the issue was fixed on loofah we have our own logic to scrub
attributes so when the whitelist serializer is used the issue was still
present.

Fix CVE-2018-3741.
  • Loading branch information
rafaelfranca committed Mar 22, 2018
1 parent 7aea595 commit f3ba1a8
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 3 deletions.
2 changes: 2 additions & 0 deletions lib/rails/html/scrubbers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,8 @@ def scrub_attribute(node, attr_node)
end

node.remove_attribute(attr_node.name) if attr_name == 'src' && attr_node.value !~ /[^[:space:]]/

Loofah::HTML5::Scrub.force_correct_attribute_escaping! node
end
end

Expand Down
2 changes: 1 addition & 1 deletion rails-html-sanitizer.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ Gem::Specification.new do |spec|
spec.test_files = Dir["test/**/*"]
spec.require_paths = ["lib"]

spec.add_dependency "loofah", "~> 2.0"
spec.add_dependency "loofah", "~> 2.2", ">= 2.2.2"

spec.add_development_dependency "bundler", "~> 1.3"
spec.add_development_dependency "rake"
Expand Down
36 changes: 34 additions & 2 deletions test/sanitizer_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -385,13 +385,13 @@ def test_should_sanitize_attributes

def test_should_sanitize_illegal_style_properties
raw = %(display:block; position:absolute; left:0; top:0; width:100%; height:100%; z-index:1; background-color:black; background-image:url(http://www.ragingplatypus.com/i/cam-full.jpg); background-x:center; background-y:center; background-repeat:repeat;)
expected = %(display: block; width: 100%; height: 100%; background-color: black; background-x: center; background-y: center;)
expected = %(display:block;width:100%;height:100%;background-color:black;background-x:center;background-y:center;)
assert_equal expected, sanitize_css(raw)
end

def test_should_sanitize_with_trailing_space
raw = "display:block; "
expected = "display: block;"
expected = "display:block;"
assert_equal expected, sanitize_css(raw)
end

Expand Down Expand Up @@ -484,6 +484,38 @@ def test_allow_data_attribute_if_requested
assert_equal %(<a data-foo="foo">foo</a>), white_list_sanitize(text, attributes: ['data-foo'])
end

def test_uri_escaping_of_href_attr_in_a_tag_in_white_list_sanitizer
html = %{<a href='examp<!--" unsafeattr=foo()>-->le.com'>test</a>}

text = white_list_sanitize(html)

assert_equal %{<a href="examp<!--%22%20unsafeattr=foo()>-->le.com">test</a>}, text
end

def test_uri_escaping_of_src_attr_in_a_tag_in_white_list_sanitizer
html = %{<a src='examp<!--" unsafeattr=foo()>-->le.com'>test</a>}

text = white_list_sanitize(html)

assert_equal %{<a src="examp<!--%22%20unsafeattr=foo()>-->le.com">test</a>}, text
end

def test_uri_escaping_of_name_attr_in_a_tag_in_white_list_sanitizer
html = %{<a name='examp<!--" unsafeattr=foo()>-->le.com'>test</a>}

text = white_list_sanitize(html)

assert_equal %{<a name="examp<!--%22%20unsafeattr=foo()>-->le.com">test</a>}, text
end

def test_uri_escaping_of_name_action_in_a_tag_in_white_list_sanitizer
html = %{<a action='examp<!--" unsafeattr=foo()>-->le.com'>test</a>}

text = white_list_sanitize(html, attributes: ['action'])

assert_equal %{<a action="examp<!--%22%20unsafeattr=foo()>-->le.com">test</a>}, text
end

protected

def xpath_sanitize(input, options = {})
Expand Down

2 comments on commit f3ba1a8

@sunsheeppoplar
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if this is an ignorant question, first time ever dealing with a security issue like this. Will upgrading loofah as per the mitigation suggestion break rails-html-sanitizer? Or is it good to go on both ends?

@kaspth
Copy link
Contributor

@kaspth kaspth commented on f3ba1a8 Apr 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should update loofah to 2.2.1 and rails-html-sanitizer to https://github.com/rails/rails-html-sanitizer/releases/tag/v1.0.4

Then that should do it 😊

Please sign in to comment.