Is my login secure (are sessions a reliable way to verify)

PHP
1

Hi, I am building a login with optional two step auth (google authenticator) and just wondering whether what i am doing is secure. What currently happens is the user turns on two step auth which changes extra_security column to 1 (activated) then on login screen once credentials correct it checks to see if activated via the switch case func and then creates the correct session.

If not activated sets the uaccess session as 1 straight away but if 2 step is enabled then it will set it as 0 and redirect you to login2 where you then enter the code and then finally if code is correct then it unsets the authCode session and changes the uaccess session to 1. Is this secure enough or how should i do it better? Right now it is literally just looking for uaccess to be = to 1 then it logs you in fully but i don’t think this is secure enough so all feedback and improvements much appreciated!

Also any advice on how i could achieve a if failed 3 attempts do this, 5 attempts do this and then 8 attempts show a recaptcha etc.?

Thanks in advance.

Login code:

    public function login()
    {
        if (startHelp::isLoggedIn()) { startHelp::redirect(''); exit(0); }
        if (startHelp::isPartLoggedIn()) { startHelp::redirect('user/login2'); exit(0); }
        if ($_SERVER['REQUEST_METHOD'] == 'POST') 
        {
            $_POST = filter_input_array(INPUT_POST, FILTER_SANITIZE_STRING);
            $this->csrf->CSRFTokenVerify();
            
            $formData = [
                'username' => trim($_POST['username']),
                'password' => trim($_POST['password']),
                
                'username_err' => '',
                'password_err' => '',
            ];
            $data = [
                'formData' => $formData,
                'title' => LANG['auth-login-title'],
                'description' => '',
            ];
            
            if (empty($data['formData']['username'])) { // Username not empty
                $data['formData']['username_err'] = 'username empty';
            }
            if (empty($data['formData']['password'])) { // Password not empty
                $data['formData']['password_err'] = 'password empty';
            }
            
            if (empty($data['formData']['username_err']) && empty($data['formData']['password_err']) && empty($data['formData']['global_err'])) {
                $userAuthenticated = $this->auth->doLogin($data['formData']['username'], $data['formData']['password']);
                if ($userAuthenticated && $this->auth->checkActive($data['formData']['username'])) {
                    
                    switch ($userAuthenticated[0]['extra_security']) {
                        case 1:
                            $this->createUserExtraSession($userAuthenticated);
                            break;
                        default:
                            $this->createUserSession($userAuthenticated);
                    }
        
                } elseif ($userAuthenticated && $this->auth->checkBan($data['formData']['username'])) {
                    $data['formData']['global_err'] = 'user banned';
                    $data['formData']['password'] = '';
                    $this->view('user/login', $data);                   
				} elseif ($userAuthenticated && !$this->auth->checkActive($data['formData']['username'])) { // If user not active lock
                    $data['formData']['global_err'] = 'user not active';
                    $data['formData']['password'] = '';
                    $this->view('user/login', $data);
                } else { // If user credentials incorrect then
                    $formData = [
                        'username' => trim($_POST['username']),
                        'password' => '',
                        'username_err' => 'invalid username',
                        'password_err' => 'invalid password',
                        'global_err' => '',
				    ];
                    $data = [
                        'formData' => $formData,
                        'title' => LANG['auth-login-title'],
                        'description' => '',
				    ];
                        		
                    $this->view('user/login', $data);
				}
            } else {
                $this->view('user/login', $data);
            }
            
        } else {
            $formData = [
                'username' => '',
                'password' => '',
                
                'username_err' => '',
                'password_err' => '',
            ];
            $data = [
                'formData' => $formData,
                'title' => LANG['auth-login-title'],
                'description' => '',
            ];

            $this->view('user/login', $data);   
        }
    }

    private function createUserSession($user) // no two step auth
    {
        $_SESSION['uid'] = $user[0]['id'];
        $_SESSION['upin'] = $user[0]['pin'];
        $_SESSION['uemail'] = $user[0]['email'];
        $_SESSION['uname'] = $user[0]['username'];
        $_SESSION['uaccess'] = ($user[0]['extra_security'] == 0 ? '1' : '0');
        $_SESSION['authCode'] = $user[0]['extra_security_code'];
        startHelp::flash('message', 'success login', 'alert alert-success mb-0', 'data-auto-dismiss="10000"');
        startHelp::redirect('');
        exit(0);
    }
    
    private function createUserExtraSession($user)
    {
        $_SESSION['uid'] = $user[0]['id'];
        $_SESSION['upin'] = $user[0]['pin'];
        $_SESSION['uemail'] = $user[0]['email'];
        $_SESSION['uname'] = $user[0]['username'];
        $_SESSION['uaccess'] = ($user[0]['extra_security'] == 0 ? '1' : '0');
        $_SESSION['authCode'] = $user[0]['extra_security_code'];
        startHelp::redirect('user/login2');
        exit(0);
    }

    public function login2()
    {
        if (startHelp::isLoggedIn()) { startHelp::redirect(''); exit(0); }
        if (!startHelp::isPartLoggedIn()) { startHelp::redirect('user/login'); exit(0); }
        if ($_SERVER['REQUEST_METHOD'] == 'POST') 
        {
            $_POST = filter_input_array(INPUT_POST, FILTER_SANITIZE_STRING);
            $this->csrf->CSRFTokenVerify();
            
            $formData = [
                'username' => $_SESSION['uname'],
               'code' => trim($_POST['code']),
                                    'code_err' => '',
            ];
            $data = [
                'formData' => $formData,
                'title' => '',
                'description' => '',
            ];
            
                              if (empty($data['formData']['code'])) { // Username not empty
                                    $data['formData']['code_err'] = 'code empty';
                                }
            
            if (empty($data['formData']['code_err']) && empty($data['formData']['global_err'])) {
                $verifyCode = $this->auth->verifyCode($_SESSION['authCode'], $data['formData']['code'], 2);    // 2 = 2*30sec clock tolerance
                
                if ($verifyCode) {
                                    
                $_SESSION['uaccess'] = '1';
                    unset($_SESSION['authCode']);
                                    startHelp::flash('message', '2 step login success', 'alert alert-success mb-0', 'data-auto-dismiss="10000"');
                            startHelp::redirect('');
                            exit(0);     
                } else {
                    $formData = [
                        'username' => $_SESSION['uname'],
                        'code_err' => 'invalid code',
                        'global_err' => '',
				    ];
                    $data = [
                        'formData' => $formData,
                        'title' => 'login to account',
                        'description' => '',
				    ];
                        		
                    $this->view('user/login2', $data);
                }

            } else {
                $this->view('user/login', $data);
            }
            
        } else {
            $formData = [
                'username' => $_SESSION['uname'],
                'code' => '',
                
                'username_err' => '',
                'code_err' => '',
            ];
            $data = [
                'formData' => $formData,
                'title' => 'enter code to complete login',
                'description' => '',
            ];

            $this->view('user/login2', $data);   
        }
    }

