Fix crypt_blowfish 8-bit chars problem (CVE-2011-2483), add tests

# See details at http://www.openwall.com/lists/announce/2011/06/21/1

diff --git a/NEWS b/NEWS
index 17e06ed4eebeadfa8672d173b0d0311a1c868cbf..0d06801f0a1d1c67a32c562c274b4c4c5be8be57 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -135,6 +135,7 @@ PHP                                                                        NEWS
   . number_format() no longer truncates multibyte decimal points and thousand
     separators to the first byte. FR #53457. (Adam)
   . Added hex2bin() function. (Scott)
+  . Fixed crypt_blowfish handling of 8-bit characters. (Stas) (CVE-2011-2483)
 
 - Improved CURL extension:
   . Added support for CURLOPT_MAX_RECV_SPEED_LARGE and

diff --git a/ext/standard/crypt.c b/ext/standard/crypt.c
index 65d83243d69a13230e6ab31363e144ec4b94ad14..03a080aa23325ea2cc4822a4406a1fa23c3db41a 100644 (file)
--- a/ext/standard/crypt.c
+++ b/ext/standard/crypt.c
@@ -240,7 +240,7 @@ PHP_FUNCTION(crypt)
                } else if (
                                salt[0] == '$' &&
                                salt[1] == '2' &&
-                               salt[2] == 'a' &&
+                           (salt[2] != 'a' && salt[2] != 'x') ||
                                salt[3] == '$' &&
                                salt[4] >= '0' && salt[4] <= '3' &&
                                salt[5] >= '0' && salt[5] <= '9' &&

diff --git a/ext/standard/crypt_blowfish.c b/ext/standard/crypt_blowfish.c
index 6c99a396d50065d990256e65090fdb911f46fd25..9e3a9241d0afc8012ccde6dbef5047fdf9d93792 100644 (file)
--- a/ext/standard/crypt_blowfish.c
+++ b/ext/standard/crypt_blowfish.c
@@ -1,5 +1,5 @@
 /*
-  $Id$ 
+  $Id$
 */
 /*
  * This code comes from John the Ripper password cracker, with reentrant
@@ -7,6 +7,7 @@
  * cracking removed.
  *
  * Written by Solar Designer <solar at openwall.com> in 1998-2002 and
+ * placed in the public domain.  Quick self-test added in 2011 and also
  * placed in the public domain.
  *
  * There's absolutely no warranty.
@@ -51,6 +52,13 @@
 #define __CONST __const
 #endif
 
+/*
+ * Please keep this enabled.  We really don't want incompatible hashes to be
+ * produced.  The performance cost of this quick self-test is around 0.6% at
+ * the "$2a$08" setting.
+ */
+#define BF_SELF_TEST
+
 #ifdef __i386__
 #define BF_ASM                         0
 #define BF_SCALE                       1
@@ -63,6 +71,7 @@
 #endif
 
 typedef unsigned int BF_word;
+typedef signed int BF_word_signed;
 
 /* Number of Blowfish rounds, this is also hardcoded into a few places */
 #define BF_N                           16
@@ -555,7 +564,7 @@ static void BF_swap(BF_word *x, int count)
        } while (ptr < &data.ctx.S[3][0xFF]);
 #endif
 
-static void BF_set_key(__CONST char *key, BF_key expanded, BF_key initial)
+static void BF_set_key(__CONST char *key, BF_key expanded, BF_key initial, int sign_extension_bug)
 {
        __CONST char *ptr = key;
        int i, j;
@@ -565,7 +574,10 @@ static void BF_set_key(__CONST char *key, BF_key expanded, BF_key initial)
                tmp = 0;
                for (j = 0; j < 4; j++) {
                        tmp <<= 8;
-                       tmp |= *ptr;
+                       if (sign_extension_bug)
+                               tmp |= (BF_word_signed)(signed char)*ptr;
+                       else
+                               tmp |= (unsigned char)*ptr;
 
                        if (!*ptr) ptr = key; else ptr++;
                }
@@ -575,8 +587,9 @@ static void BF_set_key(__CONST char *key, BF_key expanded, BF_key initial)
        }
 }
 
