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

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

Open
sohsatoh opened this issue Apr 13, 2024 · 28 comments · Fixed by #1930
Open
Assignees
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet V2 V2.1 passwords Passwords, password storage related issues _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@sohsatoh
Copy link

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 OWASP/CheatSheetSeries#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
Copy link
Collaborator

elarlang commented Apr 14, 2024

Hi, @sohsatoh ,

we had a long discussion over it here: #1772

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

  • The requirement should be independent and "usable out of context" - no references to other requirements
  • The requirement must be positive (what must be done) not negative (what must not be done)

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

@elarlang elarlang added V2.1 passwords Passwords, password storage related issues V2 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet labels Apr 14, 2024
@elarlang elarlang changed the title 2.1.2 Passwords of more than 128 characters are denied 2.1.2 Passwords of more than 128 characters are denied (make entire 2.4 more abstract) Apr 14, 2024
@jmanico
Copy link
Member

jmanico commented Apr 15, 2024

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
Copy link
Member

jmanico commented Apr 15, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

tghosth commented Apr 16, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

tghosth commented Apr 16, 2024

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.

@tghosth tghosth added the _5.0 - prep This needs to be addressed to prepare 5.0 label Apr 16, 2024
@elarlang
Copy link
Collaborator

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 (#1923 (comment)) to cover the problem in section 2.4.

@elarlang elarlang added the next meeting Filter for leaders label Apr 16, 2024
@jmanico
Copy link
Member

jmanico commented Apr 17, 2024

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
Copy link
Contributor

Sjord commented Apr 17, 2024

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
Copy link
Collaborator

elarlang commented Apr 17, 2024

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
Copy link
Collaborator

tghosth commented Apr 17, 2024

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
Copy link
Collaborator

elarlang commented Apr 17, 2024

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
Copy link
Author

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
Copy link
Collaborator

tghosth commented Apr 17, 2024

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
Copy link
Collaborator

tghosth commented Apr 17, 2024

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

@tghosth
Copy link
Collaborator

tghosth commented Apr 17, 2024

Following discussion with Elar

  • Split 2.1.2 so that the "long password" requirement is in 2.4.
    • This requirement should be like: "Verify that the application is protected against a denial of service attack caused by processing an overly long password."
    • This requirement should be level 2
  • Collapse 2.4.1-2.4.7 down to a single requirement talking in general about approved storage algorithms.
  • Make sure the note at the start of 2.4 is up to date.

@tghosth
Copy link
Collaborator

tghosth commented Apr 17, 2024

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
Copy link
Collaborator

tghosth commented Apr 17, 2024

Opened #1930

@jmanico
Copy link
Member

jmanico commented Apr 17, 2024

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

@tghosth
Copy link
Collaborator

tghosth commented Nov 20, 2024

zombie-hand

@tghosth
Copy link
Collaborator

tghosth commented Nov 20, 2024

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
Copy link
Collaborator

tghosth commented Nov 20, 2024

@elarlang
Copy link
Collaborator

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
Copy link
Collaborator

tghosth commented Nov 20, 2024

You prefer V6?

@elarlang
Copy link
Collaborator

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
Copy link
Collaborator

tghosth commented Nov 20, 2024

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet V2 V2.1 passwords Passwords, password storage related issues _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants