Hello Andrey Andreev, >I'd like to open the discussion on a new password hashing algorithm >that I've been working on, named bcrypt-wrap. > >RFC link: https://wiki.php.net/rfc/password_bcrypt_wrap
I have 8 comments. ================= Begin of comments ================= ========= Comment 1 ========= The RFC states: >Bcrypt's 72-byte limit on passwords and issue with null bytes are >inviting many userspace workarounds, poor tradeoffs, security >vulnerabilities, UX issues, and considerable toil for developers. > >Multiple attempts to solve these problems have so far failed: Why not introduce a PASSWORD_BCRYPT_STRICT variant? Which: - asserts the password is valid utf-8. - asserts the password does not contain any ASCII-control-character between 0 upto and including 31. - asserts the password does not contain the ASCII-control-character 127. - asserts there are no unicode control characters present. Such as, but not limited to, U+202E Right-to-Left Override, U+2066 Left-to-Right Isolate, U+2069 Pop Directional Isolate, U+2060 Word Joiner, U+00A0 ( ) No-Break-Space, U+FEFF Byte Order Mark. - asserts the byte-length of the password is maximum 72 bytes. - (might) asserts the minimum byte-length of the password is 8 bytes. - claims the '$2z$' identifier. And leaves the '$2y$' identifier alone. - Explicitly does not change the PASSWORD_DEFAULT constant. - Let PASSWORD_BCRYPT (and therefore also PASSWORD_DEFAULT) generate deprecation warnings. - And remove the PASSWORD_DEFAULT constant after at least 5 Php versions. The PASSWORD_DEFAULT has no meaning, it should never been there. Explicit selection of the algorithm is better. ========= Comment 2 ========= Why is the password not validated to be human typable? Repeating comment 1: - asserts the password is valid utf-8. - asserts the password does not contain any ASCII-control-character between 0 upto and including 31. - asserts the password does not contain the ASCII-control-character 127. - asserts there are no unicode control characters present. Such as, but not limited to, U+202E Right-to-Left Override, U+2066 Left-to-Right Isolate, U+2069 Pop Directional Isolate, U+2060 Word Joiner, U+00A0 ( ) No-Break-Space, U+FEFF Byte Order Mark. ========= Comment 3 ========= The RFC states: >There is (not PHP-specific) consensus that a wrapping, >prehashed-bcrypt construct is the solution (see FAQ), but no >particular choice for the prehash primitive has emerged as the de >facto standard. Where is the prehashed-bcrypt consensus listed? Who consented? Did the original Bcrypt author consent? Did OpenBsd, the owner of the Bcrypt hashing algorithm, consent? The Bcrypt specification can be found at: https://www.openbsd.org/papers/bcrypt-paper.pdf ========= Comment 4 ========= Why is bcrypt-password-prehashing needed if Php also supports PASSWORD_ARGON2I and PASSWORD_ARGON2ID out of the box? This goes against OWASP recommendations, see https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#bcrypt >The bcrypt password hashing function should only be used for >password storage in legacy systems where Argon2 and scrypt are not >available. ========= Comment 5 ========= The RFC states: >Supports two prehash primitives, chosen through a prehash option: >cshake256 (default) and sha512 (using HMAC construct). Where is cshake256 defined? I did find SHAKE256 at https://nvlpubs.nist.gov/nistpubs/fips/nist.fips.202.pdf But a *c*SHAKE256 is nowhere found? ========= Comment 6 ========= The hmac-sha512 goes against OWASP recommendations, see https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#pre-hashing-passwords-with-bcrypt >To summarize if bcrypt has to be used and the password should to be >pre-hashed you should do >bcrypt(base64(hmac-sha384(data:$password, key:$pepper)), $salt, $cost) >and store the pepper not in the database. The RFC states: >HMAC-SHA512 output is truncated (yes, this is perfectly fine). The >salt is part of the key. ... and ... >prehash_salt = "bcrypt-wrap:" || version || ":" || salt_raw The prehash_salt used in this RFC, is what OWASP calls pepper. The prehash_salt is derived from the salt, and thus stored in the database. Against OWASP recommendations. Why is hmac-sha512 being proposed, if that goes *against OWASP* recommendations? ========= Comment 7 ========= The RFC states: >Whichever prehash function is chosen, 54 bytes of binary output are >B64-encoded to fill the 72-byte space allowed by Bcrypt. Why is bcrypt-prehashing needed below 73 password-bytes? Bcrypt can handle 72 bytes just fine. Also 54 binary output bytes of the prehash-function, would truncate the password space to 54 bytes. More than 54 password-bytes is going to introduce hash-collisions. Assuming the prehash function has no collisions within the 54 byte range. The Bcrypt algorithm truncates the password space beginning at 73 password-bytes to 72 bytes. So even the flawed Bcrypt algorithm is superior to this invented algorithm. Why is something being proposed, that deteriorates security? ========= Comment 8 ========= The RFC has no test-vectors. A re-implementation in e.g. C# can never validate its correctness. Please add test-vectors. Also include vectors that include smileys and other utf-8 symbols. Not just the ASCII range. ================= End of comments ================= When the cSHAKE256 algorithm is known, I will post more comments. Kind regards, Mirco Babin