-char *php_crypt_blowfish_rn(__CONST char *key, __CONST char *setting,
-       char *output, int size)
+static char *BF_crypt(__CONST char *key, __CONST char *setting,
+       char *output, int size,
+       BF_word min)
 {
 #if BF_ASM
        extern void _BF_body_r(BF_ctx *ctx);
@@ -602,7 +615,7 @@ char *php_crypt_blowfish_rn(__CONST char *key, __CONST char *setting,
 
        if (setting[0] != '$' ||
            setting[1] != '2' ||
-           setting[2] != 'a' ||
+           (setting[2] != 'a' && setting[2] != 'x') ||
            setting[3] != '$' ||
            setting[4] < '0' || setting[4] > '3' ||
            setting[5] < '0' || setting[5] > '9' ||
@@ -613,7 +626,7 @@ char *php_crypt_blowfish_rn(__CONST char *key, __CONST char *setting,
        }
 
        count = (BF_word)1 << ((setting[4] - '0') * 10 + (setting[5] - '0'));
-       if (count < 16 || BF_decode(data.binary.salt, &setting[7], 16)) {
+       if (count < min || BF_decode(data.binary.salt, &setting[7], 16)) {
                clean(data.binary.salt, sizeof(data.binary.salt));
                __set_errno(EINVAL);
                return NULL;
@@ -621,7 +634,7 @@ char *php_crypt_blowfish_rn(__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, data.expanded_key, data.ctx.P, setting[2] == 'x');
 
        memcpy(data.ctx.S, BF_init_state.S, sizeof(data.ctx.S));
 
@@ -721,14 +734,59 @@ char *php_crypt_blowfish_rn(__CONST char *key, __CONST char *setting,
        BF_encode(&output[7 + 22], data.binary.output, 23);
        output[7 + 22 + 31] = '\0';
 
+ #ifndef BF_SELF_TEST
 /* Overwrite the most obvious sensitive data we have on the stack. Note
  * that this does not guarantee there's no sensitive data left on the
  * stack and/or in registers; I'm not aware of portable code that does. */
        clean(&data, sizeof(data));
+#endif
 
        return output;
 }
 
+char *php_crypt_blowfish_rn(__CONST char *key, __CONST char *setting,
+       char *output, int size)
+{
+#ifdef BF_SELF_TEST
+       __CONST char *test_key = "8b \xd0\xc1\xd2\xcf\xcc\xd8";
+       __CONST char *test_2a =
+           "$2a$00$abcdefghijklmnopqrstuui1D709vfamulimlGcq0qq3UvuUasvEa"
+           "\0"
+           "canary";
+       __CONST char *test_2x =
+           "$2x$00$abcdefghijklmnopqrstuuVUrPmXD6q/nVSSp7pNDhCR9071IfIRe"
+           "\0"
+           "canary";
+       __CONST char *test_hash, *p;
+       int ok;
+       char buf[7 + 22 + 31 + 1 + 6 + 1];
+
+       output = BF_crypt(key, setting, output, size, 16);
+
+/* Do a quick self-test.  This also happens to overwrite BF_crypt()'s data. */
+       test_hash = (setting[2] == 'x') ? test_2x : test_2a;
+       memcpy(buf, test_hash, sizeof(buf));
+       memset(buf, -1, sizeof(buf) - (6 + 1)); /* keep "canary" only */
+       p = BF_crypt(test_key, test_hash, buf, sizeof(buf) - 6, 1);
+
+       ok = (p == buf && !memcmp(p, test_hash, sizeof(buf)));
+
+/* This could reveal what hash type we were using last.  Unfortunately, we
+ * can't reliably clean the test_hash pointer. */
+       clean(&buf, sizeof(buf));
+
+       if (ok)
+               return output;
+
+/* Should not happen */
+       __set_errno(EINVAL); /* pretend we don't support this hash type */
+       return NULL;
+#else
+#warning Self-test is disabled, please enable
+       return BF_crypt(key, setting, output, size, 16);
+#endif
+}
+
 char *php_crypt_gensalt_blowfish_rn(unsigned long count,
        __CONST char *input, int size, char *output, int output_size)
 {

diff --git a/ext/standard/tests/strings/crypt_blowfish.phpt b/ext/standard/tests/strings/crypt_blowfish.phpt
new file mode 100644 (file)
index 0000000..cce09c1
--- /dev/null
+++ b/ext/standard/tests/strings/crypt_blowfish.phpt
@@ -0,0 +1,50 @@
+--TEST--
+Official blowfish tests (http://cvsweb.openwall.com/cgi/cvsweb.cgi/Owl/packages/glibc/crypt_blowfish/wrapper.c)
+--SKIPIF--
+<?php
+if (!function_exists('crypt') || !defined("CRYPT_BLOWFISH")) {
+    die("SKIP crypt()-blowfish is not available");
+}
+?>
+--FILE--
+<?php
+
+$tests =array(
+       array('$2a$05$CCCCCCCCCCCCCCCCCCCCC.E5YPO9kmyuRGyh0XouQYb4YMJKvyOeW', 'U*U'),
+       array('$2a$05$CCCCCCCCCCCCCCCCCCCCC.VGOzA784oUp/Z0DY336zx7pLYAy0lwK', 'U*U*'),
+       array('$2a$05$XXXXXXXXXXXXXXXXXXXXXOAcXxm9kjPGEMsLznoKqmqw7tc8WCx4a', 'U*U*U'),
+       array('$2a$05$abcdefghijklmnopqrstuu5s2v8.iXieOjg/.AySBTTZIIVFJeBui', '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789chars after 72 are ignored'),
+       array('$2x$05$/OK.fbVrR/bpIqNJ5ianF.CE5elHaaO4EbggVDjb8P19RukzXSM3e', "\xa3"),
+       array('$2a$05$/OK.fbVrR/bpIqNJ5ianF.Sa7shbm4.OzKpvFnX1pQLmQW96oUlCq', "\xa3"),
+       array('$2x$05$6bNw2HLQYeqHYyBfLMsv/OiwqTymGIGzFsA4hOTWebfehXHNprcAS', "\xd1\x91"),
+       array('$2x$05$6bNw2HLQYeqHYyBfLMsv/O9LIGgn8OMzuDoHfof8AQimSGfcSWxnS', "\xd0\xc1\xd2\xcf\xcc\xd8"),
+       array('$2a$05$/OK.fbVrR/bpIqNJ5ianF.swQOIzjOiJ9GHEPuhEkvqrUyvWhEMx6', "\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaa\xaachars after 72 are ignored as usual"),
+       array('$2a$05$/OK.fbVrR/bpIqNJ5ianF.R9xrDjiycxMbQE2bp.vgqlYpW5wx2yy', "\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55\xaa\x55"),
+       array('$2a$05$/OK.fbVrR/bpIqNJ5ianF.9tQZzcJfm3uj2NvJ/n5xkhpqLrMpWCe', "\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff\x55\xaa\xff"),
+        array('$2a$05$CCCCCCCCCCCCCCCCCCCCC.7uG0VCzI2bS7j6ymqJi9CdcdxiRTWNy', ''),
+
+);
+$i=0;
+foreach($tests as $test) {
+  if(crypt($test[1], $test[0]) == $test[0]) {
+    echo "$i. OK\n";
+  } else {
+    echo "$i. Not OK: $test[0] ".crypt($test[1], $test[0])."\n";
+  }
+  $i++;
+}
+
+?>
+--EXPECT--
+0. OK
+1. OK
+2. OK
+3. OK
+4. OK
+5. OK
+6. OK
+7. OK
+8. OK
+9. OK
+10. OK
+11. OK
\ No newline at end of file