JFTR, the new crypt.c (and accompanying m4 code) is one big mess. It
works only because embedded crypt code is used every time. Which is
just reinventing wheel again and again.
So to list all those bugs I have noticed:
1. crypt_r is checked before -lcrypt is added to LIBS, so HAVE_CRYPT_R
is never defined
2. ac_cv_crypt_SHA{256,512} vs ac_cv_crypt_sha{256,512} ends with
PHP_SHA{256,512}_CRYPT is never defined.
3. if you don't have HAVE_CRYPT_R (or neither _REENTRANT nor
_THREAD_SAFE is defined) and PHP own implementation is not used ->
PHP(crypt) function does nothing at all, since call to crypt() was
dropped in 5.3.x.
4. Blowfish code accepts 00-03 and 32-39 as count, which is invalid
5. Some more minor glitches like calling extended DES init function
every time you call PHP(crypt). At least it's optimized inside this
function to be called just once. But I am not convinced that this
magic (allocating mutexes, etc.) is needed at all.
I am not going to report them individually, but as patch to use
internal implementation only for algorithms not present in system
crypt() function. But it needs a review.
Sean, Raphael, could you take a look at attached patch (also in our git).
It compiles fine (right now dpkg-buildpackage is stuck in unit tests),
and it's not very complicated, so I guess it should be OK, but since
it heavily modifies PHP(crypt) function, it would be better if it gets
a review by more eyes.
Ondrej
--
Ondřej Surý <ond...@sury.org>
http://blog.rfc1925.org/
--- a/ext/standard/config.m4
+++ b/ext/standard/config.m4
@@ -60,6 +60,12 @@ if test "$ac_cv_func_crypt" = "no"; then
AC_DEFINE(HAVE_CRYPT, 1, [ ])
])
fi
+
+AC_CHECK_FUNCS(crypt_r, [ php_crypt_r="1" ], [ php_crypt_r="0" ])
+if test "x$php_crypt_r" = "x1"; then
+ PHP_CRYPT_R_STYLE
+ AC_DEFINE(HAVE_CRYPT_R, 1, [ ])
+fi
AC_CACHE_CHECK(for standard DES crypt, ac_cv_crypt_des,[
AC_TRY_RUN([
@@ -231,11 +237,68 @@ main() {
ac_cv_crypt_SHA256=no
])])
+dnl
+dnl Define PHP_*_CRYPT according to system
+dnl
+
+if test "$ac_cv_crypt_des" = "yes"; then
+ ac_result=1
+ ac_crypt_des=1
+else
+ ac_result=0
+ ac_crypt_des=0
+fi
+AC_DEFINE_UNQUOTED(PHP_STD_DES_CRYPT, $ac_result, [Whether the system supports standard DES salt])
+
+if test "$ac_cv_crypt_md5" = "yes"; then
+ ac_result=1
+ ac_crypt_md5=1
+else
+ ac_result=0
+ ac_crypt_md5=0
+fi
+AC_DEFINE_UNQUOTED(PHP_MD5_CRYPT, $ac_result, [Whether the system supports MD5 salt])
+
+if test "$ac_cv_crypt_blowfish" = "yes"; then
+ ac_result=1
+ ac_crypt_blowfish=1
+else
+ ac_result=0
+ ac_crypt_blowfish=0
+fi
+AC_DEFINE_UNQUOTED(PHP_BLOWFISH_CRYPT, $ac_result, [Whether the system supports BlowFish salt])
+
+if test "$ac_cv_crypt_ext_des" = "yes"; then
+ ac_result=1
+ ac_crypt_edes=1
+else
+ ac_result=0
+ ac_crypt_edes=0
+fi
+AC_DEFINE_UNQUOTED(PHP_EXT_DES_CRYPT, $ac_result, [Whether the system supports extended DES salt])
+
+if test "$ac_cv_crypt_SHA512" = "yes"; then
+ ac_result=1
+ ac_crypt_sha512=1
+else
+ ac_result=0
+ ac_crypt_sha512=0
+fi
+AC_DEFINE_UNQUOTED(PHP_SHA512_CRYPT, $ac_result, [Whether the system supports SHA512 salt])
+
+if test "$ac_cv_crypt_SHA256" = "yes"; then
+ ac_result=1
+ ac_crypt_sha256=1
+else
+ ac_result=0
+ ac_crypt_sha256=0
+fi
+AC_DEFINE_UNQUOTED(PHP_SHA256_CRYPT, $ac_result, [Whether the system supports SHA256 salt])
dnl
dnl If one of them is missing, use our own implementation, portable code is then possible
dnl
-if test "$ac_cv_crypt_blowfish" = "no" || test "$ac_cv_crypt_des" = "no" || test "$ac_cv_crypt_ext_des" = "no" || test "x$php_crypt_r" = "x0"; then
+if test "$ac_cv_crypt_SHA512" = no || test "$ac_cv_crypt_SHA256" = "no" || test "$ac_cv_crypt_blowfish" = "no" || test "$ac_cv_crypt_des" = "no" || test "$ac_cv_crypt_md5" = "no" || test "$ac_cv_crypt_ext_des" = "no" || test "x$php_crypt_r" = "x0"; then
dnl
dnl Check for __alignof__ support in the compiler
@@ -268,66 +331,16 @@ if test "$ac_cv_crypt_blowfish" = "no" |
if test "$ac_cv_attribute_aligned" = "yes"; then
AC_DEFINE([HAVE_ATTRIBUTE_ALIGNED], 1, [whether the compiler supports __attribute__ ((__aligned__))])
fi
-
- AC_DEFINE_UNQUOTED(PHP_USE_PHP_CRYPT_R, 1, [Whether PHP has to use its own crypt_r for blowfish, des, ext des and md5])
- 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 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])
+ ac_result=1
PHP_ADD_SOURCES(PHP_EXT_DIR(standard), crypt_freesec.c crypt_blowfish.c crypt_sha512.c crypt_sha256.c php_crypt_r.c)
else
- if test "$ac_cv_crypt_des" = "yes"; then
- ac_result=1
- ac_crypt_des=1
- else
- ac_result=0
- ac_crypt_des=0
- fi
- AC_DEFINE_UNQUOTED(PHP_STD_DES_CRYPT, $ac_result, [Whether the system supports standard DES salt])
-
- if test "$ac_cv_crypt_blowfish" = "yes"; then
- ac_result=1
- ac_crypt_blowfish=1
- else
- ac_result=0
- ac_crypt_blowfish=0
- fi
- AC_DEFINE_UNQUOTED(PHP_BLOWFISH_CRYPT, $ac_result, [Whether the system supports BlowFish salt])
-
- if test "$ac_cv_crypt_ext_des" = "yes"; then
- ac_result=1
- ac_crypt_edes=1
- else
- ac_result=0
- ac_crypt_edes=0
- fi
- AC_DEFINE_UNQUOTED(PHP_EXT_DES_CRYPT, $ac_result, [Whether the system supports extended DES salt])
-
- if test "$ac_cv_crypt_sha512" = "yes"; then
- ac_result=1
- ac_crypt_sha512=1
- else
- ac_result=0
- ac_crypt_sha512=0
- fi
- AC_DEFINE_UNQUOTED(PHP_EXT_SHA512_CRYPT, $ac_result, [Whether the system supports SHA512 salt])
-
- if test "$ac_cv_crypt_sha256" = "yes"; then
- ac_result=1
- ac_crypt_sha256=1
- else
- ac_result=0
- ac_crypt_sha256=0
- fi
- AC_DEFINE_UNQUOTED(PHP_EXT_SHA256_CRYPT, $ac_result, [Whether the system supports SHA256 salt])
-
- AC_DEFINE_UNQUOTED(PHP_USE_PHP_CRYPT_R, 0, [Whether PHP has to use its own crypt_r for blowfish, des and ext des])
+ ac_result=0
fi
+AC_DEFINE_UNQUOTED(PHP_USE_PHP_CRYPT_R, $ac_result, [Whether PHP has to use its own crypt_r for blowfish, des and ext des])
+
dnl
dnl Check for available functions
dnl
--- a/ext/standard/crypt.c
+++ b/ext/standard/crypt.c
@@ -32,13 +32,12 @@
#ifdef PHP_USE_PHP_CRYPT_R
# include "php_crypt_r.h"
# include "crypt_freesec.h"
-#else
-# if HAVE_CRYPT_H
-# if defined(CRYPT_R_GNU_SOURCE) && !defined(_GNU_SOURCE)
-# define _GNU_SOURCE
-# endif
-# include <crypt.h>
+#endif
+#if HAVE_CRYPT_H
+# if defined(CRYPT_R_GNU_SOURCE) && !defined(_GNU_SOURCE)
+# define _GNU_SOURCE
# endif
+# include <crypt.h>
#endif
#if TM_IN_SYS_TIME
#include <sys/time.h>
@@ -64,39 +63,34 @@
* PHP_EXT_DES_CRYPT, PHP_MD5_CRYPT and PHP_BLOWFISH_CRYPT as appropriate
* for the target platform. */
-#if PHP_STD_DES_CRYPT
-#define PHP_MAX_SALT_LEN 2
+#if defined(HAVE_CRYPT_R) && (defined(_REENTRANT) || defined(_THREAD_SAFE))
+# define PHP_USE_SYSTEM_CRYPT_R
#endif
-#if PHP_EXT_DES_CRYPT
-#undef PHP_MAX_SALT_LEN
-#define PHP_MAX_SALT_LEN 9
-#endif
+#define PHP_MAX_STD_DES_SALT_LEN 2
+#define PHP_MAX_STD_DES_HASH_LEN 11
-#if PHP_MD5_CRYPT
-#undef PHP_MAX_SALT_LEN
-#define PHP_MAX_SALT_LEN 12
-#endif
+#define PHP_MAX_EXT_DES_SALT_LEN 9
+#define PHP_MAX_EXT_DES_HASH_LEN 11
-#if PHP_BLOWFISH_CRYPT
-#undef PHP_MAX_SALT_LEN
-#define PHP_MAX_SALT_LEN 60
-#endif
+#define PHP_MAX_MD5_SALT_LEN 12
+#define PHP_MAX_MD5_HASH_LEN 22
-#if PHP_SHA512_CRYPT
-#undef PHP_MAX_SALT_LEN
-#define PHP_MAX_SALT_LEN 123
-#endif
+#define PHP_MAX_BLOWFISH_SALT_LEN 29
+#define PHP_MAX_BLOWFISH_HASH_LEN 31
+#define PHP_MAX_SHA256_SALT_LEN 37
+#define PHP_MAX_SHA256_HASH_LEN 43
-/* If the configure-time checks fail, we provide DES.
- * XXX: This is a hack. Fix the real problem! */
+#define PHP_MAX_SHA512_SALT_LEN 37
+#define PHP_MAX_SHA512_HASH_LEN 86
-#ifndef PHP_MAX_SALT_LEN
-#define PHP_MAX_SALT_LEN 2
-#undef PHP_STD_DES_CRYPT
-#define PHP_STD_DES_CRYPT 1
-#endif
+/*
+ * Maximum salt length is from Blowfish
+ * Maximum hash length is from SHA512
+ */
+#define PHP_MAX_SALT_LEN 37
+#define PHP_MAX_HASH_LEN 86
#define PHP_CRYPT_RAND php_rand(TSRMLS_C)
@@ -119,15 +113,15 @@ PHP_MINIT_FUNCTION(crypt) /* {{{ */
}
/* }}} */
+#ifdef PHP_USE_PHP_CRYPT_R
PHP_MSHUTDOWN_FUNCTION(crypt) /* {{{ */
{
-#ifdef PHP_USE_PHP_CRYPT_R
php_shutdown_crypt_r();
-#endif
return SUCCESS;
}
/* }}} */
+#endif
static unsigned char itoa64[] = "./0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
@@ -145,158 +139,171 @@ static void php_to64(char *s, long v, in
PHP_FUNCTION(crypt)
{
char salt[PHP_MAX_SALT_LEN + 1];
+ int salt_len = 0;
+ char output[PHP_MAX_SALT_LEN + PHP_MAX_HASH_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';
+ char *crypt_res = NULL;
+#if PHP_USE_PHP_CRYPT_R
+ struct php_crypt_extended_data extended_buffer;
+#endif
+#if PHP_USE_SYSTEM_CRYPT_R
+# if defined(CRYPT_R_STRUCT_CRYPT_DATA)
+ struct crypt_data buffer;
+# elif defined(CRYPT_R_CRYPTD)
+ CRYPTD buffer;
+# else
+# error Data struct used by crypt_r() is unknown. Please report.
+# endif
+#endif
- /* This will produce suitable results if people depend on DES-encryption
- * available (passing always 2-character salt). At least for glibc6.1 */
- memset(&salt[1], '$', PHP_MAX_SALT_LEN - 1);
+ salt[0] = salt[PHP_MAX_SALT_LEN] = '\0';
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s|s", &str, &str_len, &salt_in, &salt_in_len) == FAILURE) {
return;
}
if (salt_in) {
- memcpy(salt, salt_in, MIN(PHP_MAX_SALT_LEN, salt_in_len));
- }
-
- /* The automatic salt generation covers standard DES, md5-crypt and Blowfish (simple) */
- if (!*salt) {
+ salt_len = MIN(PHP_MAX_SALT_LEN, salt_in_len);
+ memcpy(salt, salt_in, salt_len);
+ salt[salt_len] = '\0';
+ } else {
#if PHP_MD5_CRYPT
- strcpy(salt, "$1$");
+ salt[0] = '$'; salt[1] = '1'; salt[2] = '$';
php_to64(&salt[3], PHP_CRYPT_RAND, 4);
php_to64(&salt[7], PHP_CRYPT_RAND, 4);
- strcpy(&salt[11], "$");
+ salt[11] = '$'; salt[12] = '\0';
+ salt_len = PHP_MAX_MD5_SALT_LEN;
#elif PHP_STD_DES_CRYPT
php_to64(&salt[0], PHP_CRYPT_RAND, 2);
salt[2] = '\0';
+ salt_len = PHP_MAX_STD_DES_SALT_LEN;
#endif
- salt_in_len = strlen(salt);
}
/* Windows (win32/crypt) has a stripped down version of libxcrypt and
a CryptoApi md5_crypt implementation */
-#if PHP_USE_PHP_CRYPT_R
{
- struct php_crypt_extended_data buffer;
+#if PHP_USE_PHP_CRYPT_R
+ memset(&extended_buffer, 0, sizeof(extended_buffer));
+#endif
+#if PHP_USE_SYSTEM_CRYPT_R
+# if defined(CRYPT_R_STRUCT_CRYPT_DATA)
+ buffer->initialized = 0;
+# else
+ memset(&buffer, 0, sizeof(buffer));
+# endif
+#endif
if (salt[0]=='$' && salt[1]=='1' && salt[2]=='$') {
- char output[MD5_HASH_MAX_LEN];
-
- RETURN_STRING(php_md5_crypt_r(str, salt, output), 1);
+ /* CRYPT_MD5 */
+#if PHP_MD5_CRYPT
+# warning Using system MD5 crypt function, which is OK on Debian system
+# if PHP_USE_SYSTEM_CRYPT_R
+ crypt_res = crypt_r(str, salt, &buffer);
+# else
+ crypt_res = crypt(str, salt);
+# endif
+#elif PHP_USE_PHP_CRYPT_R
+# error Using PHP MD5 crypt function, should not happen on Debian system
+ crypt_res = php_md5_crypt_r(str, salt, output);
+#endif
} else if (salt[0]=='$' && salt[1]=='6' && salt[2]=='$') {
- const char sha512_salt_prefix[] = "$6$";
- const char sha512_rounds_prefix[] = "rounds=";
- char *output;
- int needed = (sizeof(sha512_salt_prefix) - 1
- + sizeof(sha512_rounds_prefix) + 9 + 1
- + strlen(salt) + 1 + 43 + 1);
- output = emalloc(needed * sizeof(char *));
- salt[salt_in_len] = '\0';
-
- crypt_res = php_sha512_crypt_r(str, salt, output, needed);
- if (!crypt_res) {
- if (salt[0]=='*' && salt[1]=='0') {
- RETVAL_STRING("*1", 1);
- } else {
- RETVAL_STRING("*0", 1);
- }
- } else {
- RETVAL_STRING(output, 1);
- }
-
- memset(output, 0, PHP_MAX_SALT_LEN + 1);
- efree(output);
+ /* CRYPT_SHA512 */
+#if PHP_SHA512_CRYPT
+# warning Using system SHA512 crypt function, which is OK on Debian system
+# if PHP_USE_SYSTEM_CRYPT_R
+ crypt_res = crypt_r(str, salt, &buffer);
+# else
+ crypt_res = crypt(str, salt);
+# endif
+#elif PHP_USE_PHP_CRYPT_R
+# error Using PHP SHA512 crypt function, should not happen on Debian system
+ crypt_res = php_sha512_crypt_r(str, salt, output, sizeof(output));
+#endif
} else if (salt[0]=='$' && salt[1]=='5' && salt[2]=='$') {
- const char sha256_salt_prefix[] = "$5$";
- const char sha256_rounds_prefix[] = "rounds=";
- char *output;
- int needed = (sizeof(sha256_salt_prefix) - 1
- + sizeof(sha256_rounds_prefix) + 9 + 1
- + strlen(salt) + 1 + 43 + 1);
- output = emalloc(needed * sizeof(char *));
- salt[salt_in_len] = '\0';
-
- crypt_res = php_sha256_crypt_r(str, salt, output, needed);
- if (!crypt_res) {
- if (salt[0]=='*' && salt[1]=='0') {
- RETVAL_STRING("*1", 1);
- } else {
- RETVAL_STRING("*0", 1);
- }
- } else {
- RETVAL_STRING(output, 1);
- }
-
- memset(output, 0, PHP_MAX_SALT_LEN + 1);
- efree(output);
+ /* CRYPT_SHA256 */
+#if PHP_SHA256_CRYPT
+# warning Using system SHA256 crypt function, which is OK on Debian system
+# if PHP_USE_SYSTEM_CRYPT_R
+ crypt_res = crypt_r(str, salt, &buffer);
+# else
+ crypt_res = crypt(str, salt);
+# endif
+#elif PHP_USE_PHP_CRYPT_R
+# error Using PHP SHA256 crypt function, should not happen on Debian system
+ crypt_res = php_sha256_crypt_r(str, salt, output, sizeof(output));
+#endif
} else if (
salt[0] == '$' &&
salt[1] == '2' &&
salt[2] == 'a' &&
salt[3] == '$' &&
- salt[4] >= '0' && salt[4] <= '3' &&
- salt[5] >= '0' && salt[5] <= '9' &&
- salt[6] == '$') {
- char output[PHP_MAX_SALT_LEN + 1];
-
- memset(output, 0, PHP_MAX_SALT_LEN + 1);
-
+ salt[6] == '$' &&
+ ((salt[4] == '0' &&
+ salt[5] >= '4' && salt[5] <= '9') ||
+ (salt[4] >= '1' && salt[4] <= '2' &&
+ salt[5] >= '0' && salt[5] <= '9') ||
+ (salt[4] == '3' &&
+ salt[5] >= '0' && salt[5] <= '1'))) {
+ /* CRYPT_BLOWFISH */
+#if PHP_BLOWFISH_CRYPT
+# error Using system BlowFish crypt function, should not happen on Debian system
+# if PHP_USE_SYSTEM_CRYPT_R
+ crypt_res = crypt_r(str, salt, &buffer);
+# else
+ crypt_res = crypt(str, salt);
+# endif
+#elif PHP_USE_PHP_CRYPT_R
+# warning Using PHP BlowFish crypt function, which is OK on Debian system
crypt_res = php_crypt_blowfish_rn(str, salt, output, sizeof(output));
- if (!crypt_res) {
- if (salt[0]=='*' && salt[1]=='0') {
- RETVAL_STRING("*1", 1);
- } else {
- RETVAL_STRING("*0", 1);
- }
- } else {
- RETVAL_STRING(output, 1);
- }
-
- memset(output, 0, PHP_MAX_SALT_LEN + 1);
+#endif
+ } else if (salt[0]=='_' &&
+ salt_len == 9) {
+ /* CRYPT_EXT_DES */
+#if PHP_EXT_DES_CRYPT
+# error Using system extended DES crypt function, should not happen on Debian system
+# if PHP_USE_SYSTEM_CRYPT_R
+ crypt_res = crypt_r(str, salt, &buffer);
+# else
+ crypt_res = crypt(str, salt);
+# endif
+#elif PHP_USE_PHP_CRYPT_R
+# warning Using PHP extended DES crypt function, which is OK on Debian system
+ _crypt_extended_init_r();
+ crypt_res = _crypt_extended_r(str, salt, &extended_buffer);
+#endif
} else {
- memset(&buffer, 0, sizeof(buffer));
+ /* CRYPT_STD_DES */
+#if PHP_STD_DES_CRYPT
+# warning Using system standard DES crypt function, which is OK on Debian system
+# if PHP_USE_SYSTEM_CRYPT_R
+ crypt_res = crypt_r(str, salt, &buffer);
+# else
+ crypt_res = crypt(str, salt);
+# endif
+#elif PHP_USE_PHP_CRYPT_R
+# error Using PHP standard DES crypt function, should not happen on Debian system
_crypt_extended_init_r();
+ crypt_res = _crypt_extended_r(str, salt, &extended_buffer);
+#endif
+ }
- crypt_res = _crypt_extended_r(str, salt, &buffer);
- if (!crypt_res) {
- if (salt[0]=='*' && salt[1]=='0') {
- RETURN_STRING("*1", 1);
- } else {
- RETURN_STRING("*0", 1);
- }
+ if (!crypt_res) {
+ if (salt[0]=='*' && salt[1]=='0') {
+ RETVAL_STRING("*1", 1);
} else {
- RETURN_STRING(crypt_res, 1);
+ RETVAL_STRING("*0", 1);
}
+ } else {
+ RETVAL_STRING(crypt_res, 1);
}
- }
-#else
-# if defined(HAVE_CRYPT_R) && (defined(_REENTRANT) || defined(_THREAD_SAFE))
- {
-# if defined(CRYPT_R_STRUCT_CRYPT_DATA)
- struct crypt_data buffer;
- memset(&buffer, 0, sizeof(buffer));
-# elif defined(CRYPT_R_CRYPTD)
- CRYPTD buffer;
-# else
-# error Data struct used by crypt_r() is unknown. Please report.
-# endif
- crypt_res = crypt_r(str, salt, &buffer);
- if (!crypt_res) {
- if (salt[0]=='*' && salt[1]=='0') {
- RETURN_STRING("*1", 1);
- } else {
- RETURN_STRING("*0", 1);
- }
- } else {
- RETURN_STRING(crypt_res, 1);
+ memset(salt, 0, sizeof(salt));
+ if (output[0]!='\0') {
+ memset(output, 0, sizeof(output));
}
}
-# endif
-#endif
}
/* }}} */
#endif
--- a/configure.in
+++ b/configure.in
@@ -671,11 +671,6 @@ PHP_TIME_R_TYPE
PHP_READDIR_R_TYPE
PHP_CHECK_IN_ADDR_T
-AC_CHECK_FUNCS(crypt_r, [ php_crypt_r="1" ], [ php_crypt_r="0" ])
-if test "x$php_crypt_r" = "x1"; then
- PHP_CRYPT_R_STYLE
-fi
-
dnl divert(4)
dnl ## In diversion 4 we check user-configurable general settings.
