#1
  1. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,957
    Rep Power
    1046

    Warning: Several bugs in PHP Password Hashing extension and password_compat library


    Hi,

    both the PHP Password Hashing extension and the password_compat library from the same author have several bugs.

    Since many people (including myself) have recommended the extension or the library, I think you should be aware of those issues.

    Both the extension and the library do not handle binary salts correctly. Binary means: You haven't encoded them with the special Base64 variant used by crypt(). If you pass a binary salt through the $options parameter, you will always get a wrong hash. This can easily be verified with other bcrypt libraries like PHPass.

    For example, hashing the password “foo” with a salt consisting of 16 null bytes should yield the hash

    Code:
    $2y$10$......................0li5vIK0lccG/IXHAOP2wBncDW/oa2u
    Instead, you get

    Code:
    $2y$10$AAAAAAAAAAAAAAAAAAAAA.A6.6PO3Wv4OxvTWdUxMVpdLT2d5g/FG
    If you do encode the salt but make a mistake, the extension and the library will silently double-encode the salt, which reduces the effective length by ~1/4.

    Also note that the documentation incorrectly states that the functions will return false on error. They may also return null.

    The password_compat library has two additional bugs:

    It is incompatible with mbstring.func_overload. If you're using this feature together with a multibyte encoding, the functions won't be able to measure the length of strings. This causes them to fail and throw nonsensical error messages.

    If you're using open_basedir without /dev/urandom being included (like on a Windows system), and you neither have the OpenSSL nor the Mcrypt extension, you will get a warning for every call of password_hash().
    The 6 worst sins of securityHow to (properly) access a MySQL database with PHP

    Why can’t I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".
  2. #2
  3. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Jul 2013
    Posts
    9
    Rep Power
    0
    posted at the request of Anthony (ircmaxell):

    https://gist.github.com/ircmaxell/2c0e5484b1bf8475b3d5

    Not copied to show it's from him ...
  4. #3
  5. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,957
    Rep Power
    1046
    Hi Anthony,

    if the bugs broke the security of the library, I wouldn't have posted them in public. So, no, they don't. And I didn't claim they do.

    However, you are wrong about the effects of the bugs:

    The incorrect Base64 encoding affects the auto-generated salt as well. The only reason why we don't see it breaking the hash is because the current bcrypt implementation by Solar Designer happens to repair the last salt digit. This is pure luck. Without this internal correction, we would in fact get nonsensical hashes.

    So this is not just a problem of custom salts. In fact, the only way to circumvent the wrong encoding is to provide a custom Base64-encoded salt. In that case, it will be passed directly to crypt().

    But even if the bug only affected custom salts, how would that make things better? I mean, if you don't want the users to provide a custom salt at all, you shouldn't have this option in the first place. But providing it and then blaming people for using it is kind of weird.

    You claim that the mbstring.func_overload bug will only affect custom salts. This is wrong. Even a common encoding like UTF-16 will break both password_get_info() and password_needs_rehash() completely:

    PHP Code:
    <?php

    require_once 'password_compat/lib/password.php';

    mb_internal_encoding('utf-16');


    // a valid bcrypt salt with a cost factor of 10
    $hash '$2y$10$......................0li5vIK0lccG/IXHAOP2wBncDW/oa2u';

    $info password_get_info($hash);
    var_dump$info );            // this yields "unknown algorithm", which is incorrect

    // compare the hash with the exact same parameters
    $needs_rehash password_needs_rehash($hashPASSWORD_BCRYPT, array('cost' => 10));
    var_dump$needs_rehash );    // this yields true, which is incorrect
    Regarding the open_basedir warning, I don't really get why you make things so complicated. PHP throws a nonsensical warning, so we suppress it and maybe generate our own warning. That's it.

    I understand that the @ operator is often characterized as the ultimate evil, but this is nonsense. If used correctly, it's perfectly fine.

    In fact, I'm pretty sure this is the only valid solution in this case. You talk about writing a workaround for Windows, but this is not a Windows issue. For example, OpenBSD also has a different concept of random number generators. They do maintain /dev/urandom for legacy reasons, but as far as I know, they actually only have /dev/random. Sounds like some really ugly OS detection.

    And like I already said, some users may not have a chance to edit or remove the open_basedir setting. What are they supposed to do? Turn off the error reporting completely? Always write @password_hash() only to make the function shut up?
    Last edited by Jacques1; December 17th, 2013 at 06:25 PM.
    The 6 worst sins of securityHow to (properly) access a MySQL database with PHP

    Why can’t I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".
  6. #4
  7. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,957
    Rep Power
    1046
    Hi,

    short update: Most of the issues mentioned above are fixed now in the password_compat library. If you're using it, you should get the current version from GitHub.

    The changes in particular:

    • the salt encoding is fixed
    • the compatibility issues with the mbstring extension are fixed
    • no risk of a dangerous fallback to MD5-crypt
    • no open_basedir warnings on systems which do not have /dev/urandom
    The 6 worst sins of securityHow to (properly) access a MySQL database with PHP

    Why can’t I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".

IMN logo majestic logo threadwatch logo seochat tools logo