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

V2.1.2 - No Password Upper Bound #756

Closed
ThunderSon opened this issue May 4, 2020 · 43 comments
Closed

V2.1.2 - No Password Upper Bound #756

ThunderSon opened this issue May 4, 2020 · 43 comments
Assignees
Labels
2) Awaiting response Awaiting a response from the original poster
Milestone

Comments

@ThunderSon
Copy link
Contributor

In V2.1.2, the password is mentioned to have a lower bound, but nothing related to the upper bounds. There should be a clear mention of an acceptable upper bound. In the Password Storage CS we discussed about this and provided 2 attacks that occurred because of the lack of a maximum length.
There is no possible explanation to have passwords above 128 characters, where even security tokens do not even reach to that length, and those are used by bots and automation tools.

@jmanico
Copy link
Member

jmanico commented May 4, 2020 via email

@ThunderSon
Copy link
Contributor Author

It doesn't mention though a maximum upper limit. Not setting a maximum upper limit risks setting the applications in trouble.

@tghosth
Copy link
Collaborator

tghosth commented May 18, 2020

I agree with @ThunderSon that this is something to consider

@jmanico do you see a problem with having a requirement for this even if NIST does not specifically mention it?

@elarlang
Copy link
Collaborator

For traceability - current V2.1.2:

V2.1.2 Verify that passwords 64 characters or longer are permitted.

With just setting upper limit we don't address the base problem why it's needed - upper limit must take away potential DoS attack vector.

Maybe the new requirement (or merged to current V2.1.2) should have an idea like (random pieces, no actual requirement):
Verify that long passwords does not cause Denial-of-Service attacks / taking too much resources / have pre-hashing used, if needed.

@jmanico
Copy link
Member

jmanico commented May 19, 2020 via email

@tghosth
Copy link
Collaborator

tghosth commented May 21, 2020

@ThunderSon please can you draft a text for a new requirement along these lines and put it here. Is there a best practice you can refer to for this?

@tghosth tghosth added the 2) Awaiting response Awaiting a response from the original poster label May 21, 2020
@ThunderSon
Copy link
Contributor Author

Sure, I'll open a PR in a bit.

@tghosth
Copy link
Collaborator

tghosth commented May 23, 2020

I'd you do a PR, needs to be against master and 4.0.2 branch :)

@ThunderSon
Copy link
Contributor Author

ThunderSon commented May 24, 2020

This requires 2 branches, as these are 2 separate bases for the commit.
What do you think about this text?

V2.1.2 Verify that passwords allow for long passwords (e.g. 64 characters) and have an upper bound limit (e.g. 128 characters).

I tried multiple variations to the text. Keeping it this simple came out to be the best option to me

It's not only DoS issues. There is the truncation issue as well. Who knows what comes out next. Requirements need to stay actionable and simple.
As for guidance, my recommendation would be the password storage CS - maximum password lengths section. This CS is one of the best crafted guides for password storage.

@elarlang
Copy link
Collaborator

elarlang commented May 24, 2020

truncation

I would say, that truncation part is already covered by V2.1.3

V2.1.3 Verify that password truncation is not performed. However, consecutive multiple spaces may be replaced by a single space.

Upper bound - optional or required?

Does it makes sense to "force" upper bound or leave it as optional solution for situations when it's needed?

Let's take situation, when you don't use bcrypt, what is the reasoning to have upper-bound limit then?

In reality it's mostly theoretical problem - I don't think it's big chance that someone is using passwords longer than 128 symbols, but I want reasoning to be clear.

Basically, if I verify that some application is allowing users to use longer than 128 symbols passwords and long passwords does not cause any performance issues and does not have truncation, what is the risk they have? Why I should recommend them to use upper bound. Currently I can not see reasoning for "strong requirement" here.

@ThunderSon
Copy link
Contributor Author

I am not worried about users. I am worried about attackers. That's exactly why this requirement is required, because no normal user will set a password that is in the 250 characters length.
When an attacker sends a huge password in, are you telling me that no performance issue is going to arise?
How about I run multiple threads with automatic IP and username update (that is doable, and not theoretical)?

@elarlang
Copy link
Collaborator

elarlang commented May 24, 2020

When an attacker sends a huge password in, are you telling me that no performance issue is going to arise?

