Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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();
}
}

}

?>
share|improve this question
    
    
yea i think i read that before, if u can check my code on pastebin, isnt it the same? –  Andurit Jul 10 at 20:34
    
Thanks for your opinion, i paste code here nad hope sombody else will check it as well. Anyway thanks a lot agian! –  Andurit Jul 10 at 20:38
1  
Why are you doing a select * ... limit 1? That implies that you could have an instance of duplicate usernames and duplicate passwords. And if that is the case, the person listed second in the db will always log in as the first person. –  JohnP Jul 10 at 21:25

2 Answers 2

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;
CREAT 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.

share|improve this answer

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.

share|improve this answer
1  
I completely agree, especially with what you had to say about the constant salt. One thing to note, it could be possible the OP doesn't have PHP ver. 5.5, and therefore cannot use the recommended password_hash(). In that case, there are other alternatives that are considered safe. –  Alex L Jul 10 at 23:14
    
@AlexL: In which case, he should use the password_compat library by ircmaxell (who is the same guy who wrote password_hash() and password_verify() for the PHP core. I'll edit my answer to reflex it. –  Madara Uchiha Jul 11 at 8:49

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.