Is this piece of code secure - PHP && MySQL

Question

<?php
class test_class {

     public function __construct() { 

     }
     public function doLogin($username,$password) {

      include("connection.php");

      $query = "SELECT *
                      FROM users
                      WHERE username = '".mysql_escape_string($username)."'
                      AND password = '".mysql_escape_string($password)."'";
      $result = mysql_fetch_array(mysql_query($query));
      if(!$result) {

      return 'no';
      }
      else 
       {
      return 'yes';
       }
      }


}
?>

The above code works, but slightly worried whether its secure or not.

Note: I am not using POST method, so i have to receive it as arguments in the function and i cannot use.

if (isset($_POST['username']) && isset($_POST['password']))
     {
        $username= $_POST['username'];
        $password= $_POST['password'];

Answer 1

I think it is OK, however, if I am worried about security, I'd store the password of "username" into variable and compare it outside of query.

Answer 2

No. You should not be storing the raw password in your database. Store it hashed (preferably with a salt). Further, prepared statements are a better choice than escaping. See this PHP PDO documentation. As an added benefit (besides security), they can be more efficient.

Answer 3

uhh.... you're storing a plaintext password? That is most certainly not secure. The password should be hashed with a salt using something like sha256. Storing plaintext passwords is never a good idea.

Answer 4

The code might be secure but the implementation is not great. You should never store an authentication password as plaintext. You should salt and hash it.

I could spend an hour explaining why, but you'd do better just reading this.

Answer 5

The query itself appears secure, but if you used a DB interface that supported parameter binding, such as PDO or Zend_Db, you wouldn't have to scrutinise every SQL statement quite so nervously.

Also, the mysql-* functions are pretty much deprecated; you should look at the mysqli-* functions instead.

As a stylistic side note, there's no point in an empty constructor, and I'd suggest returning boolean true or false rather than string values.

Finally, as mentioned elsewhere, storing plaintext passwords is a bad idea.

Answer 6

The code itself looks ok, but the main issue I see is that you're passing passwords around in Plain Text.

Is the Client-to-Server connection secure (i.e. using SSL) Is the Server-to-Database connection secure

If in either case someone can sit on the wire and watch traffic going by, then you've got a security problem.

If it were me, I'd definitely have an SSL connection between the client & server.

I'd make sure you were storing a hash of the password in the database.

And I'd change your code to something like

//Pseduo Code
SELECT * FROM Table where UserName = $username
Get Row Back
if(MD5Hash($password) == DataRow[Password])
   //Valid