I don't tell anything, I was asking reasoning. If there is security requirement, it must be clear what it solves and what attack vector it takes away.

And I'm reflecting it back with question - are you telling me IT IS causing performance issues and it does not depend what kind of hashing method is in use?

How about I run multiple threads with automatic IP and username update (that is doable, and not theoretical)?

This is already anti-automation topic and it is covered by other requirements. This is something what you need to have anyway, even with 20 symbol long passwords.

My question stays - if we take anti-automation away from your example (as it is separate requirement and it is already covered), what is the risk or problem to solve and is it enough to have strong requirement instead of optional "if needed" requirement?

@ThunderSon
Copy link
Contributor Author

Hmmm I see your point of view.
People forget usually that input restriction must be applied to the password field. Just like you'd limit a variable to allow for example a certain amount of characters, the password field needs to abide by that as well. It's really straight-forward and people just ignore it for some reason?
All KDFs require processing. That's why they're used vs normal hashing. The stronger the KDF (fights off side-channel attacks, has more rounds, etc.), the more intensive it'll be (most of the times).

My attack vector is not solved by anti-automation. I am replicating a burst of users sending login requests.
You can't block my attack. It needs to be handled on the code level.
My attack changes the username, the IP, and can if need be update the user-agent string. How are you going to block me? You can't finger print the attack. If a CSRF token is added, I'll make it a 2 staged request and extract the received CSRF token and use it. As long as there is no strong identifier for the user (authenticated), an attacker will be able to bypass these mechanisms.
Following this example, the web server unlocked multiple threads to handle the logins coming in, and each of them require for example 0.3 seconds. Let's say I increase the password size to 1MB, and KDFs require memory to run through the rounds, and will need more CPU resources, that'll slowly escalate to taking the server down with a dedicated attack team (Suddenly we need a WAF to block a DDoS attack happening, and might be late since the amount of processing happening is bigger than the normal threashold).

@elarlang
Copy link
Collaborator

elarlang commented May 24, 2020

dos part

So, it is pure DoS mitigation requirement then like I pointed out in my first comment.

I would say, your attack scenario is mostly anti-automation problem to solve - hashing resources is "helping" parameter in this attack. From attack(er) perspective, you can compensate "password upper-bound" with more resources, if you claim that it's not solvable by anti-automation. Do you solve this attack vector by setting max length 128 for password? I doubt. And it is still depending on what kind of hashing function you use.

You can not make your house safe against getting smashed by bulldozer by installing stronger lock for door.

upper-bound

To be clear - I'm not against upper-bound limit and I can not find any business need for more than 128 (for example) symbols long passwords. If it is "critical" application, authentication should not be done with asking (only) username-password anyway.

(edited)
NIST actually mentions the max length also: https://pages.nist.gov/800-63-3/sp800-63b.html#a2-length

Extremely long passwords (perhaps megabytes in length) could conceivably require excessive processing time to hash, so it is reasonable to have some limit.

My question here was: Is "reasonable" with meaning "you need to have it" or "you may have it". I'm ok with both solutions. I got my info and answers to build my own risk and recommendations based on that.

@tghosth
Copy link
Collaborator

tghosth commented May 27, 2020

V2.1.2 Verify that passwords allow for long passwords (e.g. 64 characters) and have an upper bound limit (e.g. 128 characters).

So I agree with the upper bounds limit and this seems like a nice clean requirement.

However, I also note the requirement from the cheatsheet:

In order to protect against both of these issues, a maximum password length should be enforced. This should be 64 characters for Bcrypt (due to limitations in the algorithm and implementations), and between 64 and 128 characters for other algorithms.

Do we need to take this Bcrypt issue into account in ASVS as well?

@jmanico
Copy link
Member

jmanico commented May 27, 2020 via email

@elarlang
Copy link
Collaborator

@jmanico, be careful with "64 symbols limit" and context here :) - "at least 64 symbols long" vs "maximum 64 symbols long"

5.1.1.2 Memorized Secret Verifiers
URL: https://pages.nist.gov/800-63-3/sp800-63b.html#-5112-memorized-secret-verifiers

Verifiers SHALL require subscriber-chosen memorized secrets to be at least 8 characters in length. Verifiers SHOULD permit subscriber-chosen memorized secrets at least 64 characters in length.

A.2 Length
URL: https://pages.nist.gov/800-63-3/sp800-63b.html#a2-length

