Security issues with PHP's include
Pretend that we are writing a web app here, but we are a little naïve. We want to display images that are uploaded from the public on our web app, except for bandwidth conservation we want to add far future expiry headers.
<?php
$file = 'image.gif';
$expiry = 12960000;
$imageInfo = getimagesize($file);
$mime = $imageInfo['mime'];
$modified = filemtime($file);
$etag = md5_file($file);
if (file_exists($file)) {
if (strtotime($_SERVER['HTTP_IF_MODIFIED_SINCE']) == $modified
|| $_SERVER['If-None-Match'] == $etag) {
header('HTTP/1.1 304 Not Modified');
}
$headers = array(
'Content-Type' => $mime,
'Expires' => date(DATE_RFC1123, time() + $expiry ),
'Cache-Control' => "max-age=$expiry, must-revalidate",
'Last-Modified' => date(DATE_RFC1123, $modified),
'Content-Length' => filesize($file),
'Etag' => $etag
);
foreach($headers as $type => $value) {
header("$type: $value");
}
include $file;
exit;
}Looks alright, doesn't it? There is one massive security hole there, and it is still present even if we validate the MIME type. The problem is the include, it should be a readfile(). Imagine if a GIF file was uploaded with the following content.
GIF89a<?php
file_put_contents('test.txt', 'oh dear');Yep, you'd have a nasty extra file alongside your script. Of course, the example is trivial. You could do all sorts of nasty things. And getimagesize() still reports the MIME as image/gif, which it is, technically, but checking for it is useless.
I've read some suggestions for combating this on Stack Overflow. Basically, they are...
- Run images through another image manipulation tool, save that output and throw away the original.
- Disallow uploading of image files with arbitrary extensions.
- Place the uploaded images in a directory that can not run PHP.
- Rename the images uploaded. This will add a little bit more security.
I just ran some tests too, and it seems that exif_imagetype() also won't be able to validate the image, as it is returning the GIF constant always (it doesn't care what the image is, I guess as long as it can read the GIF header).
Comments
Alexander Dickson
Posted on Sunday, 5th September 2010 @ 6:30pm.@Russel Dias
I think you will only lose quality with JPEGs, as PNGs are a lossless format. I think you could even scrub JPEG files too without reducing the file size (so long as the software doesn't re-encode as a new JPEG).
Probably the best idea is to scrub JPEGs for any EXIF data, and PNGs & GIFs for their meta equivalent for privacy reasons and for the small bandwidth saved.
Russell Dias
Posted on Sunday, 5th September 2010 @ 6:05pm.Copying the original image with something like GD (and I think even ImageMagick) will reduce the quality of the image slightly. But the security benefits far outweigh this diminishing return.
I feel copying a client image should be a standard. Some image types can store enormous data about an individual. Copying the image and getting rid of the original is almost doing the client a favor.
Leave a Comment
Note: Your comment may require approval before it is posted to the site.