Bug #79501 [ReO]: TLS connections freezing on 7.4 (all versions after 7.3.17)

Edit report at https://bugs.php.net/bug.php?id=79501&edit=1 ID: 79501 Updated by: bukka@php.net Reported by: imnieves at gmail dot com Summary: TLS connections freezing on 7.4 (all versions after 7.3.17) Status: Re-Opened Type: Bug Package: OpenSSL related Operating System: Linux PHP Version: 7.4.5 -Assigned To: +Assigned To: bukka Block user comment: N Private report: N New Comment: I have done some investigation and updated the linked issue in phpredis as this might potentially be a problem with API usage there - it calls php_stream_eof before php_stream_write. Previous Comments: ------------------------------------------------------------------------ [2022-02-06 07:57:21] imnieves at gmail dot com This bug has also noticed the same underlying problem: https://github.com/reactphp/socket/issues/184 The problem is that when a server has sent no data, a client side call to feof or stream_eof will block when TLS 1.3 is used and will not block when TLS 1.2 is used. I am quite confident my fix/workaround above is not correct. ------------------------------------------------------------------------ [2022-02-02 22:29:45] imnieves at gmail dot com Here is the second part of the message. I was able to get TLS 1.3 working in PHP 7.4 by wrapping the SSL_peek function call and the logic immediately following that call with a call to SSL_pending. SSL_pending returns the count of data bytes that can be read, and these are application data bytes not the non-application data records, so if SSL_pending > 0 then SSL_peek will not block. Beyond that, SSL_pending itself will not block, so it is safe to call. To show what I did, I converted these lines from: https://github.com/php/php-src/blob/96f753a2b56b7c4927f1a64253ca60bb481ee2c3/ext/openssl/xp_ssl.c#L2466 int n = SSL_peek(sslsock->ssl_handle, &buf, sizeof(buf)); if (n <= 0) { int err = SSL_get_error(sslsock->ssl_handle, n); switch (err) { case SSL_ERROR_SYSCALL: alive = php_socket_errno() == EAGAIN; break; case SSL_ERROR_WANT_READ: case SSL_ERROR_WANT_WRITE: alive = 1; break; default: /* any other problem is a fatal error */ alive = 0; } } to these lines: if(SSL_pending(sslsock->ssl_handle)){ int n = SSL_peek(sslsock->ssl_handle, &buf, sizeof(buf)); if (n <= 0) { int err = SSL_get_error(sslsock->ssl_handle, n); switch (err) { case SSL_ERROR_SYSCALL: alive = php_socket_errno() == EAGAIN; break; case SSL_ERROR_WANT_READ: case SSL_ERROR_WANT_WRITE: alive = 1; break; default: /* any other problem is a fatal error */ alive = 0; } } } I am not claiming this fixes the problem entirely or that there are no side-effects, as I did not run any automated tests or any other manual tests. But this seemingly improved behavior in our particular use case seems to increase the likelihood of the location of the flawed code. In summary, I am thinking the issue is related how PHP streams wraps OpenSSL. The issue seems to be the incorrect assumption that if records are available to read then SSL_peek will not block. Finally, although this issue seems to arise mostly in TLS 1.3, this is quite likely also an issue in PHP streams on TLS 1.2, although very (possibly very very) rare. For reference on that last sentence, see the first comment by mattcaswell at: https://github.com/openssl/openssl/issues/7327 Further reference: https://www.openssl.org/docs/man1.1.1/man3/SSL_pending.html https://www.openssl.org/docs/man1.1.1/man3/SSL_read.html ------------------------------------------------------------------------ [2022-02-02 22:28:54] imnieves at gmail dot com I will break this message into two parts, hopefully that will help this message to not be detected as SPAM. I was able to determine the exact line that is blocking in PHP series 7.4 (7.4..27 precisely) when TLS 1.3 is being used. The offending line is: https://github.com/php/php-src/blob/96f753a2b56b7c4927f1a64253ca60bb481ee2c3/ext/openssl/xp_ssl.c#L2466 int n = SSL_peek(sslsock->ssl_handle, &buf, sizeof(buf)); For reference, the PHP method that calls this SSL_peek is itself called at: https://github.com/php/php-src/blob/96f753a2b56b7c4927f1a64253ca60bb481ee2c3/main/streams/streams.c#L790 if (!stream->eof && PHP_STREAM_OPTION_RETURN_ERR == php_stream_set_option(stream, PHP_STREAM_OPTION_CHECK_LIVENESS, 0, NULL)) { stream->eof = 1; } After reading about so-called SSL non-application data records: https://github.com/openssl/openssl/blob/6e94b5aecd619afd25e3dc25902952b1b3194edf/CHANGES#L237 https://wiki.openssl.org/index.php/TLS1.3#Non-application_data_records openssl/openssl#7327 I believe the blocking is occurring because PHP 7.4 is assuming (in xp_ssl.c @ L2466, the first code snippet above) that if there is a record that can be read, then the record must be a data record and therefore the SSL_peek will not block. This assumption is false in both TLS 1.2 and TLS 1.3, however the impact of this assumption is felt rarely (if at all) in TLS 1.2 but it arises frequently in TLS 1.3, in fact it arises instantly in TLS 1.3. In TLS 1.2, this line: https://github.com/php/php-src/blob/96f753a2b56b7c4927f1a64253ca60bb481ee2c3/ext/openssl/xp_ssl.c#L2464 } else if (php_pollfd_for(sslsock->s.socket, PHP_POLLREADABLE|POLLPRI, &tv) > 0) { evaluates to false. But in TLS 1.3 it evaluates to true, which enables SSL_peek to be called, and block. ------------------------------------------------------------------------ [2020-08-08 17:58:30] imnieves at gmail dot com According to comment from a maintainer of PhpRedis[1], they feel that the issue is not coming from PhpRedis itself: | I'm going to close this issue because it is actually not a bug in phpredis. [1] https://github.com/phpredis/phpredis/issues/1726#issuecomment-652563196 ------------------------------------------------------------------------ [2020-05-08 11:41:24] cmb@php.net According to a comment on the PhpRedis issue[1] this might be an issue with our tlsv1.3 stream wrapper implementation: | […] because right now it looks like something may be wrong in | PhpRedis even if it just wraps php streams. [1] <https://github.com/phpredis/phpredis/issues/1726#issuecomment-625319182> ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=79501 -- Edit this bug report at https://bugs.php.net/bug.php?id=79501&edit=1