777 Directory Permissions not working?


#1

Greetings,

I am very close to finishing a script to upload images from my website into an “images” folder.

In my image upload processor, I have the following line:
[php] @move_uploaded_file($_SERVER[‘DOCUMENT_ROOT’] . $directory_self . ‘images/’ .$_FILES[$fieldname][‘tmp_name’][$key], $uploadFilename[$key])[/php]

This is not moving my images to the “images” folder because permissions are denied, even with 777 chmod() permissions on it.

Although, If I use a different line:
[php] @move_uploaded_file($_FILES[$fieldname][‘tmp_name’][$key], $uploadFilename[$key])[/php]

This actually is successful in storing my images, except in the root directory, which is not where I want the images stored.

I was wondering if maybe Dreamhost has some safety thing that won’t let me turn my “images” folder to 777 or something? I read about hosting services possibly doing this.

Please let me know if you can figure this out or if there is a proper way to achieve this goal.

Thanks


#2

The first argument you’re passing to move_uploaded_file looks like it’s supposed to be the destination, not the source. The first argument must be the tmp_name of the uploaded file. Do a var_dump on $_FILES and take a close look at what all the contents are on an upload — I suspect it’s not laid out the way you think it is.


#3

Thanks for the response andrewf,

Here is my entire code if you would like to take a look at it:
[php]<?php
##########################################################################################################################################################
// filename: upload.processor.php

// first let’s set some variables

// make a note of the current working directory, relative to root.
$directory_self = str_replace(basename($_SERVER[‘PHP_SELF’]), ‘’, $_SERVER[‘PHP_SELF’]);

// make a note of the directory that will recieve the uploaded files
$uploadsDirectory = $_SERVER[‘DOCUMENT_ROOT’] . $directory_self . ‘images/’;

// make a note of the location of the upload form in case we need it
$uploadForm = ‘http://’ . $_SERVER[‘HTTP_HOST’] . $directory_self . ‘multiple.upload.form.php’;

// make a note of the location of the success page
$uploadSuccess = ‘http://’ . $_SERVER[‘HTTP_HOST’] . $directory_self . ‘multiple.upload.success.php’;

// name of the fieldname used for the file in the HTML form
$fieldname = ‘file’;

//echo’

’;print_r($_FILES);exit;

// Now let’s deal with the uploaded files

// possible PHP upload errors
$errors = array(1 => ‘php.ini max file size exceeded’,
2 => ‘html form max file size exceeded’,
3 => ‘file upload was only partial’,
4 => ‘no file was attached’);

// check the upload form was actually submitted else print form
isset($_POST[‘submit’])
or error(‘the upload form is neaded’, $uploadForm);

// check if any files were uploaded and if
// so store the active $_FILES array keys
$active_keys = array();
foreach($_FILES[$fieldname][‘name’] as $key => $filename)
{
if(!empty($filename))
{
$active_keys[] = $key;
}
}

// check at least one file was uploaded
count($active_keys)
or error(‘No files were uploaded’, $uploadForm);

