-
Notifications
You must be signed in to change notification settings - Fork 334
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
Throw error if password is too long #1120
Comments
Passwords need to have a maximum length, otherwise an attacker can send passwords with 10k chars and overload the server with hashing. However too long passwords should probably throw an error instead of quietly truncating. Code is here: https://github.com/LemmyNet/lemmy/blob/main/crates/api_common/src/utils.rs#L150 |
I became aware of the issue through this post: https://feddit.de/comment/130590 |
Please reconsider this decision. A single thread on my old Ryzen 3 CPU can already process 3 Gbit/s of data when creating a md5 hash. On a server with a bandwidth of 1 Gbit/s the hashing will never be the bottleneck. An attacker would not be able to send more data then the server CPU can process. Shortening passwords is a security thread and leads to obscure errors as we can see in this issue thread. The easiest and most secure way to fix this bug, is to get rid of the password length constraint. Just remove the "60" from line 308. Thank you in advance and thanks to all the Lemmy maintainers. Keep up the good work ❤️ |
This is not true. A randomly generated password of 20 characters or more, utilizing capital and lower case letters, numbers, and symbols, will take more time to crack than the current age of the universe. (it's actually better than that - I think something like 14 characters is sufficient, but I always advise people to just round up to 20) |
We already do throw an error. https://github.com/LemmyNet/lemmy/blob/15c84e2f7b5c82342d429547b060f848ba3962f2/crates/api_common/src/utils.rs#L307 Maybe this should be a lemmy-ui issue, to allow really long passwords in the input field rather than truncate them, so that the back end can correctly throw an error? |
The comments above about even 20 characters being more than enough, are correct, so there's no reason for us to increase this any more. |
I have faced this problem myself, however: what @dessalines said earlier about throwing an error does not happen when you configure an admin password via When I went into dev tools I noticed that the password sent via the web socket was only 60 characters. I did not notice any error during setup, the password was just being cut off and I didn't know about that. In my opinion 60 characters is too few, especially nowadays when a lot of people use password managers. I'm not saying there should not be a limit, but all my passwords in my password manager are >60 characters... |
There is absolutely no security benefit to having passwords longer than 60 chars. Still there should be proper error handling. |
Sorry but why be this opinionated? As far as i can see there is no reason (and there never should be) to set such a small limit. (Even regarding hash functions, 60 vs something like 200 chars is not going to matter on a big scale) Most password managers allow creating 128+ character passwords by default. |
If you have a lowercase alphanumeric password with 60 characters, there are 36 possible characters and 36^60 total possible passwords. TAssuming every single variant takes an attacker 0.001 seconds to try, then it will take more than 10^82 years to try every possible password. Whats the point in using a longer one? Just because its possible doesnt mean it makes any sense. |
Technology is constantly evolving, I believe it won't be long before 60 digit passwords will be an ease for any attack. Therefore, I would simply not limit it and leave the choice to each user. |
This isn't as much of an issue as you think, password hashes (or at least those that are correctly implemented, there was an issue like this for PBKDF2 in old versions of OpenSSL) only run a fast hash on the password once, then do subsequent iterations on the result of the previous iteration. In almost all cases an attacker will saturate your bandwidth before they saturate the blowfish hashing throughput.
20 characters is ok but not ideal. For random passwords with dense entropy it's fine (20 characters of fully random base64 has 120 bits of entropy), but for diceware passwords it could be only ~51 bits of entropy (assuming words with 5 characters each, 4 words with
This only makes sense if you don't understand how passwords are stored. Lemmy currently uses bcrypt, which outputs a 184 bit result. Any password containing more than 184 bits of entropy will necessarily be limited to 184 bits of entropy due to the hash output, so your 60 random characters of base64 with 360 bits of entropy don't help you beyond what 31 random characters of base64 would accomplish (and regardless, encryption keys commonly use 128 bits of entropy and aren't going to be cracked any time soon). Now the important part: 60 is fine, but if the limit is increased it should only be increased to 72 due to bcrypt's 72 character limit. Silent truncation to 72 characters is significantly worse for security than throwing an error for too-long passwords (and to be fair to the maintainer of Rust's bcrypt crate, they understand the issue but decided it's more important to maintain compatibility with other implementations). If you want to allow passwords longer than 72 characters, you could either use Argon2 or prehash the password before sending it to bcrypt, the common way to do this is to SHA256 hash it and then base64 encode it before bcrypt hashing the result (otherwise null bytes in the SHA256 output will truncate the password, which is super bad because many passwords will end up using only a few bytes of entropy). |
It's not quite that simple, since bcrypt's limit is 72 bytes and a character can span up to 4 bytes. |
Hello all,
I've read here that the password is limited to 60 show and the UI shortens the password if a longer one is entered, I think this is an absurdity.
So the user does not even know his correct password, besides there is no reason to limit passwords.
The text was updated successfully, but these errors were encountered: