Wedge

Public area => The Pub => Bug reports => Topic started by: Arantor on March 2nd, 2012, 02:21 PM

Title: SMF bug 4953 (PS adds 'cellTextIsHtml' to JPGs causing them to fail checks)
Post by: Arantor on March 2nd, 2012, 02:21 PM
There are a surprising number of ways to get false positives in images, and I'm not sure how useful the current code is in stopping threats.
Posted: March 2nd, 2012, 10:55 AM

I remember discussing this issue with several people at SMF when this originally blew up way back.

The problem that isn't really being solved here is that you have legitimate images (and other files) being uploaded and checked for common things that could be malicious - but not nearly all of the things that could be, and unfortunately blocking others in the process like this one. The problem is that you can't realistically blacklist certain constructions and then provide exceptions, unless you're making very specific exceptions - and even then it's not reliable.

What might be better to do is to do separate types of validation based on the file type (assuming it's an image that you're trying to validate) and attempt to make sense of the file itself, e.g. if it's a GIF, step through and validate the image contents vs its headers and if there's any extra content, dump it. (Being sure to validate for animated images, of course)

It doesn't help that GIF, PNG and JPEG all legitimately allow for extra non-image information to be embedded in less than pleasant ways.
Title: Re: SMF bug 4953 (PS adds 'cellTextIsHtml' to JPGs causing them to fail checks)
Post by: Nao on March 2nd, 2012, 06:00 PM
Would like to see what spuds' fix on this was.

Well, I don't care much about this bug actually... :^^;:
Title: Re: SMF bug 4953 (PS adds 'cellTextIsHtml' to JPGs causing them to fail checks)
Post by: Arantor on March 2nd, 2012, 06:12 PM
Ah, but we should - more than SMF does, in fact.

If there is any chance of a file being served to the user that we did not have a hand in (like attachments), we need to be confident that the file isn't malicious. And it's not enough to validate the image with getimagesize() because it's possible to hide even shell scripts inside legitimate images...
Title: Re: SMF bug 4953 (PS adds 'cellTextIsHtml' to JPGs causing them to fail checks)
Post by: Nao on March 2nd, 2012, 06:25 PM
I mean I don't really care in the sense that I'd rather not fix it, i.e. consider it as harmful if there's any reason to be suspicious at all.

I'm not a security specialist, though. But I don't think AeMe was ever found to have a security hole...?
Title: Re: SMF bug 4953 (PS adds 'cellTextIsHtml' to JPGs causing them to fail checks)
Post by: Arantor on March 2nd, 2012, 06:50 PM
Neither, directly, does SMF have the same vulnerability (because it isn't strictly a vulnerability), provided the same thing that SMF does (blocking PHP execution via htaccess for the entire media folders) is done.

But that's not to say that browsers don't have vulnerabilities to bad images, it has been known. How far do we go to protect users?
Title: Re: SMF bug 4953 (PS adds 'cellTextIsHtml' to JPGs causing them to fail checks)
Post by: Pandos on March 2nd, 2012, 07:36 PM
Why not validate the image header by using the PHP function called getimagesize ?
 If the image validation is invalid, e.g. the header is incorrect, the function will return a false.

OK, this will not prevent malicious files with correct headers (manipulated) from being uploaded, but most "scritpkiddys" will be kept away.
Title: Re: SMF bug 4953 (PS adds 'cellTextIsHtml' to JPGs causing them to fail checks)
Post by: Arantor on March 2nd, 2012, 08:56 PM
I have a feeling it already is, not sure, but I don't think that's enough.
Title: Re: SMF bug 4953 (PS adds 'cellTextIsHtml' to JPGs causing them to fail checks)
Post by: Nao on March 3rd, 2012, 10:49 AM
I don't think there's a getimagesize protection. Couldn't find any.

Found this, thought, it's funny, in Subs-Post:

   list ($attachmentOptions['width'], $attachmentOptions['height']) = empty($size) ? array(null, null, null) : $size;

Just because $size returns three entries doesn't mean a fallback should have three as well... :P