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.

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

experiment: use loofah attribute scrubber to explore functional drift #136

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

flavorjones
Copy link
Member

@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
Copy link
Member Author

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 flavorjones changed the title use loofah attribute scrubber experiment: use loofah attribute scrubber to explore functional drift Jun 9, 2022
@flavorjones
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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

@flavorjones
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants