Prevent image upload code injection

I have a form which the user fills with lyrics information and uploads an album cover. Submitted data will be inserted into a database and the album cover will be moved to a subfolder.

localhost/project-folder/covers

I've taken some precautions (escaping, prepated statements) against SQL Injection for the form input. Recently, I've learned that I also need to take precautions for file (image) upload, that the user can upload malicious images.

For example, adding HTML, JS or PHP code in image metadata, or embedding code directly into the image file. Since I haven't used PHP extensively, I don't how how this poses a problem, especially in my case.

I'm doing the form validation on the server-side.

localhost/project-folder/lyrics/add.php

<form action="../scripts/lyrics/submit_lyrics.php" id="lyricsForm" method="post" autocomplete="off" enctype="multipart/form-data">

localhost/project-folder/scripts/lyrics/submit_lyrics.php

$form_data  = new FormData(["artist", "album", "song", "year", "track_no", "lyrics"], "sssiis");
$file_data  = new FileUpload("cover", [
    "max_file_size" => 512 * 1024,
    "extensions"    => ["gif", "jpg", "jpeg", "png"],
    "mimes"         => ["image/gif", "image/jpeg", "image/png"],
    "max_width"     => 1024,
    "max_height"    => 1024,
]);
$cover = new Cover($mysqli, $form_data, $file_data, BASE."covers/");

Validation is done at the initialization of FormData and FileUpload. If there is an invalid field or the uploaded image is invalid, user is redirected back to the form page (add.php) with the corresponding warnings.

One of the ways to prevent malicious image upload that I've read was to create a new image from the uploaded one, and that is what I'm doing inside new Cover(). I also resize the uploaded image so this approachs works. I'm doing the resizing with this function:

public function new_image($file_data, $new_width, $new_height) {
    $img_data   = file_get_contents($file_data->tmp_name);
    $image_type = $file_data->type;
    $img_create = null;
    switch ($image_type) {
        case IMAGETYPE_GIF:
            $img_create = "imagecreatefromgif";
            break;
        case IMAGETYPE_JPEG:
            $img_create = "imagecreatefromjpeg";
            break;
        case IMAGETYPE_PNG:
            $img_create = "imagecreatefrompng";
            break;
    }
    $uploaded_image_resource = $img_create($file_data->tmp_name);
    $new_image_resource      = imagecreatetruecolor($new_width, $new_height);
    imagecopyresampled($new_image_resource, $uploaded_image_resource, 0, 0, 0, 0, $new_width, $new_height, $file_data->image["width"], $file_data->image["height"]);
    return $new_image_resource;
}

public function write_to_disk() {
    if (isset($this->image["resource"])) {
        $destination = $this->target_dir . $this->file_name . ".jpg";
        imagejpeg($this->image["resource"], $destination);
        imagedestroy($this->image["resource"]);
    }
}

This resizing also removes (I think) any code in metadata and/or code embedded in the image (if there are any) since I'm creating a new clean image.

Is this enough for file upload protection? Am I missing anything? Are there any other things that I need to be aware of?

For example, adding HTML, JS or PHP code in image metadata, or embedding code directly into the image file. Since I haven't used PHP extensively, I don't how how this poses a problem

In principle it shouldn't: if you serve the image back up to the end user with a correct media type such as image/jpeg, it should be treated and rendered as an image only.

However there are tools around which ignore that type information and can treat the content as a more dangerous type:

old browsers, particularly IE, would sniff the contents of a file to guess what type it might be, and inclusion of HTML tags in the content of the file caused it to render it as HTML instead of an image. User-supplied HTML = cross-site-scripting.
plugins; historically Java would treat any resource embedded by a third-party site as an applet, and a Flash plugin on another site could use loadPolicyFile against the file to reinterpret the contents as a crossdomain.xml policy, opening up cross-site-scripting
things aren't as bad today as these have been mitigated in various ways - for example in the squirrel example that's served as text/html and only being re-interpreted as image/jpeg, a less-powerful type (downsniffing). However, we don't really have any strong commitment that files won't we re-used as a different type on the web platform, by current or future tools.

This resizing also removes (I think) any code in metadata and/or code embedded in the image (if there are any) since I'm creating a new clean image.

It will certainly remove metadata; actually just imagecreatefromX then imagejpeg will do that, because the PHP image object doesn't retain metadata.

However it won't necessarily change the content of the image itself. In theory, an attacker who knows what image compressor you are using could construct a picture which, when compressed by that code, output a string of attacker-chosen bytes, which could be misinterpreted unsafely as above.

Is this a likely attack? No. I think I could probably pull it off against a simple lossless compressor like GIF or PNG's, but the more complicated, less-predictable lossy compression in JPEG would likely make it much trickier and more time-consuming.

I expect the image load-and-save dance would protect you from the vast majority of casual attackers (and there are certainly other good reasons to it, like ensuring a certain image size and format), but it's not a hard-and-fast guarantee of safety against XSS upload attacks.

If you need better than that, or you need to allow upload of other arbitrary filetypes where you can't re-image-compress them, the approach most serious sites go for is to serve user-uploaded content only from a separate hostname(*) so that if any kind of XSS attack succeeds against it your main site isn't affected.

(*: ideally, at some further expense, even a separate domain name. Either way the user-content hostname must not be a subdomain of the main site, otherwise it may be able to read session cookies from it and compromise it that way.)

Recommended topics

Hot tags