Convert OpenSSL resources to objects by kocsismate · Pull Request #5860 · php/php-src

kocsismate · 2020-07-15T16:03:37Z

No description provided.

ext/openssl/tests/bug38261.phpt

ext/openssl/openssl.c

Zend/zend_API.h

ext/openssl/openssl.c

ext/openssl/openssl.stub.php

ext/openssl/openssl.c

Zend/zend_API.h

ext/openssl/openssl.c

nikic · 2020-07-29T10:04:40Z

@bukka probably also want to have a look at this.

ext/openssl/openssl.c

bukka · 2020-07-29T21:39:27Z

ext/openssl/openssl.c

+	x509_certificate_object_handlers.clone_obj = NULL;
+
+	zend_class_entry csr_ce;
+	INIT_CLASS_ENTRY(csr_ce, "X509CertificateSigningRequest", class_X509CertificateSigningRequest_methods);


bukka · 2020-07-29T21:41:38Z

ext/openssl/openssl.c

+	x509_request_object_handlers.clone_obj = NULL;
+
+	zend_class_entry key_ce;
+	INIT_CLASS_ENTRY(key_ce, "OpenSSLKey", class_OpenSSLKey_methods);


Please use OpenSSLPKey or something showing that it's about pub priv key. Maybe something like OpenSSLAsymmetricKey...

I'd be very much in favour of the latter, since it took me quite some googling and time to find out if the "p" means private or public in the pkey related names. Then I realized it's for both :)

What's your stance about the OpenSSLX509CertificateSigningRequest class name? I'm not shy at all to give long names, but it's very-very close to my threshold (probably it's over it). :) Do we still need the X509 here and in the certificate's class name if the OpenSSL prefix is applied?

My preferences are: OpenSSLCertificate, OpenSSLCertificateSigningRequest, and OpenSSLAsymmetricKey (or OpenSSLKey). In 99% of the cases, I don't like to write abbreviations with all capital letters (I forgot the name of this style), but unfortunately, Ssl maybe doesn't look that good. :(

Yeah I like OpenSSLCertificate, OpenSSLCertificateSigningRequest, and OpenSSLAsymmetricKey

bukka · 2020-07-29T22:00:28Z

ext/openssl/openssl.c

-		RETURN_THROWS();
-	}
+	ZEND_PARSE_PARAMETERS_START(2, 3)
+		Z_PARAM_STR_OR_OBJ_OF_CLASS(cert_str, cert_obj, x509_certificate_ce)


so this actually prevents stringified object to be passed which has been supported so this breaking the functionality somehow.

I think this should be kept as zval

This still allows stringified objects per PHP's usual type declaration semantics. (In weak typing mode only of course, but that's a language consistency requirement.)

Ah yeah that's good then. I think we can drop that object OR string condition in that case.

ext/openssl/openssl.c

bukka · 2020-07-29T22:10:13Z

ext/openssl/openssl.c

-		RETURN_THROWS();
-	}
+	ZEND_PARSE_PARAMETERS_START(2, 3)
+		Z_PARAM_STR_OR_OBJ_OF_CLASS(cert_str, cert_obj, x509_certificate_ce)


again should be kept as zval probably...

this has been answered above and it's ok.

bukka · 2020-07-29T22:15:08Z

Nice work! Added some comments. Didn't manage to get through all of it but lots of issues are kind of the same then.

bukka · 2020-07-30T19:05:15Z

ext/openssl/openssl.c

+	x509_certificate_object_handlers.offset = XtOffsetOf(x509_certificate_object, std);
+	x509_certificate_object_handlers.free_obj = x509_certificate_free_obj;
+	x509_certificate_object_handlers.get_constructor = x509_certificate_get_constructor;
+	x509_certificate_object_handlers.clone_obj = NULL;


How is cloning going to be handled? I think we should be be using refs up in 1.1 and dup 1.0 possibly.

Cloning is currently disabled. I'm not sure I'll have enough time to incorporate its support in this PR before feature freeze starts (next Tuesday), but I can make a follow-up PR afterwards.

I don't think cloning should be part of the migration itself. It's a separate feature that allows doing something the current API doesn't (you can't clone a resource). The only case where we have added cloning support until now are curl handles, because the resource API already exposed an equivalent function (curl_copy_handle).

Ok it can be done later.

bukka · 2020-07-30T21:23:31Z

ext/openssl/openssl.c

+	zend_class_entry csr_ce;
+	INIT_CLASS_ENTRY(csr_ce, "X509CertificateSigningRequest", class_X509CertificateSigningRequest_methods);
+	x509_request_ce = zend_register_internal_class(&csr_ce);
+	x509_request_ce->ce_flags |= ZEND_ACC_FINAL | ZEND_ACC_NO_DYNAMIC_PROPERTIES;


