experiment: use loofah attribute scrubber to explore functional drift

[flavorjones]

@rafaelfranca After seeing #135 I got a little concerned about the drift between Loofah and the RHS attribute scrubber.

I posted a branch of Loofah that accepts an optional set of allowed attributes, and have modified RHS in this PR to use it. There are a few failures that I think we should discuss. (See thread below.)

[flavorjones]

Test failures:

  1) Failure:
SanitizersTest#test_sanitize_data_attributes [/home/flavorjones/code/oss/rails-html-sanitizer/test/sanitizer_test.rb:509]:
Expected: <a href="/blah">foo</a>
Actual: <a href="/blah" data-method="post">foo</a>

Since your commit flavorjones/loofah@78c7e74 in 2013, Loofah has allowed HTML5 data- attributes. I'm not sure why RHS removed this support in 297161e in 2015 citing CVE-2015-7578. I don't think that it makes sense for Loofah and RHS to have different policies on these attributes, and I'm not sure I understand the threat model that led you to make that decision.

  2) Failure:
TargetScrubberTest#test_targeting_attributes_removes_only_them [/home/flavorjones/code/oss/rails-html-sanitizer/test/scrubbers_test.rb:158]:
Expected: "<a onclick=\"c\"></a>"
  Actual: "<a class=\"a\" id=\"b\"></a>"

This has to do with the target scrubber inverting the sense of allowed/disallowed, and can be ignored for the purposes of this experiment.

  3) Failure:
TargetScrubberTest#test_targeting_tags_and_attributes_removes_only_them [/home/flavorjones/code/oss/rails-html-sanitizer/test/scrubbers_test.rb:165]:
Expected: "<a other=\"\"></a>"
  Actual: "<a></a>"

  4) Failure:
PermitScrubberTest#test_leaves_only_supplied_tags_and_attributes [/home/flavorjones/code/oss/rails-html-sanitizer/test/scrubbers_test.rb:108]:
--- expected
+++ actual
@@ -1 +1 @@
-"<tag></tag><tag cooler=\"\"></tag>"
+"<tag></tag><tag></tag>"

These failures reflect the fact that Loofah removes empty attributes (with the exception of data- attributes). I'm not sure why RHS only does this for src attributes.

[flavorjones]

Really I'm primarily asking about:

• Why are data- attributes being removed? Would it be acceptable to start leaving them in place? I'm not aware of any security considerations but am happy to be educated.
  • This feels risker than the next one.
• Why are empty attributes being left behind by RHS (except for src)? Would it be acceptable to start removing them?
  • This feels low-risk.

[rafaelfranca]

Why are empty attributes being left behind by RHS (except for src)? Would it be acceptable to start removing them?

• This feels low-risk.

Yes.

Why are data- attributes being removed? Would it be acceptable to start leaving them in place? I'm not aware of any security considerations but am happy to be educated.

• This feels risker than the next one.

In combination with rails-ujs or jquery-ujs, having control to data attributes can allow attackers to make XSS requests or even escape CSRF protection. I sent you details about this vulnerability.

[flavorjones]

For posterity, the data- attributes change was motivated by this hackerone report (public): https://hackerone.com/reports/42728

[flavorjones]

Interesting note: flavorjones/loofah#242 points out that empty HTML5 attributes are valid and probably shouldn't be removed by Loofah (if they're in the safelist). When I come back to this I'll try to figure out why this functionality was originally added in flavorjones/loofah#51