OWASP / ASVS

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

2.1.2 Passwords of more than 128 characters are denied (make entire 2.4 more abstract) #1923

Open sohsatoh opened 7 months ago

sohsatoh commented 7 months ago

In 2.1.2, it is specified that the maximum password length should not exceed 128 characters.

Although this requirement is specified here to prevent long password denial of service, "128 characters" does not seem to be based on any theoretical basis as we discussed in https://github.com/OWASP/CheatSheetSeries/issues/1376

Therefore, it seems to me that this requirement should be Verify that passwords of at least 64 characters are permitted. and then add something like Verify that DoS does not caused due to long passwords to 2.1.x.

(To be clear, I am not of the opinion that 128 characters is too few, but rather that the standard requirement should be specified based on some criteria.)

elarlang commented 7 months ago

Hi, @sohsatoh ,

we had a long discussion over it here: https://github.com/OWASP/ASVS/issues/1772

What to keep in mind - from the ASVS requirement structure point of view:

I agree that we should abstract the precise limit out with the actual goal - to provide defense against Denial of Service attack. Also, as I pointed out in #1772, the requirement 2.1.2 contains 2 parts - one is for password length for end-user and another is for storage. The storage part is section 2.4:

# Description L1 L2 L3 CWE NIST §
2.4.1 [MODIFIED] Verify that one of the following password hashing functions is used when storing the user's password for the application: argon2id, scrypt, bcrypt or PBKDF2. 916 5.1.1.2
2.4.3 [MODIFIED] Verify that if PBKDF2 is used, the iteration count should be a minimum of 1,300,000 iterations with PBKDF2-HMAC-SHA1, a minimum of 600,000 iterations using PBKDF2-HMAC-SHA256, or with a minimum of 210,000 iterations with PBKDF2-HMAC-SHA512. 916 5.1.1.2
2.4.4 [MODIFIED] Verify that if bcrypt is used, the work factor is a minimum of 10 and password size is limited to 72-bytes due to bcrypt's input limit. 916 5.1.1.2
2.4.6 [ADDED] Verify that if argon2id is used, there should be a minimum configuration of 19 MiB of memory, an iteration count of 2, and 1 degree of parallelism. 916 5.1.1.2
2.4.7 [ADDED] Verify that if scrypt is used, the configuration should be a minimum work factor of (2^17), a minimum block size of 8 (1024 bytes), and a parallelization parameter of 1. 916 5.1.1.2

Instead of giving rules and limits for every used and accepted hashing function, we should abstract out the entire section with the message (not a wording proposal!) "Verify that accepted method is used, it is used and configured according to the latest spec or recommendations, such as to have enough iterations and limitation to max input length against denial of service attacks". Then it is more future-proof as well. We should modify the current 2.4.1 for that.

One option is also to provide those limits in the chapter text: "When releasing the ASVS version v5.0, accepted limits per used function were ..." + links to resources, from where to get the latest ones.

ping @tghosth @jmanico

jmanico commented 7 months ago

Elar, this comment from the OP concerns password length and is related but separate from 2.4.x. Can you please separate your password storage comments into a separate issues, please?

jmanico commented 7 months ago

Hey @sohsatoh I agree NIST does not demand that passwords not exceed a 128 character max. But I still would like to add it in as a suggestion.

How about:

Verify that passwords of at least 64 characters are permitted, and that passwords above a certain max length (such as 128 or more characters) are denied.

elarlang commented 7 months ago

Elar, this comment from the OP concerns password length and is related but separate from 2.4.x. Can you please separate your password storage comments into a separate issues, please?

I'll do it when this issue is solved without solving 2.4. (my recommendation to solve this issue was to cover it in 2.4, so it is related).

tghosth commented 7 months ago

Hi @sohsatoh, thanks for the input and working to keep OWASP projects consistent :)

Instead of giving rules and limits for every used and accepted hashing function, we should abstract out the entire section with the message...

I agree with this concept @elarlang and I think we will need to trim 2.4 to make ASVS more usable.

However, I think that the denial of service risk will apply to all cases and password length is also a strong candidate for staying as a separate requirement (but I am open to discussing that separately).

As such, for now I would agree with Jim's suggestion.

# Description L1 L2 L3 CWE NIST §
2.1.2 [MODIFIED] Verify that passwords of at least 64 characters are permitted, and that passwords above a certain max length (such as 128 or more characters) are denied. 521 5.1.1.2

@elarlang @sohsatoh further thoughts?

elarlang commented 7 months ago

