OpenSSL extension has many inconsistencies in how errors are handled/reported/propagated

[ndossche]

Description

The OpenSSL extension has a number of error-handling inconsistencies that I'm unsure how to handle. We should come to a consensus on how to move forward.
Inconsistencies include:

• Failure reason is not properly communicated to the caller of the function, or can't be distinguished at the call site.
• Some functions emit warnings, some don't.
• Some error-handling paths call php_openssl_store_errors(), some don't.
• Some error-handling paths call php_openssl_store_errors() but still return a success.

This means that partial success isn't obviously reported to the user in some cases, so theoretically they'd always have to check the openssl_error_string() function even if the function returned success, which isn't great.

Some examples

Partial failure example (error stored but loop not aborted, may be intentional but this isn't communicated):

php-src/ext/openssl/openssl_backend_common.c

Lines 97 to 99 in 1a9a2c7

	} else {
	php_openssl_store_errors();
	}

---

The following two calls may fail because of an inexistent file or some I/O failure

php-src/ext/openssl/openssl_backend_common.c

Lines 1293 to 1297 in 1a9a2c7

	if (is_file) {
	in = BIO_new_file(file_path, PHP_OPENSSL_BIO_MODE_R(PKCS7_BINARY));
	} else {
	in = BIO_new_mem_buf(ZSTR_VAL(val_str), (int)ZSTR_LEN(val_str));
	}

but at least one of the callsites can't distinguish between different kinds of failure:

php-src/ext/openssl/openssl.c

Lines 940 to 944 in 1a9a2c7

	key = php_openssl_pkey_from_zval(zkey, 0, "", 0, 2);
	if (key) {
	RETVAL_BOOL(X509_check_private_key(cert, key));
	EVP_PKEY_free(key);
	}

---

Very similarly to the previous one, different kinds of I/O or validation failures are not distinguished here:

php-src/ext/openssl/openssl_backend_common.c

Line 511 in 1a9a2c7

X509 *php_openssl_x509_from_str(

---

Example of silently swallowing errors without returning a different return value or without emitting a warning; even though this same function emits warnings for other cases:

php-src/ext/openssl/openssl_backend_common.c

Lines 827 to 838 in 1a9a2c7

	if (nfiles == 0) {
	file_lookup = X509_STORE_add_lookup(store, X509_LOOKUP_file());
	if (file_lookup == NULL || !X509_LOOKUP_load_file(file_lookup, NULL, X509_FILETYPE_DEFAULT)) {
	php_openssl_store_errors();
	}
	}
	if (ndirs == 0) {
	dir_lookup = X509_STORE_add_lookup(store, X509_LOOKUP_hash_dir());
	if (dir_lookup == NULL || !X509_LOOKUP_add_dir(dir_lookup, NULL, X509_FILETYPE_DEFAULT)) {
	php_openssl_store_errors();
	}
	}

---

There's a bunch of calls to php_openssl_conf_get_string that silently ignore errors: because e.g. the key may be optional but note that a configuration error can also result in a silent failure.

For example here:

php-src/ext/openssl/openssl_backend_common.c

Lines 254 to 257 in 6a4d0d9

	str = php_openssl_conf_get_string(req->req_config, NULL, "oid_section");
	if (str == NULL) {
	return SUCCESS;
	}

but also in various spots in

php-src/ext/openssl/openssl_backend_common.c

Line 293 in 6a4d0d9

int php_openssl_parse_config(struct php_x509_request * req, zval * optional_args)

etc...

---

Usage of E_ERROR for some reason, which sometimes could be appropriate but sometimes isn't.
Examples:

php-src/ext/openssl/openssl_backend_common.c

Lines 742 to 747 in 1a9a2c7

	csc = X509_STORE_CTX_new();
	if (csc == NULL) {
	php_openssl_store_errors();
	php_error_docref(NULL, E_ERROR, "Memory allocation failure");
	return 0;
	}

here we're dealing with allocation failure so it may be appropriate.

However, here it seems inappropriate:

php-src/ext/openssl/openssl_backend_common.c

Lines 597 to 601 in 1a9a2c7

	} else if (!X509_digest(peer, mdtype, md, &n)) {
	php_openssl_store_errors();
	php_error_docref(NULL, E_ERROR, "Could not generate signature");
	return NULL;
	}

---

These are just some examples, but shows a much broader problem with the error-handling design of the extension.

PHP Version

Report was made on master, but applies to all versions really.

Operating System

No response