resolving bug

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
shaqa
Forum Commoner
Posts: 62
Joined: Mon Aug 13, 2007 9:11 am

resolving bug

Post 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
Last edited by Benjamin on Fri Mar 26, 2010 8:58 pm, edited 2 times in total.
Reason: Added [code=php] tags.
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Re: resolving bug

Post by Benjamin »

You have been here long enough to know that you should use

Code: Select all

 tags.  :evil:
shaqa
Forum Commoner
Posts: 62
Joined: Mon Aug 13, 2007 9:11 am

Re: resolving bug

Post 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
Alkis
Forum Commoner
Posts: 31
Joined: Fri Mar 26, 2010 8:41 am

Re: resolving bug

Post 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);
}
Post Reply