Is this PHP login technique safe considering SQL injection?

Question

I'am investigating a site that was written long ago by some PHP developer, and I'd like to know if the login technique he used was safe or not.

Here is the way he did it:

$username='';
$username = escapeshellcmd($HTTP_POST_VARS['user']);
$pwd = escapeshellcmd($HTTP_POST_VARS['pw']);

$loginerror=false;

if ($logout=="1")
{
  closesession($s_id);
  $username='';
  $logged=false;
}

$logged=checksession(session_id(), $ipaddr);

if ((!$logged) && ($username!=''))
{
        //$username = escapeshellcmd($HTTP_POST_VARS['felhasznalo']);
        //$pwd = escapeshellcmd($HTTP_POST_VARS['jelszo']);
        if (checkuser($username, $pwd, DOM))
            {
            if (sessionstore(session_id(), $username, $pwd, $ipaddr, $datum, DOM))
                {
                $logged=true;
                }
            }
        else
            {
            $loginerror=true;
            ;
            }       
}
if ($logged)
    {
    $username=getsessionuser(session_id());
    $remember=getremember($username, DOM);
    }
?>

function checkuser($u, $p, $d )
{
$sql_ell='SELECT PWD FROM USERS WHERE ACTIVE=1 AND USERNAME="'.$u.'" AND DOMAIN="'.$d.'"';
$eredm_ell= mysql_query($sql_ell);
if ($eredm_ell)
    {
    $domainnumrows=mysql_num_rows($eredm_ell);
    if ($domainnumrows==1) 
        {
        $egy_sor = mysql_fetch_row( $eredm_ell ); 
        $pwd_in_table=$egy_sor[0];
        if ($pwd_in_table==md5($u.$p))
            {
            return true;
            }
        } // rows
    } // ered
return false;   
} // func

Is this safe?

code review doesn't belong here. http://codereview.stackexchange.com/ — Jonathan Kuhn, Aug 18 '11 at 20:20
I would highly recommend reviewing this: http://stackoverflow.com/questions/60174/best-way-to-stop-sql-injection-in-php — philwinkle, Aug 19 '11 at 03:41

Nanne · Accepted Answer · 2012-05-16T11:49:01.320

9

If I see correctly, the only check done on username is escapeshellcmd. That is NOT enough. Again, if I see correctly, it gets put into this query:

$sql_ell='SELECT PWD FROM USERS WHERE ACTIVE=1 AND USERNAME="'.$u.'" AND DOMAIN="'.$d.'"'

where you can do all sorts of nasties.

So no. it's not safe.

edited May 16 '12 at 11:49

answered Aug 18 '11 at 20:18

Nanne

64,065
16
119
163

Is this PHP login technique safe considering SQL injection?

1 Answers1