Promote some OpenSSL warnings to Errors #5111

Girgias · 2020-01-24T23:05:34Z

So I went pretty conservative here with changes which seems non controversial.

bukka · 2020-02-24T18:52:26Z

Was there RFC for this?

kocsismate · 2020-02-24T19:30:58Z

@bukka It belongs to the https://wiki.php.net/rfc/engine_warnings RFC.

bukka · 2020-02-26T18:37:39Z

I would get the incorrect type (e.g. overflows) being classified like that. However the RFC doesn't cover things like "algorithm not found". There is lots of code that depends on this so this is not a good idea to do and it has nothing to do with the linked RFC.

Girgias · 2020-02-26T23:00:39Z

I would get the incorrect type (e.g. overflows) being classified like that. However the RFC doesn't cover things like "algorithm not found". There is lots of code that depends on this so this is not a good idea to do and it has nothing to do with the linked RFC.

So for the most part these conversions comes from Nikita's comment on a PoC I made last summer: #4549 (comment)

This resulted in a lot of warnings being promoted to Error/ValueError/TypeError (the type error one does have the consistent TypeError RFC to back it up).

However, as I don't know how most of the extensions work I mostly chugger through by converting things which seem reasonable to me that's why it's on review to see if some of them would be very disruptive.

I hope that you do agree that these conversions have value for stuff which is plainly false (such as type errors, or wrong flags/values).

To address the "algorithm not found" promotion, this may well be not wise, is it because some algorithms may be available on some versions of OpenSSL and not on others? In that case not promoting it would make a lot of sense.

These promotions are there to flag obvious and guaranteed mistakes, so that clearly broken code indicates that its broken. Not really making it hard to upgrade (for the most part).

kocsismate · 2020-06-22T07:41:46Z

ext/openssl/openssl.c