// check for standard uploading errors
foreach($active_keys as $key)
{
($_FILES[$fieldname][‘error’][$key] == 0)
or error($_FILES[$fieldname][‘tmp_name’][$key].’: '.$errors[$_FILES[$fieldname][‘error’][$key]], $uploadForm);
}

// check that the file we are working on really was an HTTP upload
foreach($active_keys as $key)
{
@is_uploaded_file($_FILES[$fieldname][‘tmp_name’][$key])
or error($_FILES[$fieldname][‘tmp_name’][$key].’ not an HTTP upload’, $uploadForm);
}

// validation… since this is an image upload script we
// should run a check to make sure the upload is an image
foreach($active_keys as $key)
{
@getimagesize($_FILES[$fieldname][‘tmp_name’][$key])
or error($_FILES[$fieldname][‘tmp_name’][$key].’ not an image’, $uploadForm);
}

// make a unique filename for the uploaded file and check it is
// not taken… if it is keep trying until we find a vacant one
foreach($active_keys as $key)
{
$now = time();
while(file_exists($uploadFilename[$key] = $now.’-’.$_FILES[$fieldname][‘name’][$key]))
{
$now++;
}
}

// now let’s move the file to its final and allocate it with the new filename
foreach($active_keys as $key)
{
@move_uploaded_file($_SERVER[‘DOCUMENT_ROOT’] . $directory_self . ‘images/’ .$_FILES[$fieldname][‘tmp_name’][$key], $uploadFilename[$key])
or error(‘receiving directory insuffiecient permission’, $uploadForm);
}

// If you got this far, everything has worked and the file has been successfully saved.
// make an error handler which will be used if the upload fails
function error($error, $location, $seconds = 5)
{
echo ‘’."\n\n".
’’."\n".
’ ‘."\n".
’ ‘."\n\n".
’ ‘."\n\n".
’ Upload error’."\n\n".
’ ‘."\n\n".
’ ‘."\n\n".

‘."\n\n".

Upload failure

’."\n\n".

An error has occured: ‘."\n\n".
’ . $error . ‘…’."\n\n".
’ The upload form is reloading

’."\n\n".
‘."\n\n".
’’;
exit;
} // end error handler
##########################################################################################################################################################
?>[/php]

I knew move_uploaded_file had something to do with the destination since it was near the end of the script. Let me know if you see something in this code. Everything in this script works perfectly except it is not storing the file in the correct location.

Thanks


#4

The move_uploaded_file line should probably be:

[php] @move_uploaded_file($_FILES[$fieldname][‘tmp_name’][$key], $_SERVER[‘DOCUMENT_ROOT’] . $directory_self . ‘images/’ . $uploadFilename[$key])[/php]

Since the DOCUMENT_ROOT stuff is all part of the destination path, not the source (which PHP gives you verbatim).

HOWEVER — you need to do some sort of sanity checking on the $uploadFilename. There’s no sanitization on PHP’s side, and that path may well include silly things like slashes, \0, and dangerous extensions.


#5

Awesome!

This seems to have worked exactly the way I wanted. Also, thanks for letting me know about the security vulnerability.

Would this be sufficient to sanitize the variable?:

[php]$uploadFilename = mysql_real_escape_string($uploadFilename);[/php]

I don’t know too much about sanitizing and security except for the mysql_real_escape_string() function.


#6

No, not at all — mysql_real_escape_string ONLY makes a string “safe” for MySQL’s consumption.

My (overcautious but safe) approach would be:

[php]$uploadFilename = preg_replace(’/[^a-zA-Z0-9]/’, ‘_’, $uploadFilename);[/php]

Which just replaces everything non-alphanumeric with underscores.


#7

I’m not sure if I’m putting this in the right spot, basically, I am placing these two sanitizing statements here:
[php]// make a unique filename for the uploaded file and check it is
// not taken… if it is keep trying until we find a vacant one
foreach($active_keys as $key)
{
$now = time();
while(file_exists($uploadFilename[$key] = $now.’-’.$FILES[$fieldname][‘name’][$key]))
{
$uploadFilename = mysql_real_escape_string($uploadFilename);
$uploadFilename = preg_replace(’/[^a-zA-Z0-9]/’, '
’, $uploadFilename);
$now++;
}
}[/php]

Although when I upload a picture with a “-” in it, it remains in the final filename.[hr]
I also tried this with the [$key] inside it:
[php]// make a unique filename for the uploaded file and check it is
// not taken… if it is keep trying until we find a vacant one
foreach($active_keys as $key)
{
$now = time();
while(file_exists($uploadFilename[$key] = $now.’-’.$FILES[$fieldname][‘name’][$key]))
{
$uploadFilename[$key] = mysql_real_escape_string($uploadFilename[$key]);
$uploadFilename[$key] = preg_replace(’/[^a-zA-Z0-9]/’, '
’, $uploadFilename[$key]);
$now++;
}
}[/php]

This still doesn’t remove the dashes “-” from my original filenames from my computer, which probably means the code not working as it should. I’m not sure if I’m putting this code in the right place or not. Basically, this is about all I need to finish my image uploader tool.


#8

First thing, get rid of the mysql_real_escape_string. All it’s going to do is insert random backslashes that will just get filtered out by the next step.

The second version you posted (with $uploadFilename[$key]) won’t work, because the preg_replace only runs if you’ve already decided to use a different path. Read your code carefully and you’ll see why.


#9

Ok, now I moved it to something like this:
[php]// now let’s move the file to its final and allocate it with the new filename
foreach($active_keys as $key)
{
$uploadFilename[$key] = preg_replace(’/[^a-zA-Z0-9]/’, ‘_’, $uploadFilename[$key]);
move_uploaded_file($_FILES[$fieldname][‘tmp_name’][$key], $_SERVER[‘DOCUMENT_ROOT’] . $directory_self . ‘images/’ . $uploadFilename[$key]) or error(‘receiving directory insuffiecient permission’, $uploadForm);
}[/php]

All the symbols are correctly turning into underscores “_”. The only problem now is when I have an image file like this (my-image.jpg), it gets tranformed into something like this (my_image_jpg).

The .jpg turns into _jpg. I thought this might happen but I wasn’t sure. Is there anyway around that?

Also, I left the [$key] in there since I am uploading multiple images in an array of sorts. That way each individual filename in the array would be renamed and put into the correct spot.[hr]
Actually, I removed the [$key] as well and it didn’t seem to make a difference.


#10

just out of curiosity, it seems that this kind of checking will kill most legitimately named files that are not written in some latin-based language. i’ve noticed, for example, that wordpress choked on images with chinese file names, especially if there was also a space or hyphen in the name. is there another approach that would look more specifically for the vulnerabilities that you mentioned (back slashes, \0, etc)?


#11

Thanks for the help andrewf. I was able to figure out how to store the multiple images correctly and this should be sufficient.

I was unable to implement the sanitizing stuff though and will probably drop it.

I assume that if I just leave that out, then my database should still be safe from SQL injection and someone will not be able to hack into the system, but people might have trouble uploading pictures with symbols in them?


#12

If you allow users to upload files with arbitrary names, you are unsafe, as users will be able to upload malicious files (such as PHP scripts) to your web directory and run them.


#13

Is there any other way to sanitize the filenames without turning “.jpg” into “_jpg”?

This seems to be where I keep getting stuck because it is reducing all periods into underscores.


#14

can’t you just add . to your class of preg matches? [^A-Za-z0-9.]?
alternatively, you could get the part of the filename before the extension first:
[php]

$uploadFilename[$key] = preg_replace(’/[^a-zA-Z0-9]/’, ‘_’, (preg_match(’/^[^.]/’, $uploadFilename[$key])));

[/php]

note, i’m still learning PHP, so no guarantees that the above will work


#15

As written, that won’t work at all. preg_match returns an integer indicating what position the pattern matched at, or FALSE if there wasn’t a match – trying to do substitutions on that won’t get you what you want.

Again, though, leaving the extensions unchanged is a Bad Idea. I left periods out of the accepted character set on purpose.


#16

What should I do about sanitizing the images and getting a proper filename at the same time? All images are going to turn out something like this “my_image_jpg” or “another_image_gif”


#17

getimagesize will give you an image type, if you ask it nicely. Use that to generate a new extension to tack onto the filename.


#18

just out of curiosity, have you looked into PEAR or other PHP script archives? I’m sure what you are trying to do has already been done in a way that is efficient and safe. Often times you can just modify working scripts to suit your needs. Maybe that’s why andrewf is hesitant to offer a solution? Maybe writing this sort of function in PHP is so difficult to do safely that it’s better to leave it alone?