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

Incorrect PHP_STREAM_OPTION_CHECK_LIVENESS case in ext/openssl/xp_ssl.c - causing use of dead socket #13860

Closed
jcencekibm opened this issue Apr 1, 2024 · 1 comment

Comments

@jcencekibm
Copy link

jcencekibm commented Apr 1, 2024

Description

There appears to be defective error handling located here:
https://github.com/php/php-src/blob/PHP-8.3.4/ext/openssl/xp_ssl.c#L2503

the problem is that php_socket_errno is not valid if recv returned 0 - it is only valid if recv returned an error. Instead, the code should probably look more like its counterpart in xp_streams.c, which is correct:
https://github.com/php/php-src/blob/PHP-8.3.4/main/streams/xp_socket.c#L381

The end result of this bug, from a low-level perspective, is that if the previous lingering errno happens to be EAGAIN, and there is a socket that is closed, PHP will believe that the closed socket threw the EAGAIN when in reality the recv operation succeeded (indicating that the socket is closed) and the EAGAIN in errno is not relevant. This causes PHP to believe the socket is alive and PHP_STREAM_OPTION_RETURN_OK is returned rather than PHP_STREAM_OPTION_RETURN_ERR. Then it will try to use the socket for further stream operations and fail, as will be exemplified below using SoapClient.

This is technically a bug that has been around for a long time, however, as of PHP 8.2.0, I notice there is an abundance of recv calls with MSG_PEEK|MSG_DONTWAIT before any attempt to write to any socket, which readily return EAGAIN. This proliferates the issue and makes it occur commonly whereas previously it was latent.

However, for simplicity, I am providing the simple, albeit contrived, test case, rather than a natural one. I'm not an expert on the php codebase so, apologies if there is an even simpler test case for this that does not require the presence of a soap server, but this most closely isolates the business logic in which I encountered this.


<?php

$client = new SoapClient("http://localhost/soap3/api.wsdl"); // you must have a soapserver running here
$client->authenticate("username","password"); // this should be any valid method on the remote server that succeeds

$fp = fsockopen("127.0.0.1",80);
stream_set_blocking($fp,false);
var_dump(fread($fp,100)); // force an EAGAIN from this socket, should be irrelevant to our SoapClient but will trigger the issue because errno persists as EAGAIN

sleep(100); // this should be higher than your web server's socket keepalive. the socket will be in CLOSE_WAIT after this returns

$client->authenticate("username","password"); // now we trigger PHP_STREAM_OPTION_CHECK_LIVENESS and expect the socket to be recognized as dead. it isn't - php thinks its alive and tries to reuse it

Expected result: no exception, or a SoapFault from the server, anything to indicate that the http call succeeded, etc
Actual result: Fatal error: Uncaught SoapFault exception: [HTTP] Error Fetching http headers
The actual result here implies a transport-layer error

In gdb, I confirmed that {{alive}} persists as 1, php_stream_set_option returns PHP_STREAM_OPTION_RETURN_OK, php_stream_eof returns false, etc, despite the socket being dead. stack trace:

#0  0x00007f95a38fd851 in recv () from /lib64/libc.so.6
#1  0x000055e874378ad2 in php_openssl_sockop_set_option ()
#2  0x000055e8744b0571 in _php_stream_set_option ()
#3  0x000055e8744b062a in _php_stream_eof ()
#4  0x00007f9592e0e7a2 in make_http_soap_request () from /opt/remi/php83/root/usr/lib64/php/modules/soap.so
#5  0x00007f9592df641d in zim_SoapClient___doRequest () from /opt/remi/php83/root/usr/lib64/php/modules/soap.so
#6  0x000055e8744f658e in zend_call_function ()
#7  0x000055e8744f6a99 in _call_user_function_impl ()
#8  0x00007f9592df6927 in do_request () from /opt/remi/php83/root/usr/lib64/php/modules/soap.so
#9  0x00007f9592e0094d in do_soap_call.isra () from /opt/remi/php83/root/usr/lib64/php/modules/soap.so
#10 0x00007f9592e01e9b in soap_client_call_impl () from /opt/remi/php83/root/usr/lib64/php/modules/soap.so
#11 0x000055e874571c3c in ZEND_CALL_TRAMPOLINE_SPEC_OBSERVER_HANDLER ()
#12 0x000055e87457288c in execute_ex ()
#13 0x000055e87457c152 in zend_execute ()
#14 0x000055e874505ba5 in zend_execute_scripts ()
#15 0x000055e87449a72a in php_execute_script ()
#16 0x000055e8745f42f2 in do_cli ()
#17 0x000055e874327db7 in main ()

strace of dead socket being re-used:

recvfrom(10, "", 1, MSG_PEEK|MSG_DONTWAIT, NULL, NULL) = 0
sendto(10, "POST /soap3/api.php HTTP/1.1\r\nHost:

PHP Version

PHP 8.3.4

Operating System

Red Hat Enterprise Linux 8.9

@nielsdos
Copy link
Member

nielsdos commented Apr 5, 2024

I'll take a look now, and try to come up with a self-contained test-case.

nielsdos added a commit to nielsdos/php-src that referenced this issue Apr 5, 2024
…xt/openssl/xp_ssl.c - causing use of dead socket

php_socket_errno() may return a stale value when recv returns a
value >= 0. As such, the liveness check is wrong.
This is the same bug as #70198 (fixed in phpGH-1456). So we fix it in the
same way.
nielsdos added a commit that referenced this issue Apr 7, 2024
* PHP-8.2:
  Fix GH-13860: Incorrect PHP_STREAM_OPTION_CHECK_LIVENESS case in ext/openssl/xp_ssl.c - causing use of dead socket
nielsdos added a commit that referenced this issue Apr 7, 2024
* PHP-8.3:
  Fix GH-13860: Incorrect PHP_STREAM_OPTION_CHECK_LIVENESS case in ext/openssl/xp_ssl.c - causing use of dead socket
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants