From 1c4ee17d22b91f7827836ecb423cc12a6dc962d3 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 10 May 2021 15:30:45 +0200 Subject: [PATCH 1/2] Avoid OOB reads in create_name_with_username() `accel_uname_id` and `zend_system_id` are MD5 buffers which are not NUL terminated. Thus, we must not pass them to `snprintf()`. --- ext/opcache/shared_alloc_win32.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/ext/opcache/shared_alloc_win32.c b/ext/opcache/shared_alloc_win32.c index 4b95508ef44ca..2ce9e0feb4a1e 100644 --- a/ext/opcache/shared_alloc_win32.c +++ b/ext/opcache/shared_alloc_win32.c @@ -70,8 +70,19 @@ static void zend_win_error_message(int type, char *msg, int err) static char *create_name_with_username(char *name) { - static char newname[MAXPATHLEN + 32 + 4 + 1 + 32 + 21]; - snprintf(newname, sizeof(newname) - 1, "%s@%.32s@%.20s@%.32s", name, accel_uname_id, sapi_module.name, accel_system_id); + static char newname[MAXPATHLEN + 1 + 32 + 1 + 20 + 1 + 32 + 1]; + char *p = newname; + p += strlcpy(newname, name, MAXPATHLEN + 1); + *(p++) = '@'; + memcpy(p, accel_uname_id, 32); + p += 32; + *(p++) = '@'; + p += strlcpy(p, sapi_module.name, 21); + *(p++) = '@'; + memcpy(p, accel_system_id, 32); + p += 32; + *(p++) = '\0'; + ZEND_ASSERT(p - newname <= sizeof(newname)); return newname; } From f8de7fca8ef5403bd73d90123452f19fcce975d0 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 17 May 2021 11:50:34 +0200 Subject: [PATCH 2/2] Introduce ACCEL_SYSTEM_ID_LEN and ACCEL_UNAME_ID_LEN --- ext/opcache/ZendAccelerator.c | 4 +-- ext/opcache/ZendAccelerator.h | 6 ++-- ext/opcache/shared_alloc_win32.c | 10 +++---- ext/opcache/zend_file_cache.c | 48 ++++++++++++++++---------------- 4 files changed, 35 insertions(+), 33 deletions(-) diff --git a/ext/opcache/ZendAccelerator.c b/ext/opcache/ZendAccelerator.c index 189cfca2794aa..5287e64f8cb8c 100644 --- a/ext/opcache/ZendAccelerator.c +++ b/ext/opcache/ZendAccelerator.c @@ -114,9 +114,9 @@ ZEND_TSRMLS_CACHE_DEFINE() zend_accel_shared_globals *accel_shared_globals = NULL; /* true globals, no need for thread safety */ -char accel_system_id[32]; +char accel_system_id[ACCEL_SYSTEM_ID_LEN]; #ifdef ZEND_WIN32 -char accel_uname_id[32]; +char accel_uname_id[ACCEL_UNAME_ID_LEN]; #endif zend_bool accel_startup_ok = 0; static char *zps_failure_reason = NULL; diff --git a/ext/opcache/ZendAccelerator.h b/ext/opcache/ZendAccelerator.h index c2f95d7c41c85..16085a6d4db93 100644 --- a/ext/opcache/ZendAccelerator.h +++ b/ext/opcache/ZendAccelerator.h @@ -275,9 +275,11 @@ typedef struct _zend_accel_shared_globals { zend_string_table interned_strings; } zend_accel_shared_globals; -extern char accel_system_id[32]; +#define ACCEL_SYSTEM_ID_LEN 32 +extern char accel_system_id[ACCEL_SYSTEM_ID_LEN]; #ifdef ZEND_WIN32 -extern char accel_uname_id[32]; +# define ACCEL_UNAME_ID_LEN 32 +extern char accel_uname_id[ACCEL_UNAME_ID_LEN]; #endif extern zend_bool accel_startup_ok; extern zend_bool file_cache_only; diff --git a/ext/opcache/shared_alloc_win32.c b/ext/opcache/shared_alloc_win32.c index 2ce9e0feb4a1e..0d6c4bf5304b2 100644 --- a/ext/opcache/shared_alloc_win32.c +++ b/ext/opcache/shared_alloc_win32.c @@ -70,17 +70,17 @@ static void zend_win_error_message(int type, char *msg, int err) static char *create_name_with_username(char *name) { - static char newname[MAXPATHLEN + 1 + 32 + 1 + 20 + 1 + 32 + 1]; + static char newname[MAXPATHLEN + 1 + ACCEL_UNAME_ID_LEN + 1 + 20 + 1 + ACCEL_SYSTEM_ID_LEN + 1]; char *p = newname; p += strlcpy(newname, name, MAXPATHLEN + 1); *(p++) = '@'; - memcpy(p, accel_uname_id, 32); - p += 32; + memcpy(p, accel_uname_id, ACCEL_UNAME_ID_LEN); + p += ACCEL_UNAME_ID_LEN; *(p++) = '@'; p += strlcpy(p, sapi_module.name, 21); *(p++) = '@'; - memcpy(p, accel_system_id, 32); - p += 32; + memcpy(p, accel_system_id, ACCEL_SYSTEM_ID_LEN); + p += ACCEL_SYSTEM_ID_LEN; *(p++) = '\0'; ZEND_ASSERT(p - newname <= sizeof(newname)); diff --git a/ext/opcache/zend_file_cache.c b/ext/opcache/zend_file_cache.c index 41b332579587b..68294a42474de 100644 --- a/ext/opcache/zend_file_cache.c +++ b/ext/opcache/zend_file_cache.c @@ -810,7 +810,7 @@ static void zend_file_cache_serialize(zend_persistent_script *script, zend_persistent_script *new_script; memcpy(info->magic, "OPCACHE", 8); - memcpy(info->system_id, accel_system_id, 32); + memcpy(info->system_id, accel_system_id, ACCEL_SYSTEM_ID_LEN); info->mem_size = script->size; info->str_size = 0; info->script_offset = (char*)script - (char*)script->mem; @@ -836,48 +836,48 @@ static char *zend_file_cache_get_bin_file_path(zend_string *script_path) #ifndef ZEND_WIN32 len = strlen(ZCG(accel_directives).file_cache); - filename = emalloc(len + 33 + ZSTR_LEN(script_path) + sizeof(SUFFIX)); + filename = emalloc(len + ACCEL_SYSTEM_ID_LEN + 1 + ZSTR_LEN(script_path) + sizeof(SUFFIX)); memcpy(filename, ZCG(accel_directives).file_cache, len); filename[len] = '/'; - memcpy(filename + len + 1, accel_system_id, 32); - memcpy(filename + len + 33, ZSTR_VAL(script_path), ZSTR_LEN(script_path)); - memcpy(filename + len + 33 + ZSTR_LEN(script_path), SUFFIX, sizeof(SUFFIX)); + memcpy(filename + len + 1, accel_system_id, ACCEL_SYSTEM_ID_LEN); + memcpy(filename + len + ACCEL_SYSTEM_ID_LEN + 1, ZSTR_VAL(script_path), ZSTR_LEN(script_path)); + memcpy(filename + len + ACCEL_SYSTEM_ID_LEN + 1 + ZSTR_LEN(script_path), SUFFIX, sizeof(SUFFIX)); #else len = strlen(ZCG(accel_directives).file_cache); - filename = emalloc(len + 33 + 33 + ZSTR_LEN(script_path) + sizeof(SUFFIX)); + filename = emalloc(len + ACCEL_UNAME_ID_LEN + 1 + ACCEL_SYSTEM_ID_LEN + 1 + ZSTR_LEN(script_path) + sizeof(SUFFIX)); memcpy(filename, ZCG(accel_directives).file_cache, len); filename[len] = '\\'; - memcpy(filename + 1 + len, accel_uname_id, 32); - len += 1 + 32; + memcpy(filename + 1 + len, accel_uname_id, ACCEL_UNAME_ID_LEN); + len += 1 + ACCEL_UNAME_ID_LEN; filename[len] = '\\'; - memcpy(filename + len + 1, accel_system_id, 32); + memcpy(filename + len + 1, accel_system_id, ACCEL_SYSTEM_ID_LEN); if (ZSTR_LEN(script_path) >= 7 && ':' == ZSTR_VAL(script_path)[4] && '/' == ZSTR_VAL(script_path)[5] && '/' == ZSTR_VAL(script_path)[6]) { /* phar:// or file:// */ - *(filename + len + 33) = '\\'; - memcpy(filename + len + 34, ZSTR_VAL(script_path), 4); + *(filename + len + ACCEL_SYSTEM_ID_LEN + 1) = '\\'; + memcpy(filename + len + ACCEL_SYSTEM_ID_LEN + 2, ZSTR_VAL(script_path), 4); if (ZSTR_LEN(script_path) - 7 >= 2 && ':' == ZSTR_VAL(script_path)[8]) { - *(filename + len + 38) = '\\'; - *(filename + len + 39) = ZSTR_VAL(script_path)[7]; - memcpy(filename + len + 40, ZSTR_VAL(script_path) + 9, ZSTR_LEN(script_path) - 9); - memcpy(filename + len + 40 + ZSTR_LEN(script_path) - 9, SUFFIX, sizeof(SUFFIX)); + *(filename + len + ACCEL_SYSTEM_ID_LEN + 6) = '\\'; + *(filename + len + ACCEL_SYSTEM_ID_LEN + 7) = ZSTR_VAL(script_path)[7]; + memcpy(filename + len + ACCEL_SYSTEM_ID_LEN + 8, ZSTR_VAL(script_path) + 9, ZSTR_LEN(script_path) - 9); + memcpy(filename + len + ACCEL_SYSTEM_ID_LEN + 8 + ZSTR_LEN(script_path) - 9, SUFFIX, sizeof(SUFFIX)); } else { - memcpy(filename + len + 38, ZSTR_VAL(script_path) + 7, ZSTR_LEN(script_path) - 7); - memcpy(filename + len + 38 + ZSTR_LEN(script_path) - 7, SUFFIX, sizeof(SUFFIX)); + memcpy(filename + len + ACCEL_SYSTEM_ID_LEN + 6, ZSTR_VAL(script_path) + 7, ZSTR_LEN(script_path) - 7); + memcpy(filename + len + ACCEL_SYSTEM_ID_LEN + 6 + ZSTR_LEN(script_path) - 7, SUFFIX, sizeof(SUFFIX)); } } else if (ZSTR_LEN(script_path) >= 2 && ':' == ZSTR_VAL(script_path)[1]) { /* local fs */ - *(filename + len + 33) = '\\'; - *(filename + len + 34) = ZSTR_VAL(script_path)[0]; - memcpy(filename + len + 35, ZSTR_VAL(script_path) + 2, ZSTR_LEN(script_path) - 2); - memcpy(filename + len + 35 + ZSTR_LEN(script_path) - 2, SUFFIX, sizeof(SUFFIX)); + *(filename + len + ACCEL_SYSTEM_ID_LEN + 1) = '\\'; + *(filename + len + ACCEL_SYSTEM_ID_LEN + 2) = ZSTR_VAL(script_path)[0]; + memcpy(filename + len + ACCEL_SYSTEM_ID_LEN + 3, ZSTR_VAL(script_path) + 2, ZSTR_LEN(script_path) - 2); + memcpy(filename + len + ACCEL_SYSTEM_ID_LEN + 3 + ZSTR_LEN(script_path) - 2, SUFFIX, sizeof(SUFFIX)); } else { /* network path */ - memcpy(filename + len + 33, ZSTR_VAL(script_path), ZSTR_LEN(script_path)); - memcpy(filename + len + 33 + ZSTR_LEN(script_path), SUFFIX, sizeof(SUFFIX)); + memcpy(filename + len + ACCEL_SYSTEM_ID_LEN + 1, ZSTR_VAL(script_path), ZSTR_LEN(script_path)); + memcpy(filename + len + ACCEL_SYSTEM_ID_LEN + 1 + ZSTR_LEN(script_path), SUFFIX, sizeof(SUFFIX)); } #endif @@ -1563,7 +1563,7 @@ zend_persistent_script *zend_file_cache_script_load(zend_file_handle *file_handl efree(filename); return NULL; } - if (memcmp(info.system_id, accel_system_id, 32) != 0) { + if (memcmp(info.system_id, accel_system_id, ACCEL_SYSTEM_ID_LEN) != 0) { zend_accel_error(ACCEL_LOG_WARNING, "opcache cannot read from file '%s' (wrong \"system_id\")\n", filename); zend_file_cache_flock(fd, LOCK_UN); close(fd);