1

A friend of me posted a code about how to prevent xss attack using DOM.

What do you think about this code ? Can we optimize it ?

<?php
    function parseDoc(DOMDocument $codeHtml){
      $forbiddenTag=array('script');
      $forbiddenAttr=array('onmouseover','onmouseup','onclick');
      foreach($forbiddenTag as $tag){
        $liste=$codeHtml->getElementsByTagName($tag);
        foreach($liste as $element){
          $codeHtml->removeChild($element);
        }
      }
      stripAttr($codeHtml,$forbiddenAttr);
    }

    function stripAttr(DOMNode $root, array $forbiddenAttr){
     foreach($rootl->childNodes as $child){
        foreach($forbiddenAttr as $attr){
          if($child->hasAttribute($attr)) $child->removeAttribute($attr);

        }.
        if($child->hasChildNodes())strippAttr($child,$forbiddenAttr);
      }
    }
Zeroth
  • 83
  • 1
  • 8

1 Answers1

2

This is not the correct way to combat XSS.

You're using a blacklist that will eternally fail to catch all ways to include scripts. For example, you're not catching the onload attribute or javascript: links. Instead, always use DOM methods to construct text nodes and attribute values, and you will be safe by default. If you want to have users allow formatted text, use a whitelist of allowed elements, attributes, and attribute values.

phihag
  • 278,196
  • 72
  • 453
  • 469
  • @Zeroth I don't know any php function by that name. Can you provide a link to it? – phihag Aug 26 '11 at 10:08
  • Sorry its late here and i didn't think well: http://en.wikipedia.org/wiki/DOM_Events – Zeroth Aug 26 '11 at 10:09
  • If I'm understanding you correctly, you want to use a list of all DOM events as a blacklist. Note that there are other ways than event properties (and weird stuff like `` in some browsers or [JavaScript in style sheets](http://stackoverflow.com/questions/3607894/cross-site-scripting-in-css-stylesheets)) to include JavaScript. Also, there will be new DOM events in the future which you can't possibly handle now. You should really use a whitelist instead. – phihag Aug 26 '11 at 10:15
  • I didn't know it was possible to include JS on CSS sheets... Actually which DOM events could be really dangerous for XSS attack ? – Zeroth Aug 26 '11 at 10:26
  • @Zeroth All of them. Why don't you use a whitelist? – phihag Aug 26 '11 at 10:27
  • 1
    Use htmlpurifier instead. It's whitelist based. Building a blacklist of DOM events is a bad idea as HTML5 is constantly evolving and adding new events. – Erlend Aug 27 '11 at 07:51