OWASP / ASVS

Application Security Verification Standard
Creative Commons Attribution Share Alike 4.0 International
2.69k stars 657 forks source link

Control 2.2.1 > Too specific and tolerant #906

Closed TheDauntless closed 2 years ago

TheDauntless commented 3 years ago

Concerning 2.2.1: https://github.com/OWASP/ASVS/blob/master/4.0/en/0x11-V2-Authentication.md#v22-general-authenticator-requirements

Verify that anti-automation controls are effective at mitigating breached credential testing, brute force, and account lockout attacks. Such controls include blocking the most common breached passwords, soft lockouts, rate limiting, CAPTCHA, ever increasing delays between attempts, IP address restrictions, or risk-based restrictions such as location, first login on a device, recent attempts to unlock the account, or similar. Verify that no more than 100 failed attempts per hour is possible on a single account.

100 authentication requests before lockout already seems very generous, and would allow you to perform password spraying on a large list of popular passwords. In addition, the ASVS requirement says that it should be 'per hour', which insinuates that you can just try again after an hour, continuing your attack.

The 100 seems very arbitrary, but appears to come from the NIST guide: https://pages.nist.gov/800-63-3/sp800-63b.html#throttle

However, the NIST guide does not say '100 per hour' (but rather 100 at most).

When required by the authenticator type descriptions in Section 5.1, the verifier SHALL implement controls to protect against online guessing attacks. Unless otherwise specified in the description of a given authenticator, the verifier SHALL limit consecutive failed authentication attempts on a single account to no more than 100.

The 2.2.1 control is the only one that mentions account lockout, so there are no other controls that would put more stringent requirements on account locks. For a financial application, 100 is definitely too much, especially per hour. I think either this control should be modified to the text below, or an additional control should be added that focusses on account lockout, and this control should be rewritten to focus on actual throttling against password spraying and not brute-forcing/account lockouts.

So my suggestion, though I'm not sure if you use this risk profile approach in other controls:

Verify that anti-automation controls are effective at mitigating breached credential testing, brute force, and account lockout attacks. Such controls include blocking the most common breached passwords, soft lockouts, rate limiting, CAPTCHA, ever increasing delays between attempts, IP address restrictions, or risk-based restrictions such as location, first login on a device, recent attempts to unlock the account, or similar. Verify that only a limited number of consecutive unsuccessful authentication attempts are allowed, according to the risk profile of the application

The actual number would then of course depend on the application, though some (much lower) defaults could be nice.

It also somewhat surprised me that there is no real control on account lockout and unlocking (I searched for 'lock' in the pdf). Locking accounts can result in a DoS, but since almost every application has an email address to which an unlock/login URL could be sent, that seems like a preventable problem. This is related to this discussion, but let me know if you would like a separate issue to discuss this?

elarlang commented 3 years ago

"no more than 100 attempts per hour" does not mean, that application need to provide "at least 100 attempts per hour".

100 is at least some clear limit. Your proposal actually works in opposite way as it allows now more than 100, because it does not describe the limit - everyone can interpret their own.

I don't see problem with that (part of the) requirement.

TheDauntless commented 3 years ago

We can surely keep a maximum upper limit, but even the NIST '100 before successful attempt' is much stricter than what's currently in the guide.

PCI DSS 8.1.6 has 'no more than 6' which is much more reasonable and secure: https://www.pcisecuritystandards.org/documents/Prioritized-Approach-for-PCI_DSS-v3_2.pdf?agreement=true&time=1469037392985

CIS has 'after 5 failed attempts': https://www.cisecurity.org/blog/cis-password-policy-guide-passphrases-monitoring-and-more/

So alternatives are: Remove the 'per hour' to make it more strict, or go down even further to PCI DSS and CIS recommendations, since 100 is unacceptable for high-security applications, especially since the control doesn't even enforce an account lockout after those 100 attempts, but rather suggest different potential actions:

Verify that anti-automation controls are effective at mitigating breached credential testing, brute force, and account lockout attacks. Such controls include blocking the most common breached passwords, soft lockouts, rate limiting, CAPTCHA, ever increasing delays between attempts, IP address restrictions, or risk-based restrictions such as location, first login on a device, recent attempts to unlock the account, or similar. Verify that no more than 5 failed attempts are possible on a single account before triggering a reaction.

elarlang commented 3 years ago

If you remove "per hour", then you can interpret, like "5 attempts per lifetime" which is not correct.

I still say, if the maximum is 100, does not mean your application can not have limit at 5. But it must have "timeunit" specified.

note: I'm not the author of the requirement or responsible what goes / what does not go to the project, just brainstorming ideas here :)

jmanico commented 3 years ago

i like the last suggest of adding "Verify that no more than 5 failed attempts are possible on a single account before triggering a reaction."

Can you submit a PR on this and let's get this one done? Thank you!

elarlang commented 3 years ago

I still stand for "timeunit must be specified". 5 attempts during what time? If it is not specified, it means "5 units total" or since account is created. Not PR-ready.

jmanico commented 3 years ago

5 per hour or 100 total errors should trigger the control - I think that is what we want to do.

tghosth commented 2 years ago

@elarlang can you submit a PR?

elarlang commented 2 years ago

Not really, as

I don't see problem with that (part of the) requirement.

https://github.com/OWASP/ASVS/issues/906#issuecomment-782163227

tghosth commented 2 years ago

Any further comments @TheDauntless ? Do you think you can open a PR based on the comments above. Need to decide by mid-May whether to action or close.

elarlang commented 2 years ago

Current updated proposal:

Verify that anti-automation controls are effective at mitigating breached credential testing, brute force, and account lockout attacks. Such controls include blocking the most common breached passwords, soft lockouts, rate limiting, CAPTCHA, ever increasing delays between attempts, IP address restrictions, or risk-based restrictions such as location, first login on a device, recent attempts to unlock the account, or similar. More than 5 failed authentication attempts per hour should trigger some sort of reaction or alert.

"More than 5 failed authentication attempts per hour"

"some sort of reaction"

tghosth commented 2 years ago

"More than 5 failed authentication attempts per hour"

  • per user?
  • per application?
  • per IP?

I agree I think it would be worth @TheDauntless confirming about this.

"some sort of reaction"

some may interpret, that you should lock account after 5 failed logging attempts for one user? or from one IP? and we are back to lockout scenario

I think we need to leave the reaction vague in this case as it will be very case specific

elarlang commented 2 years ago

We can have different "some sort of reactions" here, different limits and different "blocks".

If you have only "per user" limit, attacker can rotate usernames and use the same IP address for online guessing. If you have only "per IP" limit, attacker can use different IP addresses.

Numbers are "random" and subject to change, just for explaining the idea.

tghosth commented 2 years ago

@elarlang I have approved the change for now. If we want to specify something else then I am open to suggestions but I would be worried about being too specific...