Can someone look at this Security on a password check? :)

devsie

Can someone look at the code below and just tell me if there's any major flaws in it? Is it safe?

//Get results out of form input and make into SAFE variables, lowercase if text. Uppercase of postcode.
$loginname = mysql_real_escape_string(strtolower($POST["loginconame"]));
$loginpostcode1 = mysql_real_escape_string(strtoupper($POST["loginpostcode1"]));
$loginpostcode2 = mysql_real_escape_string(strtoupper($POST["loginpostcode2"]));
$loginpassword = mysql_real_escape_string(strtolower($POST["loginpassword"]));

//take info from the row of data where coname is what the user inputted
$result = mysql_query("SELECT * FROM table WHERE name='$loginconame' && postcode1='$loginpostcode1' && postcode2='$loginpostcode2'") or die(mysql_error());

$row = mysql_fetch_array( $result );

//check hashed pw's match
$hash = hash("sha256","myverylongandrandomletterswhichichangedforonhere".$loginpassword.$row['foo']);

//if login name, pw and goc number all correct, allow to proceed.
if($row['name']==$loginconame && $row['postcode1']==$loginpostcode1 && $row['postcode2']==$loginpostcode2 && $row['password']==$snphash && $row['active']=="on")

{...proceed...}
else {echo "failed";}

I could change the * to specify just the bits i need from the table, but does matter?

My main concern, is that the script is looking up details from the table before I've really checked against the password. This is because I've a 'salt' specific for that user in the table.

jazz_snob

devsie;10900012 wrote:
Can someone look at the code below and just tell me if there's any major flaws in it? Is it safe?

//Get results out of form input and make into SAFE variables, lowercase if text. Uppercase of postcode.
$loginname = mysql_real_escape_string(strtolower($_POST["loginconame"]));

//take info from the row of data where coname is what the user inputted
$result = mysql_query("SELECT * FROM table WHERE name='$loginconame' && postcode1='$loginpostcode1' && postcode2='$loginpostcode2'") or die(mysql_error());

First I recommend fixing the variable name in your sql statement, it should say name='$loginname' ....as written it is susceptible to sql-injection attacks. Edit or repost the correct code.

Also, I and not familiar with && syntax in an SQL statement; I've always used AND in my sql...in fact I've never seen &&. I'd recommend using AND.

NogDog

Just FYI: MySQL supports "&&" and "||" as alternatives to "AND" and "OR", so it will work OK in MySQL, but will not necessarily port to other DBMS's. (For instance, if I recall correctly -- it's been awhile -- Oracle SQL*Plus uses "||" as a concatenation operator, not a logical operator.)

Weedpacket

NogDog wrote:
Oracle SQL*Plus uses "||" as a concatenation operator, not a logical operator.

In fact, this is the standard SQL meaning of "||". But no-one ever said MySQL followed the standard too closely.

devsie wrote:
if($row['name']==$loginconame && $row['postcode1']==$loginpostcode1 && $row['postcode2']==$loginpostcode

There's no point checking these; you know they're correct because the only rows you selected are those where they were true.

devsie

Thankyou for your responses.

jazz_snob> ty, that was just an error copying it across. I did change/delete a few bits to post the code. In truth, it's all universal with "coname".

I had no problems using && instead of AND, but for correctness and I guess I never know where the future may lead I should try and stick to more mainstream/respected syntax and use AND. Again ty.

weedpacket > Thankyou. I did consider that myself. I kinda figured what's the point in rechecking that surely it must be the same. Then at the same time thought well whats the downside to rechecking that? I dunno, i left it in. Is there a downside to doing that again?

And did anyone have any comments on:
"My main concern, is that the script is looking up details from the table before I've really checked against the password. This is because I've a 'salt' specific for that user in the table."

Thanks for the replies 🙂