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

Passwords shouldn't be limited to 72 characters #13152

Closed
yyyyyyyan opened this issue Feb 26, 2020 · 12 comments
Closed

Passwords shouldn't be limited to 72 characters #13152

yyyyyyyan opened this issue Feb 26, 2020 · 12 comments

Comments

@yyyyyyyan
Copy link

Expected behaviour

Passwords shouldn't be limited to 72 characters. Yes, bcrypt has this limitation, but any serious platform solves this by using another hash function (usually SHA256) before passing it to bcrypt. It doesn't make it any less secure.

Actual behaviour

An error message appears if you try to set a password longer than 72 characters.

Steps to reproduce the problem

Try to set a password longer than 72 characters.

Specifications

@Gargron
Copy link
Member

Gargron commented Mar 5, 2020

Seems like a very marginal benefit to suit people who generate really long passwords over fiddling with Devise internals and upgrading millions of records.

@yyyyyyyan
Copy link
Author

yyyyyyyan commented Mar 5, 2020

You're wrong and can't seem to understand there's a whole world outside the US (or doesn't even know how bcrypt works).
Bcrypt has a limit of 72 bytes. Considering Mastodon encodes passwords using UTF-8 before passing it to bcrypt, and UTF-8 can use up to 4 bytes per character, depending on the characters someone choose for their password, password length limit can go down to 18 characters, which is ridiculous.
What's even more ridiculous is Mastodon doesn't understand the difference between bytes and characters, and will allow anyone to set a password like "áááááááááááááááááááááááááááááááááááááááááááááááááá", which has less than 72 characters (50), but more than 72 bytes (100). Of course bcrypt alone doesn't accept this, so the password is actually cut down to 72 bytes, without the user even knowing.
You clearly have a problem that needs to be solved. You can either change the validation to something bizarre that will only allow 18 characters in some cases, or you can do this right and pre-hash the passwords to sha-256 (and let everyone knows that you actually care a bit about security).

TL;DR: The 72 characters limit is only true to ASCII characters. Any non-ascii character will take up more than 1 byte with UTF-8, lowering the character-length limit

@danieljakots
Copy link
Contributor

danieljakots commented Mar 8, 2020

Please don't be rude.

@yyyyyyyan
Copy link
Author

@danieljakots sorry! English is not my first language and sometimes my words come out a bit harsher than it was supposed to be. I don't mean to be rude, sorry about that, I was just trying to be clear on why this issue is indeed important and cannot be ignored

@Gargron
Copy link
Member

Gargron commented Nov 4, 2020

My suggestion is to approach the Devise team about this issue and see what they say.

@Gargron Gargron closed this as completed Nov 4, 2020
@yyyyyyyan
Copy link
Author

Then you should do that, @Gargron, since it affects your users' security.

@ryliejamesthomas
Copy link

ryliejamesthomas commented Nov 5, 2020

Then you should do that, @Gargron, since it affects your users' security.

@yyyyyyyan I really don't think that's fair. You're the one who understands the issue, you're in the best position to remake it on their issue tracker.

@yyyyyyyan
Copy link
Author

@ryliejamesthomas I understand where you're coming from, but I really don't think that's fair to me. I work at least 8-10 hours a day, including weekends, I have my own open source projects to maintain, my own blog, my own life. Yes, I'd be glad to send an issue to a tool that's not used in any of the projects I maintain (and for all I know, not even in the projects I regularly contribute to), but I'm currently too busy for that, unfortunately.

So it's a bit strange to me when you say I'm the one being unfair. Unfair to who? I opened this issue in February. @Gargron made a comment in March and just left it there until yesterday. I'm being unfair because I expect the owner of a platform to care about its users' security?

Anyway, this is already taking too much of my time. If I get a free time someday maybe I'll open an issue to the Devise team, who knows. Good luck with Mastodon. I'm too tired for this.

@rugk
Copy link

rugk commented Nov 5, 2020

Creating this issue is faster than discussing 20 times here now who sho9uld create one. So I just did it: heartcombo/devise#5307 (I hope I choose the correct project.)
Only thing I encourage you is to follow/add details to that issue.

@Gargron
Copy link
Member

Gargron commented Nov 5, 2020

Thanks @rugk

@antja0
Copy link

antja0 commented Mar 23, 2023

I know it's 2 years late, but for anyone reading this:
I just received a security vulnerability report about not having a max password:

Hashing large data can cause significant resource consumption on behalf of the server expose the app for a DOS attack.

So please ensure to have at least some max limit. It can be 256 or 512 characters or something. 🙂

The security report was not related to mastodon

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

No branches or pull requests

7 participants
@Gargron @danieljakots @rugk @yyyyyyyan @ryliejamesthomas @antja0 and others