User registration and login

Question

I am doubtful about the security of my PHP code. I am new to programming, but want to learn how to secure things, protect my databases from SQL injection, and other best practices. I'd like to know if my databases are safe from attacks with this code.

public function __construct( $data = array() ) {
               if( isset( $data['username'] ) ) $this->username = stripslashes( strip_tags( $data['username'] ) );
              if( isset( $data['password'] ) ) $this->password = stripslashes( strip_tags( $data['password'] ) );
}

I would appreciate any suggestions to existing scripts if mine is insufficient. If somebody wants to inspect the entire code of this class, here it is:

<?php

class Users {
public $username = null;
public $password = null;
public $salt = "Zo4rU5Z1YyKJAASY0PT6EUg7BBYdlEhPaNLuxAwU8lqu1ElzHv0Ri7EM6irpx5w";

public function __construct( $data = array() ) {
if( isset( $data['username'] ) ) $this->username = stripslashes( strip_tags( $data['username'] ) );
if( isset( $data['password'] ) ) $this->password = stripslashes( strip_tags( $data['password'] ) );
}

public function storeFormValues( $params ) {
//store the parameters
$this->__construct( $params );
}

public function userLogin() {
$success = false;
try{
$con = new PDO( DB_DSN, DB_USERNAME, DB_PASSWORD );
$con->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );
$sql = "SELECT * FROM users WHERE username = :username AND password = :password LIMIT 1";

$stmt = $con->prepare( $sql );
$stmt->bindValue( "username", $this->username, PDO::PARAM_STR );
$stmt->bindValue( "password", hash("sha256", $this->password . $this->salt), PDO::PARAM_STR );
$stmt->execute();

$valid = $stmt->fetchColumn();

if( $valid ) {
$success = true;
}

$con = null;
return $success;
}catch (PDOException $e) {
echo $e->getMessage();
return $success;
}
}

public function register() {
$correct = false;
try {
$con = new PDO( DB_DSN, DB_USERNAME, DB_PASSWORD );
$con->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );
$sql = "INSERT INTO users(username, password) VALUES(:username, :password)";

$stmt = $con->prepare( $sql );
$stmt->bindValue( "username", $this->username, PDO::PARAM_STR );
$stmt->bindValue( "password", hash("sha256", $this->password . $this->salt), PDO::PARAM_STR );
$stmt->execute();
return "Registration Successful <br/> <a href='index.php'>Login Now</a>";
}catch( PDOException $e ) {
return $e->getMessage();
}
}

}

?>

Answer 1

PHP PDO

Your PHP script looks airtight, good use of PDO and prepared statements. Nothing to say about this at all other than good job.

SELECT *

SQL SELECT * statements should not be used in production code unless absolutely necessary. There are several reasons for this:

1. It's can cause inefficient query plans. And generally returns more data than you actually need. SELECT * has to go out and get the metadata for all of those columns, not just the two with a pretty index on it. (credit to @ckuhn203)

2. Or worse, if you only want 3 out of 80 columns. If you are returning 50000 results, you quickly starting eating up memory. (credit to @JohnP)

Instead...

Only select the fields you really need explicitly. For example:

$sql = "SELECT users.username, users.password FROM users WHERE username = :username AND password = :password LIMIT 1";

PROCEDURE

If you want to really speed up MySQL execution, you could create a stored procedure instead of passing ad hoc code to the RDBMS. Here is some example code that you would only need to execute once. You can do it in MySQL Workbench or pass it from PHP, doesn't matter.

DELIMITER | -- or some other non-reserved character
DROP PROCEDURE IF EXISTS sp_userLogin;
CREATE PROCEDURE sp_userLogin (
  IN p_username, 
  IN p_password
) AS 
BEGIN
  SELECT 
    users.username, 
    users.password 
  FROM users 
  WHERE username = p_username 
  AND password = p_password 
  LIMIT 1;
END|

DROP PROCEDURE IF EXISTS sp_register;
CREATE PROCEDURE sp_register (
  IN p_username, 
  IN p_password
) AS 
BEGIN
  INSERT INTO users(
    username, 
    password
  ) 
  VALUES(
    p_username, 
    p_password
);
END|
DELIMITER ; -- make certain you don't forget this

Then from PHP all you have to do is:

$sql = "CALL sp_userLogin(:username, :password)";

And:

$sql = "CALL sp_register(:username, :password)";

Hope this helps.

Answer 2

Let us review!

• Why are you strip_slashing and strip_tagging the username and passwords before they enter the database? Why do you care? (You don't, I'll get to that in the end).
• Calling __construct() inside of another method of the same object is not recommended. (What does __construct() do? As opposed to what do setUser() and setPassword() do)
• Hashing your password:
  • With your salt constant, it loses its meaning. An attacker with access to your server can see your code, and with access to the database, he can easily run a rainbow table against your password database.
  • Hashing just with SHA1 once isn't very helpful. SHA1 is too fast and is susceptible to brute force attacks.
  • The solution to both is to use password_hash() and password_verify() look them up. (Both of those functions require PHP 5.5 or higher. If you don't have PHP 5.5 or higher, you should use the password_compat library by ircmaxell, who is the same person who wrote the above functions for PHP's core.
• Indentation - Please, for the love of God.

---

What's wrong with your sanitizing

Sanitizing is an important job of the server. It makes sure nothing nasty comes in and affects what your users are seeing/experiencing.

Sanitizing is done as late as possible, for a simple reason. When the username is entered to the database, you shouldn't make assumptions on who is going to use that username. It could be your application later on, it could be an API you may or may not choose to provide one day, it could be a desktop application or a mobile application which does not care for HTML formatting.

When you extract that value from the database and are about to put it in HTML, that's the point where you should escape for HTML using htmlspecialchars(). Maybe at a different point you may want to output it to a JSON response for an API, in which case, json_encode is good, but htmlspecialchars() not as much.