- crypt() fixes and sync with Solar Designer updates (rev. 295294, 295294, 295294, 295294, 295294, 295294)

6 files changed, 154 insertions, 53 deletions

diff --git a/ext/standard/config.m4 b/ext/standard/config.m4
index 93cb73e012..92982a94fc 100644
--- a/ext/standard/config.m4
+++ b/ext/standard/config.m4
@@ -273,7 +273,7 @@ if test "$ac_cv_crypt_blowfish" = "no" || test "$ac_cv_crypt_des" = "no" || test
   AC_DEFINE_UNQUOTED(PHP_STD_DES_CRYPT, 1, [Whether the system supports standard DES salt])
   AC_DEFINE_UNQUOTED(PHP_BLOWFISH_CRYPT, 1, [Whether the system supports BlowFish salt])
   AC_DEFINE_UNQUOTED(PHP_EXT_DES_CRYPT, 1, [Whether the system supports extended DES salt])
-  AC_DEFINE_UNQUOTED(PHP_MD5_CRYPT, 1, [Whether the system supports extended DES salt])
+  AC_DEFINE_UNQUOTED(PHP_MD5_CRYPT, 1, [Whether the system supports MD5 salt])
   AC_DEFINE_UNQUOTED(PHP_SHA512_CRYPT, 1, [Whether the system supports SHA512 salt])
   AC_DEFINE_UNQUOTED(PHP_SHA256_CRYPT, 1, [Whether the system supports SHA256 salt])
 
diff --git a/ext/standard/crypt.c b/ext/standard/crypt.c
index fddfa5fc84..5123d8ff4a 100644
--- a/ext/standard/crypt.c
+++ b/ext/standard/crypt.c
@@ -15,6 +15,7 @@
    | Authors: Stig Bakken <ssb@php.net>                                   |
    |          Zeev Suraski <zeev@zend.com>                                |
    |          Rasmus Lerdorf <rasmus@php.net>                             |
+   |          Pierre Joye <pierre@php.net>                                |
    +----------------------------------------------------------------------+
 */
 
@@ -146,7 +147,7 @@ PHP_FUNCTION(crypt)
 	char salt[PHP_MAX_SALT_LEN + 1];
 	char *str, *salt_in = NULL;
 	int str_len, salt_in_len = 0;
