php / php/php-src

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

Abierto
#21,643 0 comentarios 2 reacciones 0 asignados Ver en GitHub

Nadie ha tomado este issue todavía.

Bug Extension: openssl Status: Needs Triage
Lenguaje dominante
C
Estrellas
40.4k
Forks
8.2k
Merge medio
2 d 13 h
PR fusionados (30 d)
96

Descripción

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:

https://github.com/php/php-src/blob/1a9a2c7052e3e22b21c43b31dda9355b6d975f38/ext/openssl/openssl_backend_common.c#L511


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:

https://github.com/php/php-src/blob/1a9a2c7052e3e22b21c43b31dda9355b6d975f38/ext/openssl/openssl_backend_common.c#L827-L838


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:

https://github.com/php/php-src/blob/1a9a2c7052e3e22b21c43b31dda9355b6d975f38/ext/openssl/openssl_backend_common.c#L742-L747

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

However, here it seems inappropriate:

https://github.com/php/php-src/blob/1a9a2c7052e3e22b21c43b31dda9355b6d975f38/ext/openssl/openssl_backend_common.c#L597-L601


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

Guía de contribución

Abrir la guía de contribución

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Línea de trabajo

Comienza revisando las rutas de error en ext/openssl/openssl_backend_common.c y ext/openssl/openssl.c, especialmente las llamadas citadas a php_openssl_store_errors(), php_openssl_conf_get_string() y las funciones de E/S de OpenSSL. Compara las inconsistencias enumeradas y establece una política coherente para los valores de retorno, las advertencias, los errores almacenados y el manejo de E_ERROR. La tarea estará terminada cuando la extensión tenga un diseño acordado, los sitios de llamada afectados lo sigan de forma coherente y exista cobertura para el comportamiento modificado.

Escrito por el modelo de indexación a partir del texto del issue.

Evaluación

Stack tecnológico
c
Área
backend, security
Tipo de issue
Refactorización
Dificultad
5/5
Tiempo estimado
Más de una semana
Estado de actividad
Tranquilo
Claridad
Necesita aclaración
Aptitud para principiantes
25/100

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.