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 building a library and I need suggestions on how to improve it (security, performance, etc).

<?php
/**
 * Authentication Library
 *
 * @author John Svensson
 */
class Auth
{
    public $captchaCount;

    protected $db;
    protected $PasswordLib;

    private $message;

    public function __construct(PDO $db, PasswordLib\PasswordLib $PasswordLib)
    {
        $this->db = $db;
        $this->PasswordLib = $PasswordLib;
    }

    public function login($username, $password) {
        try {
            $sth = $this->db->prepare("SELECT id, username, password, active FROM user WHERE username = ?");
            $sth->setFetchMode(PDO::FETCH_OBJ);
            $sth->execute(array($username));
            $result = $sth->fetch();
        } catch (PDOException $e) {
            throw new Exception($e->getMessage());
        }

        if ($result) { 
            if ($this->PasswordLib->verifyPasswordHash($password, $result->password)) {
                if ($result->active == 0) {
                    $this->setMessage("<p>You must activate your account!</p>");
                    return FALSE;
                }
                session_regenerate_id();
                $_SESSION['userid'] = $result->userid;
                return TRUE;
            } else {
                $this->addLoginAttempt($username);
                $this->captchaCount++;
                $this->setMessage("<p>The password you entered is incorrect. Please try again.</p><p>Forgot your password? <a href=\"forgotpassword.php\">Request a new one.</a>");
                return FALSE;
            }         
        } else {
            $this->addLoginAttempt($username);
            $this->captchaCount++;
            $this->setMessage("<p>Incorrect username</p>");
            return FALSE;
        }
    }

    public function IsLoggedIn() {
        if (isset($_SESSION['userid'])) {
            return TRUE;
        }
    }

    public function getLoginAttempts() {
        $ip_address = ip2long($_SERVER['REMOTE_ADDR']);
        $sth = $this->db->prepare("SELECT COUNT(*) FROM login_attempts WHERE ip_address = ? AND attempted > DATE_SUB(NOW(), INTERVAL 15 minute)");
        $sth->bindParam(1, $ip_address);
        $sth->execute();

        return $sth->fetchColumn();
    }

    private function addLoginAttempt($username) {
        $ip_address = ip2long($_SERVER['REMOTE_ADDR']);
        $sth = $this->db->prepare("INSERT INTO login_attempts(username, ip_address, attempted) VALUES(?, ?, NOW())");
        $sth->execute(array($username, $ip_address));
    }

    public function getMessage() {
        return $this->message;
    }

    private function setMessage($value) {
        $this->message = $value;
    }
}
?>

<?php
$data = array();

$Auth->captchaCount = $Auth->getLoginAttempts();

var_dump($Auth->captchaCount);

if ($Auth->captchaCount > 0) {
    //sleep (2 ^(intval($count) -1));
}

$data['captcha'] = false;

if ($Auth->captchaCount > 9) {
    $data['captcha'] = true;
}

if (isset($_POST['submit'])) {

    if (isset($_POST['token']) && $_POST['token'] === $_SESSION['token']) {

        if ($data['captcha']) {

            $resp = recaptcha_check_answer ($private_key,
                                        $_SERVER["REMOTE_ADDR"],
                                        $_POST["recaptcha_challenge_field"],
                                        $_POST["recaptcha_response_field"]);

            if (!$resp->is_valid) {
                $data['error'] = 'The reCaptcha was entered incorrectly.';
            }
        }

        if (!isset($data['error'])) {

            if ($Auth->login($_POST['username'], $_POST['password'])) {

            } else {
                $data['error'] = $Auth->getMessage();

                if ($Auth->captchaCount > 9) {
                    $data['captcha'] = true;
                }

            }
        }

    } else {

    }
}
$data['token'] = $_SESSION['token'];
$data['public_key'] = $public_key;

echo $twig->render('login.html', $data);
?>
share|improve this question
    
Not sure if this got mentioned below but I'd separate any DB/SQL stuff into a separate class and pass and instance of that DB class instead of PDO. –  Cobby May 15 '12 at 1:34
    
I wouldn't generate the HTML in the with class... Makes it less reusable –  SeanJA Feb 18 '13 at 5:30

5 Answers 5

Disclaimer: I am no security guru. So a lot of what I say in regards to security are assumptions. The code review is good, but you should get a second, third, maybe even fourth opinion on my suggestions regarding security. I will denote each "opinion" that way you know to take it with a grain of salt. That being said, on to the review!

