1
\$\begingroup\$

I am trying to create a unique name for my uploaded images. It works, but I wonder whether I should make a few changes or not.

HTML

    <form action="upload.php" method="post" enctype="multipart/form-data" name="uploadform">
        <input type="hidden" name="MAX_FILE_SIZE" value="350000">
        <input name="picture" type="file" id="picture" size="50">
        <input name="upload" type="submit" id="upload" value="Upload Picture!">
    </form>

PHP

// if something was posted, start the process... 
if (isset($_POST['upload'])) {

// define the posted file into variables 
    $name = $_FILES['picture']['name'];
    $tmp_name = $_FILES['picture']['tmp_name'];
    $type = $_FILES['picture']['type'];
    $size = $_FILES['picture']['size'];

// if your server has magic quotes turned off, add slashes manually 
    if (!get_magic_quotes_gpc()) {
        $name = addslashes($name);
    }

// open up the file and extract the data/content from it 
    $extract = fopen($tmp_name, 'r');
    $content = fread($extract, $size);
    $content = addslashes($content);
    fclose($extract);

// connect to the database 
    include "../cn/connect.php";

    $name = md5($name);
    $name = $name . uniqid($name);

    $path = $_FILES['picture']['name'];
    $ext = pathinfo($path, PATHINFO_EXTENSION);
    $name = $name.".".$ext;

// the query that will add this to the database 
    $stmt = $sqli->prepare("INSERT INTO image1(name, type, size) VALUES (?,?,?)");
    $stmt->bind_param("sss", $name, $_FILES['picture']['type'], $_FILES['picture']['size']);
    $stmt->execute();
    $stmt->close();
    echo "Mission has been completed, sir!";

    if (!empty($_FILES)) {
        $target = "upload/";
        $target = $target . basename($_FILES['picture']['name']);
        $ok = 1;
        $picture_size = $_FILES['picture']['size'];
        $picture_type = $_FILES['picture']['type'];
        //This is our size condition 
        if ($picture_size > 5000000) {
            echo "Your file is too large.<br>";
            $ok = 0;
        }


        //This is our limit file type condition 
        if ($picture_type == "text/php") {
            echo "No PHP files<br>";
            $ok = 0;
        }

        //Here we check that $ok was not set to 0 by an error 
        if ($ok == 0) {
            Echo "Sorry your file was not uploaded";
        } //If everything is ok we try to upload it
        else {
            if (move_uploaded_file($_FILES['picture']['tmp_name'], $target)) {
                echo "The file " . basename($_FILES['picture']['name']) . " has been uploaded <br/>";
            } else {
                echo "Sorry, there was a problem uploading your file.";
            }
        }
    }

    echo "Successfully uploaded your picture!";
} else {
    die("No uploaded file present");
}
\$\endgroup\$
  • \$\begingroup\$ Context is helpful here. For example, if you are saving the image name in a database, there are some excellent ways to create truly unique (no clash) names. Why do you need a unique name? What other resources do you have? We usually deal with more code. \$\endgroup\$ – Brythan Dec 22 '14 at 5:40
  • \$\begingroup\$ Aa, sorry. I should've provided more code. I am saving the name of the image in my database, that's correct. Editing right now. \$\endgroup\$ – salep Dec 22 '14 at 5:42
3
\$\begingroup\$

It looks like your code works fine, so these comments are my opinion on how I would handle this if I were in your shoes.

This is a pretty standard requirement. Users upload images, and you want to save them with a filename that is (a) unique, and (b) won't cause you grief down the road with funny characters in the file name.

You also need to handle the case where people upload files with the same name over and over again, without creating duplicates. For instance, for a certain model of camera the first photo on the "camera roll" might always have the same name. Several people might own the same model of camera. Or, dissatisfied with the results of uploading an image, the user edits it locally and uploads a fresh copy. With the same file name as before.

The simplest solution is to skip all the md5 and uniqid() stuff, and simply add a record to a database which has an auto-increment ID field. You would add the original name of the image to the database (for future reference, tooltip etc), but not the name of the file on disk. Once you get the new ID, you copy/move/rename the image file based on the new ID. This gives you nice short, simple file names.

I think this is tidier than inventing a unique file name before adding the database record. Databases have a built-in unique ID feature, so you can get a unique ID "for free!" with each new record. Why invent your own?

If the name of the image file ends up being used in URL's, so you don't want it to be easily guessable, then you want to mess up the file name to make it more complicated. If this is the case, you are better off placing the files where they are not accessible directly via URL.

If you, as an administrator, want to be able to browse through the image files and have the names make sense, or if you want the image names to appear in the URL to assist visitors or for SEO or Google Image search benefits, then you may want to name the images like blog post titles, i.e. simple lower-case-hyphen-separated words with a unique ID on the end. There is lots of code around that will do that for you. (I've got code if you can't find it elsewhere).

\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer. Actually I am both using image_id and image_value columns in my database. Each image has its own unique ID already and I am using junction tables (for instance, book_cover, etc), so the only thing I want to achieve is uniqueness here. \$\endgroup\$ – salep Dec 22 '14 at 8:50
0
\$\begingroup\$

As this is an image upload, it is important to not allow malicious files to be uploaded.

Your current approach is this:

    $picture_type = $_FILES['picture']['type'];
    [...]
    if ($picture_type == "text/php") {
        echo "No PHP files<br>";
        $ok = 0;
    }

There are multiple problems with this approach:

  • type is completely user controlled, so an attacker can set it to anything.
  • Even if your filter would work, you would only filter out PHP files. What about other malicious files, such as HTML files for XSS or an htaccess file which could for example make images executable by PHP?
  • The mime type my browser uses for PHP files is application/x-php, not text/php, so I don't actually need any knowledge to upload malicious files.

What you should do is check the file extension as well as the actual file type. The functions that are generally recommended for this are pathinfo and finfo_file respectively. Ideally, you should use whitelists, not blacklists.

Misc

  • Why have addslashes? It's really only useful for security, and you don't need it, as you are using prepared statements (which is recommended).
  • Why are you reading the content of the file? You never use it for anything.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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