View Issue Details

IDProjectCategoryView StatusLast Update
0026794mantisbtsecuritypublic2020-12-30 07:37
Reporterrandomdhiraj Assigned Todregad  
PriorityhighSeveritymajorReproducibilityalways
Status closedResolutionfixed 
Target Version2.24.4Fixed in Version2.24.4 
Summary0026794: User Account - Takeover
Description

It was observed that the POST request for login.php which carries username and password does not protect against bruteforce attack. A malicious actor could perform a bruteforce attack to gain access of any end users profile/account.

Steps To Reproduce
  1. Navigate to https://bugs.xxx.com/login.php enter your username and password, capture the request in proxy tool such as burpsuite.
  2. Send the captured request to Intruder tab and select 'password=' parameter to perform bruteforce attack against it.

Please find below attached PoC for your reference in which I've sent 100+ dummy request which contains wrong password for username rahulm but the 101th request's content length was different from others (960) which gives and edge to an attacker of actual password of user rahulm

Additional Information

There should be a lockout mechanism in place or throttling against such bruteforce attack.

TagsNo tags attached.

Activities

randomdhiraj

randomdhiraj

2020-03-17 05:08

reporter  

PoC.png (391,990 bytes)
atrol

atrol

2020-03-17 07:20

developer   ~0063770

There should be a lockout mechanism in place or throttling against such bruteforce attack.

There is configuration option $g_max_failed_login_count that should do the job. Do you agree?
https://mantisbt.org/docs/master/en-US/Admin_Guide/html-desktop/#admin.config.signup

randomdhiraj

randomdhiraj

2020-03-17 07:48

reporter   ~0063771

Atrol, Thanks this does the job. Aside I believe this should be enable by default as a best practice.

atrol

atrol

2020-03-17 07:59

developer   ~0063772

I believe this should be enable by default as a best practice

As always there are pros and cons.
If it's enabled, attackers could make accounts invalid by trying passwords, so users would be forced to reset their password.

dregad

dregad

2020-03-18 04:51

developer   ~0063775

I would tend to agree with @randomdhiraj that we should change this default from OFF to some fixed value - threshold can be discussed, but 5 seems reasonable.
In my opinion, it's better to force users to reset their password, than risking to have their account hacked without their knowing about it.

randomdhiraj

randomdhiraj

2020-03-18 06:50

reporter   ~0063776

Thanks Dregad & Atrol.

vboctor

vboctor

2020-12-07 23:48

manager   ~0064773

@dregad I don't think we should lock the user account forever when an attacker attempts to attack them. I wonder if we are better off using the tokens_api to have a temporary block.

We introduce a token type ACCOUNT_LOCKOUT_IPS which has a lifetime of 5 minutes and its value is an array of IP addresses.

  1. If such token doesn't exist, do regular behavior.
  2. If failed login threshold is reached, create token and add the IP to the token valid (as part of a json array).
  3. If a login attempt happens and IP not in the token, then check password, if invalid, add to token. If correct, then delete the token and sign in successfully.
  4. If a login attempt happens from IP in the token, then fail automatically without checking password.
  5. On a successful login, reset the failed login count.

I'm referring to the API in tokens_api.php.

Also changeo of this behavior is better done in a proper release and not a patch release - this may be the intention but it is hard to tell without having a specific cherry-pick commits or a PR for 2.24.x vs. master-2.25 for example.

dregad

dregad

2020-12-09 02:47

developer   ~0064776

Last edited: 2020-12-09 03:05

Thanks for the feedback Victor.

Also changeo of this behavior is better done in a proper release and not a patch release - this may be the intention but it is hard to tell without having a specific cherry-pick commits or a PR for 2.24.x vs. master-2.25 for example.

Apologies, I forgot to set the target version, have done it now (2.24.4).

The current, lockout is how the system has been working until now (assuming the admin has set $g_max_failed_login_count > 0), and this has never caused any issues. Considering that the user can unlock the account themselves by following the usual password change mechanism, I think the increased security we get by changing this config's default is a minor change that definitely fits in the scope of a hotfix release.

Admins relying on external authentication (e.g. LDAP) can -- and should -- set this to zero; maybe I can update documentation to reflect this (and mention that under $g_login_method too).

Temporary lockout would be a nice feature, but that's not something I would implement as part of a security release. I have opened #27741 to track this for later implementation.

Related Changesets

MantisBT: master 30b37742

2020-10-10 08:04:34

dregad

Details Diff
Default max_failed_login_count to 5

Update documentation accordingly.

This prevents brute force attacks to gain access to users' accounts.

Fixes 0026794
Affected Issues
0026794
mod - config_defaults_inc.php Diff File
mod - docbook/Admin_Guide/en-US/config/signup.xml Diff File