xss_clean on images

[eluser]Seppo[/eluser]
I was checking out the xss_clean on images new feature... and it just doesn't work as expected... out of 16 JPG images I had on my Computer, only 4 passed this test, most of them have a "<?" inside... also lines 754-755 (at revision 1171) says

Code:

* Adobe Photoshop puts XML metadata into JFIF images, including namespacing,
* so we have to allow this for images. -Paul

but it's not useless to allow xmlns if you are not allowing xml (forbidding the use of "<?") so no photoshop / jfif image is accepted, anyway (I tried creating a couple of jfif with photoshop and all had "<?")...
I understand the risks of allowing uploads without XSS clean up, but this functionality, as is, seems to be too much restrictive, up to a level that no site can really use it...

Any thoughts?

[eluser]Pascal Kriete[/eluser]
It's an iffy decision to make. On one hand developers should know that allowing people to post images can be harmful. On the other hand some browsers (cough opera cough) allow all kinds of nonsense in image tags.

This same line has been in EE for a while, although the rest of the class is less restrictive. Is it just that one line that's causing problems?

I do have to agree that with that many false positives it's not a great solution.

[eluser]Seppo[/eluser]
By that line I supposed you mean the line after that comment

Code:

unset($event_handlers[array_search('xmlns', $event_handlers)]);

I had no problem with it... it just allow something that it will forbid by some other rule...

Just thinking on this, a good improvement might be not to check for "<?" if short_open_tag is disabled... this way I have raised the number of allowed images to 9...

I'm trying to look for why it's failing on my other 7 images

[eluser]Pascal Kriete[/eluser]
I was referring to the opening tag. Since that one also has a comment that xml will get stripped as well. Time to crack open photoshop and make some doodles.

[eluser]Seppo[/eluser]
Yes, definetly the php short open tag and the php close tag are breaking most of them.

Also, I have one that it's breaking on line 760

Code:

/*
         * Remove JavaScript Event Handlers
         *
         * Note: This code is a little blunt.  It removes
         * the event handler and anything up to the closing >,
         * but it's unlikely to be a problem.
         *
         */
        $event_handlers = array('onblur','onchange','onclick','onfocus','onload','onmouseover','onmouseup','onmousedown','onselect','onsubmit','onunload','onkeypress','onkeydown','onkeyup','onresize', 'xmlns');

        if ($is_image === TRUE)
        {
            /*
             * Adobe Photoshop puts XML metadata into JFIF images, including namespacing,
             * so we have to allow this for images. -Paul
             */
            unset($event_handlers[array_search('xmlns', $event_handlers)]);
        }
        
        $str = preg_replace("#<([^>]+)(".implode('|', $event_handlers).")([^>]*)>#iU", "&lt;\\1\\2\\3&gt;", $str);

but it's weird... I added a count parameter and it's returning 0, and, of course, my image does not have any js events, so I'm not sure what that can be... probably some issue with binary-safe and windows...

[eluser]Pascal Kriete[/eluser]
I've done some testing, and I'm starting to question if this is even worth it. I made a dirty image of Elmo (this sounds wrong). The only browser that responded to it was IE6 (on darwine). Safari, Opera, and FF all ignored it. I can't test on IE7, but if Microsoft has fixed this issue one might consider adding more ifs to slacken the rules for images.

File names on the other hand should be checked and/or renamed.

[eluser]Seppo[/eluser]
can you upload the "dirty" image so I test it under IE 7?

[eluser]Pascal Kriete[/eluser]
Ok, so it works in IE7. It does end up being a problem then. Granted, that was a really simple script and gets caught by the class. That puts us back to square one. Lots of false positives don't help anyone, but clearly we need some level of cleaning.

I think your event handler problem might've just been some freaky decoding issue. Which really doesn't make this any easier. I really don't think there is a good solution for this short of disallowing IE on your site .

[eluser]champs[/eluser]
Global XSS filtering manages to get in the way just often enough that I have to disable it. It's a shame, because you don't want to set xss_clean validation on every field, and there's no way for the Validation class to tell the input class "hey, just don't filter this input." But even if you did... these are the fields that need the most filtering. It's a can't-win situation.

[eluser]Derek Jones[/eluser]
Yes, especially when trying to provide security for binary files, it's a balance, and we're never going to strike a balance that satisfies every individual or every security need. In ExpressionEngine, this has been employed fairly successfully for about a year or so. Super Admins are exempted from the filter, and there's an option to disable it entirely if one wishes. It can be pretty intensive both in terms of memory usage and as noted here, can generate false positives with ease, so it's definitely a personal decision.

The reason we protect against short tags in images even when short tags is disabled on the server is two fold. First, there's no way to know that that environment variable might not change in the future, leaving a previously safe image on your site suddenly a source of a scripting attack. Secondly, it's about being good internet citizens. If it is assumed that one wishes to only allow known-safe images to be accepted for a particular purpose on your own site, it is also assumed that you wouldn't want to let images through that were only safe on your server, as an unsuspecting individual might save that image and reuse it in an environment that was harmful.