From 5ddb75660d106c0c3de3f01539e5b53e7e694643 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 3 Nov 2024 17:57:43 +0100 Subject: [PATCH 1/2] Fix various memory leaks on error conditions in openssl_x509_parse() Closes GH-16690. --- NEWS | 2 ++ ext/openssl/openssl.c | 22 ++++++++++++++-------- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/NEWS b/NEWS index 6dbf059608f..a5cbb334faa 100644 --- a/NEWS +++ b/NEWS @@ -86,6 +86,8 @@ PHP NEWS (cmb) . Fixed bug GH-16433 (Large values for openssl_csr_sign() $days overflow). (cmb) + . Fix various memory leaks on error conditions in openssl_x509_parse(). + (nielsdos) - PDO_ODBC: . Fixed bug GH-16450 (PDO_ODBC can inject garbage into field values). (cmb) diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c index 4b4a8d7f356..a50a3074117 100644 --- a/ext/openssl/openssl.c +++ b/ext/openssl/openssl.c @@ -2091,7 +2091,7 @@ PHP_FUNCTION(openssl_x509_parse) /* Can return NULL on error or memory allocation failure */ if (!bn_serial) { php_openssl_store_errors(); - RETURN_FALSE; + goto err; } hex_serial = BN_bn2hex(bn_serial); @@ -2099,7 +2099,7 @@ PHP_FUNCTION(openssl_x509_parse) /* Can return NULL on error or memory allocation failure */ if (!hex_serial) { php_openssl_store_errors(); - RETURN_FALSE; + goto err; } str_serial = i2s_ASN1_INTEGER(NULL, asn1_serial); @@ -2171,19 +2171,15 @@ PHP_FUNCTION(openssl_x509_parse) bio_out = BIO_new(BIO_s_mem()); if (bio_out == NULL) { php_openssl_store_errors(); - RETURN_FALSE; + goto err_subitem; } if (nid == NID_subject_alt_name) { if (openssl_x509v3_subjectAltName(bio_out, extension) == 0) { BIO_get_mem_ptr(bio_out, &bio_buf); add_assoc_stringl(&subitem, extname, bio_buf->data, bio_buf->length); } else { - zend_array_destroy(Z_ARR_P(return_value)); BIO_free(bio_out); - if (cert_str) { - X509_free(cert); - } - RETURN_FALSE; + goto err_subitem; } } else if (X509V3_EXT_print(bio_out, extension, 0, 0)) { @@ -2198,6 +2194,16 @@ PHP_FUNCTION(openssl_x509_parse) if (cert_str) { X509_free(cert); } + return; + +err_subitem: + zval_ptr_dtor(&subitem); +err: + zend_array_destroy(Z_ARR_P(return_value)); + if (cert_str) { + X509_free(cert); + } + RETURN_FALSE; } /* }}} */ From cc39bc21e361ceb8a170ac3366b5d35372b94978 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Tue, 29 Oct 2024 19:44:22 +0100 Subject: [PATCH 2/2] Fix GH-16590: UAF in session_encode() The `PS_ENCODE_LOOP` does not protect the session hash table that it iterates over. Change it by temporarily creating a copy. Closes GH-16640. --- NEWS | 3 +++ UPGRADING.INTERNALS | 2 ++ ext/session/php_session.h | 8 +++++++- ext/session/session.c | 8 +++++++- ext/session/tests/gh16590.phpt | 36 ++++++++++++++++++++++++++++++++++ 5 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 ext/session/tests/gh16590.phpt diff --git a/NEWS b/NEWS index fe3239269e7..da14c247ee0 100644 --- a/NEWS +++ b/NEWS @@ -49,6 +49,9 @@ PHP NEWS - Reflection: . Fixed bug GH-16601 (Memory leak in Reflection constructors). (nielsdos) +- Session: + . Fixed bug GH-16590 (UAF in session_encode()). (nielsdos) + - SPL: . Fixed bug GH-16588 (UAF in Observer->serialize). (nielsdos) . Fix GH-16477 (Segmentation fault when calling __debugInfo() after failed diff --git a/UPGRADING.INTERNALS b/UPGRADING.INTERNALS index 36096ebdd41..a186dbf77fe 100644 --- a/UPGRADING.INTERNALS +++ b/UPGRADING.INTERNALS @@ -391,6 +391,8 @@ PHP 8.4 INTERNALS UPGRADE NOTES needing to create a zend_string. - The ext/session/php_session.h doesn't transitively include the ext/hash/php_hash.h header anymore. + - It is no longer allowed to return out of the PS_ENCODE_LOOP macro. + Instead, you should break out of the loop instead. i. ext/xml - Made the expat compatibility wrapper XML_GetCurrentByteIndex return a long diff --git a/ext/session/php_session.h b/ext/session/php_session.h index 65bf7de2044..dfa7632e5a4 100644 --- a/ext/session/php_session.h +++ b/ext/session/php_session.h @@ -291,8 +291,13 @@ PHPAPI zend_result php_session_reset_id(void); zend_ulong num_key; \ zval *struc; +/* Do not use a return statement in `code` because that may leak memory. + * Break out of the loop instead. */ #define PS_ENCODE_LOOP(code) do { \ - HashTable *_ht = Z_ARRVAL_P(Z_REFVAL(PS(http_session_vars))); \ + zval _zv; \ + /* protect against user interference */ \ + ZVAL_COPY(&_zv, Z_REFVAL(PS(http_session_vars))); \ + HashTable *_ht = Z_ARRVAL(_zv); \ ZEND_HASH_FOREACH_KEY(_ht, num_key, key) { \ if (key == NULL) { \ php_error_docref(NULL, E_WARNING, \ @@ -303,6 +308,7 @@ PHPAPI zend_result php_session_reset_id(void); code; \ } \ } ZEND_HASH_FOREACH_END(); \ + zval_ptr_dtor(&_zv); \ } while(0) PHPAPI ZEND_EXTERN_MODULE_GLOBALS(ps) diff --git a/ext/session/session.c b/ext/session/session.c index b23f1fce358..86e1cfa7c43 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -1056,6 +1056,7 @@ PS_SERIALIZER_ENCODE_FUNC(php) /* {{{ */ { smart_str buf = {0}; php_serialize_data_t var_hash; + bool fail = false; PS_ENCODE_VARS; PHP_VAR_SERIALIZE_INIT(var_hash); @@ -1065,12 +1066,17 @@ PS_SERIALIZER_ENCODE_FUNC(php) /* {{{ */ if (memchr(ZSTR_VAL(key), PS_DELIMITER, ZSTR_LEN(key))) { PHP_VAR_SERIALIZE_DESTROY(var_hash); smart_str_free(&buf); - return NULL; + fail = true; + break; } smart_str_appendc(&buf, PS_DELIMITER); php_var_serialize(&buf, struc, &var_hash); ); + if (fail) { + return NULL; + } + smart_str_0(&buf); PHP_VAR_SERIALIZE_DESTROY(var_hash); diff --git a/ext/session/tests/gh16590.phpt b/ext/session/tests/gh16590.phpt new file mode 100644 index 00000000000..781f6a78e09 --- /dev/null +++ b/ext/session/tests/gh16590.phpt @@ -0,0 +1,36 @@ +--TEST-- +GH-16590 (UAF in session_encode()) +--EXTENSIONS-- +session +--SKIPIF-- + +--INI-- +session.use_cookies=0 +session.cache_limiter= +session.serialize_handler=php +session.save_handler=files +--FILE-- + +--EXPECTF-- +Warning: session_encode(): Skipping numeric key 0 in %s on line %d + +Warning: session_encode(): Skipping numeric key 1 in %s on line %d +string(15) "Lz|O:1:"C":0:{}"