Failed Login Counter Reset Triggered by Found User

[justincdotme]

The failed login counter can be reset by attempting to login with a valid username. The login attempt count is reset when a valid user is found, even if authentication fails for the valid user. The if/else block on lines 96-105 of /application/model/LoginModel.php appears to be the source of the issue.

[mcanatalay]

I think failed-login-count and last-login-failed should renamed as user-not-found-count and last-user-not-found.

Here for example:

These functions can be added:

    /**
     * Increments the user not found counter
     */
    public static function incrementUserNotFoundCounter(){
        Session::set('user-not-found-count', Session::get('user-not-found-count') + 1);
        Session::set('last-user-not-found', time());
    }

    /**
     * Resets the user not found counter to 0
     */
    public static function resetUserNotFoundCounter(){
        Session::set('user-not-found-count', 0);
        Session::set('last-user-not-found', '');
    }

And as you suggested,

        // check if that user exists. We don't give back a cause in the feedback to avoid giving an attacker details.
        // brute force attack mitigation: reset failed login counter because of found user
        if (!$result){
            // brute force attack mitigation: set session failed login count and last failed login for users not found
            self::incrementUserNotFoundCounter();
            //Orginally username is wrong but we won't give any specific detail.
            Session::add('feedback_negative', Text::get('FEEDBACK_USERNAME_OR_PASSWORD_WRONG')); 
            return false;
        }

And before return:

        //Reset the user not found counter.
        self::resetUserNotFoundCounter();

Why do I renamed. Because login failed is too general name for a code. Furthermore, mysql version of login failed can also renamed as failed-password, but I don't think it is urgent. In order to keep them seperate I renamed only user not found control.

[justincdotme]

@mcanatalay I agree; IMO, renaming those methods makes the code easier to read and clarifies the functions purpose.

[mcanatalay]

Oh well also upper most if statement should be changed as,

        if (Session::get('user-not-found-count') >= 3 AND (Session::get('last-user-not-found') > (time() - 30))){
            Session::add('feedback_negative', Text::get('FEEDBACK_LOGIN_FAILED_3_TIMES'));
            return false;
        }

[panique]

This has been fixed by merging of pull request #684 ! :)
Please note that there were some tiny merge conflicts (my fault) so I merged this manually, therefore github sees this as "closed", but in reality it's "merged".

Thanks for your excellent contributions!