OpenSSL extension has many inconsistencies in how errors are handled/reported/propagated
Dieses Issue hat noch niemand übernommen.
- Vorherrschende Sprache
- C
- Sterne
- 40.4k
- Forks
- 8.1k
- Ø Merge
- 2 T. 13 Std.
- Gemergte PRs (30 T.)
- 96
Beschreibung
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):
https://github.com/php/php-src/blob/1a9a2c7052e3e22b21c43b31dda9355b6d975f38/ext/openssl/openssl_backend_common.c#L97-L99
The following two calls may fail because of an inexistent file or some I/O failure
https://github.com/php/php-src/blob/1a9a2c7052e3e22b21c43b31dda9355b6d975f38/ext/openssl/openssl_backend_common.c#L1293-L1297
but at least one of the callsites can't distinguish between different kinds of failure:
https://github.com/php/php-src/blob/1a9a2c7052e3e22b21c43b31dda9355b6d975f38/ext/openssl/openssl.c#L940-L944
Very similarly to the previous one, different kinds of I/O or validation failures are not distinguished here:
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:
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:
https://github.com/php/php-src/blob/6a4d0d9456bf872286722ad5f043ad4928e92c9a/ext/openssl/openssl_backend_common.c#L254-L257
but also in various spots in https://github.com/php/php-src/blob/6a4d0d9456bf872286722ad5f043ad4928e92c9a/ext/openssl/openssl_backend_common.c#L293
etc...
Usage of E_ERROR for some reason, which sometimes could be appropriate but sometimes isn't.
Examples:
here we're dealing with allocation failure so it may be appropriate.
However, here it seems inappropriate:
These are just some examples, but shows a much broader problem with the error-handling design of the extension.
A concrete list found by my tool:
- Function: ASN1_STRING_to_UTF8 | Source: /work/php-src/ext/openssl/openssl_backend_common.c:74 | Return: -1
- Function: BIO_new_mem_buf | Source: /work/php-src/ext/openssl/openssl_backend_common.c:1269 | Return: 0
- Function: BIO_new_mem_buf | Source: /work/php-src/ext/openssl/openssl_backend_common.c:532 | Return: 0
- Function: PEM_read_bio_PrivateKey_ex | Source: /work/php-src/ext/openssl/openssl_backend_common.c:1292 | Return: 0
- Function: X509_LOOKUP_ctrl | Source: /work/php-src/ext/openssl/openssl_backend_common.c:834 | Return: -1
- Function: X509_LOOKUP_ctrl | Source: /work/php-src/ext/openssl/openssl_backend_common.c:834 | Return: 0
- Function: X509_LOOKUP_ctrl | Source: /work/php-src/ext/openssl/openssl_backend_common.c:840 | Return: -1
- Function: X509_LOOKUP_ctrl | Source: /work/php-src/ext/openssl/openssl_backend_common.c:840 | Return: 0
- Function: X509_STORE_add_lookup | Source: /work/php-src/ext/openssl/openssl_backend_common.c:834 | Return: 0
- Function: X509_STORE_add_lookup | Source: /work/php-src/ext/openssl/openssl_backend_common.c:840 | Return: 0
- Function: NCONF_load | Source: /work/php-src/ext/openssl/openssl_backend_common.c:301 | Return: 0
- Function: NCONF_get_string | Source: /work/php-src/ext/openssl/openssl_backend_common.c:255 | Return: 0
- Function: NCONF_get_string | Source: /work/php-src/ext/openssl/openssl_backend_common.c:323 | Return: 0
- Function: NCONF_get_string | Source: /work/php-src/ext/openssl/openssl_backend_common.c:325 | Return: 0
- Function: NCONF_get_string | Source: /work/php-src/ext/openssl/openssl_backend_common.c:327 | Return: 0
- Function: NCONF_get_string | Source: /work/php-src/ext/openssl/openssl_backend_common.c:396 | Return: 0
- Function: NCONF_get_string | Source: /work/php-src/ext/openssl/openssl_backend_common.c:939 | Return: 0
- Function: NCONF_get_string | Source: /work/php-src/ext/openssl/openssl_backend_common.c:949 | Return: 0
- Function: PEM_write_bio_X509 | Source: /work/php-src/ext/openssl/openssl.c:1507 | Return: 0
- Function: PEM_write_bio_X509 | Source: /work/php-src/ext/openssl/openssl.c:1541 | Return: 0
- Function: PEM_write_bio_X509 | Source: /work/php-src/ext/openssl/openssl.c:2791 | Return: 0
- Function: PEM_write_bio_X509 | Source: /work/php-src/ext/openssl/openssl.c:3443 | Return: 0
- Function: PEM_write_bio_PrivateKey | Source: /work/php-src/ext/openssl/openssl.c:1520 | Return: 0
- Function: PEM_ASN1_read_bio | Source: /work/php-src/ext/openssl/openssl_backend_v3.c:871 | Return: 0
- Function: BIO_new | Source: /work/php-src/ext/openssl/openssl.c:1507 | Return: 0
PHP Version
Report was made on master, but applies to all versions really.
Operating System
No response
Beitragsleitfaden
Erste Schritte
- Lies das ganze Issue und danach den Beitragsleitfaden des Projekts.
- Schreib ins Issue, dass du es übernimmst — das erspart doppelte Arbeit.
- Forke das Repository und arbeite in einem Branch.
- Öffne einen Pull Request, der die Issue-Nummer nennt.
Rechercherichtung
Beginne mit der Überprüfung der Fehlerpfade in ext/openssl/openssl_backend_common.c und ext/openssl/openssl.c, insbesondere der genannten Aufrufe von php_openssl_store_errors(), php_openssl_conf_get_string() und den OpenSSL-I/O-Funktionen. Vergleiche die aufgeführten Inkonsistenzen und lege eine konsistente Richtlinie für Rückgabewerte, Warnungen, gespeicherte Fehler und die Behandlung von E_ERROR fest. Als abgeschlossen gilt die Aufgabe, wenn die Erweiterung über ein abgestimmtes Design verfügt, die betroffenen Aufrufstellen diesem konsistent folgen und das geänderte Verhalten abgedeckt ist.
Vom Indexierungsmodell aus dem Issue-Text verfasst.
Bewertung
- Tech-Stack
- c
- Bereich
- backend, security
- Issue-Typ
- Refactoring
- Schwierigkeit
- 5/5
- Geschätzter Aufwand
- Über eine Woche
- Aktivitätsstatus
- Ruhig
- Klarheit
- Muss geklärt werden
- Anfängerfreundlichkeit
- 25/100