help me to secure my php mysql form

[linux1880]

Hi friends I have a form with mysqli comnnection

 

<label for="fullname">Fullname</label>
<input type="text" name="fullname" />

<label for="photo">Upload photo</label>
<input name="photo" type="file"/>

 

and on the php ends I have

 

$fullname = $_POST['fullname'];

    $uploaddir = './uploads/';
    //upload file in folder
    $uploadfile = $uploaddir. basename($_FILES['photo']['name']);
    //insert filename in db
    $upload_filename = basename($_FILES['photo']['name']);
    move_uploaded_file($_FILES['photo']['tmp_name'], $uploadfile);
    $photo = $upload_filename;

$sql = "INSERT INTO members(fullname,photo) VALUES('$fullname', '$photo');
$stmt = $link->query($sql) or die($link->error);
    $stmt->close;

 

Please help me, I am using this on a live site

[Adam]

What's the problem with it?

[linux1880]

I think it's insecure and asking you guys how to secure it. And also how to make each file unique ? please help me to make this code better.

[termidave]

Hi friends I have a form with mysqli comnnection

 

<label for="fullname">Fullname</label>
<input type="text" name="fullname" />

<label for="photo">Upload photo</label>
<input name="photo" type="file"/>

 

and on the php ends I have

 

$fullname = $_POST['fullname'];

    $uploaddir = './uploads/';
    //upload file in folder
    $uploadfile = $uploaddir. basename($_FILES['photo']['name']);
    //insert filename in db
    $upload_filename = basename($_FILES['photo']['name']);
    move_uploaded_file($_FILES['photo']['tmp_name'], $uploadfile);
    $photo = $upload_filename;

$sql = "INSERT INTO members(fullname,photo) VALUES('$fullname', '$photo');
$stmt = $link->query($sql) or die($link->error);
    $stmt->close;

 

Please help me, I am using this on a live site

 

There are two security issues I see right off the bat.

 

The first one is that you're embedding variables directly into the SQL string.  You should instead prepare the statement, bind the variables, and then execute the statement.  This will protect you from the dreaded SQL injection.  Check out the examples here:  http://www.php.net/manual/en/mysqli.prepare.php.  If you're feeling lazy, you can at least run the fields through mysql_real_escape_string() before embedding it:

 

$fullname = mysql_real_escape_string($fullname);
$photo = mysql_real_escape_string($photo);
$sql = "INSERT INTO members(fullname,photo) VALUES('$fullname', '$photo');

 

The second is that you are not filtering the uploaded file.  A user could upload a PHP file to your server, go directly to it, and take over your site.  Validate the mimetype of the file, and since it is a photo, you can throw it through some image processing functions before moving it to its final directory.  Here are some great examples:  http://www.php.net/manual/en/features.file-upload.php

 

You can check to see if a file already exists by using file_exists()

[linux1880]

could anyone pls give me code to validate mimetype in the above script? Thanks

[DarkMantis]

Hi there,

 

There are many exploitable parts to your PHP script.

 

Below is a commented and fixed version of your script:

 

//Always sanitize every user input!

$fullname = mysql_real_escape_string(addslashes($_POST['fullname']));

    $uploaddir = './uploads/';

    //Possible SQL Injection Exploit (if the file name is something like ' or 1=1-- 
    //Also possible Full Path Disclosure exploit

    $uploadfile = $uploaddir. basename(mysql_real_escape_string(stripslashes($_FILES['photo']['name'])));

    $upload_filename = basename($_FILES['photo']['name']);
    move_uploaded_file($_FILES['photo']['tmp_name'], $uploadfile);
    $photo = mysql_real_escape_string($upload_filename);


//ALWAYS ALWAYS Sanitize SQL DATA!
$sql = "INSERT INTO members(fullname,photo) VALUES('{$fullname}', '{$photo}');
$stmt = $link->query($sql) or die($link->error);
    $stmt->close;

 

 

 

This script should be fine to use now, it should be sanitized. I cannot Guarentee 100% as I haven't seen your $stmt class or your $link->query() function.

 

Also try sanitizing the File types, use only JPEG, JPG, and PNG for example.

 

Hope this helps,

Best Regards,

Mantyy

[linux1880]

Thanks all,

as far as i believe mysqli(improved) don't need mysql_real_escape_string and addslashes.

 

I would appreceate if anyone would help me to sanitize file types. Thanks

[PFMaBiSmAd]

i believe mysqli(improved)...

 

ONLY if you are using prepared statements, which the code you have posted is NOT doing.

[linux1880]

hi friends i have parameterised the query by doing

 

$sql = "INSERT INTO members(fullname,photo) VALUES(?,?);
$stmt = $link->prepare($sql);
$stmt->bind_param('ss',$fullname,$photo);
$stmt->execute();

 

Can anyone guide me how to restrict file type ? Many thanks

[Adam]

You need to implement a check similar to:

 

$valid_mimes = array(
    'image/png',
    'image/gif',
    'image/jpeg',
    // etc..
);

if (!in_array($_FILES['photo']['type'], $valid_mimes))
{
    // handle error here
}

[linux1880]

Thank you adam, I was also trying to add mysql_insert_id() at the beginning of the file it didn't work for some reason. Could anyone please show me ? Thanks

[Adam]

You mean at the beginning of the file name you're inserting? That won't work; mysql_insert_id is only available after the query has been executed.

[linux1880]

Thanks adam, what is you best suggestion for making a file name unique ?

[fortnox007]

Thanks adam, what is you best suggestion for making a file name unique ?

 

Why dont you just take the original name and add some random numbers to it.  that would make it unique enough.

like:

<?php
$filename = "fatmonkeys.jpg";
$final_name = rand(10,10000).$filename;
echo 'filename: '.$filename.'<br />';
echo 'final_name: '.$final_name.'<br />';
//will print out something like:
//filename: fatmonkeys.jpg
//final_name: 3543fatmonkeys.jpg
?>