From bfe6f9e66a65d7c40fd486249097f932e2b237c3 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Fri, 20 Sep 2019 16:42:47 +0100 Subject: [PATCH] Introduction of timing attack safe bcmp implementation. Nothing new but to refactor usage b/w hash and password extensions but using volatile pointers to be a bit safer, allowing to expand its usage eventually. --- configure.ac | 2 +- ext/hash/hash.c | 13 +------------ ext/standard/password.c | 7 ++----- main/php.h | 4 ++++ main/safe_bcmp.c | 43 +++++++++++++++++++++++++++++++++++++++++ win32/build/config.w32 | 2 +- 6 files changed, 52 insertions(+), 19 deletions(-) create mode 100644 main/safe_bcmp.c diff --git a/configure.ac b/configure.ac index 73c7edeae3d..2dfb06dd023 100644 --- a/configure.ac +++ b/configure.ac @@ -1616,7 +1616,7 @@ PHP_ADD_SOURCES(main, main.c snprintf.c spprintf.c \ php_ini_builder.c \ php_ini.c SAPI.c rfc1867.c php_content_types.c strlcpy.c \ strlcat.c explicit_bzero.c reentrancy.c php_variables.c php_ticks.c \ - network.c php_open_temporary_file.c php_odbc_utils.c \ + network.c php_open_temporary_file.c php_odbc_utils.c safe_bcmp.c \ output.c getopt.c php_syslog.c, -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1) PHP_ADD_SOURCES_X(main, fastcgi.c, -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1, PHP_FASTCGI_OBJS, no) diff --git a/ext/hash/hash.c b/ext/hash/hash.c index c8b93313d59..ce665a57443 100644 --- a/ext/hash/hash.c +++ b/ext/hash/hash.c @@ -1111,9 +1111,7 @@ PHP_FUNCTION(hash_pbkdf2) PHP_FUNCTION(hash_equals) { zval *known_zval, *user_zval; - char *known_str, *user_str; int result = 0; - size_t j; if (zend_parse_parameters(ZEND_NUM_ARGS(), "zz", &known_zval, &user_zval) == FAILURE) { RETURN_THROWS(); @@ -1130,17 +1128,8 @@ PHP_FUNCTION(hash_equals) RETURN_THROWS(); } - if (Z_STRLEN_P(known_zval) != Z_STRLEN_P(user_zval)) { - RETURN_FALSE; - } - - known_str = Z_STRVAL_P(known_zval); - user_str = Z_STRVAL_P(user_zval); - /* This is security sensitive code. Do not optimize this for speed. */ - for (j = 0; j < Z_STRLEN_P(known_zval); j++) { - result |= known_str[j] ^ user_str[j]; - } + result = php_safe_bcmp(Z_STR_P(known_zval), Z_STR_P(user_zval)); RETURN_BOOL(0 == result); } diff --git a/ext/standard/password.c b/ext/standard/password.c index 96b74aa3955..374b0265628 100644 --- a/ext/standard/password.c +++ b/ext/standard/password.c @@ -152,7 +152,6 @@ static bool php_password_bcrypt_needs_rehash(const zend_string *hash, zend_array } static bool php_password_bcrypt_verify(const zend_string *password, const zend_string *hash) { - size_t i; int status = 0; zend_string *ret = php_crypt(ZSTR_VAL(password), (int)ZSTR_LEN(password), ZSTR_VAL(hash), (int)ZSTR_LEN(hash), 1); @@ -160,7 +159,7 @@ static bool php_password_bcrypt_verify(const zend_string *password, const zend_s return 0; } - if (ZSTR_LEN(ret) != ZSTR_LEN(hash) || ZSTR_LEN(hash) < 13) { + if (ZSTR_LEN(hash) < 13) { zend_string_free(ret); return 0; } @@ -169,9 +168,7 @@ static bool php_password_bcrypt_verify(const zend_string *password, const zend_s * resistance towards timing attacks. This is a constant time * equality check that will always check every byte of both * values. */ - for (i = 0; i < ZSTR_LEN(hash); i++) { - status |= (ZSTR_VAL(ret)[i] ^ ZSTR_VAL(hash)[i]); - } + status = php_safe_bcmp(ret, hash); zend_string_free(ret); return status == 0; diff --git a/main/php.h b/main/php.h index c8ef2d05818..9009ba032d3 100644 --- a/main/php.h +++ b/main/php.h @@ -179,6 +179,10 @@ END_EXTERN_C() #define explicit_bzero php_explicit_bzero #endif +BEGIN_EXTERN_C() +PHPAPI int php_safe_bcmp(const zend_string *a, const zend_string *b); +END_EXTERN_C() + #ifndef HAVE_STRTOK_R BEGIN_EXTERN_C() char *strtok_r(char *s, const char *delim, char **last); diff --git a/main/safe_bcmp.c b/main/safe_bcmp.c new file mode 100644 index 00000000000..27a1756d79b --- /dev/null +++ b/main/safe_bcmp.c @@ -0,0 +1,43 @@ +/* + +----------------------------------------------------------------------+ + | Copyright (c) The PHP Group | + +----------------------------------------------------------------------+ + | This source file is subject to version 3.01 of the PHP license, | + | that is bundled with this package in the file LICENSE, and is | + | available through the world-wide-web at the following url: | + | http://www.php.net/license/3_01.txt | + | If you did not receive a copy of the PHP license and are unable to | + | obtain it through the world-wide-web, please send a note to | + | license@php.net so we can mail you a copy immediately. | + +----------------------------------------------------------------------+ + | Author: David Carlier | + +----------------------------------------------------------------------+ +*/ + +#include "php.h" + +#include + +/* + * Returns 0 if both inputs match, 1 if they don't. + * Returns -1 early if inputs do not have the same lengths. + * + */ +PHPAPI int php_safe_bcmp(const zend_string *a, const zend_string *b) +{ + const volatile unsigned char *ua = (const volatile unsigned char *)ZSTR_VAL(a); + const volatile unsigned char *ub = (const volatile unsigned char *)ZSTR_VAL(b); + size_t i = 0; + int r = 0; + + if (ZSTR_LEN(a) != ZSTR_LEN(b)) { + return -1; + } + + while (i < ZSTR_LEN(a)) { + r |= ua[i] ^ ub[i]; + ++i; + } + + return r; +} diff --git a/win32/build/config.w32 b/win32/build/config.w32 index c174ebe8541..c66f1e39af6 100644 --- a/win32/build/config.w32 +++ b/win32/build/config.w32 @@ -265,7 +265,7 @@ ADD_SOURCES("main", "main.c snprintf.c spprintf.c getopt.c fopen_wrappers.c \ php_scandir.c php_ini.c SAPI.c rfc1867.c php_content_types.c strlcpy.c \ strlcat.c reentrancy.c php_variables.c php_ticks.c network.c \ php_open_temporary_file.c output.c internal_functions.c \ - php_syslog.c php_odbc_utils.c"); + php_syslog.c php_odbc_utils.c safe_bcmp.c"); ADD_FLAG("CFLAGS_BD_MAIN", "/D ZEND_ENABLE_STATIC_TSRMLS_CACHE=1"); if (VS_TOOLSET && VCVERS >= 1914) { ADD_FLAG("CFLAGS_BD_MAIN", "/d2FuncCache1");