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

Update password max length to allow passwords up to 1024000 chars #24654

Closed
wants to merge 2 commits into from

Conversation

Wild1145
Copy link

The current limitations of 72 is exceptionally small. This should hopefully be more than enough for anyone to ever ever ask for.

The current limitations of 72 is exceptionally small. This should hopefully be more than enough for anyone to ever ever ask for.
@ThisIsMissEm
Copy link
Contributor

This wouldn't be wise to do as it'd open up the attack vector of a long password denial of service, as in order to compare or store a password it must be hashed, which takes CPU time; Request the hash of something that's large takes much more CPU time, so there's a risk of locking up the server by having it calculate arbitrarily long bcrypt hashes.

This is also noted by OWASP in their Authentication guides, which actually currently specify 64 as a suitable max-length on passwords: https://cheatsheetseries.owasp.org/cheatsheets/Authentication_Cheat_Sheet.html#implement-proper-password-strength-controls

In the case of Mastodon, it's using bcrypt, which has a maximum input length of 72 bytes for many implementations (source)

@ThisIsMissEm
Copy link
Contributor

Also, if you're thinking "bcrypt? why not Argon2??", well, that'd require storing which algorithm is used and migrating all those accounts to the new password algorithm piecemeal, something Devise doesn't support out of the box. (as far as I can tell devise only supports bcrypt for DatabaseAuthenticatable strategy).

tbh, we should be looking beyond Passwords and towards Passkeys which are far more secure, and don't require users to remember or store passwords (they're biometric/hardware based).

@Wild1145
Copy link
Author

Thanks for the detailed response, I hadn't appreciated that there was a technical limitation. It's a bit disappointing as 72 is still a very small number and less than I'd usually use for any of my production critical passwords (Most being 128 or even 256 chars) so having a way to increase this limit is something I'd suggest we should seriously look into.

I have looked somewhat into alternatives to passwords, and I think Microsoft are one of the ones doing multi-factor but not necessarily passwords well, I'd like to see a solid answer to the password problem either way personally.

I'm aware this is still likely to actually break due to BCrypt but this is the sort of value I'd like to get to ideally.
@tabletcorry
Copy link
Sponsor

72 characters is already well into "multiples of the lifetime of the universe" to crack/guess, especially since Mastodon is using a reasonably modern KDF. What are you looking to do by extending the character limit @Wild1145?

Agreed that Passkeys are the future, as user education and platforms improve.

@ThisIsMissEm
Copy link
Contributor

Here's an old gem (3 years since last change) that implements using argon2 for devise, however, what isn't handled is the rolling migration from bcrypt to argon2 — so just switching to something like this would result in every mastodon user having to reset their password: https://github.com/erdostom/devise-argon2

The correct way to change algorithms would be to store the existing algorithm (bcrypt) in a column, and then after a successful login to upgrade their password to the new algorithm, and to handle this upgrade on password change.

@nightpool
Copy link
Member

nightpool commented Apr 25, 2023

I'm pretty sure devise already stores an "algorithm identifier" in the password hash column, and even if it didn't there's no need for an extra column when you can just store it inline in the hash structure.

@nightpool
Copy link
Member

I'm closing this PR since it doesn't actually work (underlying hashing algorithm will throw an error). I don't think this is really useful or necessary but if you want to suggest a kdf change then that would be a separate PR anyway

@nightpool nightpool closed this Apr 25, 2023
@Wild1145 Wild1145 deleted the patch-1 branch May 1, 2023 13:30
@Wild1145
Copy link
Author

Wild1145 commented May 1, 2023

@tabletcorry To explain the requirement a bit more, If I have a shared user account (Service account) Enabling 2FA / similar becomes significantly more difficult, and options like Yubikeys are not viable with our current setup (as I remember anyway) so you move to a position where all the credentials are easier to phish / crack. For service accounts like this that are only used programatically I've always looked to use passwords with a minimum of 128 char passwords given I have a password manager that is capable of generating and managing such complex passwords.

Likewise as I said before on this thread and the discord, there for me is a larger element of PR around erroring on what appears to the average user to be an arbitrary password length limit, most sites that are modern and recently developed have no such limits, and password length limitations are often associated with poor password storage and management (Often in plain text in the DB)

@tabletcorry
Copy link
Sponsor

For multiple users logging in, it should be possible to attach multiple webauthn tokens (which can be yubikeys or virtual security keys supported on Mac/Windows/mobile/chrome). Then each user can meet the challenge with their personal/private phishing-proof token(s).

In my experience many "modern sites" allow arbitrary password lengths in the frontend and truncate them in the backend. There is no obligation or need to report this behavior to the user, so it is basically invisible.

Mastodon simply blocking in the frontend is a little more clear, but the security is functionally equivalent either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants