How can I make my login script more secure?

[tinfish]

Hi guys, It's my first post here, not looking to leech, I'm simply here to learn and develop my skills and any contributes will be greatly appreciated!

 

Anyways I have made a simple login script, however I would like to make it more secure. However before that, can you please explain to me as to why it is not secure in the first place? A basic explanation so I can understand would be great. 

 

Then after that, could you please give help as to how I would make this login code more secure?

 

Thank you very much

 

<?php
   $rowsfound=false;
    if (isset($_GET['frmStudentId']))
    {
           // functions to make performQuery() work correctly
           require_once("dbfunctions.inc.php");
           $query = "SELECT dbStudentId, dbStudentName " .
           " FROM student " .
           " WHERE dbStudentId = '".$_GET['frmStudentId']."'" .
           " AND dbPassword = '".$_GET['frmPassword']."'";
           $result = performQuery($query);
           if(count($result) > 0)
           {
           $rowsfound=true; // allow login
           }
           }
// code continues by generating appropriate response ...

[smoseley]

For starters, prevent SQL injection:

 

$id = mysql_real_escape_string($_GET['frmStudentId']);
$pwd = mysql_real_escape_string($_GET['frmPassword']);
$sql = "SELECT dbStudentId, dbStudentName
        FROM student 
        WHERE dbStudentId = '{$id}'
        AND dbPassword = '{$pwd}'";

$result = performQuery($query);

 

 

Of course, you should probably take your passwords to the next level by hashing them with randomly salted MD5 (more secure for your users).

 

$id = mysql_real_escape_string($_GET['frmStudentId']);
$pwd = hashup($_GET['frmPassword']);
$sql = "SELECT dbStudentId, dbStudentName
        FROM student 
        WHERE dbStudentId = '{$id}'
        AND dbPassword = '{$pwd}'";

$result = performQuery($query);

function hashup($pwd) {
    $salt = "5w3we5gwgw52eg55uehgoih34we#G$34gih34g#$wegG34G#4g33weegwgwG$#G3gerG%^H75j&22gwGW";
     return md5($pwd.substr($salt, strlen($pwd), 5));
}

 

 

And then, you could add some protection against bots by limiting the # of attempts...

 

 

$id = mysql_real_escape_string($_GET['frmStudentId']);
$pwd = hashup($_GET['frmPassword']);
if (canLogin($id)) {
    $sql = "SELECT dbStudentId, dbStudentName
            FROM student 
            WHERE dbStudentId = '{$id}'
            AND dbPassword = '{$pwd}'";

    $result = performQuery($query);

    if (count($result) > 0) {
        // successful login
    } else {
        registerFailedLogin($id);
    }
} else {
    echo "Limit 5 login failures per hour.";
}
function hashup($pwd) {
    $salt = "5w3we5gwgw52eg55uehgoih34we#G$34gih34g#$wegG34G#4g33weegwgwG$#G3gerG%^H75j&22gwGW";
     return md5($pwd.substr($salt, strlen($pwd), 5));
}

function registerFailedLogin($uid) {
    $ip= $_SERVER["REMOTE_ADDR"];
    $sql = "INSERT INTO failed_logins (uid, ip_address, timestamp) VALUES ('{$uid}', '{$ip}', NOW())";
    mysql_query($sql);
}


function canLogin($uid) {
    $ip= $_SERVER["REMOTE_ADDR"];
    $sql = "SELECT COUNT(*) as failed FROM failed_logins WHERE uid = '{$uid}' OR ip_address = '{$ip}' AND timestamp > NOW() - INTERVAL 1 HOUR";
    $result = mysql_query($sql);
    $row = mysql_fetch_assoc($result);
    return $row["failed"] < 5;
}

 

 

[tinfish]

Thanks for the swift response. Your help is appreciated!

However I have also heard that using $_post as opposed to $_get is also recommended? Although in my eyes it does the same thing?

 

Thanks

[smoseley]

Yeah, def use post vs get.  I completely missed that!

 

The problem with the get method is that it uses the querystring to pass parameters. So if someone logs in to your site, it'll go to this url:

 

http://www.mysite.com/login.php?frmStudentId=myusername&frmPassword=my_unencrypted_password!

 

That url will get stored in the browser's history, which means anyone can come in and look for http://www.mysite.com/login.php in the browser history to get other people's credentials.

 

I'm sure you can infer the major pitfalls there.