this reminds me that instantiation should be also prohibited for all classes (meaning something like private constructor)

Yes, that's right! The constructors currently emit an exception with the Cannot directly construct... message, which is in line what we did for other resource migrations.

kocsismate · 2020-07-30T22:11:19Z

@bukka Thanks for the quick review! I've just added a new commit where I tried to fix as much of the comments as I could :) There's one test failing currently, so I'll have a closer look at it in the morning.

nikic · 2020-07-31T13:23:36Z

ext/openssl/openssl.c

+
+static X509 *php_openssl_x509_from_zval(zval *val, bool *free_cert)
+{
+	*free_cert = 1;


I'd move this assignment below the if.

nikic · 2020-07-31T13:27:26Z

ext/openssl/openssl.c

 	}

-	if (keyresource == NULL && s != NULL) {
+	if (free_pkey && s != NULL) {


This doesn't make sense, s and free_pkey have no relation. This branch should just be dropped. (Because free_pkey is always false in this code, it ends up not mattering.)

ext/openssl/openssl.c

bukka · 2020-07-31T14:22:57Z

ext/openssl/openssl.c

+
+/* OpenSSLAsymmetricKey class */
+
+typedef struct _openssl_key_object {


NIT: _php_openssl_pkey_object

nikic · 2020-07-31T14:26:47Z

ext/openssl/openssl.c

+
+	object_init_ex(return_value, php_openssl_pkey_ce);
+	key_object = Z_OPENSSL_PKEY_P(return_value);
+	key_object->pkey = pkey;


This may cause a use after free if it is an existing pkey and not a newly created one. Unfortunately there is no EVP_PKEY_dup, and EVP_PKEY_up_ref is OpenSSL 1.1. The previous code side-stepped the issue by reusing the existing pkey resource (as this only happens for the trivial case where you get the public key from ... a public key).

The structure is actually refcounted on older OpenSSL versions as well, but one has to use the lower-level CRYPTO_add(&pkey->references, 1, CRYPTO_LOCK_EVP_PKEY) API there.

We can redefine EVP_PKEY_up_ref if it's below 1.1 as it's done for some other functions. Might need some handling for freeing the key. It probably needs some testing. Considering the FF I'd be fine to get this in with leaking 1.0.2 if the PR to fix it is created later in the beta...

It also reminds me that we should drop support for 1.0.1. I'm kind of inclined dropping support for 1.0.2 too but might be too inconvenient for some RHEL 7 users. Considering that it's still supported in there, we should probably keep 1.0.2 support.

Sorry meant crashing 1.0.2 (use after free). But if there is time for fixing that on 1.0.2, then even better. :)

I went with using EVP_PKEY_up_ref() and defining a compatibility shim for older versions. I've also built OpenSSL 1.0.1u and checked that everything works under asan.

Ah yeah it's all good. I just double checked EVP_PKEY_free code in 1.0.2 and it's considering the ref:

void EVP_PKEY_free(EVP_PKEY *x) { int i; if (x == NULL) return; i = CRYPTO_add(&x->references, -1, CRYPTO_LOCK_EVP_PKEY); #ifdef REF_PRINT REF_PRINT("EVP_PKEY", x); #endif if (i > 0) return; #ifdef REF_CHECK if (i < 0) { fprintf(stderr, "EVP_PKEY_free, bad reference count\n"); abort(); } #endif EVP_PKEY_free_it(x); if (x->attributes) sk_X509_ATTRIBUTE_pop_free(x->attributes, X509_ATTRIBUTE_free); OPENSSL_free(x); }

Thanks, Nikita for fixing this for me. I wouldn't have found out on my own that this causes the test failure. :)

bukka · 2020-07-31T14:32:57Z

ext/openssl/openssl.c

 		NETSCAPE_SPKI_free(spki);
 	}
-	if (keyresource == NULL && pkey != NULL) {
+	if (free_pkey && pkey != NULL) {


and this should be also dropped for the same reason as the below.

And define compat shim for EVP_PKEY_up_ref().

kocsismate · 2020-07-31T21:06:43Z

ext/openssl/openssl.c


 #endif

+#if PHP_OPENSSL_API_VERSION < 0x10100


Is this the right version, isn't it?

kocsismate · 2020-07-31T21:09:32Z

ext/openssl/openssl.c

+	cert = php_openssl_x509_from_param(cert_obj, cert_str);
 	if (cert == NULL) {
-		php_error_docref(NULL, E_WARNING, "Cannot get cert from parameter 1");
+		php_error_docref(NULL, E_WARNING, "X.509 Certificate cannot be retrieved");


Should I change these kind of error messages to argument #1 ($x509) must be a valid X.509 Certificate?
UPDATE: Probably I'll do it later if you also like this format, this PR is already huge

ext/openssl/openssl.c

kocsismate · 2020-07-31T21:18:45Z

ext/openssl/openssl.c

+
+	object_init_ex(return_value, php_openssl_pkey_ce);
+	key_object = Z_OPENSSL_PKEY_P(return_value);
+	key_object->pkey = pkey;


Thanks, Nikita for fixing this for me. I wouldn't have found out on my own that this causes the test failure. :)

bukka · 2020-08-01T11:28:48Z

ext/openssl/openssl.c

+	object_init_ex(return_value, php_openssl_pkey_ce);
+	key_object = Z_OPENSSL_PKEY_P(return_value);
+	key_object->pkey = pkey;


It might be good to create a macro for this as it's constantly repeated. Doesn't need to be in the PR - just a note for future improvement.

bukka · 2020-08-01T11:31:35Z

ext/openssl/tests/001.phpt

 bool(true)
 Load key from file - array syntax
+
+Deprecated: Function openssl_pkey_free() is deprecated in %s on line %d


we should probably remove it from the test if it's deprecated - again doesn't need to be done now.

bukka · 2020-08-01T11:33:56Z

ext/openssl/tests/bug79145.phpt

 for ($i = 0; $i < 100000; $i++) {
    $a = openssl_get_publickey($b);
-    openssl_free_key($a);
+    @openssl_free_key($a);


Hmm we should probably look to this test if it's needed as well (future scope)

bukka · 2020-08-01T11:35:13Z

ext/openssl/xp_ssl.c

+		object_init_ex(&zcert, php_openssl_certificate_ce);
+		cert_object = Z_OPENSSL_CERTIFICATE_P(&zcert);
+		cert_object->x509 = peer_cert;


again macro would be good for this (future scope)

bukka · 2020-08-01T11:39:09Z

@kocsismate Think it's good to go. There are just some suggestion but for them it's probably better to do a different PR.

bukka · 2020-08-01T12:54:40Z

@kocsismate would be good if you can merge it soon so we can also get in #5111 before FF.

kocsismate · 2020-08-01T13:02:06Z

@bukka, sure, I can merge it in the late evening. I'll also provide a few followup PRs.

> - The openssl_x509_free() function is deprecated and no longer has an effect, > instead the OpenSSLCertificate instance is automatically destroyed if it is no > longer referenced. > - The openssl_pkey_free() function is deprecated and no longer has an effect, > instead the OpenSSLAsymmetricKey instance is automatically destroyed if it is no > longer referenced. Refs: * https://github.com/php/php-src/blob/71bfa5344ab207072f4cd25745d7023096338385/UPGRADING#L384-L399 * php/php-src#5860 * php/php-src@9f44eca#diff-7748eb3bfdd3bf962553f6f9f2723c45 Includes unit tests.

kocsismate marked this pull request as draft July 15, 2020 16:05

kocsismate commented Jul 15, 2020

View reviewed changes

ext/openssl/tests/bug38261.phpt Outdated Show resolved Hide resolved

ext/openssl/openssl.c Show resolved Hide resolved

kocsismate force-pushed the openssl-resource branch 2 times, most recently from 00f354b to 095b0bb Compare July 15, 2020 16:59

Girgias reviewed Jul 17, 2020

View reviewed changes

Zend/zend_API.h Show resolved Hide resolved

ext/openssl/openssl.c Show resolved Hide resolved

ext/openssl/openssl.c Show resolved Hide resolved

kocsismate force-pushed the openssl-resource branch from 3b9fadf to bdc6f88 Compare July 22, 2020 08:20

kocsismate marked this pull request as ready for review July 22, 2020 08:22

kocsismate force-pushed the openssl-resource branch 3 times, most recently from 18a02cc to 91f9233 Compare July 22, 2020 21:15

kocsismate changed the title ~~Convert OpenSSL X.509 Certificate resource to object~~ Convert OpenSSL resources to objects Jul 22, 2020

kocsismate commented Jul 22, 2020

View reviewed changes

ext/openssl/openssl.stub.php Outdated Show resolved Hide resolved

ext/openssl/openssl.c Show resolved Hide resolved

Girgias mentioned this pull request Jul 23, 2020

Resource to opaque object migration php/php-tasks#6

Open

41 tasks

kocsismate force-pushed the openssl-resource branch from 91f9233 to e6e5260 Compare July 25, 2020 13:33

kocsismate commented Jul 25, 2020

View reviewed changes

ext/openssl/openssl.c Outdated Show resolved Hide resolved

kocsismate force-pushed the openssl-resource branch from e6e5260 to 2d98cc3 Compare July 29, 2020 08:17

nikic reviewed Jul 29, 2020

View reviewed changes

Zend/zend_API.h Outdated Show resolved Hide resolved

nikic reviewed Jul 29, 2020

View reviewed changes