Is this a secure method, oop

Question

    $salt = $this->get_salt($username);

    if (is_null($salt)) {
        return FALSE;
    }

    $password = sha1(SITE_KEY . $password . $salt);
    $sth = $this->db->prepare("SELECT id, username, active FROM user WHERE username = ? AND password = ?");
    $sth->setFetchMode(PDO::FETCH_OBJ);
    $sth->execute(array($username, $password));

    if (($result = $sth->fetch()) !== FALSE) {
        return $result;
    }

    return FALSE;

This is what make me concerned:

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

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.

: Looked at it again, and while you would already need to know the login information if you were to abuse that object in that way. It is still better, in my opinion, to separate it in case there is some sort of leak. Not saying I know how someone would be able to do it. Just think of it as one less potential loop hole.

I don't pretend to understand how malicious users would do something. I just know that making it easier for them to do so doesn't seem wise. Maybe there isn't anything wrong with it, I'm not a security expert. I'm just poking at things that, if I were to write it, I would change because they seem hazardous to me. If you truly wish to know if it is dangerous or not, I'd suggest submitting that example to the regular stackoverflow community to see what they say.

Do He have some point of what he saying? The method would be used in a page controller I have:

$user = $auth->login('username, password)

if ($user) {
//do some additional checks... set session variables
}

Answer 1

No, the code you've provided is not secure.

First off, you're using a non-iterated simple hash. See this answer and this blog post for information as to why.

Second, why are you storing the salt separately? A salt must be unique to each and every record in order for it to work. So the logical place to put it is along side the password. In this case, you're making two queries instead of one (assuming that the salt is not derived form the username, and is indeed random). If it's not random, it should be. See this post for more information...

Third, you're exposed to timing attacks against the password hash, since the database isn't using a timing immune comparison.See this article and this article for more information.

Don't do this on your own. It's really difficult to do securely. Use a library. You can use PHPASS or PHP-PasswordLib...

Edit: To The Comments from your friend:

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

That's a valid point. But at that point, if the user can inject var_dump($userObject), he can run $this->db->prepare("SELECT * FROM user"); and get all your user data, not just his. So while it's something to point out, it's not worth it to try to secure. Unless you got really anal and put all of it into a stored procedure in the database and limited read access from the table (so you can't select from the user table at all). But at that point, you'd need to redesign the entire application from the ground up, since you'd never be able to get any user info except through a SP. And that's going to be hard to do well (complexity is the enemy of security).

In the end, I think you'd be far better served defending against script injection in the first place. Especially if you don't have professional admins who can secure your server to the ludicrous amount needed to prevent that sort of attack (chroot jails on the PHP process, etc).

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.

What magic property is there? Are you using those items directly? Or are you using them to populate a user object (which then knows how to handle the data)...? Just with that snippit, I don't see the problem that your friend is pointing to...

: Looked at it again, and while you would already need to know the login information if you were to abuse that object in that way. It is still better, in my opinion, to separate it in case there is some sort of leak. Not saying I know how someone would be able to do it. Just think of it as one less potential loop hole.

Actually, you'd just need to know a valid username, due to the timing attack identified above...

I don't pretend to understand how malicious users would do something. I just know that making it easier for them to do so doesn't seem wise. Maybe there isn't anything wrong with it, I'm not a security expert. I'm just poking at things that, if I were to write it, I would change because they seem hazardous to me. If you truly wish to know if it is dangerous or not, I'd suggest submitting that example to the regular stackoverflow community to see what they say.

In general this is good advice. Just remember that complexity is the enemy of security. So if making it harder adds a lot of complexity, the chances of adding a new vulnerability is significantly higher (such as the timing attack that was introduced). I err on the side of simplicity. I have libraries that are well tested to handle the complex parts (like password hashing), but the code I write is simple.

Consider the strongest encryption algorithm: One Time Pad. The algorithm is dirt simple:

encrypted = plaintext XOR key

But it's impossible to decrypt without the key. So the security to the algorithm actually lies in the secret, not in the algorithm. Which is what you want. The algorithm should be public and the security should not be dependent upon its secrecy. Otherwise it becomes security through obscurity. Which gives you a warm fuzzy feeling right until you're cracked...

Answer 2

See this answer for how to use bcrypt to hash passwords. It's more secure than yours because it allows for deliberate slow-down of the hashing algorithm (so if anyone tries to brute force, they would be limited to 1M iterations per second or so, instead of 1B, which is the difference between hours and years).

Also, (unrelated to security), you should be using PDO's named placeholders, instead of the unnamed ones (:placeholder and not ?).

Moreover, you might want to build a complete User object and return that, it will be easier to work with (assuming you have one, which you should!).