pajoye Mon, 22 Feb 2010 00:05:02 +0000
Revision: http://svn.php.net/viewvc?view=revision&revision=295340
Log:
- With "extended" hashes, detect and reject invalid "setting" strings.
With "traditional" hashes, support many "reasonable" invalid salts in
UFC-crypt compatible way, reject the rest of invalid salts. (Solar Designer)
Changed paths:
U php/php-src/branches/PHP_5_3/ext/standard/crypt_freesec.c
U php/php-src/trunk/ext/standard/crypt_freesec.c
Modified: php/php-src/branches/PHP_5_3/ext/standard/crypt_freesec.c
===================================================================
--- php/php-src/branches/PHP_5_3/ext/standard/crypt_freesec.c 2010-02-21 23:47:14 UTC (rev 295339)
+++ php/php-src/branches/PHP_5_3/ext/standard/crypt_freesec.c 2010-02-22 00:05:02 UTC (rev 295340)
@@ -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,21 +187,30 @@
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
_crypt_extended_init(void)
{
@@ -625,14 +638,24 @@
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 @@
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 @@
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 @@
{"_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 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;
}
Modified: php/php-src/trunk/ext/standard/crypt_freesec.c
===================================================================
--- php/php-src/trunk/ext/standard/crypt_freesec.c 2010-02-21 23:47:14 UTC (rev 295339)
+++ php/php-src/trunk/ext/standard/crypt_freesec.c 2010-02-22 00:05:02 UTC (rev 295340)
@@ -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,21 +187,30 @@
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
_crypt_extended_init(void)
{
@@ -625,14 +638,24 @@
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 @@
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 @@
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 @@
{"_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 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;
}
--
PHP CVS Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php
