HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Custom login system (PDO, sanitizing, hash)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
systemloginhashcustomsanitizingpdo

Problem

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:

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

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

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

-
Any other (serious) problem with this code?

Solution

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.

Context

StackExchange Code Review Q#14267, answer score: 5

Revisions (0)

No revisions yet.