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

diff --git a/ext/standard/crypt_freesec.c b/ext/standard/crypt_freesec.c
index 7ed277092dfcc9f3f42eb7d335be7635aa6ba458..49c397cca1a96be9ee002e9a0899ae4bdd1c661a 100644 (file)
--- 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;
        }