Improve parameter handling in ext/openssl #6025

kocsismate · 2020-08-20T14:06:10Z

This PR:

gets rid of the UNKNOWN default values
improves parameter names
uses string|int ZPP when possible
promotes a warning to an exception that was missed previously
adapts a few error messages to the new parameter names

kocsismate · 2020-08-20T18:26:30Z

@bukka Can you please have a look at this PR, especially at the parameter names? There are quite a few cases where I'm not very confident, so please take the current names just as a starting point.

ext/openssl/openssl.c

ext/openssl/openssl.stub.php

bukka · 2020-08-22T19:21:09Z

ext/openssl/openssl.stub.php

 */
-function openssl_pkey_export($key, &$out, ?string $passphrase = null, ?array $configargs = null): bool {}
+function openssl_pkey_export($private_key, &$output, ?string $passphrase = null, ?array $options = null): bool {}


isn't this working for public_key as well? Maybe we should just call it pkey in the same way as it's called internally and in openssl.

According to the php_openssl_pkey_from_zval(zpkey, 0, passphrase, passphrase_len); invocation, it isn't. But I'll double check it.

Yes, even in the ext/openssl/tests/openssl_pkey_export_basic.phpt test, a public key is used with openssl_pkey_get_details()

Ok I see it in the code but it's a bit artificial as it could be possible to extend it and support pubkey using PEM_write_bio_PUBKEY. Then the name wouldn't be correct and we couldn't easily change it. As pkey is already in the function name, I think it should be fine to rename the argument to pkey. It should be done for other cases as well.

I think this is the last remaining (important) question. @nikic Do you have any preference? To be honest, I don't like the pkey name at all, because it made me google for quite long whether it refers to a private or a public key when I was working on the resource migration. :( So I'd favor a $key, $asymmetric_key name if we really want to support public keys too.

Just $key sounds reasonable to me here. But really, no particular preference. I do think we should use $public_key and $private_key in the APIs that only accept one of them.

I went with $key in case of openssl_pkey_export() and openssl_pkey_export_to_file() to be a bit more forward-compatible :)

ext/openssl/openssl.c

nikic

The code changes look good, the parameter names have too few abbreviations :P

nikic · 2020-09-09T08:43:37Z

ext/openssl/openssl.c

@@ -7296,7 +7292,7 @@ PHP_FUNCTION(openssl_decrypt)
 	size_t data_len, method_len, password_len, iv_len = 0, tag_len = 0, aad_len = 0;
 	zend_string *ret;

