1

I'm currently writing an upload class for uploading images. I do extension checks to verify that the uploaded images are of the supported types, and the photos are always chmod(0664) when the uploaded file is copied to it's resting place. Is this relatively safe? I don't know much about image encoding, but even if someone went through the trouble of somehow tricking my extension check, the file could never be ran on the server anyways unless there was a security hole elsewhere and the attackers were already into my file system, correct? Here's my extension check:

function validate_ext() { //Function validates that the files extension matches the list of allowed extensions
    $extension = $this->get_ext($this->theFile);
    $ext_array = $this->extensions;
    if (in_array($extension, $ext_array)) { //Check if file's ext is in the list of allowed exts
        return true;
        echo "ext found";
    } else {
        $this->error[] = "That file type is not supported. The supported file types are: ".$this->extString;
        return false;
    }
}

And here's the function that copies the uploaded file to it's final resting place.

if ($_FILES[$this->uploadName]['error'] === UPLOAD_ERR_OK){
    $newfile = $this->uploadDir.$this->theFile;
    if (!move_uploaded_file($this->tempFile, $newfile)) {
        $this->error[] = "The file could not be moved to the new directory. Check permissions and folder paths.";
        die($this->error_text());   
    }else{
        $this->error[] = "The file ".$this->originalName." was successfully uploaded.";
        if ($this->renameFile == true){
            $this->error[] = $this->originalName." was renamed to ".$this->theFile;
        }
        chmod($newfile , $this->fileperm);
    }
}else{
    $this->error[] = $this->file_upload_error_message($_FILES[$this->uploadName]['error']);
    die($this->error_text());
}
Throttlehead
  • 1,927
  • 6
  • 22
  • 36
  • To be really sure nothing is ever going to be executed somehow, just build a wrapper script around it. The wrapper script should do a header("Content-type: $mime_type") and dump the file contents to stdout. To determine the mime-type, check out the fileinfo pecl extension (http://us3.php.net/manual/en/ref.fileinfo.php). – Friek Jul 29 '11 at 21:20

3 Answers3

2

Reading the extension really isnt a good way to check file type. You should read the file mime type... granted that can be faked too, but its more of a hassle to fake.

prodigitalson
  • 60,050
  • 10
  • 100
  • 114
1

In Linux world, as long as u gave the file non-executable permission, the file cannot execute. Whether it's .jpeg or it's .bash. That's true the other way around too, .jpeg with an executable permission could be executed too (if the content of that .jpeg file is executable file, not image content).

VOX
  • 2,883
  • 2
  • 33
  • 43
  • so just make sure about the permissions, care less on extensions. ;) – VOX Jul 29 '11 at 20:53
  • So it probably can't get much safer than doing an extension check, a mime check, and ensuring that anywhere users can upload files, those files are always saved in something like 644. Thanks a lot for all the insight guys! – Throttlehead Jul 30 '11 at 23:43
0

You can use getimagesize() to check the file itself.

djn
  • 3,950
  • 22
  • 21