The static helper functions:

    /*
     * Checks Login Authentication.
     */
    public static function isLoggedIn()
    {
        if (isset($_SESSION['upin']) && $_SESSION['uaccess'] == 1) {
            return true;
        } else {
            return false;
        }
    }
    
    /*
     * Checks Part-Login Authentication.
     */
    public static function isPartLoggedIn()
    {
        if (isset($_SESSION['upin']) && $_SESSION['uaccess'] == 0) {
            return true;
        } else {
            return false;
        }
    }
2

It looks decent, but don’t modify the user’s password. It doesn’t matter if the user is using special characters or spaces as their password. It shouldn’t matter to the database and it shouldn’t matter to you. That’s only if you are implementing a proper password hashing algorithm like password_hash().

3

It appears to me that your class(es) are nothing more than a container for a bunch of functions. I don’t see anything that even remotely resembles OOP.

4

How would that make the login less secure?

5

I guess you missed this part…

all feedback and improvements much appreciated

1 Like
6

It’s a custom made MVC based on OOP. I have not showed you the entire MVC. Most of the code is done in the core files. You have simply seem the controller code. (Not strict MVC structure or oop)

My model for db check login:

    public function dologin($username, $password) // Check login credentials are correct
    {
        $bind = [':username' => $username];
        $results = $this->db->select('everything_users', 'username = :username', $bind);
        $hashed_password = $results[0]['password'];
        if (password_verify($password, $hashed_password)) {
            return $results;
        } else {
            return false;
        }
    }
7

This is the model code that checks valid login (uses password_hash) so thanks i will remove the trim:

    public function dologin($username, $password) // Check login credentials are correct
    {
        $bind = [':username' => $username];
        $results = $this->db->select('everything_users', 'username = :username', $bind);
        $hashed_password = $results[0]['password'];
        if (password_verify($password, $hashed_password)) {
            return $results;
        } else {
            return false;
        }
    }
8

You can take out the else and tighten up the code a hair.

    public function dologin($username, $password) // Check login credentials are correct
    {
        $bind = [':username' => $username];
        $results = $this->db->select('everything_users', 'username = :username', $bind);
        $hashed_password = $results[0]['password'];
        if (password_verify($password, $hashed_password)) {
            return $results;
        } 
            return false;
    }
1 Like
9

oh right… learn something new everyday. By removing the else it returns false if anything fails right? Thanks v much.

10

You got it!

While I have your attention, I would suggest you put your entire project on a repo such as GitHub and post the link so those that want to can review your code as a whole and make recommendations for improvement.

1 Like
11

Sure i will do some time tomorrow when have more time as going out in around 10 mins. Thanks again. I will link repo on here for yall to check out and inform a 1.5 year phper of the mistakes I have made :slight_smile:

12

Also one final thing before tomorrow with my 2 step auth security. If a user goes to domain.com/user/register i would like it to check if user already logged in or if part logged in (on step 2 of 2 step login). Is that code fine and again only having 1 session value 0/1 between being fully logged in and half logged in is that secure? No one can hijack a session as they are server side correct?

Thanks bye

My current code is:

        if (startHelp::isSessionValid() && $_SESSION['uaccess'] == 1) { 
            startHelp::redirect('');
            exit(0);
        } elseif (startHelp::isSessionValid() && $_SESSION['uaccess'] == 0) {
            startHelp::redirect('user/login2');
            exit(0);            
        }

Edit* Too many === signs

13

I personally would prefer to view the code as a whole to give you the best answers. I expect there will be much room for improvement and those changes when made will change the questions & answers.

14

Don’t use mixed return types in functions. In this case that function can either return an array on success, or false when something fails. This puts the onus of checking if it works on the caller, so that if the caller doesn’t check the result you may end up in a situation where someone with an incorrect password is logged in - instead the dologin function should either return an array in case of success, or throw an exception when no user that fits the credentials was found.

public function dologin($username, $password) // Check login credentials are correct
{
    $bind = [':username' => $username];
    $results = $this->db->select('everything_users', 'username = :username', $bind);
    $hashed_password = $results[0]['password'];
    if (password_verify($password, $hashed_password)) {
        return $results;
    }

    throw new NoSuchUserException('No user found with the supplied username/password');
}

also the function name is confusing as it doesn’t actually log in the user. Naming it something like checkCredentials (which is what your comment also says …) would be better.

The NoSuchUserException is not something that is supplied in PHP, but a new Exception you’d need to create yourself:

class NoSuchUserException extends RuntimeException
{
}
19

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.