Page 1 of 1

resolving bug

Posted: Fri Mar 26, 2010 8:56 pm
by shaqa
i was checking my php files with Pixy and saw some vulnerability does any one can tell me how do i replace with nonbug-ed lines!?.

Code: Select all

$result=mysql_query($sql) or die(mysql_error());
also:

Code: Select all

$sql="select * from se_fileuploads where userupload_id=".$_GET['upid'];
$tmp = mysql_query($sql);
$fileupload = mysql_fetch_object($tmp);
 
    $sql="select * from se_users where user_id=$userid";
    $temp=mysql_query($sql);
 
$current_rating=($rating/$count) * 25;
$voted=@mysql_fetch_assoc(@mysql_query("SELECT * FROM se_fileratings WHERE user_id='$uid ' AND userupload_id='$id' "));
    if($voted){
also using this line i think is not safe :

Code: Select all

readfile("./userfiles/".$name);
lines with red color are with vulnerability.
please someone can help me to avoide these bugs

Re: resolving bug

Posted: Fri Mar 26, 2010 8:59 pm
by Benjamin
You have been here long enough to know that you should use

Code: Select all

 tags.  :evil:

Re: resolving bug

Posted: Sat Mar 27, 2010 1:01 pm
by shaqa
Benjamin wrote:You have been here long enough to know that you should use

Code: Select all

 tags.  :evil:[/quote]
i thought i used! sorry

Re: resolving bug

Posted: Sat Mar 27, 2010 10:47 pm
by Alkis
First of all, for all the fields that are expected to be primary keys, and so integers, just use casting.

For example the first line:

Code: Select all

$sql="select * from se_fileuploads where userupload_id=".(int)$_GET['upid'];
That way, you ensure that you get integer from $_GET['upid']. Even if the return is 0, you secure your following query. In the worst case you
won't just get any results.

Same goes for other variables that are epxtected to be integers.

as for the readfile line, do a regular expression replace in case there are any "/" or "." etc., and also trim for spaces on the $name:

//removing possible . or ../ or ./ from the start as also on the remaining file name and /, ~ characters:

Code: Select all

$name = trim( preg_replace("/^\.*(\./)*\/*~*/", "",$name) );

//Then check if the file exists, and is of type file:
if(file_exists("./userfiles/".$name) && is_file("./userfiles/".$name) ){
    readfile("./userfiles/".$name);
}