Help! PHP Script Being Exploited [SQL Injection]

Hey

There was an SQL injection bug released yesterday which affects the majority of the youtube clones.

It affects msg.php, here is the SQL injection string:

http://site.com/path/msg.php?id=-1/**/UNION/**/ALL/**/SELECT/**/1,0x7430705038755A7A20616E64207870726F67206F776E616765,convert(concat((SELECT/**/svalue/**/from/**/sconfig/**/where/**/soption=0x61646D696E5F6E616D65),0x3a,(SELECT/**/svalue/**/from/**/sconfig/**/where/**/soption=0x61646D696E5F70617373))/**/using/**/latin1),4,5,6,7,8,9/*

Code (markup):

And here's what i have in msg.php :

<?php

session_start();
include("include/config.php");
include("include/function.php");
chk_member_login();

        if($_GET['s']=="0")
        {
                $conn->execute("update pm set
                                        seen = 1
                                where pm_id = $_GET[id]
                                ");
        }
        
		$sql = "select * from pm where pm_id = $_GET[id] AND receiver = '".$_SESSION['USERNAME']."'";
        $rs = $conn->execute($sql);
		 		
        STemplate::assign('subject',fx_replace('$sender_name',$rs->fields['subject'],$rs->fields['sender']));
        STemplate::assign('body', $rs->fields['body']);
        STemplate::assign('sender', $rs->fields['sender']);
        STemplate::assign('date', $rs->fields['date']);
        

        /* fetch user information */
                $sql = "select UID from signup where username='".$rs->fields['sender']."'";
                $rs_u = $conn->execute($sql);

        STemplate::assign('userid', $rs_u->fields['UID']);
        STemplate::assign('err',$err);
        STemplate::assign('head_bottom',"msglinks.tpl");
        STemplate::display('head1.tpl');
        STemplate::display('msg.tpl');
        STemplate::display('footer.tpl');
?>

Code (markup):

Anybody know of a fix?

Cheers

 

Place this at the top of your script.

$_GET['id'] = intval($_GET['id']);

PHP:

 

After:

chk_member_login();

PHP:

add:

$_GET['id'] = intval($_GET['id']);

PHP:

Edit: looks like nico_swd got to that.


OFFTOPIC:

The SQL queries can also be optimized greatly.

You can add a "LIMIT 1" to each query so mysql stops searching the index as soon as it reaches the first Result. (50% faster overall)

$sql = "select * from pm where pm_id = $_GET[id] AND receiver = '".$_SESSION['USERNAME']."'";
        $rs = $conn->execute($sql);

PHP:

to

$sql = "select * from pm where pm_id = $_GET[id] AND receiver = '".$_SESSION['USERNAME']."' LIMIT 1";
        $rs = $conn->execute($sql);

PHP:

and

        /* fetch user information */
                $sql = "select UID from signup where username='".$rs->fields['sender']."'";
                $rs_u = $conn->execute($sql);

PHP:

to

        /* fetch user information */
                $sql = "select UID from signup where username='".$rs->fields['sender']."' LIMIT 1";
                $rs_u = $conn->execute($sql);

PHP:

You can also merge the two SQL queries into one.

$sql = "select * from pm where pm_id = $_GET[id] AND receiver = '".$_SESSION['USERNAME']."'";
        $rs = $conn->execute($sql);

PHP:

and

        /* fetch user information */
                $sql = "select UID from signup where username='".$rs->fields['sender']."'";
                $rs_u = $conn->execute($sql);

PHP:

TO:

$sql = "SELECT pm.*, u.uid FROM pm LEFT JOIN signup as u (on u.username = pm.sender) WHERE pm.pm_id = ".intval($_GET['id'])." AND receiver = '".$_SESSION['USERNAME']."' LIMIT 1";
 $rs = $conn->execute($sql);

PHP:

Then you can just delete:

        /* fetch user information */
                $sql = "select UID from signup where username='".$rs->fields['sender']."'";
                $rs_u = $conn->execute($sql);

PHP:

 

No, no, no, no, no... Don't ever do that again... Always check what you are getting from the user's browser before sending anything to a mysql query!
You're only checking the $_GET once by if($_GET['s']=="0") and if is 0 or anything else it will still get to the second query, $sql = "select * from pm where pm_id = $_GET[id] AND receiver = '".$_SESSION['USERNAME']."'";
Use at least something like this:

$id = mysql_real_escape_string($_GET['id']);
$sql = "SELECT * FROM pm WHERE pm_id = '$id' ";

PHP:

Always at least escape strings used in a mysql query.

 

also to help stop sql injections, you can use the php function mysql_real_escape_string, it requires you to have an active mysql connection open to use it. If you haven't supplied the data (ie hard coded it into the script) you shouldn't trust it, that applies to non sql bound data as well (ie filenames for fopen etc)