Fix the implementation of BlowFish to solve bugs concerning NUL bytes truncating strings

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

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;
@@ -206,8 +206,8 @@ PHP_FUNCTION(crypt)
 	zend_string *result;
 
 	ZEND_PARSE_PARAMETERS_START(2, 2)
-		Z_PARAM_STRING(str, str_len)
-		Z_PARAM_STRING(salt_in, salt_in_len)
+		Z_PARAM_PATH(str, str_len)
+		Z_PARAM_PATH(salt_in, salt_in_len)
 	ZEND_PARSE_PARAMETERS_END();
 
 	salt[0] = salt[PHP_MAX_SALT_LEN] = '\0';
@@ -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 {

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)) &&

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 <stddef.h>
+
+extern char *php_crypt_blowfish_rn(const char *key, size_t key_len,
+	const char *setting, char *output, int size);
 
 #endif

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) {

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 <stddef.h>
+
+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);

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

ext/standard/tests/password/password_bcrypt_null_verify.phpt

@@ -0,0 +1,25 @@
+--TEST--
+password_* handles bcrypt passwords containing null bytes
+--SKIPIF--
+<?php
+$password = "foo\0bar";
+$hash = password_hash($password, PASSWORD_BCRYPT);
+if (!is_string($hash) || !password_verify($password, $hash) || password_verify("foo", $hash)) {
+    die("skip bcrypt backend truncates NUL bytes");
+}
+?>
+--FILE--
+<?php
+$password = "foo\0bar";
+$hash = password_hash($password, PASSWORD_BCRYPT);
+
+var_dump($hash !== false);
+var_dump(password_verify($password, $hash));
+var_dump(password_verify("foo", $hash));
+var_dump(password_verify("foo\0baz", $hash));
+?>
+--EXPECT--
+bool(true)
+bool(true)
+bool(false)
+bool(false)

ext/standard/tests/strings/bug62443.phpt

@@ -2,9 +2,18 @@
 Bug #62443 Crypt SHA256/512 Segfaults With Malformed Salt
 --FILE--
 <?php
-crypt("foo", '$5$'.chr(0).'abc');
-crypt("foo", '$6$'.chr(0).'abc');
-echo "OK!";
+try {
+    crypt("foo", '$5$'.chr(0).'abc');
+} catch (ValueError $e) {
+    echo $e->getMessage(), "\n";
+}
+
+try {
+    crypt("foo", '$6$'.chr(0).'abc');
+} catch (ValueError $e) {
+    echo $e->getMessage(), "\n";
+}
 ?>
 --EXPECT--
-OK!
+crypt(): Argument #2 ($salt) must not contain any null bytes
+crypt(): Argument #2 ($salt) must not contain any null bytes

ext/standard/tests/strings/crypt_nul_bytes.phpt

@@ -0,0 +1,19 @@
+--TEST--
+crypt() rejects passwords and salts containing null bytes
+--FILE--
+<?php
+try {
+    crypt("foo\0bar", '$2y$05$CCCCCCCCCCCCCCCCCCCCC.');
+} catch (ValueError $e) {
+    echo $e->getMessage(), "\n";
+}
+
+try {
+    crypt("foo", '$2y$05$CCCCCCCCCCC' . "\0" . 'CCCCCCC.');
+} catch (ValueError $e) {
+    echo $e->getMessage(), "\n";
+}
+?>
+--EXPECT--
+crypt(): Argument #1 ($string) must not contain any null bytes
+crypt(): Argument #2 ($salt) must not contain any null bytes