Security Review - password_hash implementation for PHP

Question

I'm currently working on a "helper function" for PHP's core to make password hashing more secure and easier for the majority of developers. Basically, the goal is to make it so easy, that it's harder to invent your own implementation than to use the core secure one. It's also designed to be updated and extended in the future as hashing methods improve.

I've written an RFC for the addition, and have an implementation of the functions as well.

The basic premise is that crypt is too difficult to directly use correctly for the majority of developers. The weird error returns, the base64-like (but using a different alphabet) salts, etc. So these functions are designed to take that guesswork out, and provide a dirt simple API.

string password_hash(string $password, string $algo = PASSWORD_DEFAULT, array $options = array())
bool password_verify(string $password, string $hash)
string password_make_salt(int $length, bool $raw_output = false)

Password_Hash takes in a password, an optional algorithm specifier (right now, only the improved CRYPT_BCRYPT implementation is supported, but would like to add scrypt as an option later), and a options array. The options array can specify the cost parameter to bcrypt, as well as a predefined salt value.

Password_Verify accepts a password, and an existing hash. It then re-hashes the password (identical to $tmp = crypt($password, $hash)). Then, it uses a constant-time comparison function to determine if the two hashes are indeed equal.

Password_Make_Salt exists to generate a random string of the given length. If raw_output is set to false (default), the outputted "salt" will be base64 encoded in a way that's directly compatible with crypt(). If raw_output is true, it will return the same length string using random full-bytes (0-255).

Example:

$hash = password_hash("foo");
if (password_verify("foo", $hash)) {
    // always should be true
} else {
}

A note on reading PHP's source code: PHP functions (those exposed to php code) are surrounded by the PHP_FUNCTION() macro. Additionally, php variables (zval's) are used in a few places. The macros that access parts of them are

• Z_TYPE_P() (finding the type of a pointer to a zval)
• Z_STRVAL_P() (get the pointer to the string value)
• Z_STRLEN_P() (get the int length of the string type)
• Z_LVAL_P() (get the long value of the integer type)

Additionally, zval_ptr_dtor() is a refcount mechanism to decrease the refcount on a zval, and clean it up if it hits 0.

I'm looking for a review of the implementation by at least a few security experts prior to officially proposing the change. It's reasonably short (only about 300 lines of code)...

Update

The API has been approved, so I've set up a Pull Request for the functionality. Please review it if you have time. Thanks!: https://github.com/php/php-src/pull/191

Answer 1

In C, I usually recommend that you should be consistently using size_t to store all length values, not int or long. Using int and long exposes you to integer signed/unsigned bugs. Reference: INT01-C from the CERT C Secure Coding Standard.

However, there is a complication in this case, due to the way the PHP native code interface works:

• When you read an integer using zend_parse_parameters(), PHP expects you to store it in a long. Therefore, you should pass zend_parse_parameters() a pointer to a long, then convert the long to a size_t carefully: validate that the number is non-negative and falls within the range for size_t (l >= 0 && l < SIZE_MAX), then cast it to a size_t and use a size_t thereafter.

• When you read a string using zend_parse_parameters(), PHP expects you to pass a buffer to store the string and a pointer to an int to store the string length. Therefore, you should pass zend_parse_parameters() what it is expecting. Then, carefully convert the int to a size_t: validate that the length is non-negative and falls within the range for size_t (len >= 0 && len < SIZE_MAX), then cast it to a size_t and use a size_t thereafter.

  Similarly, Z_STRLEN_PP() returns an int (I think) so you'll probably want to do the same for it.

(Beware of passing a long as a length to memcpy(); I don't know for sure, but that code smells pretty dubious to me and of high risk for an integer casting/truncation bug.)

buffer = php_base64_encode((unsigned char*) str, str_len, NULL);
for (pos = 0; pos < out_len; pos++) {
    if (buffer[pos] == '+') {
         ...
    } else {
         ret[pos] = buffer[pos];

That looks like it could read past the end of buffer to me. I think you need an additional check to make sure that pos is smaller than the length of buffer.

Answer 2

Very nice initiative!

Quick comments
(I will read the RFC in more detail later.)

'salts only need to be unique in a system' is a faulty assumption; please also read: this answer on salting

I do not see what the need is to manually provide a salt for the BCrypt algorithm. As far as I can see, it only opens up an option for people to make unnecessary mistakes.

password_make_salt seems to be more related to the crypt_ family of functions; maybe rename it crypt_make_salt instead, as to avoid confusion.

the first Basic usage example promotes the use of a constant value 'usesomesillystringfor', many developers will make the mistake of using some sort of constant value instead of a proper (random) salt.

The password_make_salt should use the best possible random source, cryptographically safe if at all possible (I'm not sure about the php_win32_get_random_bytes() quality, the /dev/urandom is good)

the user_needs_rehash.php example is not very readable; could use a cleanup.

the specify_salt.php example suggests that you can supply random bytes as a valid salt value for the BCrypt algorithm. This contradicts the requirement of the custom base64 alphabet for the BCrypt.

Answer 3

From an hashing perspective, using Bcrypt is one of the most recommended techniques these days, so it’s good.

Letting the user choose to use another algorithm (such as Scrypt, and maybe their own implementation later?) would also be a great option: By default (and for non-expert PHP developers), they’ll just have to use the default BCrypt, and if they want to, they’ll be able to adapt for their needs.