php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Doc Bug #66564 crypt() seems to silently discard input after a certain length
Submitted: 2014-01-24 00:06 UTC Modified: 2015-08-23 04:52 UTC
Votes:3
Avg. Score:3.7 ± 0.9
Reproduced:0 of 0 (0.0%)
From: ss23 at ss23 dot geek dot nz Assigned: googleguy (profile)
Status: Closed Package: Documentation problem
PHP Version: Irrelevant OS:
Private report: No CVE-ID: None
 [2014-01-24 00:06 UTC] ss23 at ss23 dot geek dot nz
Description:
------------
It seems there is a limit to the input length of the password/str parameter to crypt(), however this is not documented anywhere.

This has profound security implications, and all users should be aware of the issue.

My preference would be a warning/notice triggered when you exceed the length, as well as documentation on this.

Test script:
---------------
$long = str_repeat('a', 100);

var_dump(crypt($long . "1", '$2y$04$saltysaltysaltysaltytt'));
var_dump(crypt($long . "2", '$2y$04$saltysaltysaltysaltytt'));
var_dump(crypt($long . "12", '$2y$04$saltysaltysaltysaltytt'));

Expected result:
----------------
A different hash in each case

Actual result:
--------------
string(60) "$2y$04$saltysaltysaltysaltyte9usMwh4/IIx0al18sl5oEFVM2Z/XJ7q"
string(60) "$2y$04$saltysaltysaltysaltyte9usMwh4/IIx0al18sl5oEFVM2Z/XJ7q"
string(60) "$2y$04$saltysaltysaltysaltyte9usMwh4/IIx0al18sl5oEFVM2Z/XJ7q"

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2014-01-24 00:07 UTC] googleguy@php.net
-Assigned To: +Assigned To: googleguy
 [2014-01-24 00:07 UTC] googleguy@php.net
Will assign to myself for now.
 [2014-01-28 10:29 UTC] googleguy@php.net
It seems that input from the $str argument of crypt will truncate at exactly 73 characters when using CRYPT_BLOWFISH $salt. password_hash, is obviously also affected in the same way.

Tested on release bracnhes of 5.3.0 through 5.6.0alpha1 with 3v4l.org and independently on my own system.

Reproducible results for crypt:

http://3v4l.org/3icqi

Reproducible results for password_hash:

http://3v4l.org/GbOo4

Reference to php-src

http://lxr.php.net/xref/PHP_5_5/ext/standard/crypt_blowfish.c#819

Will update the documentation to reflect this limit more clearly in the documentation since it is defined behavior.
 [2014-01-28 13:10 UTC] googleguy@php.net
Automatic comment from SVN on behalf of googleguy
Revision: http://svn.php.net/viewvc/?view=revision&revision=332747
Log: Add cautionary statement about truncation for crypt and password_hash using BCRYPT. Fixes Bug #66564.

This includes a cautionary statement that the CRYPT_BLOWFISH algorithm in crypt/password_hash functions
will truncate the input string at a maxmimum length of 72 characters. Typically not a problem for the
average use case since this is only likely used for passwords and assuming each hash has a unique salt.
However, it's still a good idea to document this behavior so that users are aware of the side effect.
 [2014-01-28 13:14 UTC] googleguy@php.net
-Status: Assigned +Status: Closed
 [2014-01-28 13:14 UTC] googleguy@php.net
This bug has been fixed in the documentation's XML sources. Since the
online and downloadable versions of the documentation need some time
to get updated, we would like to ask you to be a bit patient.

Thank you for the report, and for helping us make our documentation better.


 [2014-02-22 16:22 UTC] ircmaxell@php.net
This is a double-edged sword.

On one hand, it should be documented. On the other, it shouldn't.

Why do I say that it shouldn't be documented? Mainly because you shouldn't do anything about it. Pretty much any technique that you use to "get around" this will actually open up significantly worse security issues.

For an in-depth analysis, check this post out: http://stackoverflow.com/questions/16594613/how-to-hash-long-passwords-72-characters-with-blowfish/16597402#16597402

So IMHO, it would be better for users to not know, so they would not be tempted to "fix it" (and make things far worse).

Practically, bcrypt is sufficiently strong for *all* passwords >= 72 characters. Could it be better? Absolutely. But it's not something people should "guess at" because they think it's weak. Instead, cryptographers should create a new algorithm (already being worked on) to replace bcrypt in the future. This will not happen for several years, but it's being worked on. In the mean time, bcrypt is plenty strong enough for even the most sensitive use-cases. It can be strengthened by encrypting after hashing as well.

So, considering that any attempt to "rectify" the 72 character limit will decrease security, the only way I think this should be documented would be *at most* a note with explanation that this is still secure and you shouldn't try to "work around it".

Under no circumstances should a warning or notice be triggered when the length is exceeded.

Also, you should **never** be using a static salt in the first place (except for testing), so in practice you should never see a duplicate hash. The use of the static salt is the security vulnerability...

As far as documenting it, I've thought about this for a long time, and I firmly believe that it should stay out of the documentation. There are a few reasons for this:

1. It will scare developers: it will make it seam like bcrypt is weaker than it is, when it's actually the best option at present. This may lead them to pick other, weaker algorithms instead.

2. It will lead developers to try to "work around it" themselves: they will try pre-hashing, or raising errors if long passwords are used. These will reduce the effective security of their implementation. 

So the best thing to do for security is to omit it. It's a hard line, but one that I think is important. It's less omitting information, and more preventing misleading information from being distributed.

My $0.02...
 [2014-02-23 14:00 UTC] mail at michalspacek dot cz
"This is only a concern if are using the same salt to hash strings with this algorithm that are over 72 bytes in length, as this will result in those hashes being identical."

This is slightly misleading. It sounds like all strings longer than 72 bytes will produce identical hashes, which is not true.

The crucial part here is that the leading 72 bytes must be identical as the password parameter is truncated as per the first sentence of the warning.

So it should be changed to something like:
"This is only a concern if you are using the same salt to hash strings with this algorithm that are over 72 bytes in length and have the first 72 bytes identical, as this will result in those hashes being identical, too."
 [2014-02-24 16:22 UTC] googleguy@php.net
@ircmaxell

You make a good point about misinterpretations in the documentation, but in retrospect people can misinterpret almost anything and we have very little control over how information is interpreted.

The one thing I do know is that people rely on the manual to find documented behavior. I, myself, have to rely on it because I can't always remember how everything works, off the top of my head. So I'm a strong believer in documenting what it does and not how to use it.

As for resolving your concerns over security, I propose we use your SO answer as a reference point for the security page in the manual and include a link to it from cryp/password_hash pages. It's informative and I think necessary to explain at length what's going on.
 [2014-02-24 16:22 UTC] googleguy@php.net
-Status: Closed +Status: Re-Opened
 [2014-02-24 16:24 UTC] googleguy@php.net
@ mail at michalspacek dot cz

Your concerns are valid and duly noted.

I will be updating this for clarity and bumping it down to a note instead of a caution in the param list.
 [2014-03-06 23:02 UTC] narf at devilix dot net
The currently used "Caution" block to describe this does indeed make it look like other options are better than BCrypt. And it's also kind of embarassing in the case of password_hash(), because it doesn't actually support anything else at this time.

IMO, a reference to a nice description of BCrypt would be a better option and if anything should be noted in a Caution block, it is DES for trimming passwords to 9 characters.
 [2014-06-27 15:00 UTC] tyrael@php.net
Automatic comment from SVN on behalf of tyrael
Revision: http://svn.php.net/viewvc/?view=revision&revision=333973
Log: removing the misleading sentence (reported by Solar Designer, also pointed out by Michal Špaček previously in a comment for #66564)
 [2014-07-17 22:14 UTC] ircmaxell@php.net
Automatic comment from SVN on behalf of ircmaxell
Revision: http://svn.php.net/viewvc/?view=revision&revision=334309
Log: Revert 334297 and 334297, as:

1) there was no discussion prior to edits (even in #66564)
2) It is incorrect, misleading and not the overall sentiment that needs to be communicated
 [2015-08-23 04:52 UTC] googleguy@php.net
-Status: Re-Opened +Status: Closed
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Fri Apr 19 19:01:28 2024 UTC