Custom login system (PDO, sanitizing, hash)

Question

I have made a custom login form for my site, with the help of PDO (until now I used simple MySQL connection and sanitizing), and it looks like this:

session_name('NEWCOOKIE');
session_start();
require_once "config.php";
$member_username = $_POST['username']; 
$member_password = $_POST['password'];
 $crypt_pass = crypt($member_password,"somesalt");
try{
 $dbh = new PDO('mysql:host=localhost;dbname='.DB_NAME, DB_USERNAME, DB_PASS, array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8"));
 $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
} 
catch (PDOException $e) {
 echo "Fatal error.";
 file_put_contents('PDOErrors.txt', $e->getMessage(), FILE_APPEND);
} 
$sth = $dbh->prepare("SELECT * FROM ".DB_PREFIX."_users WHERE username = :user AND password = :pass");
$sth->bindParam(':user', $member_username);
$sth->bindParam(':pass', $crypt_pass);
$sth->execute();
$total = $sth->rowCount();
$row = $sth->fetch();
if($total > 0){
 if($row['activated']){
 $_SESSION["user_username"] = $member_username;
 $_SESSION["user_logedIn"] = true;
 $_SESSION["user_id"] = $row['user_id'];
 $_SESSION["timeout"] = time();
 header("location: index.php"); 
 }
 else{
 echo "ACCOUNT NOT ACTIVE";
 }
}
else{
 echo "WRONG PASSWORD OR USERNAME";
}

Is this safe? I know that nothing is 100% safe, but I have a small number of users (around 200-300), and 1000 visitors a month, so I don't expect very "professional" attacks and programming experts.

My questions are:

1. As you can see, I don't use sanitizing at all. I thought that PDO will do that part of the job, or I am wrong?

2. I hope that crypting of password is good (with no MD5 or SHA, is crypt() recommended)?

3. File config.php contains username and pass of my database. Should I protect it somehow, what permissions to set?

4. Any other (serious) problem with this code?

• alfasin: You might want to read this
• msanford: Something about that if($total > 0) condition irks me; I want it to be == 1 but I can't devise a compelling reason.

Answer 1

Well, let's see:

• You're using PDO and prepared statements, no risk of SQL injection there. Great!
• Your PDO code may throw an Exception (Specifically, a PDOException) at any time, so the whole database code block should be kept inside of the try/catch block.
• crypt in on itself isn't 100% secure. See This question for more details.
• Database credentials are not constant. They can change over time, and you may want to change the server, change the user, add an additional database etc in the near/far future. The solution is to use functions (or better yet, classes) and pass the credentials as variables, rather then applying them as app-wide constants.

• SomeoneS: Tnx for the answers.So, should i end "catch" block at the end of file, or just after $row = $sth->fetch(); ? Regarding database credentials, this is small-sized project, and i dont expect it to expand over time. Also, i am not very good with OOP, so i am totaly unfamilliar with classes :)
• Madara's Ghost: @SomeoneS: The try/catch block should encapsulate the database code. Personally, I would encapsulate the whole thing in either a mapper class, or some sort of function schema, and try/catch when calling the functions (keeping the actual database code clean from distractions).

Answer 2

Be sure that you're checking for a one-time (per page load) random token in your form to help protect against CSRF attacks. You can use mcrypt or bcrypt to ensure your tokens are cryptographically-sound.

Simply add a hidden form to your field with the value being your token:

<input type="hidden" name="token" value="<?php echo $token; ?>">

Store that token in your session array:

$_SESSION['token'] = $token;

And finally, match the tokens from the post and sessions arrays:

if (!isset($_SESSION['token'])) { return false; } // Session token isn't set
else if (!isset($_POST['token'])) { return false; } // Post token isn't set
else if ($_SESSION['token'] !== $_POST['token']) { return false; } // Possible CSRF attempt
else if ($_SESSION['token'] === $_POST['token']) { return true; } // Looks valid!
else { return false; }

You can store most of this in a separate method for re-use with other forms.

• Kid Diamond: I wouldn't recommend a new CSRF token per page load as this creates browser usability issues. Per session based is fine.

Answer 3

Not much of an security wizard, not at all really, but I've seen a few answers here regarding this and they all tend towards the advice I'm about to give.

1. From what I've seen its still good to sanitize even though you are using PDO, couldn't hurt anyways.
2. md5(), if I'm remembering correctly, is not as secure as sha(), but both are more secure than crypt(). BTW: crypt() is an encryption algorithm, not a hashing algorithm, though, from what I've seen, they are very similar but the distinction was made clear so its obviously important for some reason. Both md5() and sha() are still vulnerable to collision attacks, but are still widely used right now because of how unlikely it is to succeed. I'm not sure of a good alternative, if there even is one. Its hard to find a "perfect" encryption because they don't exist. Eventually they all will be beaten.
3. I honestly do not see a problem with saving your database connection variables as constants in a config file. While, yes, they will change over time, it won't be very frequent, and changing a variable in a function is not much different from changing a constant in a config file. The best way to secure it is to make sure it is below the web root directory and that your htaccess file prevents access to unauthorized files.
4. Would consider throwing in an exit after your header to prevent the code from continuing to run after the header has been sent. Common practice. Also, your try/catch block is flawed. Truth tried explaining it, but I don't think it translated very well. What he is trying to say is that the logic for accessing and reading the database should be in the try block. In other words, your binding of parameters, and anything else that is dependent upon a successful database connection should be between the try braces. As it stands right now it will attempt to connect and if it fails it will still attempt to bind those parameters and probably throw more errors.