Session Variables

Are these session variables, userid and username, not the same? Most people only use one or the other, but I'm assuming that since you use both that means userid is a numerical representation of that person. I would try finding some way of limiting the amount of dependencies you need, especially if those two are more or less the same. Code is like a machine. The more moving parts you have, the more things there are to break. If you need both username and userid in your code I would find some way of producing one fromt the other. Also, I do not see where you set these variables. If they are crucial for checking if a user is logged in I would set them inside the class that way there is no confusion about where they come from.

Logging In

When you are checking if a user is logged in you only return TRUE. What if they aren't logged in? You should return either TRUE or FALSE that way you don't have to write anything extra when checking for it.

This is one of those grain of salt moments, though I feel confident in what I'm saying, I may be missing a step or may have misunderstood my source. From what I've read the more secure way is to compare the current session id to that of the one last used to log in. If the user has not logged in in a while then it would force the user to the login page to refresh credentials. After verifying session id, it would THEN verify that userid and username are set.

Logging Out

When logging out, it is unnecessary to use session_unset(). At least from what I gather. session_destroy() destroys everything in the session so that it is completely empty, wheras session_unset() merely unsets everything but leaves information such as session id available meaning the session still technically exists. So, even if it was necessary to use both, you got them backwards as there would be nothing left to unset once everything was destroyed, but plenty to destroy after everything has been unset.

I also do not think that exit is necessary. After setting a header your script will stop running before even getting to it.

Expanding Variables

This is a preference, but one MANY agree with. Variables should be expanded so it is completely obvious at first glance what they are used for. I won't argue for expanding $db or any other commonly used abbreviation that everyone seems to know, but I would argue that you shouldn't go making your own abbreviations. Its just a matter of legibility. So, in this instance $dest should probably be expanded to $destination. Normally I would point out $sth but having seen it so many times recently, I think that is also one of those standardly accepted abbreviations.

Separating Code

I do not think that the create_account() and generate_salt() methods should be included in this class as it does not have anything to do with authentication. It does share a common resource, $db, but it does two completely different things with it. One class should only read it, while the other manipulates it. However, this might be considered one of those design preferences. So I leave this up to you. Just figured I'd point it out.

Also, I would group my public/private/protected methods separately. I personally separate my code with long comment breaks that visually as well as physically separate each section so that it is easier to find the beginning of each section when scrolling. I'm not saying you need to do that, but I would at least separate them from each other. Makes it much easier to find methods if they are sorted in some fashion.

Code Dependency

This is a common mistake. Classes, generally, should not be dependent on any other file. No outside magical constants, variables, or classes should find their way into your models or controllers. Even if you aren't using an MVC pattern, it is good practice to make your classes completely self dependent, that way if something breaks you wont have to go hunting for it, it will all be self contained. So SITE_KEY and, in my opinion, those session variables should be defined inside this class.

Braces

Please, please, please use braces {}. I say this a lot, so please excuse me if I rant a little. A matching brace pair only adds 2 bits to your file size so there is no reason to exclude them for speed or file size limitations, though you should never have a file size "limit" to begin with. Braces, when combined with brace-matching in IDE's make it easy to determine where one statement ends, and another begins. Not only in IDE's, but also just in plain reading. The fact that PHP allows this sadens me to no end. It just supports poor coding habits and I wish they would deprecate and do away with its support.

Verifying Variables or Attributes Exist

What do you do if, for some unknown reason, get_salt() doesn't have a salt set? For one, your code will produce a nice pretty error for your clients. Even if you are sure of the source, if you did not set it in the imediate scope, you should verify that it is there and, if necessary, provide error support. There is always room for errors or unforseen circumstances.

Reusing Code

If possible, I would try to find some way of reusing the code with get_attempts() and add_attempt(). They are so similar it would be a shame to not reuse that code. And why are these public? I can only imagine that they would be necessary when performing a login, so I would make them private and then filter all calls to them through the login function.

I've been seeing a lot of these prepared statements recently and something I end up doing for all of them is cleaning them up. To prevent yourself from having to rewrite code, or in case you ever wish to expand upon this, as you did mention you wanted to reuse this, I would rewrite your prepared statements like so...

$fields = array(
    'username' => $username,
    'password' => $password,
    'salt' => $salt,
    'email' => $email,
    'created' => $created,
);

$attributes = implode(', ', array_keys($fields));
$values = implode(', :', array_keys($fields));