@@ -780,7 +777,7 @@ static int php_openssl_parse_config(struct php_x509_request * req, zval * option
 		zend_long cipher_algo = Z_LVAL_P(item);
 		const EVP_CIPHER* cipher = php_openssl_get_evp_cipher_from_algo(cipher_algo);
 		if (cipher == NULL) {
-			php_error_docref(NULL, E_WARNING, "Unknown cipher algorithm for private key.");
+			zend_value_error("Unknown cipher algorithm for private key.");


The punctuation mark is to be avoided :)

However, in case of issues with array items of arguments, we usually show a similar message to this:
"deflate_init(): \"memory\" option must be between 1 and 9".

Maybe this message could be improved by also showing the argument name somehow. We just haven't found a good way yet to do so.

The same comment applies to the 2 changes below.

kocsismate · 2020-06-22T07:49:11Z

ext/openssl/openssl.c

+
+	mdtype = php_openssl_get_evp_md_from_algo(algo);
+	if (!mdtype) {
+		zend_argument_value_error(3, "Unknown signature algorithm");


Suggested change

zend_argument_value_error(3, "Unknown signature algorithm");

zend_argument_value_error(3, "must be a valid signature algorithm");

or

Suggested change

zend_argument_value_error(3, "Unknown signature algorithm");

zend_argument_value_error(3, "is an unknown signature algorithm");

I think we use the first format more often

kocsismate · 2020-06-22T07:54:53Z

ext/openssl/openssl.c


 	if (x509 == NULL) {
-		php_error_docref(NULL, E_WARNING, "Supplied parameter cannot be coerced into an X509 certificate!");
-		RETURN_FALSE;
+		zend_argument_type_error(1, "cannot be coerced into an X509 certificate");


this sounds very weird. Couldn't we use must be of type string or OpenSSL-resource, %s given?

It's been a while as I was just rebasing, but there are multiple OpenSSL resources (see: https://www.php.net/manual/en/openssl.resources.php) so that message wouldn't be correct either, maybe we should first work on the resource to object conversion and then fave another look at promoting the warnings?

Conversion is done :)

kocsismate · 2020-06-22T07:58:27Z

ext/openssl/openssl.c

@@ -2958,9 +2962,9 @@ PHP_FUNCTION(openssl_csr_export_to_file)
 	csr = php_openssl_csr_from_zval(zcsr, 0, &csr_resource);
 	if (csr == NULL) {
 		if (!EG(exception)) {
-			php_error_docref(NULL, E_WARNING, "Cannot get CSR from parameter 1");
+			zend_argument_type_error(1, "cannot be coerced into an Certificate Signing Request (CSR)");


A similar message could be used what I suggested in case of the X509 certificate

kocsismate · 2020-06-22T08:01:35Z

ext/openssl/openssl.c

@@ -3427,7 +3431,7 @@ static EVP_PKEY * php_openssl_evp_from_zval(
 		/* get passphrase */

 		if ((zphrase = zend_hash_index_find(Z_ARRVAL_P(val), 1)) == NULL) {
-			php_error_docref(NULL, E_WARNING, "Key array must be of the form array(0 => key, 1 => phrase)");
+			zend_value_error("Key array must be of the form array(0 => key, 1 => phrase)");


Suggested change

zend_value_error("Key array must be of the form array(0 => key, 1 => phrase)");

zend_argument_value_error(val_argnum, "must be of the form array(0 => key, 1 => phrase)");

kocsismate · 2020-06-22T08:04:10Z

ext/openssl/openssl.c

@@ -3588,7 +3592,7 @@ static EVP_PKEY * php_openssl_generate_private_key(struct php_x509_request * req
 	EVP_PKEY * return_val = NULL;

 	if (req->priv_key_bits < MIN_KEY_LENGTH) {
-		php_error_docref(NULL, E_WARNING, "Private key length is too short; it needs to be at least %d bits, not %d",
+		zend_value_error("Private key length is too short; it needs to be at least %d bits, not %d",


Suggested change

zend_value_error("Private key length is too short; it needs to be at least %d bits, not %d",

zend_argument_value_error(argnum, "must be at least %d bits", ...);

kocsismate · 2020-06-22T08:05:13Z

ext/openssl/openssl.c

@@ -4082,7 +4086,7 @@ PHP_FUNCTION(openssl_pkey_new)
 					}

 					if (group == NULL) {
-						php_error_docref(NULL, E_WARNING, "Unknown curve_name");
+						zend_argument_value_error(1, "Unknown curve_name");


Suggested change

zend_argument_value_error(1, "Unknown curve_name");

zend_argument_value_error(1, "must be a valid curve name");

kocsismate · 2020-06-22T08:06:16Z

ext/openssl/openssl.c

 	if (key == NULL) {
 		if (!EG(exception)) {
-			php_error_docref(NULL, E_WARNING, "Cannot get key from parameter 1");
+			// TypeError?
+			zend_argument_value_error(1, "cannot retrieve key");


Something like:

Suggested change

zend_argument_value_error(1, "cannot retrieve key");

zend_argument_value_error(1, "doesn't have a key");

I'm not exactly sure how to phrase this though

That doesn't seem to be correct looking at the implementation of php_openssl_evp_from_zval() which looks it can fail for weird reasons.

Should I drop this ?

kocsismate · 2020-06-22T08:06:25Z

ext/openssl/openssl.c

 	if (key == NULL) {
 		if (!EG(exception)) {
-			php_error_docref(NULL, E_WARNING, "Cannot get key from parameter 1");
+			zend_argument_value_error(1, "cannot retrieve key");


same as before

Shoud I drop this?

kocsismate · 2020-06-22T08:06:48Z

ext/openssl/openssl.c

@@ -4718,15 +4733,10 @@ PHP_FUNCTION(openssl_pbkdf2)
 	}

 	if (!digest) {
-		php_error_docref(NULL, E_WARNING, "Unknown signature algorithm");
-		RETURN_FALSE;
+		zend_argument_value_error(5, "Unknown signature algorithm");


bad format :)

Girgias · 2020-06-22T13:05:45Z

@kocsismate I think I've address all the error messages so that they adhere to the common format, however I did not address the content of them when they needed me to start passing the argument number, reason why is that most are part of the extension API and I'm not exactly sure how to go about this.

bukka · 2020-08-01T12:29:02Z

ext/openssl/openssl.c

 /* check if size_t can be safely casted to unsigned int */
-#define PHP_OPENSSL_CHECK_SIZE_T_TO_UINT(_var, _name) \
-	PHP_OPENSSL_CHECK_NUMBER_CONVERSION(ZEND_SIZE_T_UINT_OVFL(_var), _name)
+#define PHP_OPENSSL_CHECK_SIZE_T_TO_UINT(_var, _name, arg_num) \


Please use _arg_num - macro arguments should start with _

bukka · 2020-08-01T12:29:08Z

ext/openssl/openssl.c

-#define PHP_OPENSSL_CHECK_SIZE_T_TO_INT_NORET(_var, _name) \
-	PHP_OPENSSL_CHECK_NUMBER_CONVERSION_NORET(ZEND_SIZE_T_INT_OVFL(_var), _name)
+#define PHP_OPENSSL_CHECK_SIZE_T_TO_INT(_var, _name, arg_num) \
+	PHP_OPENSSL_CHECK_NUMBER_CONVERSION(ZEND_SIZE_T_INT_OVFL(_var), _name, arg_num)


Please use _arg_num - macro arguments should start with _

bukka · 2020-08-01T12:29:22Z

ext/openssl/openssl.c

-/* check if long can be safely casted to int */
-#define PHP_OPENSSL_CHECK_LONG_TO_INT_NORET(_var, _name) \
-	PHP_OPENSSL_CHECK_NUMBER_CONVERSION_NORET(ZEND_LONG_EXCEEDS_INT(_var), _name)
+#define PHP_OPENSSL_CHECK_LONG_TO_INT(_var, _name, arg_num) \


Please use _arg_num - macro arguments should start with _

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

ext/openssl/openssl.c

@@ -537,14 +534,14 @@ static time_t php_openssl_asn1_time_to_time_t(ASN1_UTCTIME * timestr) /* {{{ */
 	size_t timestr_len;

 	if (ASN1_STRING_type(timestr) != V_ASN1_UTCTIME && ASN1_STRING_type(timestr) != V_ASN1_GENERALIZEDTIME) {
-		php_error_docref(NULL, E_WARNING, "Illegal ASN1 data type for timestamp");
+		zend_value_error("Illegal ASN1 data type for timestamp");


Please do not change this one as it would be inconsistent with some warnings below. This is used just by openssl_x509_parse so there is still value to not fail the whole operation to see the rest of the fields for in this case incorrectly formatted cert.

bukka · 2020-08-01T12:36:57Z

ext/openssl/openssl.c

 		return (time_t)-1;
 	}

 	timestr_len = (size_t)ASN1_STRING_length(timestr);

 	if (timestr_len != strlen((const char *)ASN1_STRING_get0_data(timestr))) {
-		php_error_docref(NULL, E_WARNING, "Illegal length in timestamp");
+		zend_value_error("Illegal length in timestamp");


The same as above - please do not change this one.

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

ext/openssl/openssl.c

@@ -780,7 +777,7 @@ static int php_openssl_parse_config(struct php_x509_request * req, zval * option
 		zend_long cipher_algo = Z_LVAL_P(item);
 		const EVP_CIPHER* cipher = php_openssl_get_evp_cipher_from_algo(cipher_algo);
 		if (cipher == NULL) {
-			php_error_docref(NULL, E_WARNING, "Unknown cipher algorithm for private key.");
+			zend_value_error("Unknown cipher algorithm for private key");


So this cipher is used only if there is a private key passphrase and it can be caused by invalid conflict but doesn't need to be an issue because passprhase is not always used or it can be just something wrong in the config. I'd keep warning in here rather than prevent the whole operation to minimize BC breaks.

bukka · 2020-08-01T12:42:14Z

ext/openssl/openssl.c

@@ -811,7 +808,7 @@ static int php_openssl_parse_config(struct php_x509_request * req, zval * option
 		&& Z_TYPE_P(item) == IS_STRING) {
 		req->curve_name = OBJ_sn2nid(Z_STRVAL_P(item));
 		if (req->curve_name == NID_undef) {
-			php_error_docref(NULL, E_WARNING, "Unknown elliptic curve (short) name %s", Z_STRVAL_P(item));
+			zend_value_error("Unknown elliptic curve (short) name %s", Z_STRVAL_P(item));


again this doesn't need to be always an issue so please keep it as warning.

bukka · 2020-08-01T12:42:45Z

ext/openssl/openssl.c

@@ -822,7 +819,7 @@ static int php_openssl_parse_config(struct php_x509_request * req, zval * option
 	if (str == NULL) {
 		php_openssl_store_errors();
 	} else if (!ASN1_STRING_set_default_mask_asc(str)) {
-		php_error_docref(NULL, E_WARNING, "Invalid global string mask setting %s", str);
+		zend_value_error("Invalid global string mask setting %s", str);


same as above - please keep warning here

bukka · 2020-08-01T12:50:43Z

ext/openssl/openssl.c

@@ -3588,7 +3592,7 @@ static EVP_PKEY * php_openssl_generate_private_key(struct php_x509_request * req
 	EVP_PKEY * return_val = NULL;

 	if (req->priv_key_bits < MIN_KEY_LENGTH) {
-		php_error_docref(NULL, E_WARNING, "Private key length is too short; it needs to be at least %d bits, not %d",
+		zend_value_error("Private key length is too short; it needs to be at least %d bits, not %d",


if you are changing this, you should change this one as well:

php-src/ext/openssl/openssl.c

Line 3707 in 8c1fdd2

php_error_docref(NULL, E_WARNING, "Unsupported private key type");

bukka · 2020-08-01T12:51:56Z

ext/openssl/openssl.c

@@ -4939,6 +4949,7 @@ PHP_FUNCTION(openssl_pkcs7_encrypt)

 			cert = php_openssl_x509_from_zval(zcertval, 0, &certresource);
 			if (cert == NULL) {
+				// TODO Convert to ValueError?


yeah that should be done for consistency

ext/openssl/openssl.c

bukka · 2020-08-03T20:20:22Z

@Girgias @kocsismate @nikic So I have checked out the branch and have been looking closely on this. I think x509 coercion errors make things actually quite inconsistent. For example it throws if you pass an invalid input file (e.g. when path is invalid) but for output file parameters it is just a warning (e.g. the output files). Then there are bunch of other warnings that are result of the parameters that are passed in or potential failures in openssl and they are still warnings. What I want to say is that we should either throw exception for all cases or just use warnings. To be honest the only ones that should be probably converted if we stick with warnings are the int overflows (although you could still argue that the type is not really wrong from PHP point of view - it's just the conversion to the OpenSSL expected type but it's a bit edge case so exception probably makes more sense).

Considering how big break it would be to change everything to exception and the fact that the resource to objects is already quite a big break, it might be best to keep warnings. I think it would be better to later create a new objective API (methods on the classes that were added) and use exceptions for everything in there.

Girgias · 2020-08-04T07:49:39Z

ext/openssl/openssl.c

 	if (key == NULL) {
 		if (!EG(exception)) {
-			php_error_docref(NULL, E_WARNING, "Cannot get key from parameter 1");
+			// TypeError?
+			zend_argument_value_error(1, "cannot retrieve key");


Should I drop this ?

Girgias · 2020-08-04T07:49:57Z

ext/openssl/openssl.c

 	if (key == NULL) {
 		if (!EG(exception)) {
-			php_error_docref(NULL, E_WARNING, "Cannot get key from parameter 1");
+			zend_argument_value_error(1, "cannot retrieve key");


Shoud I drop this?

ext/openssl/openssl.c

bukka · 2020-08-04T14:59:31Z

ext/openssl/openssl.c

@@ -3563,7 +3564,7 @@ static EVP_PKEY *php_openssl_pkey_from_zval(zval *val, int public_key, char *pas
 		/* get passphrase */

 		if ((zphrase = zend_hash_index_find(Z_ARRVAL_P(val), 1)) == NULL) {
-			php_error_docref(NULL, E_WARNING, "Key array must be of the form array(0 => key, 1 => phrase)");
+			zend_value_error("Key array must be of the form array(0 => key, 1 => phrase)");
