php / php/php-src

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

Đang mở
#21,643 0 bình luận 2 reaction 0 người được giao Xem trên GitHub

Chưa có ai nhận issue này.

Bug Extension: openssl Status: Needs Triage
Ngôn ngữ chính
C
Star
40.4k
Fork
8.2k
Merge trung bình
2 ngày 13 giờ
Pull request đã merge (30 ngày)
96

Mô tả

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

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Hướng nghiên cứu

Bắt đầu bằng việc xem xét các đường dẫn lỗi trong ext/openssl/openssl_backend_common.c và ext/openssl/openssl.c, đặc biệt là các lệnh gọi được nêu đến php_openssl_store_errors(), php_openssl_conf_get_string() và các hàm I/O của OpenSSL. So sánh các điểm không nhất quán đã liệt kê và thiết lập một chính sách nhất quán cho các giá trị trả về, cảnh báo, lỗi được lưu trữ và việc xử lý E_ERROR. Công việc được xem là hoàn tất khi extension có một thiết kế đã được thống nhất, các vị trí gọi bị ảnh hưởng tuân theo thiết kế đó một cách nhất quán và hành vi đã thay đổi có coverage.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
c
Lĩnh vực
backend, security
Loại issue
Tái cấu trúc
Độ khó
5/5
Thời gian dự kiến
Hơn một tuần
Mức độ hoạt động
Ít trao đổi
Độ rõ ràng
Cần làm rõ
Mức phù hợp với người mới
25/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.