php / php/php-src

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

オープン
#21,643 コメント 0 件 リアクション 2 件 担当者 0 名 GitHub で見る

まだ誰も着手していません。

Bug Extension: openssl Status: Needs Triage
主要言語
C
スター
40.4k
フォーク
8.2k
平均マージ
2日 13時間
マージ済み PR(30日)
96

説明

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

コントリビューションガイド

コントリビューションガイドを開く

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

調査の方向性

まず ext/openssl/openssl_backend_common.c と ext/openssl/openssl.c のエラーパスを確認します。特に、記載されている php_openssl_store_errors()、php_openssl_conf_get_string() の呼び出し、および OpenSSL I/O 関数を重点的に確認してください。列挙された不整合を比較し、戻り値、警告、保存されるエラー、E_ERROR の処理について一貫した方針を確立します。拡張機能について合意済みの設計が定まり、影響を受ける呼び出し箇所が一貫してそれに従い、変更された動作に対するカバレッジが確保されれば完了です。

索引モデルが issue の本文から書いたものです。

評価

技術スタック
c
領域
backend, security
issue の種類
リファクタリング
難易度
5/5
見積もり時間
1週間以上
活発さ
静か
明瞭さ
説明が足りない
初心者へのやさしさ
25/100

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。