Users should be encouraged to make their passwords as lengthy as they want, within reason. Since the size of a hashed password is independent of its length, there is no reason not to permit the use of lengthy passwords (or pass phrases) if the user wishes. Extremely long passwords (perhaps megabytes in length) could conceivably require excessive processing time to hash, so it is reasonable to have some limit.

@jmanico
Copy link
Member

jmanico commented May 27, 2020 via email

@tghosth
Copy link
Collaborator

tghosth commented Jun 2, 2020

V2.1.2 Verify that passwords of up to 64 characters are allowed but no more than that to avoid resource overuse and other bugs.

Maybe we add a note in the 2.1 intro which explains why.

Any further comments?

@elarlang
Copy link
Collaborator

elarlang commented Jun 2, 2020

Is there any known other "other bug" than truncation?

@tghosth
Copy link
Collaborator

tghosth commented Jun 2, 2020

Maybe not, I guess we could say:

V2.1.2 Verify that passwords of up to 64 characters are allowed but no more than that to avoid resource overuse and the bcrypt truncation bug.

@jmanico
Copy link
Member

jmanico commented Jun 3, 2020 via email

@tghosth
Copy link
Collaborator

tghosth commented Jun 4, 2020

@jmanico so how to we handle the fact that bcrypt can only handle ~64 characters, advise pre-hashing with bcrypt?

@jmanico
Copy link
Member

jmanico commented Jun 4, 2020 via email

@tghosth
Copy link
Collaborator

tghosth commented Jun 10, 2020

Well this has been a ride.
We currently have v2.4.4 which mentions bcrypt. I think we should add the pre-hashing requirement to this requirement and leave the password length requirement simple. This would leave us with:

V2.1.2 Verify that passwords allow for long passwords (e.g. 64 characters) and have an upper bound limit (e.g. 128 characters).
V2.4.4 Verify that if bcrypt is used, the work factor SHOULD be as large as verification server performance will allow (typically at least 13) and passwords are pre-hashed to avoid truncation issues.

We can then add a note to 2.4.4 about the truncation issue.

Any further comments?

@jmanico
Copy link
Member

jmanico commented Jun 10, 2020 via email

@elarlang
Copy link
Collaborator

More fuel to the fire: do we need strong pre-hashing is must have or "if needed" requirement?
... "if longer than 64 symbols long passwords are accepted by the application"

@tghosth
Copy link
Collaborator

tghosth commented Jun 12, 2020

More fuel to the fire: do we need strong pre-hashing is must have or "if needed" requirement?
... "if longer than 64 symbols long passwords are accepted by the application"

I am a little worried it is getting too complicated.

V2.1.2 Verify that passwords allow for long passwords (e.g. 64 characters) and have an upper bound limit (e.g. 128 characters).
V2.4.4 Verify that if bcrypt is used, the work factor SHOULD be as large as verification server performance will allow (typically at least 13) and passwords are pre-hashed to avoid bcrypt's 72 byte limits.

@jmanico what do you think about this change now?

@jmanico
Copy link
Member

jmanico commented Jun 15, 2020 via email

@tghosth
Copy link
Collaborator

tghosth commented Jun 16, 2020

V2.1.2 Verify that passwords allow for long passwords (e.g. 64 characters) and have an upper bound limit (e.g. 128 characters).

@jmanico This requirement requires an upper bound limit to be defined meaning that pre-hashing should not be necessary, no?

@jmanico
Copy link
Member

jmanico commented Jun 16, 2020 via email

@tghosth
Copy link
Collaborator

tghosth commented Jun 18, 2020

So @jmanico maybe forget the change to v2.4.4 and go for :

V2.1.2 Verify that passwords 50 characters or longer are permitted and that pre-hashing is used for passwords longer than 50 characters to avoid resource overuse and the bcrypt truncation bug.

Source of 50 characters is this: https://security.stackexchange.com/questions/39849/does-bcrypt-have-a-maximum-password-length

@vanderaj vanderaj added this to the 4.0.2 milestone Jun 23, 2020
@vanderaj vanderaj self-assigned this Jun 23, 2020
@kingthorin
Copy link
Contributor

kingthorin commented Jul 7, 2020

https://www.acunetix.com/vulnerabilities/web/long-password-denial-of-service/

By sending a very long password (1.000.000 characters) it's possible to cause a denial a service attack on the server. This may lead to the website becoming unavailable or unresponsive. Usually this problem is caused by a vulnerable password hashing implementation. When a long password is sent, the password hashing process will result in CPU and memory exhaustion.

Associated with CWE-400 and CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H

@rbsec
Copy link

rbsec commented Jul 7, 2020

Hi @tghosth,

I was one of the authors of the re-written password storage cheat sheet. We had a lot of discussion (and a certain amount of disagreement) about password length and pre-hashing, so I thought I'd weigh in with some of the reasoning behind it.


Bcrypt Maximum Lengths

The Stack Exchange thread you linked talks about the 50 character limit, but as per the second response, this "limit" seems to be largely based on a difference between the implementations and the original article about Bcrypt, and although it's commonly repeated, I wasn't able to find any real-world implementations that have this limit - they all seem to truncate at 71/72 bytes (the null at the end).

In order to give a bit of a safety margin (as characters don't necessarily equal bytes), and so that it's not necessarily obvious the site is using Bcrypt, we settled on 64 characters as a maximum length. This still gives about 420 bits of security (assuming 95 possible characters), which in practical terms is no different to the 473 bits that you'd get with the full 72 characters.


Pre-Hashing

Pre-hashing is an interesting solution, and one that I'm personally not a fan of for a number of reasons - mostly because it's easy to do wrong. There are a few different thing you need to consider, such as:

  • What algorithm do you use?
    • If you use SHA-256 then you're actually reducing the security to 256 bits (compared to 420 with truncation)
    • If you use SHA-512, the common hex output will be more than 72 chars, so you need to encode as base64 or something.
  • How do you encode the output? Null bytes could cause problems if they get passed, so you need to have some kind of encoding.
  • If an attacker has a leaked database that uses sha256($password) and you're using bcrypt(sha256($password)), they can use the leaked hashes to quickly identify people who are re-using passwords, and then focus on cracking the much weaker SHA-256 hashes instead of your strong Bcrypt ones.
  • Who actually benefits from pre-hashing rather than truncating at 64 characters?
    • A random 64 character password is uncrackable, so there's no real security benefit going from 420 bits to 512 bits.
    • A weak/predictable 128 character password (common phrase, repeated string, etc) isn't significantly stronger than a 64 character one.
    • The vast, vast majority of users won't even use 20 characters, let alone 64.

All in all, pre-hashing adds complexity and creates far more opportunities for you to screw up the password hashing code compared to just truncating at 64 characters, without providing any tangible security befits. That's not to say that there are never cases where it might be appropriate, but for the majority of use-cases it's better just to truncate.


My thoughts on the key points are something like:

  • The maximum password length should be at least 64 characters
  • The maximum password length should not be more than 128 (256?) characters unless pre-hashing is used.
  • If pre-hashing is used, the initial algorithm should be at least 256 (512?) bits, and the initial hash must not be truncated
  • If Bcrypt is used, the maximum length should not be more than 71 characters (to account for the null byte).

These are simple, actionable points, and should be easy to both follow and check.

Hopefully this won't start too much of an argument - I know that @jmanico and I don't quite see eye-to-eye on this topic.

@jmanico
Copy link
Member

jmanico commented Jul 7, 2020 via email

@ThunderSon
Copy link
Contributor Author

Scrypt didn't live up properly (it failed frankly), and here is a tweet by the people that chose argon2 to be the next PHF and decided it based on it being the best KDF (doesn't mean it's the best PHF), and as such they don't see it to be the almighty best.
https://twitter.com/TerahashCorp/status/1155129705034653698

I hope you understand that whenever we write a full answer, myself or @rbsec , we don't cut it short on any side. PBKDF2 is harder to configure than bcrypt, and isn't much better!
Every offensive cryptographer we chat with tells us to just use bcrypt properly. My question as well, why argon2i and not argon2id? Argon2i has a vulnerability published and isn't GPU resistant attacks, unlike argon2d and argon2id.
I am answering from my phone as well, so not the best quoted answer.
Please, ASVS should recommend the firmest and most robust. Bcrypt does its job perfectly. Again, as Robin stated. Why do you need more than 64 characters? Add complexity for what?

@jmanico
Copy link
Member

jmanico commented Jul 10, 2020 via email

@vanderaj vanderaj assigned jmanico and unassigned vanderaj Jul 12, 2020
@jmanico
Copy link
Member

jmanico commented Jul 13, 2020

The "only bcrypt advice" is flat out wrong, even if passed around by some legacy cryptographers.

Here are the strength categories we need to consider for password storage.

Class 1
    PBKDF2
Class 2
    BCRYPT
    ARGONX
Class 3
    SCRYPT
    ARGON2ID

Others might argue class-equivalence as it pertains to [b|s]crypt. The reason I don’t is more of a 15-20 page paper than an GitHub post. Scrypt’s configurable parameters address all known weaknesses in underlying primitives. They also resist all known HW-accelerated approaches to brute force and any partial result/lookup optimizations.

One -might- argue that scrypt is hard to wield (tune, configure) compared with bcrypt, and that scrypt is horror show for the defenders’ servers to execute compared to bcrypt – and those arguments I’ll accept. However, in terms of “attack resistance” of emitted cipher text, scrypt is at least on a par with bcrypt – and properly configured is significantly stronger now and in the future.

I believe it’s out of scope and highly dangerous for ASVS to split hairs on preferential implementation such as this. There’s a difference between verify the application:

(bad) Supplies only cryptographically-strong pseudo-random data for key generation; or
(much better) Stores passwords and other credentials by obfuscating them using a cryptographically analyzed and yet-unbroken adaptive one-way function, tuned to use a work factor that generates at least 300ms “proof of work” for normal users, and 1000ms PoW for administrative users.

And saying

Only developers who use BCRYPT are cool.

Think about it. Would ASVS say,

React is whack, use Angular only;
XML is so passe, use JSON; or

No, because why on earth. Preferences on HOW are opinion. Driving developers to a WHAT that’s inarguable industry best practice (like an adaptive one-way function), and providing objective SUCCESS CRITERIA (such as a PoW of 300ms) is the way to go as opposed to continuing this never-ending debate on bcrypt and prehashing and more.

@jmanico
Copy link
Member

jmanico commented Jul 13, 2020

I am addressing the original poster's issues here 336c235 if you wish to address the many other issues in this mega-thread, please open a new issue

@Sjord
Copy link
Contributor

Sjord commented Jun 23, 2021

@ThunderSon, I am researching the issue of long password DoS again, and I have a couple of questions perhaps you can help me with. In the initial comment you state:

provided 2 attacks that occurred because of the lack of a maximum length

What are those two attacks? Can you elaborate on that?

Furthermore, you said that you discussed this in the cheat-sheet team. The cheat sheet used to suggest a maximum of 1000 characters, but I can't find a maximum in the current version. Did I miss it? Did they remove the maximum? Do you have information on that?

Feel free to drop me an email if you want to take this outside of this GitHub issue.

@jmanico
Copy link
Member

jmanico commented Jun 23, 2021 via email

@Sjord
Copy link
Contributor

Sjord commented Jun 23, 2021

The underlying problem with Django was that they had a bug in their PBKDF2 implementation, which hashes the password on every iteration instead of just once at the beginning. Once they solved that, they also removed the maximum length for passwords.

In a proper password hashing function, the password length is only important once, not on every iteration. Calculating a PBKDF2 hash of a 1 MiB password takes only a couple of milliseconds longer than a one character password on my machine.

Denial of service can still be possible, but I don't think it's as straightforward to say that long passwords reliably cause denial of service.

@ThunderSon
Copy link
Contributor Author

It's correct as you say. If you extend the length by a certain amount it's still almost the same. It's important to profile CPU and memory as well, across several algorithms and their "parameters".

It was the call of "What is a sensible upper bound that we are looking to set?"
After reviewing the current state, 128 was a strong recommendation. One might ask "Why not 256", then again, what is the improvement you're getting?

Feel free to change it if you can find a "sensible" value that makes sense and provides more security.

The whole concern on this came from:

  1. Extremely large payloads -- Let's not forget that any input field should have a limit, passwords are not different.
  2. Bcrypt limitation (we know it's not 128, it raised flags for us to think about)
  3. Bad implementations, like the one Django had.

And as usual, closed issues should only be referenced, and not be a place for discussions. Please open a new ticket for this to give proper visibility to your concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2) Awaiting response Awaiting a response from the original poster
Projects
None yet
Development

No branches or pull requests

8 participants