...

View Full Version : secure delete script?



student101
02-21-2010, 01:26 PM
Is this script secure enough?


<?php
// check if URL src exists...
if (isset($_GET['src'])){
// setup folder paths to $vars...
$path="path/to/folder/".$_GET['src'];
$path2="path2/to/folder/".$_GET['src'];

// check if the file exists
if (file_exists($path)) {

// check if the file is deleted
if(@unlink($path)) {

// goto the page you need to go to...
$deleteGoTo = "page_to_goto.php";
header(sprintf("Location: %s", $deleteGoTo));
}

}else{

// check if the file is deleted...
if(@unlink($path2)) {

// goto the page you need to go to...
$deleteGoTo = "page_to_goto.php";
header(sprintf("Location: %s", $deleteGoTo));
}
}

}else{
// goto the page if URL src exists...
$deleteGoTo = "page_to_goto.php";
header(sprintf("Location: %s", $deleteGoTo));
}
?>

MattF
02-21-2010, 03:17 PM
What would happen if someone supplied the following, (or suchlike), as the src value in $_GET['src']?

../../../../file

Inigoesdr
02-21-2010, 05:28 PM
What would happen if someone supplied the following, (or suchlike), as the src value in $_GET['src']?

../../../../file

The script would attempt to delete a file 4 levels above the directory specified in $path. So student101, to answer your question in a word: No.

student101
02-21-2010, 06:28 PM
Ok so what does ../../../../file do or is it metophorical?



// check if URL src exists...
if (isset($_GET['src'])){


// escapes special characters (PHP 4 >= 4.3.0, PHP 5)
$thesrc = mysql_real_escape_string($_GET['src']);


// setup folder paths to $vars...
$path="path/to/folder/".$thesrc;
$path2="path2/to/folder/".$thesrc;

// check if the file exists
if (file_exists($path)) {

// check if the file is deleted
if(@unlink($path)) {

// goto the page you need to go to...
$deleteGoTo = "page_to_goto.php";
header(sprintf("Location: %s", $deleteGoTo));
}

}else{

// check if the file is deleted...
if(@unlink($path2)) {

// goto the page you need to go to...
$deleteGoTo = "page_to_goto.php";
header(sprintf("Location: %s", $deleteGoTo));
}
}

}else{
// goto the page if URL src exists...
$deleteGoTo = "page_to_goto.php";
header(sprintf("Location: %s", $deleteGoTo));
}


On another note; would this be of worth?


// convert numeric url request to $var
$id = (int)$_GET['id'];

// check if $var is valid and an integer
if (isset ($id) && $id != "" && (is_int($id)) ){

// ... do things here
}

MattF
02-21-2010, 07:32 PM
../ traverses up the directory tree. You're going one dir up for every ../.

You should allow alphanumeric characters, underscores, a single period and hyphens at most in your src var. There's no reason for any other characters to be there. Sanitise the path.

student101
02-21-2010, 07:38 PM
Good point, will do!


$path= mysql_real_escape_string("path/to/folder/".$thesrc);
$path2= mysql_real_escape_string("path2/to/folder/".$thesrc);



Should I do the check first then assign $vars or vice versa?


