diff --git a/NEWS b/NEWS index d688ef5aad39..785f9bcec40f 100644 --- a/NEWS +++ b/NEWS @@ -138,6 +138,9 @@ PHP NEWS while COW violation flag is still set). (alexandre-daubois) . Added form feed (\f) in the default trimmed characters of trim(), rtrim() and ltrim(). (Weilin Du) + . Fixed bug GH-21673 (the bundled bcrypt implementation no longer truncates + passwords at NUL bytes, and password_hash()/password_verify() no longer + reject them). (Weilin Du) . Invalid mode values now throw in array_filter() instead of being silently defaulted to 0. (Jorg Sowa) . Fixed bug GH-21058 (error_log() crashes with message_type 3 and diff --git a/ext/standard/crypt.c b/ext/standard/crypt.c index 54687f6cdf30..82fc39a36ff8 100644 --- a/ext/standard/crypt.c +++ b/ext/standard/crypt.c @@ -67,7 +67,7 @@ PHP_MSHUTDOWN_FUNCTION(crypt) /* {{{ */ } /* }}} */ -PHPAPI zend_string *php_crypt(const char *password, const int pass_len, const char *salt, int salt_len, bool quiet) +PHPAPI zend_string *php_crypt(const char *password, size_t pass_len, const char *salt, int salt_len, bool quiet) { char *crypt_res; zend_string *result; @@ -129,7 +129,7 @@ PHPAPI zend_string *php_crypt(const char *password, const int pass_len, const ch memset(output, 0, PHP_MAX_SALT_LEN + 1); - crypt_res = php_crypt_blowfish_rn(password, salt, output, sizeof(output)); + crypt_res = php_crypt_blowfish_rn(password, pass_len, salt, output, sizeof(output)); if (!crypt_res) { ZEND_SECURE_ZERO(output, PHP_MAX_SALT_LEN + 1); return NULL; @@ -220,7 +220,7 @@ PHP_FUNCTION(crypt) salt_in_len = MIN(PHP_MAX_SALT_LEN, salt_in_len); salt[salt_in_len] = '\0'; - if ((result = php_crypt(str, (int)str_len, salt, (int)salt_in_len, 0)) == NULL) { + if ((result = php_crypt(str, str_len, salt, (int)salt_in_len, 0)) == NULL) { if (salt[0] == '*' && salt[1] == '0') { RETURN_STRING("*1"); } else { diff --git a/ext/standard/crypt_blowfish.c b/ext/standard/crypt_blowfish.c index 351d40308089..8147f6086254 100644 --- a/ext/standard/crypt_blowfish.c +++ b/ext/standard/crypt_blowfish.c @@ -530,10 +530,10 @@ static void BF_swap(BF_word *x, int count) *(ptr - 1) = R; \ } while (ptr < &data.ctx.S[3][0xFF]); -static void BF_set_key(const char *key, BF_key expanded, BF_key initial, - unsigned char flags) +static void BF_set_key(const char *key, size_t key_len, BF_key expanded, + BF_key initial, unsigned char flags) { - const char *ptr = key; + size_t key_pos = 0; unsigned int bug, i, j; BF_word safety, sign, diff, tmp[2]; @@ -559,8 +559,7 @@ static void BF_set_key(const char *key, BF_key expanded, BF_key initial, * information - that is, we mostly use fixed-cost bitwise operations instead * of branches or table lookups. (One conditional branch based on password * length remains. It is not part of the bug aftermath, though, and is - * difficult and possibly unreasonable to avoid given the use of C strings by - * the caller, which results in similar timing leaks anyway.) + * difficult and possibly unreasonable to avoid here.) * * For actual implementation, we set an array index in the variable "bug" * (0 means no bug, 1 means sign extension bug emulation) and a flag in the @@ -577,25 +576,33 @@ static void BF_set_key(const char *key, BF_key expanded, BF_key initial, sign = diff = 0; + /* + * bcrypt cycles over the password bytes plus a trailing NUL terminator. + * The explicit length keeps embedded NUL bytes significant while + * preserving the historical behavior for ordinary C strings. + */ for (i = 0; i < BF_N + 2; i++) { tmp[0] = tmp[1] = 0; for (j = 0; j < 4; j++) { + unsigned char c = key_pos < key_len ? (unsigned char) key[key_pos] : 0; + tmp[0] <<= 8; - tmp[0] |= (unsigned char)*ptr; /* correct */ + tmp[0] |= c; /* correct */ tmp[1] <<= 8; - tmp[1] |= (BF_word_signed)(signed char)*ptr; /* bug */ + tmp[1] |= (BF_word_signed)(signed char)c; /* bug */ /* * Sign extension in the first char has no effect - nothing to overwrite yet, * and those extra 24 bits will be fully shifted out of the 32-bit word. For * chars 2, 3, 4 in each four-char block, we set bit 7 of "sign" if sign * extension in tmp[1] occurs. Once this flag is set, it remains set. */ - if (j) + if (j) { sign |= tmp[1] & 0x80; - if (!*ptr) - ptr = key; - else - ptr++; + } + key_pos++; + if (key_pos > key_len) { + key_pos = 0; + } } diff |= tmp[0] ^ tmp[1]; /* Non-zero on any differences */ @@ -636,7 +643,7 @@ static const unsigned char flags_by_subtype[26] = {2, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 4, 0}; -static char *BF_crypt(const char *key, const char *setting, +static char *BF_crypt(const char *key, size_t key_len, const char *setting, char *output, int size, BF_word min) { @@ -679,7 +686,7 @@ static char *BF_crypt(const char *key, const char *setting, } BF_swap(data.binary.salt, 4); - BF_set_key(key, data.expanded_key, data.ctx.P, + BF_set_key(key, key_len, data.expanded_key, data.ctx.P, flags_by_subtype[(unsigned int)(unsigned char)setting[2] - 'a']); memcpy(data.ctx.S, BF_init_state.S, sizeof(data.ctx.S)); @@ -800,10 +807,10 @@ static int _crypt_output_magic(const char *setting, char *output, int size) * The performance cost of this quick self-test is around 0.6% at the "$2a$08" * setting. */ -char *php_crypt_blowfish_rn(const char *key, const char *setting, - char *output, int size) +char *php_crypt_blowfish_rn(const char *key, size_t key_len, + const char *setting, char *output, int size) { - const char *test_key = "8b \xd0\xc1\xd2\xcf\xcc\xd8"; + static const char test_key[] = "8b \xd0\xc1\xd2\xcf\xcc\xd8"; const char *test_setting = "$2a$00$abcdefghijklmnopqrstuu"; static const char * const test_hashes[2] = {"i1D709vfamulimlGcq0qq3UvuUasvEa\0\x55", /* 'a', 'b', 'y' */ @@ -819,7 +826,7 @@ char *php_crypt_blowfish_rn(const char *key, const char *setting, /* Hash the supplied password */ _crypt_output_magic(setting, output, size); - retval = BF_crypt(key, setting, output, size, 16); + retval = BF_crypt(key, key_len, setting, output, size, 16); save_errno = errno; /* @@ -838,17 +845,17 @@ char *php_crypt_blowfish_rn(const char *key, const char *setting, } memset(buf.o, 0x55, sizeof(buf.o)); buf.o[sizeof(buf.o) - 1] = 0; - p = BF_crypt(test_key, buf.s, buf.o, sizeof(buf.o) - (1 + 1), 1); + p = BF_crypt(test_key, sizeof(test_key) - 1, buf.s, buf.o, sizeof(buf.o) - (1 + 1), 1); ok = (p == buf.o && !memcmp(p, buf.s, 7 + 22) && !memcmp(p + (7 + 22), test_hash, 31 + 1 + 1 + 1)); { - const char *k = "\xff\xa3" "34" "\xff\xff\xff\xa3" "345"; + static const char k[] = "\xff\xa3" "34" "\xff\xff\xff\xa3" "345"; BF_key ae, ai, ye, yi; - BF_set_key(k, ae, ai, 2); /* $2a$ */ - BF_set_key(k, ye, yi, 4); /* $2y$ */ + BF_set_key(k, sizeof(k) - 1, ae, ai, 2); /* $2a$ */ + BF_set_key(k, sizeof(k) - 1, ye, yi, 4); /* $2y$ */ ai[0] ^= 0x10000; /* undo the safety (for comparison) */ ok = ok && ai[0] == 0xdb9c59bc && ye[17] == 0x33343500 && !memcmp(ae, ye, sizeof(ae)) && diff --git a/ext/standard/crypt_blowfish.h b/ext/standard/crypt_blowfish.h index a1150b5f66c7..40aefc26a9dd 100644 --- a/ext/standard/crypt_blowfish.h +++ b/ext/standard/crypt_blowfish.h @@ -17,7 +17,9 @@ #ifndef _CRYPT_BLOWFISH_H #define _CRYPT_BLOWFISH_H -extern char *php_crypt_blowfish_rn(const char *key, const char *setting, - char *output, int size); +#include + +extern char *php_crypt_blowfish_rn(const char *key, size_t key_len, + const char *setting, char *output, int size); #endif diff --git a/ext/standard/password.c b/ext/standard/password.c index a8aab315657c..307a8deb28cf 100644 --- a/ext/standard/password.c +++ b/ext/standard/password.c @@ -153,7 +153,7 @@ static bool php_password_bcrypt_needs_rehash(const zend_string *hash, zend_array static bool php_password_bcrypt_verify(const zend_string *password, const zend_string *hash) { int status = 0; - zend_string *ret = php_crypt(ZSTR_VAL(password), (int)ZSTR_LEN(password), ZSTR_VAL(hash), (int)ZSTR_LEN(hash), 1); + zend_string *ret = php_crypt(ZSTR_VAL(password), ZSTR_LEN(password), ZSTR_VAL(hash), (int)ZSTR_LEN(hash), 1); if (!ret) { return false; @@ -181,11 +181,6 @@ static zend_string* php_password_bcrypt_hash(const zend_string *password, zend_a zval *zcost; zend_long cost = PHP_PASSWORD_BCRYPT_COST; - if (memchr(ZSTR_VAL(password), '\0', ZSTR_LEN(password))) { - zend_value_error("Bcrypt password must not contain null character"); - return NULL; - } - if (options && (zcost = zend_hash_str_find(options, "cost", sizeof("cost")-1)) != NULL) { cost = zval_get_long(zcost); } @@ -206,7 +201,7 @@ static zend_string* php_password_bcrypt_hash(const zend_string *password, zend_a zend_string_release_ex(salt, 0); /* This cast is safe, since both values are defined here in code and cannot overflow */ - result = php_crypt(ZSTR_VAL(password), (int)ZSTR_LEN(password), ZSTR_VAL(hash), (int)ZSTR_LEN(hash), 1); + result = php_crypt(ZSTR_VAL(password), ZSTR_LEN(password), ZSTR_VAL(hash), (int)ZSTR_LEN(hash), 1); zend_string_release_ex(hash, 0); if (!result) { diff --git a/ext/standard/php_crypt.h b/ext/standard/php_crypt.h index b7111dfd21b1..dd2574a18211 100644 --- a/ext/standard/php_crypt.h +++ b/ext/standard/php_crypt.h @@ -19,7 +19,9 @@ #ifndef PHP_CRYPT_H #define PHP_CRYPT_H -PHPAPI zend_string *php_crypt(const char *password, const int pass_len, const char *salt, int salt_len, bool quiet); +#include + +PHPAPI zend_string *php_crypt(const char *password, size_t pass_len, const char *salt, int salt_len, bool quiet); PHP_MINIT_FUNCTION(crypt); PHP_MSHUTDOWN_FUNCTION(crypt); PHP_RINIT_FUNCTION(crypt); diff --git a/ext/standard/tests/password/password_bcrypt_errors.phpt b/ext/standard/tests/password/password_bcrypt_errors.phpt index 5d823cba0217..10c3483f5a80 100644 --- a/ext/standard/tests/password/password_bcrypt_errors.phpt +++ b/ext/standard/tests/password/password_bcrypt_errors.phpt @@ -14,14 +14,7 @@ try { } catch (ValueError $exception) { echo $exception->getMessage() . "\n"; } - -try { - var_dump(password_hash("null\0password", PASSWORD_BCRYPT)); -} catch (ValueError $e) { - echo $e->getMessage(), "\n"; -} ?> --EXPECT-- Invalid bcrypt cost parameter specified: 3 Invalid bcrypt cost parameter specified: 32 -Bcrypt password must not contain null character diff --git a/ext/standard/tests/password/password_bcrypt_null_verify.phpt b/ext/standard/tests/password/password_bcrypt_null_verify.phpt new file mode 100644 index 000000000000..8c7a04e8db83 --- /dev/null +++ b/ext/standard/tests/password/password_bcrypt_null_verify.phpt @@ -0,0 +1,24 @@ +--TEST-- +password_* handles bcrypt passwords containing null bytes +--SKIPIF-- + +--FILE-- + +--EXPECT-- +bool(true) +bool(true) +bool(false) +bool(false) diff --git a/ext/standard/tests/strings/crypt_blowfish_null.phpt b/ext/standard/tests/strings/crypt_blowfish_null.phpt new file mode 100644 index 000000000000..a099834334d2 --- /dev/null +++ b/ext/standard/tests/strings/crypt_blowfish_null.phpt @@ -0,0 +1,22 @@ +--TEST-- +crypt() bcrypt distinguishes passwords containing null bytes +--SKIPIF-- + +--FILE-- + +--EXPECT-- +bool(true) +bool(false) +bool(false)