Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #3899 (closed defect: duplicate)

Opened 2 years ago

Last modified 1 year ago

TextHelper.sanitize(html) doesn't fully protect against XSS

Reported by: otto@atrus.org Assigned to: David
Priority: normal Milestone: 1.2.4
Component: ActionPack Version: 1.0.0
Severity: normal Keywords:
Cc:

Description

From the rdoc ActionView::Helpers::TextHelper.sanitize(html) :

Sanitizes the given HTML by making form and script tags into regular text, and removing all "onxxx" attributes (so that arbitrary Javascript cannot be executed). Also removes href attributes that start with "javascript:".

Unfortunately, this is not nearly enough. This list should help people get started. I recommend using a whitelist of tags and attributes. The reason for not using a blacklist is because browsers add features and whatnot. The new -moz-binding CSS property in Firefox 1.5 is a prime example.

Change History

03/04/06 13:28:29 changed by otto@atrus.org

  • severity changed from normal to critical.

I hate raising the severity, etc. of my own bug reports and was hoping someone else would adjust it to Rails norm. It hasn't happened, though, so I'm marking what I feel. This bug introduces vulnerabilities in a lot of Rails apps :(

03/27/06 18:57:10 changed by anonymous

  • keywords set to needs_review.
  • milestone set to 1.1.

All it takes is one exploit to make headlines and dominate search engine results about rails (when all the Java sites link to the story). I hope this gets fixed too.

In the meantime, use mod_security (Apache module) to prevent XSS and SQL injection attacks (and many other forms of attacks). I've been using mod_security-1.9.2 and it works great.

03/28/06 06:48:57 changed by david

  • keywords deleted.
  • severity changed from critical to normal.

NOTHING will rid your app entirely of XSS short of escaping HTML. This is just going to do some of it. And hackers will come up with more. Do work on an alternative feature that uses a white list instead.

06/18/07 14:32:17 changed by josh

  • status changed from new to closed.
  • resolution set to invalid.

06/18/07 15:58:56 changed by nikolasco

  • status changed from closed to reopened.
  • resolution deleted.

Reopening because I don't see why this qualifies as "invalid." David said to work on alternative, after all. If a new ticket was created for the alternative and this one was closed in favor of it, that would make sense. At the moment it seems that at least "wontfix" makes more sense.

06/19/07 13:03:39 changed by jeremymcanally

  • status changed from reopened to closed.
  • resolution set to duplicate.

A patch in the right direction has been provided here: http://dev.rubyonrails.org/ticket/8222

If it's not the same issue, then please feel free to reopen.

06/19/07 14:22:05 changed by nikolasco

I hate playing close/reopen ... That's another partial fix; In bugzilla land, I'd probably mark this ticket as depending on #8222. Sorry to be such an annoyance ... The way this ticket is worded makes it suitable for either a whitelist-based sanitize OR a meta-ticket for the variety of fixes for the current blacklist version. If it's for a whitelist-based sanitize, making another ticket that clearly states that seems to be the way to go (the current one is clearly confusing). I don't think Trac supports meta-tickets in any useful way (a wiki page listing "problems with sanitize" would be more useful), so it would be reasonable to close this.

To summarize, my current plan is to:

1. Leave this bug closed

2. Create a new ticket for the whitelist-based sanitize

3. Maybe do some searches for holes in the blacklist and create a wiki page pointing to tickets to fix them

Is that reasonable? I'll try to ask on IRC later if a decision hasn't been made here.