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

Added ability to whitelist particular functions #122 #123

Merged
merged 1 commit into from
Feb 6, 2018
Merged

Added ability to whitelist particular functions #122 #123

merged 1 commit into from
Feb 6, 2018

Conversation

NikoRoberts
Copy link
Contributor

A PR for #122
I'm not 100% on the XSS concerns with enabling calc() by default.

@flavorjones
Copy link
Owner

This seems reasonable to me, the W3C spec on expression syntax doesn't seem to allow for any opportunities for XSS:

https://www.w3.org/TR/css3-values/#calc-notation

I'm inclined to merge this in. Will wait a day or two for comment.

@JackLaBarba
Copy link

+1 hooray for solving the exact problem my team is dealing with right now!

@webgago
Copy link

webgago commented Oct 5, 2017

+1 as actually even rgb is treated as a function:

Loofah::HTML5::Scrub.scrub_css 'color: rgb(204,204,204);' #=> ""
tree = Crass.parse_properties 'color: rgb(204,204,204);'
node = tree.first
node[:value] #=> rgb(204,204,204)
node[:children].find { |c| c[:node] == :function }[:name] #=> 'rgb'

@@ -137,6 +137,8 @@ module WhiteList
purple red right solid silver teal top transparent underline white
yellow]

ACCEPTABLE_CSS_FUNCTIONS = Set.new %w[calc]
Copy link

Choose a reason for hiding this comment

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

In the light of this comment: #123 (comment) I think this should be extended to cover rgb, rgba, hls, hlsa and probably a bunch more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, but I'd suggest merging in as it is and raising a separate issue for assessing other functions as defaults.
Until this is merged in, users of loofah can't whitelist any themselves.

Copy link

Choose a reason for hiding this comment

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

Looking at the two weeks this PR has been open, I think it wouldn't hurt to make it completely functional. Whitelisting just calc is very very limited. However, it's your PR, so it's yours and maintainers choice :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whitelisting many functions by default, opens up the gem as being the root cause of sanitization an XSS attack vector which, unless I'm mistaken, could potentially undermine many Rails projects.
The primary concern of this PR is to allow individuals to whitelist functions themselves, that they can assess in their own time.
I'd be quite happy to even remove calc as a default as long as the gem allowed defining ACCEPTABLE_CSS_FUNCTIONS it would fulfil the functionality aim I had.

Copy link

@pjg pjg Oct 5, 2017

Choose a reason for hiding this comment

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

Sure, for your use-case this is enough. I'm rather thinking of the bigger picture. Gems like this one only make sense if they whitelist all allowed (according to W3C) tags, attributes, css functions, etc. etc. If you had to define all such lists on your own, this gem would be a major PITA to use. Not to mention it would lose all of its security features as a lot of ppl would whitelist stuff which shouldn't be allowed in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enabling ppl to whitelist to their own demise is exactly what the gem should enable.
Rushing into having a large whitelist could potentially mean undermining the safety of sanitizing strings.

For example if the wrong function is accidently whitelisted, it could mean sanitize(unsafe_html) in all apps running a particular version of Loofah could be targets of a XSS attack.
If each individual is responsible for whitelisting their own functions, it is unlikely that malicious attackers will have a way of identifying the version of Loofah being used through automated testing.

I'm not saying more shouldn't be added to the default function whitelist like the color functions. But before they are, there should be some research done into ways that each function may be used maliciously.

Copy link

Choose a reason for hiding this comment

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

image

Copy link

Choose a reason for hiding this comment

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

For reference: https://www.quackit.com/css/functions/ -- all those functions should be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quick note: at least url() and image() can be used for arbitrary JS code execution and parameter snooping.
They can also be used to snoop URL params by logging all requests via a remote call.

@oluwayetty
Copy link

Any idea when this PR gets to be merged? @pjg - Wondering if there's any reason why it's still opened. Thanks!

@pjg
Copy link

pjg commented Oct 10, 2017

It's up to the maintainers. Personally I would extend the default list by a lot, but even with just one function there the PR is still very useful.

@ramzizaz
Copy link

Hello @NikoRoberts , any idea of when this will be merged? Thanks

@ramzizaz
Copy link

Pinging again to see if any date is planned for the merge @NikoRoberts

@NikoRoberts
Copy link
Contributor Author

I’m not a maintainer, so have no control over this being merged @ramzizaz

@ramzizaz
Copy link

Sorry @NikoRoberts . @flavorjones do you have any timelines for merging this

@ramzizaz
Copy link

ramzizaz commented Nov 2, 2017

@flavorjones any update on the merge timing?

@ramzizaz
Copy link

@flavorjones any idea on merge timing?

@tschoppi
Copy link

@flavorjones I would love to have this so that my rgb() functions aren't sanitized away. Is there an update on the ETA of the merge?

@sudoremo
Copy link

I'd also love an ETA on this update, thanks a lot 👍

@NikoRoberts
Copy link
Contributor Author

Happy New Year! @flavorjones
Can I do anything else to make this a more acceptable merge?

@tschoppi
Copy link

tschoppi commented Feb 6, 2018

@flavorjones, is there something specific holding up the merge of this PR?

@flavorjones
Copy link
Owner

Sorry, been behind on email. Merging. Will plan on cutting a release this weekend.

@flavorjones flavorjones merged commit a414233 into flavorjones:master Feb 6, 2018
@flavorjones
Copy link
Owner

Planned release is 2.2.0

@NikoRoberts NikoRoberts deleted the enable_whitelisting_css_functions branch February 8, 2018 00:36
flavorjones added a commit that referenced this pull request Feb 11, 2018
[fixes #129]
[related to #122]
[related to #123]
@flavorjones
Copy link
Owner

Note I've whitelisted rgb as well in 6b81467

@sudoremo
Copy link

Thanks a lot for the release. I think the version number in the gemspec is still wrong, correct?

@flavorjones
Copy link
Owner

@remofritzsche do you mean the fake gemspec that's checked into git? I forget that people still do that. Will bump it.

@flavorjones
Copy link
Owner

Please note that upcoming v2.3.0 will whitelist almost all of the CSS3 functions as suggested by @pjg.

flavorjones added a commit that referenced this pull request Oct 28, 2018
still omit `url` and `image`

related to #122 and #123
also see #143
@NikoRoberts
Copy link
Contributor Author

@flavorjones is allowing attr safe? It strikes me that it could potentially enable data leakage from the page via injection attacks if a combination of url() and attr() are used.
Just being devils advocate 👿

@flavorjones
Copy link
Owner

@NikoRoberts Can you say more? Was planning to allow attr() but not url().

@NikoRoberts
Copy link
Contributor Author

I haven’t got any concrete examples of how to do it. attr() just seems more powerful than say calc() because it can access data outside of its explicit params

@flavorjones
Copy link
Owner

Ah, interesting. I looked into it, and there are a couple of reasons I feel mostly OK about including it:

  • it's not possible to refer to attribute values for other elements than the subject of the selector
  • no browsers actually support the url type argument
  • it doesn't appear possible to combine the url() and attr() arguments

I'm not at all a CSS expert though, so I would really like to hear from anyone with deeper knowledge on whether it's acceptable to allow attr() unsanitized.

There are some interesting opinions expressed on this topic at this SO post:

https://stackoverflow.com/questions/26967890/css-set-background-image-by-data-image-attr

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.

9 participants