-
+	char *crypt_res;
 	salt[0] = salt[PHP_MAX_SALT_LEN] = '\0';
 
 	/* This will produce suitable results if people depend on DES-encryption
@@ -195,9 +196,13 @@ PHP_FUNCTION(crypt)
 			output = emalloc(needed * sizeof(char *));
 			salt[salt_in_len] = '\0';
 
-			php_sha512_crypt_r(str, salt, output, needed);
+			crypt_res = php_sha512_crypt_r(str, salt, output, needed);
+			if (!crypt_res) {
+				RETVAL_FALSE;
+			} else {
+				RETVAL_STRING(output, 1);
+			}
 
-			RETVAL_STRING(output, 1);
 			memset(output, 0, PHP_MAX_SALT_LEN + 1);
 			efree(output);
 		} else if (salt[0]=='$' && salt[1]=='5' && salt[2]=='$') {
@@ -209,9 +214,14 @@ PHP_FUNCTION(crypt)
 						+ strlen(salt) + 1 + 43 + 1);
 			output = emalloc(needed * sizeof(char *));
 			salt[salt_in_len] = '\0';
-			php_sha256_crypt_r(str, salt, output, needed);
 
-			RETVAL_STRING(output, 1);
+			crypt_res = php_sha256_crypt_r(str, salt, output, needed);
+			if (!crypt_res) {
+				RETVAL_FALSE;
+			} else {
+				RETVAL_STRING(output, 1);
+			}
+
 			memset(output, 0, PHP_MAX_SALT_LEN + 1);
 			efree(output);
 		} else if (
@@ -225,14 +235,25 @@ PHP_FUNCTION(crypt)
 			char output[PHP_MAX_SALT_LEN + 1];
 
 			memset(output, 0, PHP_MAX_SALT_LEN + 1);
-			php_crypt_blowfish_rn(str, salt, output, sizeof(output));
 
-			RETVAL_STRING(output, 1);
+			crypt_res = php_crypt_blowfish_rn(str, salt, output, sizeof(output));
+			if (!crypt_res) {
+				RETVAL_FALSE;
+			} else {
+				RETVAL_STRING(output, 1);
+			}
+
 			memset(output, 0, PHP_MAX_SALT_LEN + 1);
 		} else {
 			memset(&buffer, 0, sizeof(buffer));
 			_crypt_extended_init_r();
-			RETURN_STRING(_crypt_extended_r(str, salt, &buffer), 1);
+
+			crypt_res = _crypt_extended_r(str, salt, &buffer);
+			if (!crypt_res) {
+				RETURN_FALSE;
+			} else {
+				RETURN_STRING(crypt_res, 1);
+			}
 		}
 	}
 #else
@@ -247,8 +268,12 @@ PHP_FUNCTION(crypt)
 #  else
 #    error Data struct used by crypt_r() is unknown. Please report.
 #  endif
-
-		RETURN_STRING(crypt_r(str, salt, &buffer), 1);
+		crypt_res = crypt_r(str, salt, &buffer);
+		if (!crypt_res) {
+			RETURN_FALSE;
+		} else {
+			RETURN_STRING(crypt_res, 1);
+		}
 	}
 # endif
 #endif
diff --git a/ext/standard/crypt_blowfish.c b/ext/standard/crypt_blowfish.c
index b640a1081b..6f728ed795 100644
--- a/ext/standard/crypt_blowfish.c
+++ b/ext/standard/crypt_blowfish.c
@@ -606,6 +606,7 @@ char *php_crypt_blowfish_rn(__CONST char *key, __CONST char *setting,
 	    setting[3] != '$' ||
 	    setting[4] < '0' || setting[4] > '3' ||
 	    setting[5] < '0' || setting[5] > '9' ||
+	    (setting[4] == '3' && setting[5] > '1') ||
 	    setting[6] != '$') {
 		__set_errno(EINVAL);
 		return NULL;
diff --git a/ext/standard/crypt_freesec.c b/ext/standard/crypt_freesec.c
index 7ed277092d..49c397cca1 100644
--- a/ext/standard/crypt_freesec.c
+++ b/ext/standard/crypt_freesec.c
@@ -5,8 +5,9 @@
  * This version is derived from the original implementation of FreeSec
  * (release 1.1) by David Burren.  I've reviewed the changes made in
  * OpenBSD (as of 2.7) and modified the original code in a similar way
- * where applicable.  I've also made it reentrant and did a number of
- * other changes -- SD.
+ * where applicable.  I've also made it reentrant and made a number of
+ * other changes.
+ * - Solar Designer <solar at openwall.com>
  */
 
 /*
@@ -57,14 +58,17 @@
  * posted to the sci.crypt newsgroup by the author and is available for FTP.
  *
  * ARCHITECTURE ASSUMPTIONS:
- *	This code used to have some nasty ones, but I believe these have
- *	been removed by now.  The code isn't very portable and requires a
- *	32-bit integer type, though -- SD.
+ *	This code used to have some nasty ones, but these have been removed
+ *	by now.  The code requires a 32-bit integer type, though.
  */
 
 #include <sys/types.h>
 #include <string.h>
 
+#ifdef TEST
+#include <stdio.h>
+#endif
+
 #include "crypt_freesec.h"
 
 #define _PASSWORD_EFMT1 '_'
