Registration and login

Question

Right now I'm creating a community. You can register and create your own profile and post on a forum etc. I'm just doing this for fun and learning. I'm not planning on becoming a professional programmer this is just a hobby of mine.

Anyway, I want do it right though. Please review my script and tell me if it seems OK and safe. And please if you have a suggestion or anything go ahead.

This is the login (I use sessions):

// Trim values sent from form
$username = trim($_POST['username']);
$password = trim($_POST['password']);

// Get user info and their rights matching login attempt
$stmt = $db->prepare("SELECT id, username, g_can_delete_users, g_can_edit_post, g_can_handle_reports FROM users LEFT JOIN groups ON groups.g_id = users.group_id WHERE users.username = ? AND SHA1(CONCAT(users.salt, ?)) = users.password");
$stmt->execute(array($username, $password));
$row = $stmt->fetch();

// Did we find a match?
if ($row) {
    $_SESSION['user'] = $row; 
    header("Location: index.php");
    exit;
} else {
header("Location: index.php");
exit;
}

And the registration part:

// Example of a generated salt: &O93Wrl`x#iY


$username = trim($_POST['username']);
$email = strtolower(trim($_POST['email']));
    $password1 = trim($_POST['password1']);
    $password2 = trim($_POST['password2']);

    // Validate the username
    if (strlen($username) < 4)
        $errors[] = "Username must be at least 4 characters.";
    // ... and the password
    else if (strlen($password1) < 4)
        $errors[] = "Password too short";
    else if ($password1 != $password2)
        $errors[] = "Passwords do not match";
    else 


$stmt = $db->prepare("INSERT INTO users (username, password, email, salt, registered, registration_ip) VALUES (:username, :password, :email, :salt, :registered, :registration_ip)");
        $stmt->execute(array(':username'=>$username, ':password'=>sha1($salt.$password1), ':email'=>$email, ':salt'=>$gen_salt, ':registered'=>time(), ':registration_ip'=>$_SERVER['REMOTE_ADDR']));

Answer 1

A few things to add to Michael Zedeler's review:

---

Your formatting is a little odd. Pick an indenting scheme and stick with it. (This seems to be part of a bigger script though, so maybe it's just copy/paste gone wrong?)

---

Don't assume array indexes exist (read the first section of this).

---

You assume username's are available?

---

By the HTTP standard, Location headers must have a full URL.

---

I would pull the hashed password into PHP land and do the comparison there. This has multiple advantages:

1) You can know whether the error is because the user doesn't exist or because the password is wrong 2) The MySQL query log is guaranteed to not contain the password in plain text

---

Your last if clause is a perfect example of why you should always use braces with if/else. It comes down to a stylistic choice whether or not to actually always use them, but in this situation, your code is wrong without them on the last clause:

    else 
        $stmt = $db->prepare("INSERT INTO users (username, password, email, salt, registered, registration_ip) VALUES (:username, :password, :email, :salt, :registered, :registration_ip)");
        $stmt->execute(array(':username'=>$username, ':password'=>sha1($salt.$password1), ':email'=>$email, ':salt'=>$gen_salt, ':registered'=>time(), ':registration_ip'=>$_SERVER['REMOTE_ADDR']));

Is the same thing as:

    else {
        $stmt = $db->prepare("INSERT INTO users (username, password, email, salt, registered, registration_ip) VALUES (:username, :password, :email, :salt, :registered, :registration_ip)");
    }
    $stmt->execute(array(':username'=>$username, ':password'=>sha1($salt.$password1), ':email'=>$email, ':salt'=>$gen_salt, ':registered'=>time(), ':registration_ip'=>$_SERVER['REMOTE_ADDR']));

This means if that if any of the other branches are followed, you'll get an invalid object fatal error since you'll be trying to call execute on null.

---

What is the data type of registered? I'm a little concerned that it might be an int. It should be either a timestamp or a datetime (likely a timestamp in this situation).

---

While you've made good steps using sha1 and a salt, you might should consider using a better scheme for hashing. There's a lot of good information here.

---

I'd use a different flow to handle the errors. In particular, there's no reason that a username error and a password error can't happen at the same time:

$errors = array();

if (strlen($username) < 4) {
    $errors[] = "Username must be at least 4 characters.";
}

if (strlen($password1) < 4) {
    $errors[] = "Password too short";
} else if ($password1 != $password2) {
    $errors[] = "Passwords do not match";
}

if (!$errors) {
    //Create the user
} 

---

You should declare $errors ahead of appending to it. $errors[] will create the variable fine without complaining, but it's much easier to read code if you know where all of the variables come into play. And, from a technical perspective, reading a non-existent $errors will complain (and error reporting should be cranked all the way up! especially in development).

---

I don't like the g_ prefix for the groups table. What happens when you run out of letters? What happens if you have another g table? It seems odd. I would just name the columns naturally, and then when you join on it, alias the columns when necessary.

(I agree with Michael though: your authentication and access code should be separate. Authenticate first and then worry about permissions.)

---

I don't know how I feel about triming a password. That could be confusing for someone who legitimately wants a space on the end of a password (and I'm a huge fan of not restricting what characters are allowed in passwords). Then again, it's a rather questionable choice to end a password with white space anyway.

---

I like to always use named parameters in prepared statements. It's easier to read, and it can save you from a "Wtf?" later if you rearrange or add clauses.

---

I don't see a session_start() anywhere? But I guess this is just a snippet of a bigger script?

Answer 2

I can't say it is plain wrong, but the LEFT JOIN against groups makes me twitch when I read it, because it is so crucial that this password check works. I'd split it off into a separate query that you run after the user has been authorized.

Then, in $_SESSION['user'] = $row it seems that the code ignores any following rows (i.e. if the user is a member of more than one group). Otherwise the database hasn't been normalized.

Another thing is that this requires the user submits the password in plain text. Using HTTPS should solve that problem, but another solution is using a challenge response-scheme. If you have neither, well then there is a security issue to address.