Reduntant code in a log in script?

Question

I have a login script that I found on some page a while ago, and while looking at the code that checks if the user is valid, it seems that a small amount of the code is redundant.

$qry = "SELECT username FROM users WHERE ".
"username = '". $username ."' AND password = '" . md5($password) . "'";
$result = mysql_query($qry);
if(mysql_num_rows($result) == 1) {
    while($row = mysql_fetch_assoc($result)) {
        $_SESSION['USERNAME'] = $username;
        $_SESSION['PASSWORD'] = $password;
    }
    session_write_close();
    header("location: memberpage.php");
} else { .... }

To me, the while-loop seems redundant since the if-code already checks if the user is valid (1 row returned). Can I just remove the while-loop and get the same result or is should i be there like some sort of extra security to really check that the number of rows are valid?

It is good that you are using a hash function for your passwords, but bad that you are [using md5 (it isn't secure) and failing to use a salt](http://php.net/manual/en/faq.passwords.php). You [shouldn't build SQL by mashing strings together](http://bobby-tables.com/) or use the old `mysql_*` functions (replaced by `mysqli_*` and PDO) either. — Quentin, Jun 14 '12 at 06:27
also, I wouldn't recommend storing the PW in the session vars, especially unhashed; it really serves no purpose. — mpen, Jun 14 '12 at 06:29
@Mark. So basically, it's perfectly secure to authenticate a logged on user with $_SESSION['USERNAME'] only? — Sandokan, Jun 14 '12 at 06:34
@Sandokan: Been awhile since I wrote any authentication code, but the session vars are stored on the server, I don't believe they can be hacked. You can hijack a session, however. Usually the server knows what session to resume via a cookie that is passed by the client. If the client spoofs that cookie, he can hijack, regardless of what session vars you're keeping. I'd recommend checking that the request is coming from the same IP and perhaps user-agent string (store those in the session var instead of the PW). — mpen, Jun 14 '12 at 17:55

score 3 · Accepted Answer · edited May 23 '17 at 12:03

3

Yes it is absolutely redundant, you can safely remove the while loop. As for security for your queries, check out:

Best way to prevent SQL Injection in PHP

edited May 23 '17 at 12:03

Community

1
1

answered Jun 14 '12 at 06:25

Sarfraz

355,543
70
511
562

score 1 · Answer 2 · answered Jun 14 '12 at 06:27

1

No need of while loop, if there is no result then if condition fails

answered Jun 14 '12 at 06:27

Riskhan

4,082
11
44
70

score 0 · Answer 3 · answered Jun 14 '12 at 06:36

0

 $result=db_query("SELECT username FROM users WHERE ".
  "username = '". $username ."' AND password = '" . md5($password) . "'");
 $res=db_fetch_array($result);
if(isset($res['username']))
{
   $_SESSION['USERNAME'] = $username;
    $_SESSION['PASSWORD'] = $password;

 }

answered Jun 14 '12 at 06:36

chaitu

589
6
13

Beeing overly paranoid here, wouldn't this just check IF a username is set, not validating that it's an actual valid user? Or is it impossible to "fake" a session username? – Sandokan Jun 14 '12 at 06:40

Reduntant code in a log in script?

3 Answers3