2
\$\begingroup\$

I am beginning the cycle of creating my role based access control into my framework. I now want to log the user into my application and my _user table looks like this:

CREATE TABLE `_users` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `username` varchar(255) NOT NULL,
  `hash` varchar(255) NOT NULL,
  `session` varchar(255) DEFAULT NULL,
  PRIMARY KEY (`id`)
)

I create my user like so in my model, where bin2hex() is my IV / salt.

Database::getInstance()->Prepare( 'INSERT INTO _users (username, hash, session) VALUES (?, ?, ?)' )
                       ->execute( array(
                           'Test',
                           password_hash( 'Test', PASSWORD_BCRYPT ),
                           md5( time() . bin2hex( random_bytes( 32 ) ) )
                       ) );

To then log into the user, I use a method. The method I use in the request is POST because my application has built in CRSF tokens. If the CRSF token is passed, then a xauth_protected aliased class is instanced which ensures that X-Auth header exists and is of md5(session_id()) to ensure that no cross-site-origin requests can be made.

public function login( Request $request )
{
    $this->middleware( 'json_response', $request );
    $this->middleware( 'xauth_protected', $request );
    
    if( !$request->has( 'username' ) || !$request->has( 'password') )
    {
        echo json_encode( array( 
            'status' => FALSE,
            'reason' => 'Please fill in the required fields...',
        ) );
        return;
    }
    
    $stmt = Database::getInstance()->Prepare( 'SELECT id, hash, session FROM _users WHERE username = ? LIMIT 1' );
    $stmt->execute( array( $request->username ) );
    $row  = (object) $stmt->fetch();
    
    if( isset( $row->id ) )
    {
        if( password_verify( $request->password, $row->hash ) )
        {
            // $_SESSION['oath'] = $row->session
            // This is used to find the user that is logged in, if it is set
            $request->setSession( 'oauth', $row->session );
            
            echo json_encode( array( 
                'status' => TRUE,
            ) );
            
            return;
        }
        
        echo json_encode( array( 
            'status' => FALSE,
            'reason' => 'Invalid credentials...',
        ) );
        
        return;
    }
    
    echo json_encode( array( 
            'status' => FALSE,
            'reason' => 'Sorry, that username was not found in our records...',
        ) );
}

To then test this method, I can use:

const formData = new FormData();
    
formData.append( 'crsf_token', sessionStorage.getItem( 'token' ) ); // CRSF token is set elsewhere
formData.append( 'username', document.getElementById( 'username' ).value ); // Example input field
formData.append( 'password', document.getElementById( 'password' ).value ); // Example input field

fetch( App.__viewFactory.homepage.login, { // This is a prestored route that looks like /oauth/login
    method: 'POST',
    headers: {
        'X-Auth': sessionStorage.getItem( 'session' ) // md5( session_id() ) which is set elsewhere
    },
    body: formData
} )
.then( response => response.json() )
.then( data     => {
    if( data.result ) {
        window.location.href = App.__viewFactory.dashboard.view; // prestored route to the dashboard
        return;
    }
        
    document.getElementById( 'login-error' ).innerHTML = data.reason;
} )
.catch(error => {
    document.getElementById( 'login-error' ).innerHTML = 'Oh no! Something went wrong.';
});

I have incorporated CRSF protected, custom headers using the session_id() to prevent cross-origin requests (since my framework rewrites all requests to my index page, this then turns on CORS). Is everything I am doing on the database side as secure? Is there something I could be doing better?

\$\endgroup\$
12
  • 1
    \$\begingroup\$ "Is everything I am doing on the database side as secure?" Secure against what/who? \$\endgroup\$
    – Mast
    Jul 28 '20 at 8:06
  • \$\begingroup\$ what is session? \$\endgroup\$ Jul 28 '20 at 8:07
  • \$\begingroup\$ session is just the output of md5(session_id()) - it probably has absolutely no adversary effect but deception is a key defence principle even if so. These are set on a .tpl file which my page executes. I can show that if needed @YourCommonSense \$\endgroup\$
    – Jaquarh
    Jul 28 '20 at 8:10
  • \$\begingroup\$ I am sorry, what? \$\endgroup\$ Jul 28 '20 at 8:10
  • \$\begingroup\$ I appreciate you want me to expand on 'secure' but I am trying to generalise the noun. Am I doing everything correctly is what I am trying to ask, what is best practice, am I generating my sessions correctly etc etc @Mast \$\endgroup\$
    – Jaquarh
    Jul 28 '20 at 8:11
