Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ReDoS Vulnerability in PermitScrubber and Add Performance Test #186

Closed
wants to merge 4 commits into from
Closed

Fix ReDoS Vulnerability in PermitScrubber and Add Performance Test #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
26a34b1
Fix ReDoS vulnerability in PermitScrubber by optimizing regex
ch4n3-yoon Aug 13, 2024
f461d2a
Add linear performance test to verify ReDoS mitigation in PermitScrubber
ch4n3-yoon Aug 13, 2024
e1c1537
Fix: Add missing require 'benchmark' to scrubbers_test.rb
ch4n3-yoon Aug 13, 2024
713241f
Revert "Add linear performance test to verify ReDoS mitigation in Per…
ch4n3-yoon Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/rails/html/scrubbers.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ def scrub_attribute(node, attr_node)
Loofah::HTML5::Scrub.scrub_attribute_that_allows_local_ref(attr_node)
end

if Loofah::HTML5::SafeList::SVG_ALLOW_LOCAL_HREF.include?(node.name) && attr_name == "xlink:href" && attr_node.value =~ /^\s*[^#\s].*/m
if Loofah::HTML5::SafeList::SVG_ALLOW_LOCAL_HREF.include?(node.name) && attr_name == "xlink:href" && attr_node.value =~ /^\s*[^#].*/m
Copy link

Choose a reason for hiding this comment

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

I think the meaning of the regexp is changed. It began to match to any character that starts with /\s/.

I'm not sure what the original regexp want to match to. maybe:

  • /^[^\S\n]*[^#\s]/: nearly or identical to the original regexp
  • /\A\s*[^#\s]/: matches to "\n\n a..." but doesn't match to " #\n #\n a"

or perhaps using attr_node.value !~ allow_pattern would be more simple?

Copy link
Member

Choose a reason for hiding this comment

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

This regular expression originally came from the html5lib library:

https://github.com/html5lib/html5lib-ruby/blob/master/lib/html5/sanitizer.rb#L141-L143

The goal of this line is to accept "local links" (like #some-id) but not full URLs, with initial whitespace being ignored. There are test cases for this behavior in Loofah, which rails-html-sanitizer originally copied this code from.

Copy link
Member

@flavorjones flavorjones Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So @tompng is correct, this patch changes the behavior in unwanted ways. One example test case is:

<animatemotion xlink:href='\n#foo'>/</animatemotion>

Specifically, in this case the xlink:href attribute should not be removed but is being removed because:

val = "\n#foo"
# => "\n#foo"

val =~ /^\s*[^#\s].*/
# => nil

val =~ /^\s*[^#].*/
# => 0

attr_node.remove
end

Expand Down
1 change: 1 addition & 0 deletions test/scrubbers_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# frozen_string_literal: true

require "benchmark"
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this line, it's no longer necessary.

require "minitest/autorun"
require "rails-html-sanitizer"

Expand Down