-
Notifications
You must be signed in to change notification settings - Fork 57
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
Default hash length is twice what it needs to be #36
Comments
I thought it would be better to make the `check` method more portable by just getting the key length from the hash as opposed to always using a hard-coded `$_keyLength` property. As it is now, if someone decides to change that property, any hashes created prior to that will not verify correctly. Note: The `$isShadowed` variable is `false` on my server. I'm not sure if this would make a difference in calculating the string length in a way that would cause it to not get the right length to put in the hash function. Or maybe it's possible just to use `strlen` instead of `self::strlen`, in the lines I changed, but I am unable to test it.
That would be because it is in hex. The key length determines how long the actual hash will be (in binary), but since hex characters only represent 4 bits, it will double when converted into hex. |
@Parent5446 Thanks for the explanation of why it's twice as long. However since the first 32 hex characters are always the same no matter what the key length, anything after that is unnecessary. |
The hex output is only for connivance, given that the binary output would be preffered for most encoding schemes (see https://gitorious.org/scrypt/scrypt-unix-crypt) which uses the crypt(3) base64 encoding. That said, we can't change the output of the hex encoding, simply because it will break any existing hashes which have been made. The best I can suggest is I update the documentation to state the the key length is in bytes, thus the hex encoding will be twice the length. |
If you're talking about changing the type of encoding then I would agree that that can't be done, even if the crypt(3) base64 encoding would be preferred. However setting the key length to 16 wouldn't break current hashes if you truncated anything after 32 characters in the
(any time after the |
but is it even safe when larger keys directly start with the smaller key? I dont know how muhc the work differentiates between long and short keys but if shorter keys are easier to compute an attacker could use shorter keys for trying and when found a collision try with larger key lengths. |
@My1 The chances of a collision happening with scrypt is essentially zero, so you can basically assume that no two different strings will ever be hashed to produce the same ciphertext. Since the first 32 characters of the ciphertext is always the same when increasing the key length, whether the key length increases the hash time or not is irrelevant because you only ever have to compare the first 32 characters to validate the input text. To quote @DomBlack:
If hash difficulty is what you're after, the other parameters will affect this significantly more than the key length and do directly affect your hash's security. The point of this bug is that increasing key length to anything beyond 16 will unnecessarily use up extra storage space, and changing it to 16 will not break any current hashes as long as they are truncated to 32 characters in the verify function. And since the first 32 characters is the only part that affects security, truncating anything after that would not matter. |
well my point is that if creating an scrypt hash with just 10 characters would be easier to make with the same parameters (which should be made properly, obviously enough) then an attacker could instead bruteforce for those 10 first and check if he's gotten the pw, I am not even talking about collisions here, but especially with the simple passwords you can see in the world any vulnerability to a algorhythm used for password storage is bad. |
You can't create a hash with 10 characters. The minimum hash length is 32 characters, which is what is produced when using a key length of 16. |
so you cannot make scrypt shorter than usual, that's good because I tried to invert the logic of this issue and wanted to know whether that works which seems not to be the case, which is good. |
Just out of curiosity I just did a quick test and hashing a password using a key length of 16 took 0.3019 seconds and a key length of 2^16 took 0.3052 seconds. So key length appears to have a negligible effect hash calculation time anyway. |
wow a key length of 2^16 in such a short time that is not bad |
use $hash = pack("H*", $hash); |
The key length can safely be set to 16 instead of 32 because the string length of the hash returned by
scrypt()
for some reason is always doubled. So if you put 16 in, you get a 32 character hash, if you put 32 in you get 64 characters being produced. Since only the first 32 characters of the hash are ever used, making it longer than this is unnecessary.Produces the output:
The text was updated successfully, but these errors were encountered: