Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[For Improvement] Removal of checking of passwords during login against a set of breached passwords for 2.1.7 #841

Closed
csfreak92 opened this issue Sep 16, 2020 · 30 comments
Assignees

Comments

@csfreak92
Copy link
Collaborator

ASVS 4.0 - 2.1.7 verification requirement is:

Verify that passwords submitted during account registration, login, and password change are checked against a set of breached passwords either locally (such as the top 1,000 or 10,000 most common passwords which match the system's password policy) or using an external API. If using an API a zero knowledge proof or other mechanism should be used to ensure that the plain text password is not sent or used in verifying the breach status of the password. If the password is breached, the application must require the user to set a new non-breached password.

I propose an improvement here for this control is to remove checking of passwords during login against a set of breached passwords. It makes more sense to leave only registration and password change modules where checking of passwords against a set of breached passwords since that is the only time a password will be checked with the system's password policy. This eliminates the redundant checking every time the user logs in. If the user has set a weak password (common examples are Password1 or 12345678 during password selection (either through registration or password change modules), then every time he logs in with that password this would always return a negative result if the system has a set of breached passwords like SecList's common passwords: (https://github.com/danielmiessler/SecLists/tree/master/Passwords/Common-Credentials).

I do not see the relevance of always checking the login of someone's password if the mechanism to check its strength has accepted the weak or breached password. Thoughts on this suggestion please?

@elarlang
Copy link
Collaborator

I don't agree with this proposal.

Requirement for periodical password changing is removed now and pre-condition for this is that password is checked against leakages. If user have been using some password really long time and this password is leaked, then it makes sense to check it after every login (not only when account was created). Leakages are the first source of data for dictionary attacks, it's not smart to ignore that check.

@danielcuthbert
Copy link
Collaborator

I'm with @elarlang here, what we are trying to do is better protect the user and the application from password stuffing attacks, which is now an increasingly common (and effective) tactic.

@csfreak92
Copy link
Collaborator Author

If the intention of having a check every time a user logs in is for leaked credentials, then does checking it against top 1,000 or common 10,000 passwords sufficient? It may not even be enough. Now that you pointed it, I do agree that it is not smart to ignore this check. While I have no solid answer to my question too, but I'm thinking if those numbers are good against preventing credential stuffing attacks.

@elarlang
Copy link
Collaborator

elarlang commented Sep 16, 2020

It's not limited to use only 1,000 or 10,000 passwords, I would point out here phrase "or using an external API".

If someone start testing with password from top 1,000 list, then you can block this IP because users can not have those passwords by rules (users can not set those as passwords) - so it can be only bad intent.

But if there some leakage like have been from LinkedIn, then attackers may not test multiple passwords at all - they probably use more targeted approach to check, do some user use the same password like exists in a leakage.

@jmanico
Copy link
Member

jmanico commented Sep 17, 2020 via email

@jmanico
Copy link
Member

jmanico commented Sep 17, 2020 via email

@elarlang
Copy link
Collaborator

If we want to be ready for scenario "password leakage from one site gives access to another site" without expecting that all users are good and use unique password for every site, then only option to do it is to check against breaches. And for me it really makes sense to do it.

Yes, users MAY be frustrated when choosing their unique password, but this can be also alarming and educational for them - "oh, wow, this password have been leaked, where else I have been using it".

@jmanico
Copy link
Member

jmanico commented Sep 18, 2020 via email

@csfreak92
Copy link
Collaborator Author

@jmanico, I agree with points 1 and 2 that you wrote, however how do we propose to monitor for the third point - locking all users and forcing them a password change in the event of a major 3rd party breach? Is there an elegant solution for the third point?

@jmanico
Copy link
Member

jmanico commented Mar 12, 2021

Most of my clients just do a search on username and look for a username match and that's it. And there is no elegant solution which is why I suggest only matching on username. PR time?

@elarlang
Copy link
Collaborator

This is the change I'm going to fight against and loudly :)

Only thing here which may need improvement - the word "login" does not describe, should it be successful or not, but the point for requirement is: if someones password is leaked and it's used to login to the application, then after successful authentication account should be locked and some account activation should be triggered (like activation link to e-mail).

@jmanico
Copy link
Member

jmanico commented Mar 13, 2021

I agree with the original comment. It is not performance friendly or reasonable to do this control at login. The standard defense which is SP800-63 approved is:

  1. check at password CHANGE or password ADD time (new registration)
  2. React to major breaches by locking effected accounts

Every login requiring checks against a 11+ billion account database is not reasonable.

So +1 for the OP's suggestion.

csfreak92 added a commit to csfreak92/ASVS that referenced this issue Mar 15, 2021
Drafted this for the following changes on this control: 
	* removing breached password checking during login
	* changed minimum number of breached passwords to be checked against from old value of 1,000 - 10,000 to 50,000 - 100,000 as per recommended by Jim 
	* added the phrase for reacting to major third party breaches

This Pull Request relates to issue OWASP#841 as per discussed.
@elarlang
Copy link
Collaborator

elarlang commented Mar 17, 2021

Could we get new proposal in the issue before they are going to PR? Merge first, discuss later is not really effective way. Please read comments in PR template. (https://raw.githubusercontent.com/OWASP/ASVS/master/.github/pull_request_template.md)

Latest commit:
d1c9152

Now entire password security is standing that application have working mechanism to check against breaches:

  • If user created account long time ago and as we don't have password periodical change rule anymore, most-likely users will never change it. So if password was not checked on registration against leakage, it will be never checked again.
  • If you have already existing user-base and you want to implement this rule, it does not give any effect.
  • To be able to force users to reset their password, application must be able check based on username or email? How to match accounts in application and in leakage? I think it's not realistic.
  • To force entire user-base to change password every time when there is some leakage, is not nice for users at all. If users have unique passwords for every site, they are not affected by the leakage but need to change their password anyway? Not user-friendly and hard to imagine that someone could implement it.
  • What is "major" leakage? Not clear.

Problem is only for leaked passwords and the goal should be to avoid using leaked passwords. At the moment I my opinion you create more new problems than you solve.

Not PR ready.

@elarlang
Copy link
Collaborator

elarlang commented Mar 17, 2021

Main argument against "check leakage on login" is that it may cause some Denial of Service vector and in general I agree with that concern.

First - you should check leakage after successful login, not before.

How many users you have? How many times per day/week/month they log in?

If I need to develop password check against leakage today what I could do:

  • use "last time password checked from leakage" for password field
  • use "last known leakage" time in general

If user authenticates, you need to check password from leakage, if:

  • "last known leakage" time is later than "last time password checked from leakage"
  • "last time password checked from leakage" is older than

if password is found from leakage, log the situation and call "account recovery scenario" - it may be an attacker who used leaked password and may now take account over with known/correct password.

And obviously you need to check password from leakages (and set value for "last time password checked from leakage") when users sets or changes the password.

@Sjord
Copy link
Contributor

Sjord commented Mar 17, 2021

Perhaps we can split this up into several requirements:

  1. Users should not be able to set easy passwords, so check against top-1000 on register and password change.
  2. Users should not be able to set any breached password, so check against API on register and password change.
  3. Users should not be able to authenticate with any breached password, even if they created their password before it was breached, so check against API on login.

We can make these L1, L2, L3 requirements.

@elarlang
Copy link
Collaborator

And ofcourse, agree with split: #683 (comment) Recommendation nr 3.

(I try to split this monster issue in near future to smaller pieces)

@jmanico
Copy link
Member

jmanico commented Mar 17, 2021

Hey @elarlang in the face of difficult conversation and being ignored for many months, you still persisted in order to help this standard. You have no idea how much respect I have for you and how much I appreciate your participation.

Thank you very much.

@jmanico
Copy link
Member

jmanico commented Mar 17, 2021

PR has been merged! Nice work! I'm closing this out but please re-open if I did it in haste!

And thanks for the PR @csfreak92 :)

@jmanico jmanico closed this as completed Mar 17, 2021
@jmanico
Copy link
Member

jmanico commented Mar 17, 2021

Elar, I missed your earlier comments and am re-opening to make sure your comments are considered. I did accept the PR but can continue to modify after more discussion.

@jmanico jmanico reopened this Mar 17, 2021
danielcuthbert added a commit that referenced this issue Jul 20, 2021
@tghosth
Copy link
Collaborator

tghosth commented Jul 20, 2021

The leader team had an extensive discussion on this.

It was decided to simplify and split the requirements (2.1.7, 2.1.14).

Real-world experience shows that the top 3000 are sufficient for 2.1.7.

2.1.14 is non-trivial because it requires the company to acquire a database of this information which is why it is level 3.

@danielcuthbert
Copy link
Collaborator

Closing this, thank you for the good discussions

@Sjord
Copy link
Contributor

Sjord commented Jul 21, 2021

The leader team had an extensive discussion on this. It was decided ...

Can you summarize the arguments from that discussion? Why did you decide on the current solution? The requirements state that passwords should be checked, not that they should be denied. Is that intentional? What is meant by "an available set"? Any recommendations on which set to use? Why top 3000 passwords? What real-world experience was used? In 2.1.14, what if the password is breached but doesn't belong to the username? Is the username even relevant? There currently is no requirement for checking for breached passwords on login. Did you consider that in your discussion?

I think it's important to document the rationale for requirements, so that it's possible to interpret and modify them later. Furthermore, I feel a bit left out if in the decision process, if the leader group decides things without explanation.

@elarlang
Copy link
Collaborator

I'm reopening the issue because I think those arguments should be provided for everyone, probably those changes will cause a lot of opened issues without making it clear.

ping @danielcuthbert @jmanico

@elarlang elarlang reopened this Jul 21, 2021
@elarlang elarlang assigned Sjord and danielcuthbert and unassigned csfreak92 Jul 21, 2021
@danielcuthbert
Copy link
Collaborator

Happy to.

Firstly I think this is a solid discussion and one where there is no real right or wrong approach. The goal should be to stop credential stuffing attacks (or as us older hackers call it, brute-force attacks) against authentication functions. Passwords suck, we all know this, and the more we can do to stop people from choosing known bad passwords (as in passwords included in the ever-growing breach datasets we now have available to us), the better we are.

What we have proposed for 2.1.7 is that during account creation and password changes, the proposed password selected by the user is checked against a data set of known passwords. We don't want to recommend a single commercial/free tool but there are many out there now and the most popular is Troy Hunt's Haveibeenpwned

As for the top 3000 as a number, it's rare to see such random passwords being used by any of the datasets. The issue we have is that not all passwords are totally random, so to try and balance usability and security and reduce friction, you can limit it to the top 3000 found in say a 12GB password breach file. The number was based on research I've done for my employer using very large datasets but you might decide you want to test the hash against every single one.

On the username aspect, which we added as an L3 check in 2.1.14. This is a harder one to look at implementing and it's one @jmanico was incredibly passionate about adding.

On checking a breached password upon login, yes we discussed this in-depth and personally I feel this isn't the best approach. Now one could argue that continual checking of all passwords should be a thing, and yes I'd tend to agree but where it starts to become an issue is the implementation. Will it add delays to the auth process? How would you handle it if say a user's random solid passphrase is indeed leaked, do you redirect them to a new function that tells them this and gets them to change it? I guess there are other compensation controls you could have to stop credential stuffing attacks from being successful such as 2.2.1.

I know we are keen on hearing from the community if this should actually be a thing? So please comment

@elarlang
Copy link
Collaborator

I just write down my own opinion here as well - my technical ideas, how I could build breach or stupid password checks, are written above in previous comments.

The main goal should be - authentication is not built only asking username and password. If you have at least something valuable in an application, you must have MFA in place. That's it. Breach, stupid passwords etc vectors gone. That's why I "let it go" and also closed my own related issue #683

I tell it in my Web Application Security training since 2012. Now it's 2021 ...

@jmanico
Copy link
Member

jmanico commented Jul 21, 2021 via email

@jmanico
Copy link
Member

jmanico commented Jul 21, 2021 via email

@cmlh
Copy link
Contributor

cmlh commented Jul 21, 2021

There is no change window with "Have I Been Pwned?" to anticipate when new breach[es] will be published so this Service Level Agreement would need to be defined by OWASP unless it has been defined by NIST already?

What impact would incidents such as "Collection 1" have on verification or would this be managed under OpenSAMM/BSIMM?

ADDED 23 July 2021

Will the compromised password file have to bundled to application or is it acceptable to reuse a web API in light of continued attacks against k-anonymity?

Is zxcvbn a possible reference implementation too?

@cmlh
Copy link
Contributor

cmlh commented Sep 30, 2021

As for the top 3000 as a number, it's rare to see such random passwords being used by any of the datasets. The issue we have is that not all passwords are totally random, so to try and balance usability and security and reduce friction, you can limit it to the top 3000 found in say a 12GB password breach file. The number was based on research I've done for my employer using very large datasets but you might decide you want to test the hash against every single one.

I have put forth to @jmanico that we reuse PwnedPasswordsTop100k.txt from NCSC UK for #1055 due to the following:

Please let me know if you want me to update PR #1061 to include 2.17 also?

@jmanico
Copy link
Member

jmanico commented Oct 12, 2021

This is a lingering conversation and I am closing it for now. Please re-open with a straight forward issue if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants