Simple PHP Auth Class feedback

Question

this is a basic PHP auth class I've put together for use with simple sites I do from time to time which don't warrant using a framework. I'm wondering if i'm separating out responsibilities all that well, even at this early stage! For example, I'm not sure where I should be filtering user input with mysql_real_escape_string, and I don't know if I should be passing the DB into the constructor, but then I need it to be able to run my auth check.

Anyway, any pointers would be great thanks!

class Auth {
 protected $db;
 /**
 * Auth::__construct()
 * 
 * @return
 */
 public function __construct($db = null)
 {
 $this->db = $db;
 }
 /**
 * Auth::login()
 * 
 * @param mixed $username
 * @param mixed $password
 * @return
 */
 public function login($username, $password)
 {
 $query = $this->db->query("SELECT username, password 
 FROM users 
 WHERE username = '".mysql_real_escape_string($username)."'
 AND password = '".self::crypt_pass(mysql_real_escape_string($password))."'
 ");
 $result = $this->db->fetch_array($query);
 if (count($result) == 0)
 {
 return FALSE;
 }
 else
 {
 //set session variable 
 Session::instance()->set('logged_in', TRUE);
 return TRUE;
 }
 }
 /**
 * Auth::logout()
 * 
 * @return
 */
 public function logout()
 {
 //set session variable 
 return Session::instance()->delete('logged_in');
 }
 private function crypt_pass($pass)
 {
 if (CRYPT_BLOWFISH == 1)
 {
 $salt = SHA1('acssalty457');
 $blowfish_salt = "\2ドルa\07ドル\$".substr($salt, 0, CRYPT_SALT_LENGTH);
 return crypt($pass, $blowfish_salt);
 }
 }
}

Answer 1

This part doesn't make sense to me:

private function crypt_pass($pass)
{
 if (CRYPT_BLOWFISH == 1)
 {
 $salt = SHA1('acssalty457');
 $blowfish_salt = "\2ドルa\07ドル\$".substr($salt, 0, CRYPT_SALT_LENGTH);
 return crypt($pass, $blowfish_salt);
 }
}

Functions that only return on condition are a bad idea. You should return something anyway, even if it's false. Now for the specific problem, what if CRYPT_BLOWFISH != 1? It's ok to want to use blowfist, but why cripple the library for everyone else? You could rewrite as:

private function crypt_pass($pass) {
 $salt = "acssalty457";
 if (CRYPT_BLOWFISH == 1) {
 $blowfish_salt = "\2ドルa\07ドル\$" . substr($salt, 0, CRYPT_SALT_LENGTH);
 return crypt($pass, $blowfish_salt);
 }
 return sha1($pass . $salt); 
}

Notice that I don't hash the salt? That's because security wise that's useless. You can ask around at Cryptography Stack Exchange for more. If you want to make the sha1 crypted pass a little bit harder you could do something weird like:

return sha1(strrev($salt) . $pass . $salt);

I'm using sha1 as an example of a natively included function, you can use any hash algorithm you like.

Answer 2

Not sure what version of php they implemented this in, but you don't need to use the __construct, you can use Auth instead like many other OOP langs.

You should check if $this->db is null before you do any sql work, otherwise login will fail with errors when used.

Another thing you should look into is using PDO (or anything that supports prepared statements). mysql_real_escape_string is not secure these days. http://php.net/manual/en/book.pdo.php

You should also filter the email and password using regex (preg_match) or the php filter function: http://www.php.net/manual/en/filter.examples.sanitization.php
http://www.php.net/manual/en/book.filter.php
http://www.php.net/manual/en/function.filter-input.php

• Charles Sprayberry: Actually, depending on the version of PHP it is much better to use __construct(). With namespaces in 5.3.3+ the class name is not used as constructor. Granted, the use case for this might be small but there is nothing wrong with the use of __construct() and may actually be preferred if using namespaces.
• PiZzL3: @Charles Sprayberry what's the difference mechanically? I just like to use it as it matches other langs I program it so it's just one less difference I have to remember. I didn't mean to imply there's anything wrong with using it, just pointing out it can be used and may be more readable and/or future proof.
• Charles Sprayberry: There may not be any difference mechanically...as long as you aren't using namespaces in 5.3.3. But, for PHP the Auth method in the Auth class used as constructor is included for backward compatibility purposes. Using this mechanic actually appears to be less future proof as it changes depending on the situation, whereas __construct() is always seen as the class constructor.

Answer 3

Try these blowfish wrapper functions on for size:

https://gist.github.com/1151912

They wrap the functionality, restrict it to where it's available (php 5.3), and provide the hash generation function, and a test for whether a hash is blowfish or not, making it easier to move from an earlier hash system to a newer hash system.

And yeah, skip mysql_real_escape_string and move towards parameterized queries from PDO, you'll be better off:

I just have a set of parameterized PDO wrapper functions like this:

query('select * from users where user_id = :user_id', array(':user_id'=>$user_id));

To make parameterizing queries at dead simple as possible, so that it becomes the default as opposed to a troublesome addition.