2
\$\begingroup\$

Is it safe to upload images via MySQL in Blob? I've constructed my code like the following:

    public function upload_avatar($file, $user_id = false, $maxbytes = 102400) {
    global $sql;
    if($user_id == false) {
        $user_id = $this->_id;
    }
    $check = getimagesize($file);
    if($check == false) {
        return false;
    }
    $width = $check[0];
    $height = $check[1];
    if($width > 300) {
        return false;
    }
    if($height > 300) {
        return false;
    }
    $fp = fopen($file, 'rb');
    if(strlen($file) < $maxbytes) {
        $q = $sql->prepare("UPDATE `users` SET `avatar` = :file WHERE id = :user_id;");
        try {
            $q->bindParam(':file', $fp, PDO::PARAM_LOB);
            $q->bindParam(':user_id', $user_id);
            $q->execute();
        }
        catch(PDOException $e) {
            die($e->getMessage());
        }
        return true;
    } else {
        return false;
    }
}

Is it possible for a hacker to upload viruses via this script?

\$\endgroup\$
1
  • \$\begingroup\$ ..What should we review about your code? \$\endgroup\$
    – Gonz
    Jan 4, 2015 at 11:11

1 Answer 1

1
\$\begingroup\$

Nitpicks

public function upload_avatar($file, $user_id = false, $maxbytes = 102400) {
global $sql;

These two lines are indented inconsistently. The first line should be indented less than the second. Perhaps a copy/paste error.

There's an argument that the global line should be used closer to where the $sql variable is used.

I personally don't like the use of $sql to represent a database connection. To me, SQL is a language rather than a generic name for a database.

To be consistent with the other names (e.g. $user_id), $maxbytes should be $max_bytes.

I'd like this function better if it had more vertical whitespace. In particular, I would expect a blank line after global $sql; to separate the declaration from the code that follows. Then another line after the if block to separate the user ID check from the code relating to the getimagesize call. This is called paragraphing, and it breaks your function into smaller segments that are easier to read and process.

$check = getimagesize($file);
if($check == false) {
    return false;
}

This is the security portion of your code. It verifies that the file is something that getimagesize would consider to be an image. You should probably comment this, as it is rather important in the context of this function. It's also not immediately obvious that it is security related. The more common interpretation would be that you are failing here purely because you can't get the size information.

I might call $check $image_data instead. That would be clearer about what is being returned.

$width = $check[0];
$height = $check[1];

Technically speaking, this is unnecessary. You only use those values once each and could just use them rather than reassigning them. However, this does make it considerably clearer what the code is doing.

if(strlen($file) < $maxbytes) {

This is unlikely to be doing what you want. Presumably you want to check if the image size is less than $maxbytes. But what this actually does is check that the file name is less than $maxbytes.

        $q->bindParam(':user_id', $user_id);

This is binding $user_id as a string. If $user_id is numeric, you may be better off telling it so.

Security Issues

As I said earlier, you are checking that $file is an image with the getimagesize function. Unfortunately though, this might not work. The problem is that in PHP $file can be either a local file or a remote file. If it's a local file, then this should be fine so long as the file doesn't change from under you. Since it's local, hopefully you can control that.

If it's a remote file, then someone else can answer the getimagesize with one file and then return a different file to the fopen. They control the file, so the first access can be to a nice image and the second access to a dangerous file. So if this function is actually doing the upload, because you are passing it a URL, then it is insecure. You should get the file before doing the getimagesize (or do getimagesize twice: once before and once after). That way you can ensure that the getimagesize file is the same as the fopen file.

You can also use the filesize function to check the file size of a local file against $maxbytes. That may not work with a remote file.

Performance

This approach does put more pressure on the database. You are storing static image content in the database. It's not clear that you need to do this. It would generally be easier to store the image on your server and supply it from there. If you have multiple servers, you could designate one to hold the content. Or you can use a service like Amazon's S3 to hold your images and load them from there.

It's also unclear how you are going to display the image, but it will certainly require reading the image into memory from the database. That can stomp over things like the query cache, displacing smaller, more cacheable content.

Display Issues

There are other issues that may impact performance and security based on how you display the image. Rather than try to guess at the specifics of your method, I'll leave that to a future code review. But you should be aware that there are additional issues that may arise depending on exactly how you display the image. Since it is untrusted user data, this could include a security hole that is not visible from just this perspective. We really need to know the whole sequence to evaluate it.

More context makes it easier to evaluate code. Some problems may only appear in the interaction of two systems. Or something may be an irrelevant comment because it's handled outside the code that we can see.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.