2

I'm working on a web framework and am trying to build XSS prevention into it. I have set it up so it will escape incoming data for storage in the database, but sometimes you want to save html that the user generates. I am trying to make a custom tag that will prevent any javascript from executing, here is my first hack at it:

<html>
  <head>
    <script type="text/javascript" src="/js/jquery.min.js"></script>
  </head>
  <body>

    <preventjs>
      <div id="user-content-area">
        <!-- evil user content -->
          <p onclick="alert('evil stuff');">I'm not evil, promise.</p>
          <p onmouseover="alert('evil stuff');">Neither am I.</p> 
        <!-- end user content -->
      </div>
    </preventjs>

    <script type="text/javascript">
      // <preventjs> tags are supposed to prevent any javascript events
      // but this does not unbined DOM events
      $("preventjs").find("*").unbind();
    </script>

  </body>
</html>

I tried using jQuery to unbind everything, but it doesn't unbind events in the DOM, which is exactly what I'm trying to do. Is it possible to unbind all events for a DOM element?

regality
  • 6,496
  • 6
  • 29
  • 26

4 Answers4

4

You're problem is that you are doing this on the wrong end of things -- you should be filtering all user input of potentially hostile content when you receive it.

The first rule of thumb when doing this is "always whitelist, never blacklist". Rather than allowing any and all attributes in your user-generated HTML, simply keep a list of allowed attributes and strip away all others when you receive the HTML (possibly on the client side -- definitely on the server side.)

Oh, and HTML is not a regular language. You'll want to use an HTML parser, not a regular expression for this task.

Sean Vieira
  • 155,703
  • 32
  • 311
  • 293
  • Can you recommend an HTML parser? I'm coding everything in PHP on the server side. – regality Aug 25 '11 at 19:37
  • @regality -- see the answers to this question for some good ones: http://stackoverflow.com/questions/292926/robust-mature-html-parser-for-php – Sean Vieira Aug 25 '11 at 19:38
2

.unbind will only unbind events attached using jQuery. You can get rid of inline event handler code by setting them to null, e.g.:

$("preventjs *").removeAttr("onclick").removeAttr("onmouseover");

Demo.

EDIT: Here's an evil solution, you can remove all attributes starting with "on":

$("preventjs *").each(function() {
    var attribs = this.attributes;
    var that = this;
    $.each(attribs, function(i, attrib) {
        if(attrib.name.indexOf("on") === 0) {
            $(that).removeAttr(attrib.name);
        }
    });
});

Demo.

karim79
  • 339,989
  • 67
  • 413
  • 406
  • I thought of doing something like that, but there are a lot of obscure events that I don't want to hunt down, keep track of, and keep up to date. – regality Aug 25 '11 at 19:26
  • That's creative. Are there any attributes that could slip through or that would be removed and shouldn't? – regality Aug 25 '11 at 19:41
  • @regality - I don't know, are there any non-eventy ones which start with "on"? I doubt it. – karim79 Aug 25 '11 at 19:42
  • @karim - the only issue with this is the `onload` and `onerror` attributes, which will fire before this code can get to it - otherwise, quite impressive! – Sean Vieira Aug 25 '11 at 19:51
1

The problem is that you've inline handlers. unbind cannot remove inline handlers.

<p onclick="alert('evil stuff'...
   ^^^^

To remove inline handlers, use removeAttr

$("preventjs").find("*").removeAttr('onclick');
$("preventjs").find("*").removeAttr('onmouseover');
Mrchief
  • 75,126
  • 20
  • 142
  • 189
0

You can unbind the events individually:

$('p').each(function(){ this.onclick = this.onmouseover = undefined; });

If you want to unbind other events like mouseout you have to add them to that list:

$('p').each(function(){ this.onclick = 
                          this.onmouseover = 
                            this.onmouseout = undefined; });

Of course you'll want to use a selector other than $('p'), I just didn't want to put your other one because preventjs is not an HTML tag

Paul
  • 139,544
  • 27
  • 275
  • 264