diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c index f6994a06c54..caef462f02a 100644 --- a/ext/openssl/openssl.c +++ b/ext/openssl/openssl.c @@ -2153,7 +2153,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); @@ -2161,7 +2161,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); @@ -2233,19 +2233,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)) { @@ -2260,6 +2256,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; } /* }}} */ 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 db3cda305d1..813c008b5c3 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -1079,6 +1079,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); @@ -1088,12 +1089,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:{}"