Hi Anthony, On Sun, Jul 20, 2014 at 12:27 AM, Anthony Ferrara <ircmax...@gmail.com> wrote:
> > I'll suggest users to use SHA512 raw output as password to > > remove 72 chars limitation if it is needed. > > Then you either misunderstood what I was saying, or completely ignored it. > SHA512 raw output may truncate bytes from 72 to 64 in return of limitless password length. Do you suggest developers not to workaround 72 bytes limitation by themselves like this? SHA512 raw results could weaken password length longer than 64. However, allowed chars for passwords are limited almost always. It would not be an issue to worry much. > Raising E_NOTICE for too long password for PASSWORD_BCRYPT > > makes sense. I'll add it later. > > > > https://bugs.php.net/bug.php?id=67653 > > Please, don't change anything without first proposing an RFC so that > sufficient discussion can occur. Literally every single person in this > thread has disagreed with you, yet you want to go ahead and make > changes. It's not saying that you are wrong, but that discussion needs > to happen before any changes to security sensitive code and > documentation. > > Also, Deprecating crypt() without first discussing it (and having an > RFC to vote on) is not cool (and has been reverted): > > http://svn.php.net/viewvc/phpdoc/en/trunk/reference/strings/functions/crypt.xml?r1=333973&r2=334296 > > > > > Generally speaking, it would not be serious issue. 72 bytes constant > prefix > > would > > not be used most likely. > > Correct. So if it's not a serious issue, why are we talking about > adding serious warnings to documentation pages (big red boxes), and > notices and pre-hashing? > > > However, bug like this in "authentication" code must be detected and > > fixed. > > It's not a bug. It's by design. You could argue if it's a good design > or not, but it's very much by design. > I'm not saying that 72 bytes length limit is a blowfish bug. I'm saying it's a _user_ code bug that blindly accepts password exceeds its limit. > > > If password should be truncated, it should be truncated by app developers > > explicitly and > > notified users that their password had been truncated. IMHO. > > We disagree. So rather than just committing changes, why not draft up > an RFC with your suggestions, which can be discussed and voted upon? > I think you misunderstood me. What I'm going to change is adding E_NOTICE for password longer than 72 bytes. It's just a simple input validation. > > There's already a notice about this in the password_hash() docs, one > that > > almost looks like is designed to scare users, which is bad. Throwing an > > E_NOTICE will cause more problems than it would supposedly solve. > > I agree 100%. It's on my list of things to do to "clean up" that > warning, to word it to be less scary. And I agree 100% on the notice > side as well. > > Application developers should just state this limitation on their > > registration/password change pages, anything else is pointless. > > I don't think so. If the end user enters a 72 character+ password, the > chances of a collision because of the truncation are so small that > it's not worth worrying about. > I agree it would not be an serious issue. However, it's worth documenting, as what you don't want is people > inventing their own crypto by prefixing their own secret: > > password_hash(hash('sha512', SECRET_KEY) . $password, PASSWORD_DEFAULT) > > Which would then always produce the same hash (since the sha512 hash > is 128 bytes long, truncation would happen too early). > Good example. People do this kind of things. E_NOTICE error should help to prevent such code. > We should be educating people to avoid their own crypto. That was the > entire point of the password_hash APIs. Make it so simple that it's > difficult to screw up. Yet people will always find a way. The only way > to solve that problem is education. And adding big red boxes isn't > education. It's fear. I'm not closely following doc commit. It seems my commit was modified. http://jp2.php.net/manual/en/function.password-hash.php "Caution Using the PASSWORD_BCRYPT for the algo parameter, will result in the password parameter being truncated to a maximum length of 72 characters." This may be too much. I've added truncation spec in PASSWORD_BCRYPT constant explanation. I suppose it was better than this. (Besides grammar) Anyway, there are 2 things to resolve. - If we have E_NOTICE for password_hash() with PASSWORD_BCRYPT or not - How we document password_*() Your example password_hash(hash('sha512', SECRET_KEY) . $password, PASSWORD_DEFAULT) shows that we are better to have E_NOTICE for too long password. I'm OK with any documentation as long as it's precise. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net
