Login implementation security concerns

Question

In many of my personal projects, I have a login system very similar to the one I am about to describe. I understand how difficult correctly implementing authorization systems can be. I do this as a hobby and if I can do this securely myself, I would very much like to. (At work, I avoid homebrewed login solutions). I want to know if my approach is secure, and how I can improve it. I am using PHP 5.6 and the CodeIgniter 3 framework.

• On register, I use password_hash with PASSWORD_BCRYPT.

• Usernames must be unique, case insensitive (john and jOhN can't both exist).

• I only allow an IP to login x amount of time in x minutes (failed or pass because alt accounts are usually discouraged). I have similar restrictions on registering accounts.

• I set a max password length of 64 and perform trim on start and end.

• On login, I use password_verify to compare database result of username row with user input.

For completeness, here is the most relevant (but not all) parts of auth code.

This is my controller

public function login()
{
    // Check if this is ip has logged in too many times
    $ip = $_SERVER['REMOTE_ADDR'];
    $timestamp = date('Y-m-d H:i:s', time() - $this->login_limit_window * 60);
    $route_url = 'user/login';
    $check_request_results = $this->main_model->check_request_route($ip, $route_url, $timestamp);
    if (count($check_request_results) > $this->login_limit && !is_dev()) {
        echo 'Too many login attempts from this IP. Please wait ' . $this->login_limit_window . ' minutes.';
        die();
    }

    // Validation
    $this->load->library('form_validation');
    $this->form_validation->set_rules('username', 'Username', 'trim|required|max_length[32]');
    $this->form_validation->set_rules('password', 'Password', 'trim|required|max_length[64]|callback_login_validation');

    // On fail set fail message and redirect to map
    if ($this->form_validation->run() == FALSE) {
        $this->session->set_flashdata('failed_form', 'login');
        $this->session->set_flashdata('validation_errors', validation_errors());
        redirect(base_url(), 'refresh');
        return false;
    }
    // Success
    redirect(base_url(), 'refresh');
}

// Validate Login Callback
public function login_validation($password)
{
    // Get other parameters
    $username = $this->input->post('username');
    // Compare to database
    $result = $this->user_model->login($username, $password);
    // Username not found
    if (!$result) {
        $this->form_validation->set_message('login_validation', 'Invalid username or password');
        return false;
    }
    // Password does not match
    else if (!password_verify($password, $result['password'])) {
        $this->form_validation->set_message('login_validation', 'Invalid username or password');
        return false;
    }
    // Success, do login
    $sess_array = array(
        'id' => $result['id'],
        'username' => $result['username']
    );
    $this->session->set_userdata('logged_in', $sess_array);
    return true;
}

And this is my model

// Login database call
function login($username, $password)
{
   $this->db->select('*');
   $this->db->from('user');
   $this->db->where('username', $username);
   $this->db->limit(1);
   $query = $this->db->get();
   if ($query->num_rows() == 1) {
       $result = $query->result_array();
       return isset($result[0]) ? $result[0] : false;
   }
   else {
       return false;
   }
}

Answer 1

Not a comprehensive review, but I'll list a few things which came to my notice right away:

• Define the login_limit variable as login_limit_count to keep it consistent with login_limit_window.
• Instead of defining the above 2 as class attributes, define them as constants in config/constants.php file.
• Use === in the condition: ($this->form_validation->run() == FALSE)
• The login method inside your user_model doesn't actually log-in the user. It just fetches the user details based off of $username. No need to provide it with $password and rename it to get_user or similar. Alternatively, put the password_verify inside the model, and return a user on successful login.
• A select * is very bad. You should always know what values you'd be using from the result.
• Hardcoding $route_url inside each controller method, and then calling check_request_route can be avoided using $_SERVER['REQUEST_URI'] variable and defining a hook. The hooks need to be enabled first in config/config.php. This way, you can track visits to multiple pages from a single location.

Answer 2

You have some good advice in answer from @hjpotter92. I will add some thoughts in addition to those already mentioned there.

---

Why restrict login based on IP address? There are a number of valid reasons that many users could be coming from a common IP address, and taking this approach could lead to poor user experience problems with users being blocked from login outside of their control.

---

Absolutely do not message end user telling them why login failed. You are giving a potential attacker critical information to aid them in attacking you application. Here you give an attacker very precise information to get around your IP restriction.

---

Don't trim username and password automatically for end user. You should return validation errors if there is spurious whitespace around these values. Who knows, maybe user intended to put whitespace in these values and by trimming them you are actually incorrectly mapping this user to a different username or password in your database.

---

It seems odd to actually perform the login action (verifying password, changing session authentication status, etc.) in a form validation callback. Separate these concerns.

Validate the form data and if successful then perform the login verification.

---

I don't understand having a login method on the user model at all. Basically all you are really doing here is instantiating a user object based on input user name. Why not just read the password property from user object (or from getter on user object)?

---

Your login_verification method seem asymmetric in it's input. Why only pass the password (again this is likely tied to the odd calling of this function from form validation)? Consider passing this method a user object and the password from input, or the username from input and password from input. So it is not getting passed some information and having to gather the rest itself.

I would prefer workflow like:

• validate username and password input for proper syntax.
• try to instantiate user object.
• if successful, compare input password against password on user object.
• if successful, set user object to session in valid logged in state

---

You should always regenerate session id around authorization level changes (login, logout, escalation of privileges). I see no sign that this is happening here.

---

Your session data structure seems odd to me. Why have user information stored under a logged_in key? Perhaps separate user and login_status keys? It would just seem odd to me in other areas of the application, when you want to get information about the user, to have to look under a key named logged_in.

---

How do you enforce the case-insensitivity of the username? Only through non-case-sensitive collation on the database column? If so, this seems to be obfuscated business logic. Should you be casting username to lowercase here (something that can also be done in the form in javascript) to make logic clear and non-fragile to potential changes in DB schema? I actually not really sure that I would agree with taking a case-insensitive approach to a username anyway, as again you are potentially hiding away what the user thinks their username is from what is in the data store.

---

From the looks of your code, the class is probably doing way too much. Why does this class need to load libraries, perform form validation, manage session state, manage user input, etc. Do you really need to set all these different functionalities to be properties on this class? Could dependency injection be useful here?

---

You are not showing your password_hash() code. Are you setting any salt or cost options? Typically you should not set salt (just let it be generated for you), and you should tune cost (with value between - 04 & 31) to make sure that the hash takes sufficient time to be calculated (the default cost factor of 10 can oftentimes be considered to be "too fast" to calculate).