$sth = $this->db->prepare("INSERT INTO `user` ($attributes) VALUES (:$values)");

If you are wondering why I added those values in the $fields array then never used them, it is so you can set all of that information once then reuse it later. I noticed that you reuse this information in multiple methods and you just keep passing them as arguments. Do it once, save it as a class property and then reuse it.

Also, you have this perfectly good check_record() method, but don't use it to its fullest. You could convert it to execute more of your prepared statements by simply passing another argument.

protected function check_record($field, $select, $value) {
    $sth = $this->db->prepare("SELECT $select FROM user WHERE $field = ?");
    $sth->execute(array($value));
    return $sth;
}

protected function get_salt($username) {
    $sth = $this->check_record('username', 'salt', $username);

    if ($sth->rowCount() > 0) {
        $row = $sth->fetch();
        return $row['salt'];
    }
}

There's more to it than this of course. You'll notice I removed a bit from the end of the check_record() method, so you'd have to do some tweaking, but it would clean up some of your code elsewhere.

By the way. Those braces you hade in your prepared statement were unnecessary. The only time you need to use braces between double quotes is if you are using a class property or array element. There may be a couple of more instances, but I can't remember them off the top of my head right now. Just know that they aren't necessary for a basic variable.

"{$array['element']} or {$this->property}"

Clearing Up Confusion

You should work on that return statement for check_record(), it is rather confusing. You have it set up to return a boolean, then typecast it to return an integer... So you really want a 1 for true or 0 for false... Then its to return null if $count was initially false... Kind of confusing.

!!!UPDATE!!!

New Code

In your "login.php" example, I'm not sure if its just some stackoverflow quirk, but if you do indeed have two if statements on the same line I would separate them. Just helps with legibility.

Also, where did $user->active come from? I've scanned the authentication class again and I didn't see an active property instantiated at the beginning, nor one in the code. After hunting it down I determined that its coming from the returned SQL object you got from the login method, but otherwise I would be lost as to where this came from. I would suggest that you create a new method for queries such as this. Let the class handle gathering information and the controller, or view, or whatever you are using, request it. Hope this makes sense.

Logged In Method

What you have for the logged_in() method is fine I just want to point out that the else statement is unnecessary. If the if statement is true, it will never get to the rest of the code, so there is no need for an else.

I will look for a resource, but it was a while ago that I was reading that, so it might not even be the same one that I find for you. I will look through any resource I find to make sure its similar enough though. When I find such a resource I will leave you a comment so that you are aware of the update, much like I did for this update.

Separating Code

It was just a suggestion. A lot of people do combine functionality in their authentication classes I just find it counter intuitive.

Reusing Your Class

If you wish to leave the class unchanged between each use then I would suggest using a config file to set those variables and adding an include for it in the construct method so that they are registered on instantiation gauranteed. I suggest a config file that way you can keep a copy with your class and then only have to update the config file for the new information. I don't know if this is the cleanest way of doing it, just the first that came to mind.

Also, just because you don't want to change it after you have it the way you want it, I would still come back to it periodically and read over it. This does two things. First, it keeps you familiar with it. And second, as you grow as a programmer your style and knowledge will grow with you and you may find that you could do something better.

Verifying Variables

If there is no "salt" key PHP would throw an unknown offset, or some other similar error. If you are not seeing it then you either have error reporting set too low or it just hasn't happened to you yet. As for if that is the proper way to go about it, not a clue. I'd have to see it in action and still wouldn't be sure. If you think that it is proper, more than likely it will be, as you know more about your system than I do :) Everything here is, more or less, suggestions. In the end you know more about your program and what is best for it and you have the final call.

Check Record Method

When I said you'd still have to tweak your check_record() method, I meant that my version no longer did everything the original did. The original check_record() function got the column count and returned it. The one I suggested only returns the statement and expects anything further to be done outside of the method. If you wished to continue to get a count, you'd have to tweak your code in the appropriate places to do so. Because I changed it to be more reusable and handle different kinds of records, you'd have to find some other way for getting the desired results. I'd personally lean more towards creating a new method, maybe with a switch statement for different kinds of results, that way it could be reused.

!!!Final Update!!!

Your last update should have been a new question as it completely changed the code, making my previous comments seem out of place. I will leave them as I believe they are still pertinent to you and I don't feel like reading through them to trim whats now unnecessary. This answer has gotten a bit large and difficult to read, and possibly redundant, so this will be the last update I do for it. If you wish further help, I would suggest starting a new question. Also, my apologies for that couple days of silence. Mother's day and all...

