Added ability to whitelist particular functions #122 #123

NikoRoberts · 2017-09-21T05:35:54Z

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

flavorjones · 2017-09-24T19:46:04Z

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 · 2017-10-04T22:20:37Z

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

webgago · 2017-10-05T06:16:10Z

+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'

pjg · 2017-10-05T09:43:27Z

lib/loofah/html5/whitelist.rb

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

+      ACCEPTABLE_CSS_FUNCTIONS = Set.new %w[calc]


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.

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.

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 :)

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.

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.

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.

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

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 · 2017-10-10T18:06:54Z

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

pjg · 2017-10-10T18:20:48Z

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 · 2017-10-17T16:49:56Z

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

ramzizaz · 2017-10-26T16:54:04Z

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

NikoRoberts · 2017-10-26T20:00:31Z

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

ramzizaz · 2017-10-26T20:38:12Z

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

ramzizaz · 2017-11-02T20:34:12Z

@flavorjones any update on the merge timing?

ramzizaz · 2017-11-21T17:27:31Z

@flavorjones any idea on merge timing?

tschoppi · 2017-11-29T14:57:45Z

@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 · 2017-12-18T14:44:47Z

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

NikoRoberts · 2018-01-11T18:05:20Z

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

tschoppi · 2018-02-06T12:27:46Z

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

flavorjones · 2018-02-06T13:39:58Z

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

flavorjones · 2018-02-06T13:48:59Z

Planned release is 2.2.0

[fixes #129] [related to #122] [related to #123]

flavorjones · 2018-02-11T20:25:59Z

Note I've whitelisted rgb as well in 6b81467

sudoremo · 2018-02-12T13:16:18Z

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

flavorjones · 2018-02-12T13:58:47Z

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

flavorjones · 2018-10-28T19:02:14Z

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

still omit `url` and `image` related to #122 and #123 also see #143

NikoRoberts · 2018-11-06T00:04:17Z

@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 · 2018-11-12T17:09:44Z

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

NikoRoberts · 2018-11-12T19:53:05Z

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 · 2018-11-14T04:20:46Z

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

added ability to whitelist particular functions #122

1bc5a45

pjg reviewed Oct 5, 2017

View reviewed changes

oluwayetty mentioned this pull request Nov 8, 2017

Feature/aperta 11087 enable indentation and list styles in tinymce Aperta-project/Aperta#3487

Closed

6 tasks

tschoppi mentioned this pull request Nov 29, 2017

Problem with scrub_css when value is rgb #129

Closed

flavorjones merged commit a414233 into flavorjones:master Feb 6, 2018

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

whitelist CSS function rgb

6b81467

[fixes #129] [related to #122] [related to #123]

flavorjones added a commit that referenced this pull request Oct 28, 2018

expand set of allowed CSS functions

efb182c

still omit `url` and `image` related to #122 and #123 also see #143

flavorjones assigned flavorjones and unassigned flavorjones Dec 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ability to whitelist particular functions #122 #123

Added ability to whitelist particular functions #122 #123

NikoRoberts commented Sep 21, 2017

flavorjones commented Sep 24, 2017

JackLaBarba commented Oct 4, 2017

webgago commented Oct 5, 2017

pjg Oct 5, 2017

NikoRoberts Oct 5, 2017

pjg Oct 5, 2017

NikoRoberts Oct 5, 2017

pjg Oct 5, 2017 •

edited

NikoRoberts Oct 5, 2017

pjg Oct 5, 2017

pjg Oct 5, 2017

NikoRoberts Oct 5, 2017

oluwayetty commented Oct 10, 2017

pjg commented Oct 10, 2017

ramzizaz commented Oct 17, 2017

ramzizaz commented Oct 26, 2017

NikoRoberts commented Oct 26, 2017

ramzizaz commented Oct 26, 2017

ramzizaz commented Nov 2, 2017

ramzizaz commented Nov 21, 2017

tschoppi commented Nov 29, 2017

sudoremo commented Dec 18, 2017

NikoRoberts commented Jan 11, 2018

tschoppi commented Feb 6, 2018

flavorjones commented Feb 6, 2018

flavorjones commented Feb 6, 2018

flavorjones commented Feb 11, 2018

sudoremo commented Feb 12, 2018

flavorjones commented Feb 12, 2018

flavorjones commented Oct 28, 2018

NikoRoberts commented Nov 6, 2018

flavorjones commented Nov 12, 2018

NikoRoberts commented Nov 12, 2018

flavorjones commented Nov 14, 2018

Added ability to whitelist particular functions #122 #123

Added ability to whitelist particular functions #122 #123

Conversation

NikoRoberts commented Sep 21, 2017

flavorjones commented Sep 24, 2017

JackLaBarba commented Oct 4, 2017

webgago commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjg Oct 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oluwayetty commented Oct 10, 2017

pjg commented Oct 10, 2017

ramzizaz commented Oct 17, 2017

ramzizaz commented Oct 26, 2017

NikoRoberts commented Oct 26, 2017

ramzizaz commented Oct 26, 2017

ramzizaz commented Nov 2, 2017

ramzizaz commented Nov 21, 2017

tschoppi commented Nov 29, 2017

sudoremo commented Dec 18, 2017

NikoRoberts commented Jan 11, 2018

tschoppi commented Feb 6, 2018

flavorjones commented Feb 6, 2018

flavorjones commented Feb 6, 2018

flavorjones commented Feb 11, 2018

sudoremo commented Feb 12, 2018

flavorjones commented Feb 12, 2018

flavorjones commented Oct 28, 2018

NikoRoberts commented Nov 6, 2018

flavorjones commented Nov 12, 2018

NikoRoberts commented Nov 12, 2018

flavorjones commented Nov 14, 2018

pjg Oct 5, 2017 •

edited