@@ -183,19 +187,28 @@ static uint32_t comp_maskl[8][128], comp_maskr[8][128];
 static inline int
 ascii_to_bin(char ch)
 {
-	if (ch > 'z')
-		return(0);
-	if (ch >= 'a')
-		return(ch - 'a' + 38);
-	if (ch > 'Z')
-		return(0);
-	if (ch >= 'A')
-		return(ch - 'A' + 12);
-	if (ch > '9')
-		return(0);
-	if (ch >= '.')
-		return(ch - '.');
-	return(0);
+	signed char sch = ch;
+	int retval;
+
+	retval = sch - '.';
+	if (sch >= 'A') {
+		retval = sch - ('A' - 12);
+		if (sch >= 'a')
+			retval = sch - ('a' - 38);
+	}
+	retval &= 0x3f;
+
+	return(retval);
+}
+
+/*
+ * When we choose to "support" invalid salts, nevertheless disallow those
+ * containing characters that would violate the passwd file format.
+ */
+static inline int
+ascii_is_unsafe(char ch)
+{
+	return !ch || ch == '\n' || ch == ':';
 }
 
 void
@@ -625,14 +638,24 @@ _crypt_extended_r(const char *key, const char *setting,
 	if (*setting == _PASSWORD_EFMT1) {
 		/*
 		 * "new"-style:
-		 *	setting - underscore, 4 bytes of count, 4 bytes of salt
+		 *	setting - underscore, 4 chars of count, 4 chars of salt
 		 *	key - unlimited characters
 		 */
-		for (i = 1, count = 0; i < 5; i++)
-			count |= ascii_to_bin(setting[i]) << (i - 1) * 6;
+		for (i = 1, count = 0; i < 5; i++) {
+			int value = ascii_to_bin(setting[i]);
+			if (ascii64[value] != setting[i])
+				return(NULL);
+			count |= value << (i - 1) * 6;
+		}
+		if (!count)
+			return(NULL);
 
-		for (i = 5, salt = 0; i < 9; i++)
-			salt |= ascii_to_bin(setting[i]) << (i - 5) * 6;
+		for (i = 5, salt = 0; i < 9; i++) {
+			int value = ascii_to_bin(setting[i]);
+			if (ascii64[value] != setting[i])
+				return(NULL);
+			salt |= value << (i - 5) * 6;
+		}
 
 		while (*key) {
 			/*
@@ -651,35 +674,25 @@ _crypt_extended_r(const char *key, const char *setting,
 			if (des_setkey((u_char *) keybuf, data))
 				return(NULL);
 		}
-		strncpy(data->output, setting, 9);
-		/*
-		 * Double check that we weren't given a short setting.
-		 * If we were, the above code will probably have created
-		 * wierd values for count and salt, but we don't really care.
-		 * Just make sure the output string doesn't have an extra
-		 * NUL in it.
-		 */
+		memcpy(data->output, setting, 9);
 		data->output[9] = '\0';
-		p = (u_char *) data->output + strlen(data->output);
+		p = (u_char *) data->output + 9;
 	} else {
 		/*
 		 * "old"-style:
-		 *	setting - 2 bytes of salt
+		 *	setting - 2 chars of salt
 		 *	key - up to 8 characters
 		 */
 		count = 25;
 
+		if (ascii_is_unsafe(setting[0]) || ascii_is_unsafe(setting[1]))
+			return(NULL);
+
 		salt = (ascii_to_bin(setting[1]) << 6)
 		     |  ascii_to_bin(setting[0]);
 
 		data->output[0] = setting[0];
-		/*
-		 * If the encrypted password that the salt was extracted from
-		 * is only 1 character long, the salt will be corrupted.  We
-		 * need to ensure that the output string doesn't have an extra
-		 * NUL in it!
-		 */
-		data->output[1] = setting[1] ? setting[1] : data->output[0];
+		data->output[1] = setting[1];
 		p = (u_char *) data->output + 2;
 	}
 	setup_salt(salt, data);
@@ -733,6 +746,7 @@ static struct {
 	char *hash;
 	char *pw;
 } tests[] = {
+/* "new"-style */
 	{"_J9..CCCCXBrJUJV154M", "U*U*U*U*"},
 	{"_J9..CCCCXUhOBTXzaiE", "U*U***U"},
 	{"_J9..CCCC4gQ.mB/PffM", "U*U***U*"},
@@ -745,6 +759,30 @@ static struct {
 	{"_J9..SDizxmRI1GjnQuE", "zxyDPWgydbQjgq"},
 	{"_K9..SaltNrQgIYUAeoY", "726 even"},
 	{"_J9..SDSD5YGyRCr4W4c", ""},
+/* "old"-style, valid salts */
+	{"CCNf8Sbh3HDfQ", "U*U*U*U*"},
+	{"CCX.K.MFy4Ois", "U*U***U"},
+	{"CC4rMpbg9AMZ.", "U*U***U*"},
+	{"XXxzOu6maQKqQ", "*U*U*U*U"},
+	{"SDbsugeBiC58A", ""},
+	{"./xZjzHv5vzVE", "password"},
+	{"0A2hXM1rXbYgo", "password"},
+	{"A9RXdR23Y.cY6", "password"},
+	{"ZziFATVXHo2.6", "password"},
+	{"zZDDIZ0NOlPzw", "password"},
+/* "old"-style, "reasonable" invalid salts, UFC-crypt behavior expected */
+	{"\001\002wyd0KZo65Jo", "password"},
+	{"a_C10Dk/ExaG.", "password"},
+	{"~\377.5OTsRVjwLo", "password"},
+/* The below are erroneous inputs, so NULL return is expected/required */
+	{"", ""}, /* no salt */
+	{" ", ""}, /* setting string is too short */
+	{"a:", ""}, /* unsafe character */
+	{"\na", ""}, /* unsafe character */
+	{"_/......", ""}, /* setting string is too short for its type */
+	{"_........", ""}, /* zero iteration count */
+	{"_/!......", ""}, /* invalid character in count */
+	{"_/......!", ""}, /* invalid character in salt */
 	{NULL}
 };
 
@@ -752,8 +790,12 @@ int main(void)
 {
 	int i;
 
-	for (i = 0; tests[i].hash; i++)
-	if (strcmp(crypt(tests[i].pw, tests[i].hash), tests[i].hash)) {
+	for (i = 0; tests[i].hash; i++) {
+		char *hash = crypt(tests[i].pw, tests[i].hash);
+		if (!hash && strlen(tests[i].hash) < 13)
+			continue; /* expected failure */
+		if (!strcmp(hash, tests[i].hash))
+			continue; /* expected success */
 		puts("FAILED");
 		return 1;
 	}
diff --git a/ext/standard/tests/strings/bug51059.phpt b/ext/standard/tests/strings/bug51059.phpt
new file mode 100644
index 0000000000..baf8a12c9c
--- /dev/null
+++ b/ext/standard/tests/strings/bug51059.phpt
@@ -0,0 +1,11 @@
+--TEST--
+Bug #51059 crypt() segfaults on certain salts
+--FILE--
+<?php
+
+if (crypt('a', '_') === FALSE) echo 'OK';
+else echo 'Not OK';
+
+?>
+--EXPECT--
+OK
diff --git a/ext/standard/tests/strings/crypt_blowfish_invalid_rounds.phpt b/ext/standard/tests/strings/crypt_blowfish_invalid_rounds.phpt
new file mode 100644
index 0000000000..6d40b0770e
--- /dev/null
+++ b/ext/standard/tests/strings/crypt_blowfish_invalid_rounds.phpt
@@ -0,0 +1,22 @@
+--TEST--
+Test Blowfish crypt() with invalid rounds
+--FILE--
+<?php
+
+foreach(range(32, 38) as $i) {
+  if (crypt('U*U', '$2a$'.$i.'$CCCCCCCCCCCCCCCCCCCCCC$') === FALSE) {
+    echo "$i. OK\n";
+  } else {
+    echo "$i. Not OK\n";
+  }
+}
+
+?>
+--EXPECT--
+32. OK
+33. OK
+34. OK
+35. OK
+36. OK
+37. OK
+38. OK