Namespaces

I don't have much experience with namespaces myself, but this advice should still be valid. If you are using namespaces, I would make them more unique. PasswordLib\PasswordLib is not very informative and kind of confusing, especially followed by $PasswordLib. Also, it seems like we're taking a step backwards here. This is using a dependency that will only cause you to have to update your class later, which you wanted to avoid. Using an object of PasswordLib, like you did for PDO would be fine, but using the namespace could cause issues. Should you ever decide to use some other source to supply the instance of PasswordLib other than the file where it is defined for instance.

Reusing Code

I would reuse the following code from your login() method. Just make it part of the addLoginAttempt() method.

    $this->captchaCount++;
    $this->setMessage($message);
    return FALSE;

Then you can just do return addLoginAttempt($username, $message);.

share|improve this answer
    
I updated my post with an answer to your POST, thanks! –  John May 11 '12 at 12:39
    
@John: I updated my answer to cover your update. –  mseancole May 11 '12 at 14:22
    
I think you missunderstand the login method, its function is to return a object of the user if its available, and then on a login.php file check like $user->active == 1 (is the user activated or not?) and if all validation goes through it sets sessions. (basically, the controller grab a object does some additional checks then decides whetever the user should be logged in or not) –  John May 11 '12 at 15:47
    
@John: I did not misunderstand the login method. I just don't think it should return that object. I may be wrong and what you are doing is perfectly fine, but I doubt it. You are returning the full user object from the database, password and all, to a potentially insecure script. Someone could potentially create a new file and then do something like var_dump($userObject); and have all that information. –  mseancole May 11 '12 at 16:33
    
@John: Also, I just find it unintuitive to return a "magic" property from something. It's just another one of those things that is not verified before being used. If you were to move that to a separate method in your auth class and have it return the value of "active" you could run any verification you needed to without your login.php script being any the wiser. –  mseancole May 11 '12 at 16:33

To add to showerhead's answer, I would suggest to keep your functions as simple and specialized as possible. The logout function in particular should do just that - logout the user. It shouldn't redirect to anything, and it should definitely not kill the current PHP session by calling exit. That will make your library unusable by many developers.

What if the developer wants to use their own redirect function and not a header() call?

What if the developer wants to do something else, clean up, etc. after calling logout?

Those are questions you won't actually need to answer if you keep your functions simple and specialized.

share|improve this answer

I think for sql injection you're ok. Some tips that are general points though:

Don't catch exceptions just to throw a new exception

try {
    $sth = $this->db->prepare("SELECT id, username, password, active FROM user WHERE username = ?");
    $sth->setFetchMode(PDO::FETCH_OBJ);
    $sth->execute(array($username));
    $result = $sth->fetch();
/* Remove this
} catch (PDOException $e) {
    throw new Exception($e->getMessage());
*/
}

Don't catch exceptions that you don't know how to handle. Unless there's a reason you're catching PDO exceptions - in which case document it.

Use less nesting.

Don't needlessly use } else { only one possibility }.

This is easier to read than the original:

if ($result) { 
    if ($this->PasswordLib->verifyPasswordHash($password, $result->password)) {
        if ($result->active == 0) {
            $this->setMessage("<p>You must activate your account!</p>");
            return FALSE;
        }
        session_regenerate_id();
        $_SESSION['userid'] = $result->userid;
        return TRUE;
    }
    $this->addLoginAttempt($username);
    $this->captchaCount++;
    $this->setMessage("<p>The password you entered is incorrect. Please try again.</p><p>Forgot your password? <a href=\"forgotpassword.php\">Request a new one.</a>");
    return FALSE;
}
$this->addLoginAttempt($username);
$this->captchaCount++;
$this->setMessage("<p>Incorrect username</p>");
return FALSE;

Be aware of the difference between isset and empty

This will return true if the userid in the session is e.g. ""

public function IsLoggedIn() {
    if (isset($_SESSION['userid'])) {
        return TRUE;
    }
}

This will return true only if the userid is truethy:

public function IsLoggedIn() {
    if (!empty($_SESSION['userid'])) {
        return TRUE;
    }
}

As one of the other answers mentions you can also just write it as:

public function IsLoggedIn() {
    return isset($_SESSION['userid']);
}

Don't assume html

