Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop special casing for IIS to cover TLS protocol error #17863

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Feb 19, 2025

This had been introduced to fix bug 23220[1], although it is not even clear that IIS caused the problem (some comments mention that this happened with Apache).

Anyhow, in hindsight it seems to be a bad idea to brush warnings about improper implementation of the TLS protocol under the carpet; if a server does not properly implement TLS, users should be aware of that, and report that to the respective developers.

[1] https://bugs.php.net/bug.php?id=23220


The remaining ERR_get_error() != 0 condition is also somehow fishy; it was supposed to be a fix for https://bugs.php.net/bug.php?id=41770, but the OP claimed it wouldn't fix their issue. And even more confusing: that check is in a branch that is only executed if ERR_peek_error() == 0, so the warning appears to be unreachable.

And from the OpenSSL 1.1.1 documentation;

The SSL_ERROR_SYSCALL with errno value of 0 indicates unexpected EOF from the peer. This will be properly reported as SSL_ERROR_SSL with reason code SSL_R_UNEXPECTED_EOF_WHILE_READING in the OpenSSL 3.0 release because it is truly a TLS protocol error to terminate the connection without a SSL_shutdown().

This had been introduced to fix bug 23220[1], although it is not even
clear that IIS caused the problem (some comments mention that this
happened with Apache).

Anyhow, in hindsight it seems to be a bad idea to brush warnings about
improper implementation of the *TLS* protocol under the carpet; if a
server does not properly implement TLS, users should be aware of that,
and report that to the respective developers.

[1] <https://bugs.php.net/bug.php?id=23220>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant