Page 1 of 1

Upload Files Script, Problem encountered...

Posted: Fri Aug 05, 2005 9:41 am
by influx
Hey all,

I run a small blog website just for a few friends/colleagues of mine, and I give them the option to upload up to five files to their account. I had about 200 folders with their files (meaning 200 users uploaded at least one file) and I lost all the files one day...the result of some poor coding on my part and a swift, uncanny Googlebot.

I had uploaded my sitemap to Google Sitemaps, and at around the time I lost everything, Google was crawling my site...everywhere. I don't have a robots.txt file to disallow any robots, so it felt the need to browse my site, even where I didn't want it to go.

First things first: in my upload script I had three options, 1) Remove file, 2) Add one file, 3) Remove all files. In my remove all files code, long story short, Googlebot crawled and removed all the files for each user of my site. My fault, clearly.

Since the occurrence, I have gotten rid of the 3) Remove all files feature, so now in my script they can only upload one file, or remove one file. This should make it a bit easier on myself.

Now, I have a few questions about my current script code:

Code: Select all

<?php
$path_prefix='../../'; //set root directory for template reference
$code=$_GET['code']; //code 1 is upload one file, code 2 is remove single file
$fileid=$_GET['f']; //file number
$_SESSION['username']=strtolower($_SESSION['username']); //lowercase username

if($code==1) //upload one file
{
    //Replace spaces with underscores, and no characters allowed, numbers and letters only, periods and underscores acceptable, everything lowercase
    $form_data_name = ereg_replace('[ ]+', '_', strtolower($_FILES['userfile']['name']));
    $form_data_name = ereg_replace('[^a-zA-Z0-9._]', '', $form_data_name);

    $form_data_size = $_FILES['userfile']['size']; 
    //get file extension in next two lines
    $fileInfo = pathinfo($_FILES['userfile']['name']);
    $form_data_extension = $fileInfo['extension'];
    
    //if file doesn't match one of the extensions listed below, redirect and give err code 5
    $allowExtensions = array('html', 'htm', 'doc', 'png', 'rtf', 'jpg', 'jpeg', 'gif', 'pdf', 'ppt', 'psd', 'txt', 'xls'); //add the ones you require here
    $fileInfo = pathinfo($_FILES['uploadname']['name']);
    if(!in_array($form_data_extension, $allowExtensions))
    {
        header('Location: upload_add.php?code=5&f='.$fileid);
        exit;
    }

    //if file already exists in slot, delete old and upload new file
    $ress=mysql_query("SELECT * FROM upload WHERE u_username='".$_SESSION['username']."'"); //fetch upload information
    $row=mysql_fetch_array($ress);
    $old_file_name=$row["u_file".$fileid."_name"];
    
    if($old_file_name)
    {
        unlink("../../_user_upload/".$_SESSION['username']."/".$old_file_name);
    }

    //check if user-specific directory exists, if not, make directory
    if(!file_exists($path_prefix."_user_upload/".$_SESSION['username']."/"))
    {
        mkdir($path_prefix."_user_upload/".$_SESSION['username']);
        chmod($path_prefix."_user_upload/".$_SESSION['username'], 0774); //chmod it, read/write for user, group.
    }

    //now that file is not malicious and has passed all tests, move it from tmp to upload directory
    $targetdir = $path_prefix."_user_upload/".$_SESSION['username']."/";
    move_uploaded_file($_FILES['userfile']['tmp_name'], $targetdir.$form_data_name);
    
    $query = "UPDATE upload
    SET u_file".$fileid."_name = '".$form_data_name."',
    u_file".$fileid."_date = '".date("m.d.Y")."'
    WHERE
    u_username = '".$_SESSION['username']."'";
    mysql_query($query) or die(mysql_error());
    
    chmod($targetdir.$form_data_name, 0754); //chmod it, read/write for user, group, and world.
    
    header('Location: upload.php?code=1&f='.$fileid); //if file uploaded successfully, redirect and give success code 1
    exit;
}

if($code==2) //remove single file
{
    $ress=mysql_query("SELECT * FROM upload WHERE u_username='".$_SESSION['username']."'"); //fetch upload information
    $row=mysql_fetch_array($ress);
    $file_name=$row["u_file".$fileid."_name"];

    //delete file:
    if(file_exists($path_prefix."_user_upload/".$_SESSION['username']."/".$file_name))
    {
        unlink($path_prefix."_user_upload/".$_SESSION['username']."/".$file_name);
    }
    
    $query = "UPDATE upload
    SET u_file".$fileid."_name = '',
    u_file".$fileid."_date = '00.00.0000'
    WHERE
    u_username = '".$_SESSION['username']."'";
    mysql_query($query) or die(mysql_error());
    
    header('Location: upload.php?code=2&f='.$fileid); //if remove single file successful, redirect and give success code 2
    exit;
}
?>
1) After looking at the script, do you see any major vulnerabilities/something to be afraid of?

2) Are both my chmod permissions set correctly for folders and files? I am told to be very careful with these.

3) Is my upload script vulnerable with propagating either 'code 1' or 'code 2' through the URL? I mean, can they tamper with the 'code number' as you mentioned earlier to replace it with something completely different? Should I use $_POST instead of $_GET to send either 'code 1' (remove file) or 'code 2' (add file)? It's not possible to execute an "SQL Injection" manually changing the code number, right? Is my script vulnerable to an SQL Injection?

4) Could they still upload malicious files and execute them on the server? Even if I only accept the following filetypes: .html, .htm, .doc, .png, .rtf, .jpg, .jpeg, .gif, .pdf, .ppt, .psd, .txt, and .xls?

NOTE: I have removed some code that checks filesize, char length, and filetype to make the code easier for you all to read.

Thanks in advance fellas (and ladies)...I really appreciate the help.

Cheers,
-influx


P.S: I will be very attentive to this forum, for I will be working on the script all day today, so expect quick responses! :)

Posted: Fri Aug 05, 2005 10:25 am
by s.dot
You should use $_POST instead of $_GET if a bot crawling your pages executed one of your scripts. Either way you should specify where the information is coming from.

For example

Code: Select all

if($_POST['code'] == 1)
// or
if($_GET['code'] == 1)
Your allowed extensions are OK, those won't harm you. But, you should check for mime type along with extensions. A .exe file could be disguised as a .jpg, and uploaded on your script.

SQL injection can easily be prevented by using mysql_real_escape_string on everything that is input by a user to be used in an SQL query.

Posted: Fri Aug 05, 2005 10:35 am
by influx
So for example, in the upload script if I use mysql_real_escape_string, I should use it on the following variable: $_SESSION['username'], right?

So basically, the variable found in the following statement:

Code: Select all

WHERE u_username='".$_SESSION['username']."'");
Is there any harm if I target an incorrect variable with mysql_real_escape_string?


Also, how exactly would I used $_POST as opposed to $_GET?

if I have the following HTML:

Code: Select all

<form action="setup_test.php?code=1" method="post">
I can only use $_GET to retrieve the code...how would I propagate the code number through $_POST in place of $_GET?

Thanks.

Posted: Fri Aug 05, 2005 10:39 am
by nielsene
influx wrote: Also, how exactly would I used $_POST as opposed to $_GET?

if I have the following HTML:

Code: Select all

<form action="setup_test.php?code=1" method="post">
I can only use $_GET to retrieve the code...how would I propagate the code number through $_POST in place of $_GET?

Thanks.

Code: Select all

<form action="setup_test.php" method="post">
<input type="hidden" name="code" value="1" />
...

Posted: Fri Aug 05, 2005 10:40 am
by s.dot
using mysql_real_escape string won't hurt you if you use it on anything, it's a good precautionary practice.

to post a variable through $_POST

Code: Select all

<form action="page.php" method="post">
<input type="hidden" name="code" value="1">
then in your script

Code: Select all

if($_POST['code'] == 1)

Posted: Fri Aug 05, 2005 10:55 am
by influx
Oh, easy enough, thanks!

Now, one last question. What if I am not using a form and just have a link like: <a href="post_test.php?code=2>Remove this post</a>

Is there any way I can propagate this using $_POST instead of $_GET?

-influx

Posted: Fri Aug 05, 2005 11:00 am
by s.dot
using $_GET is not a problem, just verify that's where it's coming from

Code: Select all

$_GET['code']
// NOT $code
And make sure that the user going to that link, has the proper permission to execute it

Code: Select all

<form action="page.php?code=2&username=<? echo $_SESSION['username']; ?>" method="post">

Code: Select all

if($_GET['username'] == $_SESSION['username']_
{
   // process code
} ELSE
{
   die("nope");
}
$_GET is secure as long as you know how to use it properly

Posted: Fri Aug 05, 2005 12:12 pm
by influx
What about in the case of Googlebot destroying my site =(

It is much safer going with the $_POST method in place of $_GET, right?

Also, disallowing the Googlebot from my upload directory is a good idea, right?

Posted: Fri Aug 05, 2005 12:21 pm
by s.dot
if you check that the username logged in ($_SESSION['username']) is the same as in the url (&username=username) then googlebot won't be able to execute that code... although I believe this is the first instance I've heard of where a robot has executed code.

Posted: Fri Aug 05, 2005 12:21 pm
by shiznatix
well if you check if the user has permission but doing the username="$_SESSION['username']"
in the form you can prevent googlebot from deleting your posts but you can also just have a link with some javascript in a form so do a onclick this.form.submit or somtinbut you still are definatly going to want to make sure that the user actually has permission to delete that post becuase if someone happens to come across your site they might just delete everything and that will be that