MyBB Community Forums

This is a serious problem.

That setting is there to prevent bruteforcing into accounts. In ACP under the Login settings there is parameters for protecting against malicious login attempts. In the mybb_users table a column exists for loginattempts.

For the past few years I have seen occasional bruteforce attempts into accounts and my logs show an error that the loginattempt column was full because I have my type as tinyint(2) which is max limit 127. So that number is reached and I get a mysql error logged. I figured MyBB just did a poor job of checking the limit wasn't reached before the login ran.

Looking into inc/functions.php I can see the login_attempt_check() function. In there you can see it uses the cookie instead of the column.

But this is the important part....the setting "maxloginattempts" is NOWHERE TO BE FOUND IN MyBB. This is a bruteforce prevention setting and it has no function for members. It is used in the ACP though. Yet for members it is not.

I'm not even sure how to consider this bug. Do you just remove the setting? Do you make it function properly inside login_attempt_check()? Or maybe just fix the query that gives the error.

				$update_array = array(
					'loginattempts' => 1
				);
				$db->update_query("users", $update_array, "uid = '{$mybb->user['uid']}'");

I'm aware that if it does a check to the database first and uses the setting then locks out accounts you could potential bruteforce someone to prevent them from logging in. So there is that problem. But I do believe that is the intent of the setting we all assumed worked in that way. It does offer a disable "0" option in the description.

I do hope this bug gets labelled important and is addressed quickly.

I was stunned by this. I wonder if this setting ever had any code written in any version of MyBB? I been getting this error since 1.6x.

It's probably never been implemented, looking at the Git blame: https://github.com/mybb/mybb/blame/6e88b....php#L6350 - the last changes were when the code was imported into Git, so it's been that way since 1.6 at the very least, possibly even going back to 1.4.

The fact it uses cookies at all is weird - there's even a comment in the code that it should use sessions instead to prevent cookies being cleared and causing problems.

Hi,

Thank you for your report. We have pushed this issue to our Github repository for further analysis where you can track our commits and progress with fixing this bug. Discussions regarding this bug may also take place there too.

Follow this link to visit the issue on Github: https://github.com/mybb/mybb/issues/3109

Thanks for contributing to MyBB!

Regards,
The MyBB Group

A couple of notes:

In member.php at about line 1773 I see:

$logins = login_attempt_check();

login_attempt_check() is sourced at inc/functions.php
There is a bit of code that I don't think is ever ran: (Lines 6413-6419)

if($mybb->user['uid'] != 0)
 {
$update_array = array(
'loginattempts' => 1
);
$db->update_query("users", $update_array, "uid = '{$mybb->user['uid']}'");
}

In my tests $mybb->user['uid']  is never actually set to anything besides 0 for failed login attempts.
The user didn't login so $mybb->user['uid'] should always be 0. In the loginHandler (inc/datahandlers/login.php) you have this bit of code that will actually update the loginattempts column on a successful login: (Line 321)

$db->update_query("users", array("loginattempts" => 1), "uid = '{$user['uid']}'");

Because of this I do not see why the login_attempt_check needs to attempt to reset loginattempts (database column) unless the data is actually used somewhere besides just using the cookie. Which it should be.

The loginattempts column in the user table must be used to actually prevent logins instead of just the cookie data. I feel that another column needs to be added to the users table to track the last login attempt, the same way it is tracked in cookies and then tested in the same manner.

inc/functions.php (Lines 6409 - 6421)

 if($mybb->cookies['failedlogin'] < ($now - $mybb->settings['failedlogintime'] * 60))
 {
my_setcookie('loginattempts', 1);
my_unsetcookie('failedlogin');
if($mybb->user['uid'] != 0)
{
$update_array = array(
'loginattempts' => 1
);
$db->update_query("users", $update_array, "uid = '{$mybb->user['uid']}'");
}
return 1;
}

So basically this just needs to be updated to perform the same check on the database column of loginattempts that MyBB currently checks in cookies (and add a column to store the last fail login time).

My bad, same issue but actually I have the wrong setting name.

In ACP the maxloginattempts is used. While for member login it's "failedlogincount".

But unfortunately this is still a bug because a cookie reset allows unlimited login attempts and the failedlogincount is easily nullified.

Sorry about the inaccurate first bug report. Hopefully though a solution can be found to improve MyBB against Bruteforce attempts.