XSS? #102

Open
dvv opened this Issue Aug 23, 2011 · 8 comments

Comments

Projects
None yet
5 participants

dvv commented Aug 23, 2011

(npm version)

"About me" field allows to include <script>alert('hacked')</script> and it gets executed successfully in when viewing the profile.

Best regards,
--Vladimir

Owner

cliftonc commented Aug 23, 2011

Well spotted, I'll do a run through to make sure we are scanning all inputs
properly.

On 23 August 2011 17:50, dvv <
reply@reply.github.com>wrote:

(npm version)

"About me" field allows to include <script>alert('hacked')</script> and
it gets executed successfully in when viewing the profile.

Best regards,
--Vladimir

Reply to this email directly or view it on GitHub:
#102

Collaborator

dennishall commented Aug 23, 2011

That sounds like a good interim solution.

This should probably be partly the job of the content type to declare whether a given field should allow [ no html, filtered html, or full html ] + defined per role. Of course, on further thought, that should probably only apply to textareas, and all other fields should most likely simply disallow any form of html.

dvv commented Aug 23, 2011

I think the problem is not in the input, but rather in output. No need to limit user's input (<script>alert('hacked')</script> is a pretty valid description of someone's profile :) really.), but output should always be escaped (maybe at layout template level?).

Collaborator

dennishall commented Aug 23, 2011

Good point. I had mistakenly considered this as input filtering. So, to combine your point with mine, the output should be filtered as one of [plain text, filtered html, full html]. This article focuses on your point, but also touches on mine: http://www.lullabot.com/articles/drupal-input-formats-and-filters

Collaborator

dennishall commented Aug 23, 2011

Just to round out this conversation a little more, it should be noted that input filtering will probably give better performance for most cases (filter once, on db insert/update, instead of filtering every time it's viewed) - at the cost of some content flexibility. (Depending on the performance difference between input and output filtering, that might be an easy choice for most people.)

@cliftonc cliftonc added a commit that referenced this issue Aug 24, 2011

@cliftonc cliftonc Ensure all fields escaped in output via = rather than - in EJS (as pe…
…r issue #102), need to check across all modules.
9665b3a
Collaborator

dennishall commented Aug 24, 2011

One potential method of sanitization - albeit probably heavy+/expensive - might be to use jsdom (+jquerify if needed) to get innerText.

Another potential method - but labor-intensive: port htmLawed to js [http://www.bioinformatics.org/phplabware/internal_utilities/htmLawed]. NOTE: It also has some useful test cases that we might want to run against whatever solution is used.

Similar, but already written in js - This question, http://stackoverflow.com/questions/1637275/simple-html-sanitizer-in-javascript, is answered with a link to a plugin for the 'caja' library: http://code.google.com/p/google-caja/source/browse/trunk/src/com/google/caja/plugin/html-sanitizer.js

Contributor

arosboro commented Apr 15, 2013

I made a modification to use the sanitizer npm module which uses caja as dennishall suggested. Changed the content and user modules to sanitize output. Pull request to follow.

Collaborator

richtera commented Jun 29, 2013

The sanitizer stuff causes problems.
Do you know how to configure the sanitizer? I is removing links inside of each post.
So if the post has an <a href="..."> it ends up with <a> only removing href.
I understand that a href can pose a problem but in a CMS you need to be able to post items which have HTML that might be considered problematic since the posts are not just rich text. It looks like the sanitizer assumes we're just dealing with HTML formatted text and not HTML pages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment