Fix memory leaks and missing error propagation when php_openssl_csr_make() fails to set a version

[ndossche]

The leaks appears to be at least somewhat dependent on the OpenSSL version, but it is reproducible on an Ubuntu 24.04 container.

Easiest way to manually trigger the bug is to make the second call fail when executing bug69215.phpt:

diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c
index 12383ac8c2c..6721d841d16 100644
--- a/ext/openssl/openssl.c
+++ b/ext/openssl/openssl.c
@@ -2957,7 +2957,9 @@ static zend_result php_openssl_csr_make(struct php_x509_request * req, X509_REQ
 		}
 	}
 	/* setup the version number: version 1 */
-	if (X509_REQ_set_version(csr, 0L)) {
+	static int counter = 0;
+	counter++;
+	if (counter!=2&&X509_REQ_set_version(csr, 0L)) {
 		int i, nid;
 		char *type;
 		CONF_VALUE *v;

ASAN report:

Direct leak of 384 byte(s) in 1 object(s) allocated from:
    #0 0x7fd75dcb19c7 in malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x7fd75d54d7c4 in CRYPTO_zalloc (/lib/x86_64-linux-gnu/libcrypto.so.3+0x2237c4) (BuildId: 0698e1ff610cb3c6993dccbd82c1281b1b4c5ade)
    #2 0x7fd75d40cd13  (/lib/x86_64-linux-gnu/libcrypto.so.3+0xe2d13) (BuildId: 0698e1ff610cb3c6993dccbd82c1281b1b4c5ade)
    #3 0x7fd75d40ce19 in ASN1_item_new_ex (/lib/x86_64-linux-gnu/libcrypto.so.3+0xe2e19) (BuildId: 0698e1ff610cb3c6993dccbd82c1281b1b4c5ade)
    #4 0x7fd75d6b89f9 in X509_new_ex (/lib/x86_64-linux-gnu/libcrypto.so.3+0x38e9f9) (BuildId: 0698e1ff610cb3c6993dccbd82c1281b1b4c5ade)
    #5 0x7fd75d8bdb9d  (/lib/x86_64-linux-gnu/libssl.so.3+0x7eb9d) (BuildId: 5f3b12d47114f9fbdc7765266cd0bb8f1b5ee8fc)
    #6 0x7fd75d8a825d  (/lib/x86_64-linux-gnu/libssl.so.3+0x6925d) (BuildId: 5f3b12d47114f9fbdc7765266cd0bb8f1b5ee8fc)
    #7 0x5630a25351d9 in php_openssl_enable_crypto /work/php-src/ext/openssl/xp_ssl.c:1850
    #8 0x5630a2539c86 in php_openssl_sockop_set_option /work/php-src/ext/openssl/xp_ssl.c:2516
    #9 0x5630a334c610 in _php_stream_set_option /work/php-src/main/streams/streams.c:1466
    #10 0x5630a33557c1 in php_stream_xport_crypto_enable /work/php-src/main/streams/transports.c:387
    #11 0x5630a25387be in php_openssl_tcp_sockop_accept /work/php-src/ext/openssl/xp_ssl.c:2279
    #12 0x5630a2539fcd in php_openssl_sockop_set_option /work/php-src/ext/openssl/xp_ssl.c:2551
    #13 0x5630a334c610 in _php_stream_set_option /work/php-src/main/streams/streams.c:1466
    #14 0x5630a3354d3a in php_stream_xport_accept /work/php-src/main/streams/transports.c:307
    #15 0x5630a3150161 in zif_stream_socket_accept /work/php-src/ext/standard/streamsfuncs.c:298
    #16 0x5630a35dacfb in ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER /work/php-src/Zend/zend_vm_execute.h:1355
    #17 0x5630a3740689 in execute_ex /work/php-src/Zend/zend_vm_execute.h:116469
    #18 0x5630a37558b0 in zend_execute /work/php-src/Zend/zend_vm_execute.h:121962
    #19 0x5630a38ba0ab in zend_execute_script /work/php-src/Zend/zend.c:1980
    #20 0x5630a32ec8bb in php_execute_script_ex /work/php-src/main/main.c:2645
    #21 0x5630a32ecccb in php_execute_script /work/php-src/main/main.c:2685
    #22 0x5630a38bfc16 in do_cli /work/php-src/sapi/cli/php_cli.c:951
    #23 0x5630a38c21e3 in main /work/php-src/sapi/cli/php_cli.c:1362
    #24 0x7fd75cfac1c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    #25 0x7fd75cfac28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
    #26 0x5630a2409b34 in _start (/work/php-src/build-dbg-asan/sapi/cli/php+0x609b34) (BuildId: aa149f943514fff0c491e1f199e30fed0e977f7c)

... etc ...

This was found by a hybrid static-dynamic analyser that looks for inconsistent handling of error checks in bindings.

[bukka]

Can X509_REQ_set_version(csr, 0L) fail in reality? From what I see, the only part that could potentially fail is realloc in ASN1_STRING_set but it doesn't look like it could be called for setting 0 int. I might be missing something though.

[ndossche]

Well the code already had the check.
I believe this branch can still be true when the number is 0:

https://github.com/openssl/openssl/blob/8782e9a7ddc45e21ea02457b1db9c7791033b4a4/crypto/asn1/asn1_lib.c#L308

But besides that, even if it were true today that the implementation cannot fail under these circumstances, there is no guarantee that it might not fail tomorrow. We should be coding to the spec not to the implementation.

[bukka]

I meant more if it should be 8.4 or master. But it's small so it won't hurt to do in 8.4