$this->setMessage("<p>...

A message inside a class like this shouldn't assume it is to serve html - what if it's responding to an ajax request, in xml, json or even just a log file?

Don't sleep

I'm not sure if this is just code for demonstration but this is actually the one "problem" with the code in the question.

Using sleep will tie up your resources - that's not in your interest. Consider instead bailing early and just returning a 403. Right now before prompting a user with a captcha - you are holding the request open for more than a minute. If you get enough users (and enough could be a relatively small number) trying to login at once and getting it wrong - you're going to take your own server offline by occupying all connections with 1 minute long sleep calls.

Don't use empty if blocks

Again, not sure if this is just for demo purposes:

if ($Auth->login($_POST['username'], $_POST['password'])) {
    // why is this empty

Either invert the test (!) or put some logic in there - e.g. redirecting the user to their profile page.

share|improve this answer
    
The sleep() is to prevent bruteforce attacks, sleep a while before you can try login in again, (Delays) –  John May 17 '12 at 10:32
    
How would I rather handle the process of delaying a login attempt? –  John May 17 '12 at 11:50
1  
I know what it's for, it's simply a bad idea. Consider this analogy: you go to check in at the airport and you are too early - instead of immediately telling you to come back in 30mins - you get silence for 30 minutes and then "certainly, tickets please". How do these 2 responses affect the queue of people behind, who want to go somewhere else? Don't tie up your own resources with such logic - finish the process as soon as posible for malicious users, and leave available for 'real' users. –  AD7six May 17 '12 at 11:50

Want to know about security? Don't claim that I'm paranoid sociopath.

$this->db = $db; So, you agree to accept (and use later) any DB connection? Don't even mention that it may be simply NULL (in the best case?)

$_SESSION do you really trust global variables? Especially if you insetr their values into your (well, already compromized) data base? Look at create_account from this point of view.

mt_srand is obsolete since PHP 4.2.0.

md5 is not safe anymore

check_email will accept anything, but should (usually) accept valid e-mail only.

IP address check does not care if IP is at least routable.

Etc, etc, etc

The answer if way less than full analysis.

share|improve this answer
    
The validation of the data is seperated from the library, the e-mail has a validated format (by FILTER_VALIDATE) + I use prepared statement? –  John May 10 '12 at 18:48
1  
For the $db to be a security problems - you'd need access to the code on the server to be able to inject your own db instance - which is.. not particularly useful. If someone can inject their own $db - they can also just edit the code to do whatever they want. The session is always global and accessed as $_SESSION in php - that's fine (in the context of php) unless you're doing extract($userSubmitted) somewhere - which I don't think is in scope. –  AD7six May 14 '12 at 16:43

The first thing I would change is the way the else blocks inside of login() are handled. They are almost doing the exact same thing.

if ($result) {

    if ($this->PasswordLib->verifyPasswordHash($password, $result->password)) {
    } else {
        $this->addLoginAttempt($username);
        $this->captchaCount++;
        $this->setMessage("<p>The password you entered is incorrect. Please try again.</p><p>Forgot your password? <a href=\"forgotpassword.php\">Request a new one.</a>");
        return FALSE;
    }         
} else {
    $this->addLoginAttempt($username);
    $this->captchaCount++;
    $this->setMessage("<p>Incorrect username</p>");
    return FALSE;
}

To help make your code more DRY I would combine the addLoginAttempt and setMessage to one method. Which would make your code look like this.

if ($result) {

    if ($this->PasswordLib->verifyPasswordHash($password, $result->password)) {
    } else {
        $this->addLoginAttemptWithMessage($username, "<p>The password you entered is incorrect. Please try again.</p><p>Forgot your password? <a href=\"forgotpassword.php\">Request a new one.</a>");
        $this->captchaCount++;
        return FALSE;
    }         
} else {
    $this->addLoginAttemptWithMessage($username, "<p>Incorrect username</p>");
    $this->captchaCount++;
    return FALSE;
}

/* added after `addLoginAttempt` */
private function addLoginAttemptWithMessage($username, $message) {
  $this->addLoginAttempt($username);
  $this->setMessage($message);
}

You could possibly put the $this->captchaCount++ call inside of the new method as well.


IsLoggedIn can be written more simply as

public function IsLoggedIn() {
    return isset($_SESSION['userid']);
}

Regarding security your code looks fine, just make sure you use something like bcrypt when hashing your passwords.

The most important thing you could do is write tests using PHPUnit or something similar. You have a lot of routes that can be taken when someone enters a wrong username or incorrect login information.

share|improve this answer

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.