-	if (zend_parse_parameters(ZEND_NUM_ARGS(), "sss|lsss", &data, &data_len, &method, &method_len,
+	if (zend_parse_parameters(ZEND_NUM_ARGS(), "sss|lss!s", &data, &data_len, &method, &method_len,


Is it possible to use $tag = "" instead?

Yes, it is, based on } else if (!enc && tag && tag_len > 0) { in php_openssl_cipher_init().

ext/openssl/openssl.stub.php

nikic · 2020-09-09T08:43:37Z

ext/openssl/openssl.stub.php

 */
-function openssl_seal(string $data, &$sealdata, &$ekeys, array $pubkeys, string $method, &$iv = UNKNOWN): int|false {}
+function openssl_seal(string $data, &$sealed_data, &$encrypted_keys, array $public_key, string $cipher_algorithm, &$initialization_vector = null): int|false {}


Suggested change

function openssl_seal(string $data, &$sealed_data, &$encrypted_keys, array $public_key, string $cipher_algorithm, &$initialization_vector = null): int|false {}

function openssl_seal(string $data, &$sealed_data, &$encrypted_keys, array $public_key, string $cipher_algorithm, &$iv = null): int|false {}

IV is typically not spelled out.

I don't feel particularly strongly about this one though. Maybe @bukka has a preference.

Yes this should be changed to iv

I'll do it soon

ext/openssl/openssl.stub.php

nikic

This looks good to me. Any further comments from @bukka?

nikic · 2020-09-11T08:26:04Z

ext/openssl/openssl.c

@@ -4847,7 +4849,7 @@ PHP_FUNCTION(openssl_pkcs7_verify)

 	RETVAL_LONG(-1);

-	if (zend_parse_parameters(ZEND_NUM_ARGS(), "pl|pappp", &filename, &filename_len,
+	if (zend_parse_parameters(ZEND_NUM_ARGS(), "pl|p!a!p!p!p!", &filename, &filename_len,


As far as I can see $cainfo = [] should work.

Though looking at other signatures, I see that this parameter is declared as ?array in other cases, so this makes sense for consistency. Though the default is sometimes null and sometimes []...

You are right, so I'll change the default value of all of them to [].

bukka · 2020-09-12T20:28:54Z

ext/openssl/openssl.stub.php

-/** @param OpenSSLAsymmetricKey $privkey */
-function openssl_csr_new(array $dn, &$privkey, ?array $configargs = null, ?array $extraattribs = null): OpenSSLCertificateSigningRequest|false {}
+/** @param OpenSSLAsymmetricKey $private_key */
+function openssl_csr_new(array $distinguished_names, &$private_key, ?array $options = null, ?array $extra_options = null): OpenSSLCertificateSigningRequest|false {}


So in this case, we are really talking about extra attributes (not options) because those are CSR attributes.

bukka · 2020-09-12T20:31:39Z

ext/openssl/openssl.stub.php


-function openssl_pkcs7_verify(string $filename, int $flags, string $signerscerts = UNKNOWN, array $cainfo = UNKNOWN, string $extracerts = UNKNOWN, string $content = UNKNOWN, string $pk7 = UNKNOWN): bool|int {}
+function openssl_pkcs7_verify(string $filename, int $flags, ?string $output_filename = null, array $ca_info = [], ?string $untrusted_certificates_filename = null, ?string $content = null, ?string $pk7_filename = null): bool|int {}


If you are already changing that parameter, then pk7 is of course incorrect as it doesn't mean anything. It should be pkcs7_filename

From the really quick look I'm not 100% sure what's the use case of that param. It's a bit confusing because output_filename is not really right name here. It should be probably called signers_certificates_filename and the last one should be probably pkcs7_output_filename. Although it's a bit inconsistent with other naming.

also first param should be then pkcs7_input_filename which will make it a bit more consistent.

bukka · 2020-09-12T20:45:59Z

ext/openssl/openssl.stub.php

-/** @param OpenSSLCertificate|array|string $recipcerts */
-function openssl_pkcs7_encrypt(string $infile, string $outfile, $recipcerts, ?array $headers, int $flags = 0, int $cipher = OPENSSL_CIPHER_RC2_40): bool {}
+/** @param OpenSSLCertificate|array|string $certificate */
+function openssl_pkcs7_encrypt(string $filename, string $output_filename, $certificate, ?array $headers, int $flags = 0, int $cipher_algorithm = OPENSSL_CIPHER_RC2_40): bool {}


Maybe we should make clear what's pkcs7 and name output_filename as pkcs7_output_filename. The first param should be input_filename then.

bukka · 2020-09-12T20:47:13Z

ext/openssl/openssl.stub.php

-/** @param OpenSSLAsymmetricKey|OpenSSLCertificate|array|string $signkey */
-function openssl_pkcs7_sign(string $infile, string $outfile, OpenSSLCertificate|string $signcert, $signkey, ?array $headers, int $flags = PKCS7_DETACHED, ?string $extracertsfilename = null): bool {}
+/** @param OpenSSLAsymmetricKey|OpenSSLCertificate|array|string $private_key */
+function openssl_pkcs7_sign(string $filename, string $output_filename, OpenSSLCertificate|string $certificate, $private_key, ?array $headers, int $flags = PKCS7_DETACHED, ?string $untrusted_certificates_filename = null): bool {}


Here again input_filename and then pkcs7_output_filename

bukka · 2020-09-12T20:48:23Z

ext/openssl/openssl.stub.php

 */
-function openssl_pkcs7_decrypt(string $infilename, string $outfilename, $recipcert, $recipkey = null): bool {}
+function openssl_pkcs7_decrypt(string $filename, string $output_filename, $certificate, $private_key = null): bool {}


pkcs7_input_filename

bukka · 2020-09-12T20:48:39Z

ext/openssl/openssl.stub.php

-/** @param array $certs */
-function openssl_pkcs7_read(string $infilename, &$certs): bool {}
+/** @param array $certificates */
+function openssl_pkcs7_read(string $filename, &$certificates): bool {}


pkcs7_input_filename

bukka · 2020-09-12T20:49:06Z

ext/openssl/openssl.stub.php

-function openssl_cms_verify(string $filename, int $flags = 0, ?string $signerscerts = null, ?array $cainfo = null, ?string $extracerts = null, ?string $content = null, ?string $pk7 = null, ?string $sigfile = null, int $encoding = OPENSSL_ENCODING_SMIME): bool {}
+function openssl_cms_verify(string $filename, int $flags = 0, ?string $certificates = null, array $ca_info = [], ?string $untrusted_certificates_filename = null, ?string $content = null, ?string $pk7 = null, ?string $sigfile = null, int $encoding = OPENSSL_ENCODING_SMIME): bool {}

-/** @param OpenSSLCertificate|array|string $recipcerts */
-function openssl_cms_encrypt(string $infile, string $outfile, $recipcerts, ?array $headers, int $flags = 0, int $encoding = OPENSSL_ENCODING_SMIME,  int $cipher = OPENSSL_CIPHER_RC2_40): bool {}
+/** @param OpenSSLCertificate|array|string $certificate */
+function openssl_cms_encrypt(string $filename, string $output_filename, $certificate, ?array $headers, int $flags = 0, int $encoding = OPENSSL_ENCODING_SMIME,  int $cipher_algorithm = OPENSSL_CIPHER_RC2_40): bool {}

-/** @param OpenSSLAsymmetricKey|OpenSSLCertificate|array|string $signkey */
-function openssl_cms_sign(string $infile, string $outfile, OpenSSLCertificate|string $signcert, $signkey, ?array $headers, int $flags = 0, int $encoding = OPENSSL_ENCODING_SMIME, ?string $extracertsfilename = null): bool {}
+/** @param OpenSSLAsymmetricKey|OpenSSLCertificate|array|string $private_key */
+function openssl_cms_sign(string $filename, string $output_filename, OpenSSLCertificate|string $certificate, $private_key, ?array $headers, int $flags = 0, int $encoding = OPENSSL_ENCODING_SMIME, ?string $untrusted_certificates_filename = null): bool {}

 /**
- * @param OpenSSLCertificate|string $recipcert
- * @param OpenSSLAsymmetricKey|OpenSSLCertificate|array|string $recipkey
+ * @param OpenSSLCertificate|string $certificate
+ * @param OpenSSLAsymmetricKey|OpenSSLCertificate|array|string|null $private_key
 */
-function openssl_cms_decrypt(string $infilename, string $outfilename, $recipcert, $recipkey = UNKNOWN, int $encoding = OPENSSL_ENCODING_SMIME): bool {}
+function openssl_cms_decrypt(string $filename, string $output_filename, $certificate, $private_key = null, int $encoding = OPENSSL_ENCODING_SMIME): bool {}

-/** @param array $certs */
-function openssl_cms_read(string $infilename, &$certs): bool {}
+/** @param array $certificates */
+function openssl_cms_read(string $filename, &$certificates): bool {}


basically the same names like for pkcs7 but using cms instead.

Do adding the pkcs7 and cms prefixes make the meaning of $input_filename/$output_filename more clear?

bukka · 2020-09-12T20:49:46Z

ext/openssl/openssl.stub.php

 */
-function openssl_open(string $data, &$opendata, string $ekey, $privkey, string $method, string $iv = UNKNOWN): bool {}
+function openssl_open(string $data, &$output, string $encrypted_key, $private_key, string $cipher_algorithm, ?string $initialization_vector = null): bool {}


iv should be used

bukka · 2020-09-12T20:50:35Z

ext/openssl/openssl.stub.php


 /** @param string $tag */
-function openssl_encrypt(string $data, string $method, string $password, int $options = 0, string $iv = '', &$tag = UNKNOWN, string $aad = '', int $tag_length = 16): string|false {}
+function openssl_encrypt(string $data, string $cipher_algorithm, string $passphrase, int $options = 0, string $initialization_vector = "", &$tag = null, string $additional_authentication_data = "", int $tag_length = 16): string|false {}


please use iv and aad as they are standard terms.

bukka · 2020-09-12T20:50:44Z

ext/openssl/openssl.stub.php


-function openssl_decrypt(string $data, string $method, string $password, int $options = 0, string $iv = '', string $tag = UNKNOWN, string $aad = ''): string|false {}
+function openssl_decrypt(string $data, string $cipher_algorithm, string $passphrase, int $options = 0, string $initialization_vector = "", string $tag = "", string $additional_authentication_data = ""): string|false {}


please use iv and aad as they are standard terms.

I'm waiting for the end of your review, and commit the changes together.

bukka · 2020-09-12T20:53:27Z

@kocsismate Nice work in general but this still needs some additional changes. Please let me know if you are able to create a PR or if I need to do that.

kocsismate · 2020-09-12T20:54:21Z

@bukka I can create one!

bukka · 2020-09-12T21:00:21Z

Nice thanks

bukka · 2020-09-12T21:12:44Z

ext/openssl/openssl.stub.php

 */
-function openssl_private_decrypt(string $data, &$crypted, $key, int $padding = OPENSSL_PKCS1_PADDING): bool {}
+function openssl_private_decrypt(string $data, &$encrypted_data, $private_key, int $padding = OPENSSL_PKCS1_PADDING): bool {}


This is really other way round. First param is from (encrypted data) and the second param is to which is decrypted data. If you want to keep first as data, then the second one should be decrypted_data.

Oops, it's very fortunate that you had another look :) Thank you!

bukka · 2020-09-12T21:13:02Z

ext/openssl/openssl.stub.php

 */
-function openssl_public_decrypt(string $data, &$crypted, $key, int $padding = OPENSSL_PKCS1_PADDING): bool {}
+function openssl_public_decrypt(string $data, &$encrypted_data, $public_key, int $padding = OPENSSL_PKCS1_PADDING): bool {}


Again the second one should be decrypted_data

bukka · 2020-09-12T21:15:56Z

@kocsismate I think that's everything for today. It's a bit hard to get the correct naming as even the names in C code are a bit confusing. Will take another look when you create the next PR.

kocsismate marked this pull request as draft Aug 20, 2020

kocsismate force-pushed the kocsismate:openssl-param-handling branch 6 times, most recently from ef48987 to 3399178 Aug 20, 2020

kocsismate marked this pull request as ready for review Aug 20, 2020

kocsismate requested a review from bukka Aug 20, 2020