From ac8d0e57d9a1e641fe880082a010cfdf50b26ad7 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 3 Nov 2024 21:58:06 +0100 Subject: [PATCH 1/3] Prevent unexpected array entry conversion when reading key When passing an array, the key entry can get converted to a string if it is an object, but this actually modifies the original array entry. The test originally outputted: ``` array(2) { [0]=> string(...) => ... [1]=> string(0) "" } ``` This is unexpected. Use zval_try_get_string() to prevent this behaviour. Closes GH-16693. --- NEWS | 3 +++ ext/openssl/openssl.c | 18 ++++++++----- ..._pkey_export_to_file_object_to_string.phpt | 27 +++++++++++++++++++ 3 files changed, 42 insertions(+), 6 deletions(-) create mode 100644 ext/openssl/tests/openssl_pkey_export_to_file_object_to_string.phpt diff --git a/NEWS b/NEWS index 6f370c9f096..805a45daceb 100644 --- a/NEWS +++ b/NEWS @@ -8,6 +8,9 @@ PHP NEWS - FPM: . Fixed GH-16432 (PHP-FPM 8.2 SIGSEGV in fpm_get_status). (Jakub Zelenka) +- OpenSSL: + . Prevent unexpected array entry conversion when reading key. (nielsdos) + - PDO: . Fixed memory leak of `setFetchMode()`. (SakiTakamachi) diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c index a50a3074117..d756c3bf25c 100644 --- a/ext/openssl/openssl.c +++ b/ext/openssl/openssl.c @@ -3577,19 +3577,21 @@ static EVP_PKEY *php_openssl_pkey_from_zval( if (!(Z_TYPE_P(val) == IS_STRING || Z_TYPE_P(val) == IS_OBJECT)) { TMP_CLEAN; } - if (!try_convert_to_string(val)) { + zend_string *val_str = zval_try_get_string(val); + if (!val_str) { TMP_CLEAN; } - if (Z_STRLEN_P(val) > 7 && memcmp(Z_STRVAL_P(val), "file://", sizeof("file://") - 1) == 0) { - if (!php_openssl_check_path_str(Z_STR_P(val), file_path, arg_num)) { + if (ZSTR_LEN(val_str) > 7 && memcmp(ZSTR_VAL(val_str), "file://", sizeof("file://") - 1) == 0) { + if (!php_openssl_check_path_str(val_str, file_path, arg_num)) { + zend_string_release_ex(val_str, false); TMP_CLEAN; } is_file = true; } /* it's an X509 file/cert of some kind, and we need to extract the data from that */ if (public_key) { - cert = php_openssl_x509_from_str(Z_STR_P(val), arg_num, false, NULL); + cert = php_openssl_x509_from_str(val_str, arg_num, false, NULL); if (cert) { free_cert = 1; @@ -3599,10 +3601,11 @@ static EVP_PKEY *php_openssl_pkey_from_zval( if (is_file) { in = BIO_new_file(file_path, PHP_OPENSSL_BIO_MODE_R(PKCS7_BINARY)); } else { - in = BIO_new_mem_buf(Z_STRVAL_P(val), (int)Z_STRLEN_P(val)); + in = BIO_new_mem_buf(ZSTR_VAL(val_str), (int)ZSTR_LEN(val_str)); } if (in == NULL) { php_openssl_store_errors(); + zend_string_release_ex(val_str, false); TMP_CLEAN; } key = PEM_read_bio_PUBKEY(in, NULL,NULL, NULL); @@ -3615,10 +3618,11 @@ static EVP_PKEY *php_openssl_pkey_from_zval( if (is_file) { in = BIO_new_file(file_path, PHP_OPENSSL_BIO_MODE_R(PKCS7_BINARY)); } else { - in = BIO_new_mem_buf(Z_STRVAL_P(val), (int)Z_STRLEN_P(val)); + in = BIO_new_mem_buf(ZSTR_VAL(val_str), (int)ZSTR_LEN(val_str)); } if (in == NULL) { + zend_string_release_ex(val_str, false); TMP_CLEAN; } if (passphrase == NULL) { @@ -3631,6 +3635,8 @@ static EVP_PKEY *php_openssl_pkey_from_zval( } BIO_free(in); } + + zend_string_release_ex(val_str, false); } if (key == NULL) { diff --git a/ext/openssl/tests/openssl_pkey_export_to_file_object_to_string.phpt b/ext/openssl/tests/openssl_pkey_export_to_file_object_to_string.phpt new file mode 100644 index 00000000000..0e504bfa4ac --- /dev/null +++ b/ext/openssl/tests/openssl_pkey_export_to_file_object_to_string.phpt @@ -0,0 +1,27 @@ +--TEST-- +openssl_pkey_export_to_file object to string conversion +--EXTENSIONS-- +openssl +--FILE-- + +--EXPECT-- +array(2) { + [0]=> + object(Test)#1 (0) { + } + [1]=> + string(0) "" +} From 2f4f09f7e65b8f731b1d1d9055f2f738a35dbd38 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 3 Nov 2024 21:42:47 +0100 Subject: [PATCH 2/3] Fix various memory leaks related to openssl exports Closes GH-16692. --- NEWS | 1 + ext/openssl/openssl.c | 19 +++++++++++-------- .../openssl_csr_export_to_file_leak.phpt | 14 ++++++++++++++ .../openssl_pkey_export_to_file_leak.phpt | 15 +++++++++++++++ .../openssl_x509_export_to_file_leak.phpt | 14 ++++++++++++++ 5 files changed, 55 insertions(+), 8 deletions(-) create mode 100644 ext/openssl/tests/openssl_csr_export_to_file_leak.phpt create mode 100644 ext/openssl/tests/openssl_pkey_export_to_file_leak.phpt create mode 100644 ext/openssl/tests/openssl_x509_export_to_file_leak.phpt diff --git a/NEWS b/NEWS index 805a45daceb..fadcea21068 100644 --- a/NEWS +++ b/NEWS @@ -10,6 +10,7 @@ PHP NEWS - OpenSSL: . Prevent unexpected array entry conversion when reading key. (nielsdos) + . Fix various memory leaks related to openssl exports. (nielsdos) - PDO: . Fixed memory leak of `setFetchMode()`. (SakiTakamachi) diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c index d756c3bf25c..4a8aca8f999 100644 --- a/ext/openssl/openssl.c +++ b/ext/openssl/openssl.c @@ -1495,7 +1495,7 @@ PHP_FUNCTION(openssl_x509_export_to_file) } if (!php_openssl_check_path(filename, filename_len, file_path, 2)) { - return; + goto exit_cleanup_cert; } bio_out = BIO_new_file(file_path, PHP_OPENSSL_BIO_MODE_W(PKCS7_BINARY)); @@ -1513,13 +1513,14 @@ PHP_FUNCTION(openssl_x509_export_to_file) php_error_docref(NULL, E_WARNING, "Error opening file %s", file_path); } - if (cert_str) { - X509_free(cert); - } - if (!BIO_free(bio_out)) { php_openssl_store_errors(); } + +exit_cleanup_cert: + if (cert_str) { + X509_free(cert); + } } /* }}} */ @@ -3070,7 +3071,7 @@ PHP_FUNCTION(openssl_csr_export_to_file) } if (!php_openssl_check_path(filename, filename_len, file_path, 2)) { - return; + goto exit_cleanup; } bio_out = BIO_new_file(file_path, PHP_OPENSSL_BIO_MODE_W(PKCS7_BINARY)); @@ -3090,6 +3091,7 @@ PHP_FUNCTION(openssl_csr_export_to_file) php_error_docref(NULL, E_WARNING, "Error opening file %s", file_path); } +exit_cleanup: if (csr_str) { X509_REQ_free(csr); } @@ -4567,7 +4569,7 @@ PHP_FUNCTION(openssl_pkey_export_to_file) } if (!php_openssl_check_path(filename, filename_len, file_path, 2)) { - RETURN_FALSE; + goto clean_exit_key; } PHP_SSL_REQ_INIT(&req); @@ -4603,8 +4605,9 @@ PHP_FUNCTION(openssl_pkey_export_to_file) clean_exit: PHP_SSL_REQ_DISPOSE(&req); - EVP_PKEY_free(key); BIO_free(bio_out); +clean_exit_key: + EVP_PKEY_free(key); } /* }}} */ diff --git a/ext/openssl/tests/openssl_csr_export_to_file_leak.phpt b/ext/openssl/tests/openssl_csr_export_to_file_leak.phpt new file mode 100644 index 00000000000..e6ce373d355 --- /dev/null +++ b/ext/openssl/tests/openssl_csr_export_to_file_leak.phpt @@ -0,0 +1,14 @@ +--TEST-- +openssl_csr_export_to_file memory leak +--EXTENSIONS-- +openssl +--FILE-- + +--EXPECTF-- +Warning: openssl_csr_export_to_file(output_filename): must be a valid file path %s +bool(false) diff --git a/ext/openssl/tests/openssl_pkey_export_to_file_leak.phpt b/ext/openssl/tests/openssl_pkey_export_to_file_leak.phpt new file mode 100644 index 00000000000..5e2bdff6b48 --- /dev/null +++ b/ext/openssl/tests/openssl_pkey_export_to_file_leak.phpt @@ -0,0 +1,15 @@ +--TEST-- +openssl_pkey_export_to_file memory leak +--EXTENSIONS-- +openssl +--FILE-- + +--EXPECTF-- +Warning: openssl_pkey_export_to_file(output_filename): must be a valid file path %s +bool(false) diff --git a/ext/openssl/tests/openssl_x509_export_to_file_leak.phpt b/ext/openssl/tests/openssl_x509_export_to_file_leak.phpt new file mode 100644 index 00000000000..5775c2597c3 --- /dev/null +++ b/ext/openssl/tests/openssl_x509_export_to_file_leak.phpt @@ -0,0 +1,14 @@ +--TEST-- +openssl_x509_export_to_file memory leak +--EXTENSIONS-- +openssl +--FILE-- + +--EXPECTF-- +Warning: openssl_x509_export_to_file(output_filename): must be a valid file path %s +bool(false) From 994e866cf2ad3b84882e7070e7097ee5553130e1 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 3 Nov 2024 21:18:34 +0100 Subject: [PATCH 3/3] Fix memory leak in php_openssl_pkey_from_zval() Closes GH-16691. --- NEWS | 1 + ext/openssl/openssl.c | 1 + .../php_openssl_pkey_from_zval_leak.phpt | 23 +++++++++++++++++++ 3 files changed, 25 insertions(+) create mode 100644 ext/openssl/tests/php_openssl_pkey_from_zval_leak.phpt diff --git a/NEWS b/NEWS index fadcea21068..72b3d645cf9 100644 --- a/NEWS +++ b/NEWS @@ -11,6 +11,7 @@ PHP NEWS - OpenSSL: . Prevent unexpected array entry conversion when reading key. (nielsdos) . Fix various memory leaks related to openssl exports. (nielsdos) + . Fix memory leak in php_openssl_pkey_from_zval(). (nielsdos) - PDO: . Fixed memory leak of `setFetchMode()`. (SakiTakamachi) diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c index 4a8aca8f999..9e703f75863 100644 --- a/ext/openssl/openssl.c +++ b/ext/openssl/openssl.c @@ -3533,6 +3533,7 @@ static EVP_PKEY *php_openssl_pkey_from_zval( } else { ZVAL_COPY(&tmp, zphrase); if (!try_convert_to_string(&tmp)) { + zval_ptr_dtor(&tmp); return NULL; } diff --git a/ext/openssl/tests/php_openssl_pkey_from_zval_leak.phpt b/ext/openssl/tests/php_openssl_pkey_from_zval_leak.phpt new file mode 100644 index 00000000000..2b19dd31115 --- /dev/null +++ b/ext/openssl/tests/php_openssl_pkey_from_zval_leak.phpt @@ -0,0 +1,23 @@ +--TEST-- +php_openssl_pkey_from_zval memory leak +--EXTENSIONS-- +openssl +--FILE-- +getMessage(), "\n"; +} + +?> +--EXPECT-- +create a leak