search

OWASP / ASVS

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

2.1.14 needs changing after researching this issue #1055

Closed jmanico closed 3 years ago

jmanico commented 3 years ago

2.1.14 is currently:

[ADDED, SPLIT FROM 2.1.7, LEVEL L1 > L3] Verify that passwords submitted during account registration or password changes are checked against a set of breached username/password pairs. (C6) After reviewing the various services and talking to both @tghosth and Troy Hunt on this issue, I suggest we change this to:

`[ADDED, SPLIT FROM 2.1.7, LEVEL L1 > L3] Verify that passwords submitted during account registration or password changes are checked against a list of breached passwords. (C6)

cmlh commented 3 years ago

What defines the "breached password list"?

jmanico commented 3 years ago

https://auth0.com/breached-passwords describes is briefly. What do you think of:

`[ADDED, SPLIT FROM 2.1.7, LEVEL L1 > L3] Verify that passwords submitted during account registration or password changes are checked against a list of breached passwords discovered in third party data breaches. (C6)

cmlh commented 3 years ago

I meant "who" defines the passwords in the "breached password list".

I believe we're on the same page to not declare a commercial but free service as the standard similar to @jmanico stance on BugCrowd et al.

Below is the list of breaches defined by Google's Password Manager which is a commercial but free service too:

As far as I am aware @HaveIBeenPwned doesn't publish the same corresponding list of breaches for "Pwned Passwords" as their methodology differs.

jmanico commented 3 years ago

Adding some kind of reference to OSS breached password lists seems reasonable. I also think it's ok to just list "breached password lists" and not mention commercial or other sources.

cmlh commented 3 years ago

A standard needs to be measured against otherwise the minimum will become acceptable.

Perhaps we should make this into an OWASP Project?

cmlh commented 3 years ago

I'll give interim approval to PwnedPasswordsTop100k.txt as it is published by NCSC @jmanico until their dataset is public domain.

jmanico commented 3 years ago

https://www.ncsc.gov.uk/static-assets/documents/PwnedPasswordsTop100k.txt is certainly a good resource to add to ASVS.

elarlang commented 3 years ago

Related issue and discussion: https://github.com/OWASP/ASVS/issues/841

Sjord commented 3 years ago

a set of breached username/password pairs. a list of breached passwords.

I agree. I think this is an improvement. I think this requirement can be more clear on that passwords that are in the list should be denied immediately.

I don't think we need to be specific which list should be used. For non-English sites it may make sense to use a non-English list. Some developers may prefer using an API over a static word list.

jmanico commented 3 years ago

I agree with all of your comments. I want this password list added as a reference at the bottom of this section as a secondary reference, not in a requirement itself.

cmlh commented 3 years ago

I don't think we need to be specific which list should be used. For non-English sites it may make sense to use a non-English list. Some developers may prefer using an API over a static word list.

PwnedPasswordsTop100k.txt isn't limited to English.

Will ASVS mitigate https://cablej.io/blog/k-anonymity/ of the upstream API too?

jmanico commented 3 years ago

I am not sure, and I think the password list is a good resource to add at the end of the section.

There are MANY ways to address credential stuffing other than the PwnedPasswordsTop100k.txt list. That is why we need the requirement to be something other than using that specific list.

cmlh commented 3 years ago

There are MANY ways to address credential stuffing other than the PwnedPasswordsTop100k.txt list. That is why we need the requirement to be something other than using that specific list.

I believe ASVS should mandate a minimum standard but have actioned https://github.com/OWASP/ASVS/pull/1061 in the interim.

jmanico commented 3 years ago

Christian I'm closing this out now as the original issue is solved, but I'll take a PR where https://www.ncsc.gov.uk/blog-post/passwords-passwords-everywhere#:~:text=PwnedPasswordsTop100k.txt is listed as a reference at the end of the section. If you give me an ok I'll take it on as well.

cmlh commented 3 years ago

I can rename https://github.com/OWASP/ASVS/pull/1061 Pull Request and then resubmit https://github.com/OWASP/ASVS/pull/1061 Pull Request from a different branch @jmanico

Also https://github.com/OWASP/ASVS/issues/1071 is missing the list of Google's Password Manager above i.e. https://github.com/OWASP/ASVS/issues/1055#issuecomment-926298144

jmanico commented 3 years ago

Please just submit a new PR if that’s ok, I’m new to administering GIT it’s not intentional.

I also opened a new issue for this as well.

https://github.com/OWASP/ASVS/issues/1071

Is this ok for now, Christian?