# Protect database from SQL Injection and similar attacks - PHP

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

$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(); } } } ?>  - – Madara Uchiha Jul 10 '14 at 20:32 yea i think i read that before, if u can check my code on pastebin, isnt it the same? – Andurit Jul 10 '14 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 '14 at 20:38 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 '14 at 21:25 ## 2 Answers # 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
AS
BEGIN
FROM users
LIMIT 1;
END|

DROP PROCEDURE IF EXISTS sp_register;
AS
BEGIN
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.

-
Thumb up for this, thank you :) –  Andurit Feb 24 at 10:19

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

-
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 '14 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 '14 at 8:49