Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I wrote a simple php script, which allows user to upload a file to the server. It works, but I'm not very sure about security. Could you give me some hints about security mistakes?

In a nutshell: User uploads a file from form.html, then a file is saved in uploads/ directory on the server, and a direct link to it is recorded in a database. Then that link is presented to the user, so he is able to download the file. The execution of that file is forbidden using .htaccess.

form.html

<div id="quickFileSubmit" style="margin-left: 20px; display: none">
    <table>
        <tr>
            <td>
                <form enctype="multipart/form-data" class="smallForm"
                      action="action.php?act=quickFileSubmit" method="post"
                      style="margin-top: 5px;">
                    <label style="font-size: 12px;">File:</label>
                    <input type="file" name="uploadfile" readonly="true" size="30"/>
                    <input type="submit" value="Submit" />
                </form>
            </td>
            <td style="width: 100%;"></td>
        </tr>
    </table>
</div>

action.php

//some actions

else if ($_REQUEST["act"] == "quickFileSubmit") {
    $basename = getUploadBasename();
    uploadFile();
    header("location:index.php");
}

function uploadFile() {

    if (strlen($_FILES['uploadfile']["name"]) > 0) {
        $target_path = "uploads/";
        $basename = basename( $_FILES['uploadfile']['name']);
        $target_path = $target_path . $basename;
        if (encode($basename) == $basename) {
            if(move_uploaded_file($_FILES['uploadfile']['tmp_name'], $target_path)) {

                // here goes saving of file info

                return $basename;
            } else{
                // upload failed.
                echo "File upload failed.";
                return null;
            }
        } else {
            echo "Illegal characters in file name.";
            return null;
        }
    }
}

Also, I have put .htaccess in the uploads dir:

.htaccess

SetHandler default-handler
Options -Indexes
share|improve this question
1  
Most php functions return a sensible response or FALSE. I suggest you return FALSE instead of NULL. –  Simon Mar 25 '11 at 14:15
add comment

1 Answer 1

up vote 1 down vote accepted

One thing that immediately stands out to me is whether $_FILES['uploadfile'] actually exists for each upload request. Do an isset on it first to make sure a client doesn't send a post with fabricated input values - for example: some custom html form of their own making. If they do, you'll get 'function expects some value, boolean/null given' errors all over the place as is.

Or better yet, to allow client flexibility if they choose to use some sort of API for your website, just cycle through $_FILES array to work on whatever files they submit to this script. Your choice though if you want to instead maintain strict input, but it wouldn't matter really as long as your script receives the file input of a field named "uploadfile" from any source.

Also, for $_FILES[$x]["name"], validate or sanitize it to ensure it contains only alphanumeric, underscore, dash, and dot characters. Yes OS's can work with more characters than that, but not every protocol or application can (text, browsers or email editors breaking long filenames or links on spaces, etc).

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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