3
\$\begingroup\$

Your querying practices look secure to me. Your are using a prepared statement with bound parameters and using password_hash() and password_verify(). It really can be as simple as doing those basic/essential things.

As for other refinements, I recommend writing the failure branches before successful ones, doing early exit()s, and only passing back an empty or populated reason to reduce the data structure to its vital value.

  • I will assume/hope that the username is a UNIQUE table column, so LIMIT 1 provides no value.
  • If you want the result set / row to be an object, just tell pdo that that is what you want -- fetch(PDO::FETCH_OBJ).
  • The result set will either be an object or false, so just check for a falsey result set. For this reason, you can remove id from the SELECT.
  • I don't like to give too much specificity when giving failed login responses. I would tell the user that the credentials generally failed without spelling out which field was the problem.

Recommendation:

public function login(Request $request)
{
    $this->middleware('json_response', $request);
    $this->middleware('xauth_protected', $request);
    
    if(!$request->has('username') || !$request->has('password')) {
        exit(json_encode(['reason' => 'Please fill in the required fields']);
    }
    
    $stmt = Database::getInstance()->Prepare(
        "SELECT hash, session FROM _users WHERE username = ?"
    );
    $stmt->execute([$request->username]);
    $rowObject = $stmt->fetch(PDO::FETCH_OBJ);
    
    if (!$rowObject || !password_verify($request->password, $rowObject->hash)) {
        exit(json_encode(['reason' => 'Invalid credentials']));
    }

    $request->setSession('oauth', $rowObject->session);
    exit(json_encode(['reason' => null]));
}

Then in your js, you can use this:

.then(data => {
    if (data.reason) {
        document.getElementById('login-error').innerHTML = data.reason;
    } else {
        window.location.href = App.__viewFactory.dashboard.view;
    }
})
\$\endgroup\$
6
  • \$\begingroup\$ It is nice to see that, even though there may be no direct server security issues, that the thought of brute forcing ties in with the ability of giving the user a specific output rather than generalising it. I think I should perhaps integrate an "attempts" into it also. I appreciate your time in writing this, its nice to know I can use fetch(PDO::FETCH_OBJ) without having to cast the array to an object. The reason I do not use exit() is because the middleware in my script runs functions after the route has executed which could be things like re-generating the CRSF token for POST requests \$\endgroup\$
    – Jaquarh
    Jul 28 '20 at 21:22
  • \$\begingroup\$ It would be nice to add you as a collaborator to see your opinion of the framework so far \$\endgroup\$
    – Jaquarh
    Jul 28 '20 at 21:23
  • \$\begingroup\$ Sorry, but due to my 9 to 5 dev job that is rarely only 9 to 5, my Stack Exchange addiction (can't argue that it is anything other than an addiction because my wife is a neuroscientist and mental wellness professional), my family, and what is left of my sleep schedule -- there isn't much left over to devote to new endeavours. \$\endgroup\$ Jul 28 '20 at 22:08
  • \$\begingroup\$ The drawback of using an attempts count to prevent brute force attacks, is that it is even simpler for an attacker to lockout your users by smashing out N number of attempts for their rainbow list of usernames. I suppose though, if you want to allow the lesser evil, it would be better that your users get locked out for a while versus your system being infiltrated. \$\endgroup\$ Jul 28 '20 at 23:14
  • \$\begingroup\$ Could you not lock the client IP rather than the account itself? Granted they could use a VPN or packet switch network but eventually they'll run out of IP addresses to use limiting their brute force & most "free" brute forces don't have the functionality of "remembering" where it stopped in a words list - but again, maybe its worth building but can be turned on or off via the configuration? \$\endgroup\$
    – Jaquarh
    Jul 29 '20 at 6:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.