CODEWITHSUNDEEP
phpmysql
user39980
user39980

Reputation:

Is my SQL request secure with mysql_real_escape_string?

Below is a page that handles a login script and I am wondering if I have put it any security holes. I have been reading articles on protecting from injections and others and wanted to make sure that my code is secure.

It is submitted via ajax and returns JSON based on the login being correct or not.

<?php
ob_start();
session_start();
include ("config.inc.php");
include ("jsonEncode.php");

// ausername and apassword sent from form
$ausername = '';
$apassword = '';
$ausername = mysql_real_escape_string(stripslashes($_GET['username']));
$apassword = mysql_real_escape_string(stripslashes($_GET['password']));

$sql    = "SELECT * FROM admin WHERE ausername='$ausername' AND apassword='$apassword' LIMIT 1";
$result = mysql_query($sql) or die(mysql_error());

$data   = mysql_fetch_array($result);
$count  = mysql_num_rows($result);

if($count==1){
    $_SESSION['ausername'] = $ausername;
    $_SESSION['apassword'] = $apassword;
    $_SESSION['admin_id']  = $data['a_id'];
    $a_id = $data['a_id'];
    $_SESSION['LastLogin'] = $data['last_login'];
    $query = "UPDATE admin SET last_login = Now() WHERE `a_id`= $a_id";
    mysql_query($query);
    //echo $query;
    $_SESSION['aloggedin'] = "1234";
    // valid
    $var = array('avalid' => 1, 'ausername' => $ausername, 'apassword' => $apassword);
    print php_json_encode($var);
}else{
    // invalid
    $var = array('avalid' => 0, 'ausername' => $ausername, 'apassword' => $apassword);
    print php_json_encode($var);
}
?>

Upvotes: 0

Views: 252

Answers (6)

Mario
Mario

Reputation: 1525

All good answers above.

Only one thing I want to add that hasn't been mentioned... I tend to fetch the account password and do a PHP comparison rather than putting the password in the query and looking if the row exists.

Upvotes: 0

Kristen
Kristen

Reputation: 4311

(I'm an MSSQL bod, so don't know if any of these points are irrelevant to MySQL)

This isn't really to do with security, just general observations in case helpful:

Don't use SELECT * - list the columns you want back - looks like you only need a_id & last_login. You might add a Blob in that table with their photograph in the future, or personal notes etc. - it will kill performance in all the places where you did SELECT * in the past and didn't need the picture.

I wouldn't do LIMIT 1 - I'd quite like to know if there are DUPs at this point, and raise an error.

I would put the last_login column in another table linked 1:1 with your User / password table. Its a frequent-change item, and if you decide to introduce an Audit table on the user/Password table (i.e. store the old values whenever it changes) having a frequently changing "info" column mucks that up a bit.

Personally I would want to keep the column naming convention and the SESSION / variable one the same.

admin_id / a_id, LastLogin / last_login

Personally I wouldn't store password in the session unless you need it later on. I would store something to indicate the "permissions" the user has, and then use that to decide if they can view PageX or PageY etc.

Upvotes: 0

rick
rick

Reputation: 1547

Also, don't store the password in the session. Php session data is stored in the OS tmp/temp directory by default so the data could be viewed by others. Normally, I'll just keep the username in the session and query the database when needed. That avoids problems when a user's information is changed, but the session isn't updated.

Upvotes: 0

nobody
nobody

Reputation: 20174

You should be using bound parameters to put user data into your SQL, not string concatenation.

Also, you should probably be storing password hashes in your database - not the original plaintext passwords.

Finally, not a security issue, but setting $ausername and $apassword to '' immediately before giving them new values is entirely pointless.

Upvotes: 1

pawstrong
pawstrong

Reputation: 954

You don't need to strip the slashes. Unless you are also stripping slashes when these columns are populated, you've actually introduced a security hole -- if for whatever reason you don't have a unique constraint on the username field, and/or you have slashes in the in the stored username or password fields, and their passwords differed only by a slash, you could get one user logged in as another.

Upvotes: 1

Paige Ruten
Paige Ruten

Reputation: 176803

You might want to use the POST method rather than GET with the login form, otherwise their password will appear in the URL and URLs aren't very secure (they might get bookmarked or sent to another server as a referral URL, for example).

Upvotes: 4

Related Questions