SMF bug 4953 (PS adds 'cellTextIsHtml' to JPGs causing them to fail checks)

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
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.
When we unite against a common enemy that attacks our ethos, it nurtures group solidarity. Trolls are sensational, yes, but we keep everyone honest. | Game Memorial

Nao

  • Dadman with a boy
  • Posts: 16,064

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
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...

Nao

  • Dadman with a boy
  • Posts: 16,064

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
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?

Pandos

  • Living on the edge of Wedge
  • Posts: 635
Re: SMF bug 4953 (PS adds 'cellTextIsHtml' to JPGs causing them to fail checks)
« Reply #5, on March 2nd, 2012, 07:36 PM »Last edited on March 2nd, 2012, 07:44 PM by Pandos
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.
# dpkg-reconfigure brain
error: brain is not installed or configured

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278

Nao

  • Dadman with a boy
  • Posts: 16,064
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