Is this piece of code secure - PHP && MySQL

StackOverflow https://stackoverflow.com/questions/915057

  •  06-09-2019
  •  | 
  •  

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'];
Was it helpful?

Solution

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.

OTHER TIPS

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.

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.

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.

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

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.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top