2
\$\begingroup\$

I literally spend all day working on PHP and I think I figured it out, but I need some reviews in order to make my code secure & okay. I am asking you about functionality here. The code below works.

index.php

<form action="upload.php" method="post" enctype="multipart/form-data">
    <div id="file_container">
        <input name="images[]" multiple type="file" id="file[]"/><br/>
        <input type="submit">
    </div>
</form>

upload.php

$target = "upload/";

$uploadit = 1; // I will change it with extension checker.
$checksize = // will check file size

foreach($_FILES['images']['name'] as $key => $value){
    $path = $_FILES['images']['name'][$key];
    $ext = pathinfo($path, PATHINFO_EXTENSION);

    $name = md5($name);
    $generate1 = md5(date('Y-m-d H:i:s:u'));
    $randomizer = uniqid($name);
    $name = $name . $generate1 . $randomizer;
        if ($uploadit == 1) {
               if (move_uploaded_file($_FILES['images']['tmp_name'][$key], $target.$name.".".$ext)){
                echo "<strong>".$value ."</strong>Your files have been upload successfully<br />\n";
            }
        } else {
            echo "Failed";
        }
\$\endgroup\$
2
\$\begingroup\$

If $target is accessible from a web browser, you need to verify that it does not have a dangerous extension. For example, that it's not a PHP file. Note that there are probably other dangerous extensions on your web server other than just '.php'. For example, '.html' is a dangerous extension because of cross-site scripting type vulnerabilities.

You include a flag variable for this, but you aren't doing the checking.

$uploadit = 1; // I will change it with extension checker.

Why not use a Boolean here?

$uploadit = false; // I will change it with extension checker.

Also, it probably makes more sense to default to no and change to yes than to default to yes.

foreach($_FILES['images']['name'] as $key => $value){
    $path = $_FILES['images']['name'][$key];
    $ext = pathinfo($path, PATHINFO_EXTENSION);

Could just be

foreach ( $_FILES['images']['name'] as $key => $path ) {
    $ext = pathinfo($path, PATHINFO_EXTENSION);

In general, prefer more descriptive names than $key => $value. In this case, I believe that $key is a numeric index, so $i or $index might be more appropriate.

$name = md5($name);

What's $name?

    if ($uploadit == 1) {

Given PHP's quirkiness, this should probably be

    if ( true === $uploadit ) {

According to the PHP date documentation,

$generate1 = md5(date('Y-m-d H:i:s:u'));

The date() function will always return '000000' for microseconds (u).

$date = new DateTime();
$generate1 = md5($date->format('Y-m-d H:i:s:u'));

Use DateTime::format() instead.

$randomizer = uniqid($name);

This does nothing for security. Its purpose is to avoid clashes. Note that this will put $name in the name twice.

$randomizer = $name . uniqid();

This does the same thing as the previous statement.

$name = $name . $generate1 . $randomizer;

You can write

$name .= $generate1 . $randomizer;

Not important now, but the next time you hit this situation, you might want to go with the shorter version.

Note that it is safer to put $target outside the web accessible area. If you do that, then it is less of a concern about the extension. If you do not do that, then you should care about the extension even if the filename is hard to guess. It's not clear to me why you need it in a web accessible area if you are going to obfuscate the name.

Obfuscating the name does not provide security. There have been exploits that have allowed attackers to view directory contents. Also, there are any number of existing users that can do that. Finally, you can't use the image without sharing the obfuscated name--possibly with an attacker. So you might as well use the original name and secure that.

If you are trying to avoid name clashes, you might be better off with a database solution. An autoincrement identifier with a unique index will be consistently unique and kept that way by the database.

It is preferable to whitelist extensions rather than blacklist them. That way a change in the web server configuration can't open up a new dangerous extension when you aren't looking.

There's things you can do in either .htaccess or httpd configuration to prevent files in $target being evaluated in scripts. For example, disable CGI. You should probably do something like this in addition to an extension whitelist.

\$\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.