$id = (int)$_GET['id'];
if (isset ($id) && $id != "" && (is_int($id)) ){

OR

if (isset ($_GET['id'];) && $_GET['id']; != "" && is_int($_GET['id'];) ){
$id = (int)$_GET['id'];

MattF
02-21-2010, 07:48 PM
Good point, will do!


$path= mysql_real_escape_string("path/to/folder/".$thesrc);
$path2= mysql_real_escape_string("path2/to/folder/".$thesrc);



That will make bugger all difference to the particular scenario.

For the other question:



$id = ((isset($_GET['id'])) ? intval($_GET['id']) : 0);

if ($id > 0)
{
[code here]
}

student101
02-21-2010, 07:51 PM
That will make bugger all difference to the particular scenario.
I take it something is incorrect?

MattF
02-21-2010, 07:53 PM
I take it something is incorrect?

You're escaping the input for insertion into the DB. You aren't doing any sanitisation of the path whatsoever through the use of *escape_string().

student101
02-21-2010, 07:58 PM
Sanitise the path.
What other path is there?
"path/to/file".$thesrc makes up the path..

MattF
02-21-2010, 08:09 PM
What other path is there?
"path/to/file".$thesrc makes up the path..



// escapes special characters (PHP 4 >= 4.3.0, PHP 5)
$thesrc = mysql_real_escape_string($_GET['src']);


// setup folder paths to $vars...
$path="path/to/folder/".$thesrc;
$path2="path2/to/folder/".$thesrc;


If the value of $GET['src'] is something along the lines of ../../../../[the name of any file some cretin wishes to delete] or similar, you are effectively allowing your users to delete, (provided your httpd user has permissions), any file anywhere on your system, if they can find a valid path. That example value above would mean that the user is now accessing files outside of the httpd root directory.

kbluhm
02-21-2010, 08:12 PM
The first thing you should do, before even trying to delete anything, is check if some type of admin access has been granted. You still want to sanitize the deleted file paths afterward, but it is much less likely that someone you have trusted with admin access will be toying with the script looking for exploits.

student101
02-21-2010, 08:14 PM
So what would you recommend I do to secure the URL request?


admin access has been granted?
Yes suPHP is enabled on the server.

kbluhm
02-21-2010, 08:16 PM
Well, in what context is this URL being used? Is it linked directly from an open page for anyone to click? Is it already behind a secured page? Is it being included with an already secured file?

MattF
02-21-2010, 08:16 PM
The first thing you should do, before even trying to delete anything, is check if some type of admin access has been granted. You still want to secure the deleted files path afterward, but it is less likely that someone you have trusted with admin access will be toying with the script looking for exploits.

I'd say the opposite, personally. Scripts should be secure by default, before any type of permissions system is taken into account. Sanitisation and validation should be the first port of call, not the second.

MattF
02-21-2010, 08:19 PM
So what would you recommend I do to secure the URL request?

Do as I suggested earlier and strip any characters which aren't required from the value of $_GET['src'], (and any and all other user submitted input). Then, visit Google and search for input validation, input sanitisation, XSS, cross site scripting, CSRF etc, etc.

student101
02-21-2010, 08:23 PM
Well, in what context is this URL being used? Is it linked directly from an open page for anyone to click? Is it already behind a secured page? Is it being included with an already secured file?
Good one, totally forgot about my secure pages!!!
Yes that's it, add user authentication script for login!!!

I didn't think of that, always get lost in the little things - totally confuses me!

Thanks for the reminder!

student101
02-21-2010, 08:25 PM
Then, visit Google and search for input validation, input sanitisation, XSS, cross site scripting, CSRF etc, etc.
I have been on google since yesterday looking for a solution my last solution after searching is to ask. :D

kbluhm
02-21-2010, 09:08 PM
I'd say the opposite, personally. Scripts should be secure by default, before any type of permissions system is taken into account. Sanitisation and validation should be the first port of call, not the second.

I didn't say sanitisation wasn't important. Here is my pseudo-logic:


<?php

// Establish admin access
if ( ! $user->isAdmin() )
{
return;
}

// Is a file requested?
if ( ! isset( $_GET['file'] ) )
{
return;
}

// Grab requested file path
$file = $_GET['file'];

// Establish our root file path, could be defined elsewhere in a global include script
define( 'DIR_FILES', dirname( __FILE__ ) );

// Normalise file path slashes
$file = preg_replace( '/[\/\\\\]+/', '/', $file );
$file = '/' . trim( $file, '/' );

// Remove file system traversing
$safe_file = str_replace( array( '/../', '/./' ), '/', $file );

// You could now use $safe_file as the file path...
// ...or kill it if it had to be altered
if ( $file != $safe_file )
{
return;
}

// Establish the file path
$path = realpath( DIR_FILES . $file );
//$path = realpath( DIR_FILES . $safe_file );

// Do we have a valid file?
if ( ! is_file( $path ) )
{
return;
}

// Delete the file
unlink( $path );

MattF
02-21-2010, 09:13 PM
I didn't say sanitisation wasn't important. Here is my pseudo-logic:

My apologies then. :) I thought you were suggesting he should place more emphasis on merely restricting the access as the security measure.

student101
02-21-2010, 09:15 PM
Was about it implement a class that would check which version of PHP first, then do accordingly...


if ( !function_exists('version_compare') || version_compare( phpversion(), '5', '<' ) )
// use php4 code here
else
// use php5 code here



EZ Archive Ads Plugin for vBulletin Copyright 2006 Computer Help Forum