If it is not against the hashing method and denial of service attack, what risk do you mitigate with limiting "too long" passwords? (it may make sense to re-read #1772)

tghosth commented 7 months ago

Maybe I didn't explain myself clearly. I think all the approved hashing methods are necessarily slow and are therefore exposed to a DoS risk which is why this applies in all cases and why I think we should still mention it.

elarlang commented 7 months ago

The underlying reason to have max length for password is not to make the user-chosen password more secure and it's not a defense against user-impersonation vector as the entire section "V2.1 Password Security" is.

Maybe I didn't explain myself clearly. I think all the approved hashing methods are necessarily slow and are therefore exposed to a DoS risk which is why this applies in all cases and why I think we should still mention it.

And as you agree, the problem to solve is "the safe usage of hashing methods" and that is why I proposed (https://github.com/OWASP/ASVS/issues/1923#issuecomment-2053957053) to cover the problem in section 2.4.

jmanico commented 7 months ago

I would rather see this requirement here with the other password rules. I do not think the password storage section is the right place add a max-length password rule.

Sjord commented 7 months ago

I wrote something about the DoS risk: Long passwords don't cause denial of service when using proper hash functions.

The first step a password-hashing algorithm does is hashing the password and then processing that hash further. So only the first hashing step depends on the length of the password, and subsequent operations are performed on a fixed-length hash. The length of the password does not severely influence the execution time. As such, I think a password length upper limit is not needed.

"128 characters" does not seem to be based on any theoretical basis

I agree, this is just some number someone picked, and that doesn't make for a strong requirement. One possible solution is to adopt the limit of some other system, such as bcrypt (72 bytes) or WordPress (4096 bytes).

elarlang commented 7 months ago

I would rather see this requirement here with the other password rules. I do not think the password storage section is the right place add a max-length password rule.

I try to be patient... but well, I'm not.

@jmanico , please, FACTS and arguments!

First, why do we need this (max length for user-chosen passwords) requirement at all, and what risk it mitigates? Do we need this requirement? And if we need to, then we going to choose the correct section for that, based on the area which is impacted (password hashing functionality in this case).

Note: I'm ok to keep the requirement itself as part of the current requirement, the main priority is to clarify the need for that based on facts. At the moment it seems to be too random.

tghosth commented 7 months ago

Ok, having read back through the most recent comments, I agree that '"128 characters" does not seem to be based on any theoretical basis'. I also read back through #756 for a little more context.

I think the aim here was to provide a simple requirement. There is no reason in the world to allow a regular password of longer than 128 chars and maybe we could even shorten that. That isn't science it is just common sense. Maybe it would be even more sensible to restrict to 70 max to avoid the bcrypt issue. Again, not science, just trying to avoid footguns.

The alternative is that we reframe this requirement:

Verify that the application is protected against a denial of service attack caused by processing an overly long password.

Thoughts @Sjord @ThunderSon @jmanico @elarlang @sohsatoh

elarlang commented 7 months ago

There is no reason in the world to allow a regular password of longer than 128

It is not a reason to have a requirement in the ASVS.

.. and we reach close to my proposal:

Instead of giving rules and limits for every used and accepted hashing function, we should abstract out the entire section with the message (not a wording proposal!) "Verify that accepted method is used, it is used and configured according to the latest spec or recommendations, such as to have enough iterations and limitation to max input length against denial of service attacks".

Denial of Service attack is just one thing to address with max-length, with bcrypt the issue is truncation.

sohsatoh commented 7 months ago

Ensure that your application is protected against denial of service attacks caused by processing too long passwords.

I agree with removing the "128 character" limit and implementing this in itself.

There is no reason in the world to allow a regular password of longer than 128 chars and maybe we could even shorten that. That isn't science it is just common sense. Maybe it would be even more sensible to restrict to 70 max to avoid the bcrypt issue. Again, not science, just trying to avoid footguns.

I think you are right, as long as the password has sufficient entropy. As for bcrypt, I don't see the need to apply this across the board, since it is a restriction of the bcrypt hashing algorithm itself. (I think we can all agree on this)

tghosth commented 7 months ago

So I still think we should mention the denial of service issue separately as it applies to all algorithms. Algorithm specific details like bcrypt 72 bytes we should leave for the cheatsheet.

tghosth commented 7 months ago

Note that the denial of service from long password is not even mentioned in the password storage cheat sheet.

tghosth commented 7 months ago

Following discussion with Elar

tghosth commented 7 months ago

Note that I also fixed the missing tags on 2.4.2 and 2.4.5 as I think that practically speaking these requirements were not correct because the correct response would be to use the right algorithms and the provisions which they provided.

tghosth commented 7 months ago

Opened #1930

jmanico commented 7 months ago

https://www.sjoerdlangkemper.nl/2021/07/02/long-password-denial-of-service/ is a solid argument. I am ok with dropping this requirement.

tghosth commented 1 week ago

zombie-hand

tghosth commented 1 week ago

Elar pointed out that section 2.4 is now empty aside from the requirement created in this issue and that the requirement doesn't really fit anyway.

# Description L1 L2 L3 CWE
2.4.6 [ADDED, SPLIT FROM 2.1.2] Verify that the application is protected against a denial of service attack caused by processing an overly long password.

The background is that previously we mandated a maximum password length but didn't really explain why and in reality the reason is very algorithmic specific.

Given that the rest of this section has disappeared, I would recommend that we merge this back into the original requirement but in a clearer way as follows:

# Description L1 L2 L3 CWE
2.1.2 [MODIFIED] Verify that passwords of at least 64 characters are permitted but that the application is protected against a denial of service attack caused by processing an overly long password. 521

What do people think?

tghosth commented 1 week ago

@elarlang @jmanico @Sjord @sohsatoh

elarlang commented 1 week ago

I don't like this idea. The problem to solve is not "force users to choose strong passwords", but incorrect or not safe usage of hashing functions. It must be part of safe hashing function usage.

tghosth commented 1 week ago

You prefer V6?

elarlang commented 1 week ago

You prefer V6?

Yes, something to V6.6, because the defense relies in configuration of hashing functions. We need to make the requirement more abstract, and not to be password storage specific (which is wrong anyway).

Current 2.4.5:

Verify that the application is protected against a denial of service attack caused by processing an overly long password.

Point of view for V6.6 (not a wording proposal):

Verify that the application is protected against a denial of service attack caused by processing an overly long input for hashing functions (such as passwords), by limiting the input length via input validation for maximum length.

... or using configuration, or using configuration that takes it into account, or what are the other options to mitigate it

At the same time I would say, the merge to 6.6.2 should be (at least partially) reverted.

tghosth commented 1 week ago

I also think V6 would make sense as it fits well into 6.6.

I think we could then have a reference in V2.1 Password Security to this section and hopefully that would allay your concerns about 6.6.2:

V6.6.2 "passwords" > "end-user passwords" - for service password there is need to use key-vault solution. Maybe it is worth keeping it in a password storage section in V2? .... with 6.6.2 (password hashing) it is a bit the same situation like it was with 2.10.4, at the end it does not feel like pure crypto issue .... the point is - do not store password plain text, the solution - crypto .... it is not wrong to put it to V6, but I would like to find the requirement from V2

elarlang commented 1 week ago

V6.6.2 merge/split is a separate issue maybe, I mentioned it because it may happen, that something gets back to current V2.4 section.

jmanico commented 1 week ago

Perhaps a small change to 6.6.2?

6.6.2 | [MODIFIED, MOVED FROM 2.4.1, MERGED FROM 2.4.3, 2.4.4] Verify that passwords are stored using an approved, computationally intensive, hashing algorithm with parameter settings configured based on current guidance. The settings should balance security and performance to make brute-force and denial-of-service attacks more challenging. |   | ✓ | ✓

tghosth commented 1 week ago

Perhaps a small change to 6.6.2?

6.6.2 | [MODIFIED, MOVED FROM 2.4.1, MERGED FROM 2.4.3, 2.4.4] Verify that passwords are stored using an approved, computationally intensive, hashing algorithm with parameter settings configured based on current guidance. The settings should balance security and performance to make brute-force and denial-of-service attacks more challenging. | | ✓ | ✓

It would be nice to integrate it into a single requirement but I don't love the idea because I think that the parameter settings are a little different to the DoS problem which is more about how the hashing function is used.

I would suggest instead a modification of @elarlang's suggestion:

# Description L1 L2 L3 CWE
6.6.5 [ADDED, SPLIT FROM 2.1.2] Verify that, where computationally intensive hashing functions are used (such as with passwords), the application is protected against overly long input leading to a denial of service condition. This could be done by limiting input length or by performing an initial hash using a fast algorithm to reduce the size of the input to a standard length before it is passed to the computationally intensive function.
jmanico commented 3 days ago

I think pre-hashing should never be necessary if you use a modern password hashing algorithm. And from the recent research I have read, even very very long passwords do not cause a performance problem, that only shows up on "legacy" password cracking algorithms or bad implementations.

The main case study we see is the PBKDF2 issue from Django for performance. But that was really a implementation problem of the algorithm, not a developer problem.

So I suggest we just say, use a modern password hashing algorithm, suggest Argon2id in the notes somewhere, and call it a day.

Sjord commented 22 hours ago

Verify that the application is protected against a denial of service attack caused by processing an overly long input for hashing functions (such as passwords), by limiting the input length via input validation for maximum length.

Hashing functions are not special in a way that they are a particular risk for denial of service. Hashing functions are normally very fast, and the resources they take scale linearly with the input size. This makes it hard to abuse it for a denial of service: to increase the resources the server uses, the attacker must linearly increase their resources spent in the attack.

I think the general case would make a better requirement:

Verify that the application is protected against a denial of service attack caused by processing an overly long input by limiting the input length via input validation for maximum length.

However, that is already required in 5.1.4 and similar requirements.

elarlang commented 22 hours ago

If the DoS vector against hashing function is not widespread enough to have a separate requirement for highlighting it, we can also delete the current requirement with arguments - it is a problem for legacy solutions (be up to date).

In theory, there is also validation possible, but in the password context, it also means limiting the password max length. We require at least 64 64-character long password to be supported, I think it is good enough. One should not rely only on authentication for username and password (without MFA) anyway.

jmanico commented 19 hours ago

+1 Elar. This is where my research led me on this issue too. I think dropping the password hashing DOS vector is ok.

tghosth commented 19 hours ago

I just did some experimentation and using the cheat sheet recommendations for parameters, a million character password on my laptop takes less than 100ms.

I would also be inclined to drop this.