Skip to content

Commit ed5f832

Browse files
committed
fix(security,tls): handle no SSL/TLS shutdown from peer in server
- Handle clearing error queue on SSL_read and SSL_write in `transport.cc` on error. - Treat `0` and `<0` as errors in SSL_read and SSL_write, use SSL_get_error for retries. - Bump patch version. See https://docs.openssl.org/3.1/man3/SSL_read/ for error handling. See https://docs.openssl.org/3.1/man3/SSL_write/ for error handling. See #1309 for more details.
1 parent 2d9e29b commit ed5f832

File tree

4 files changed

+79
-4
lines changed

4 files changed

+79
-4
lines changed

include/pistache/peer.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
#ifdef PISTACHE_USE_SSL
2626

27+
#include <openssl/err.h>
2728
#include <openssl/ssl.h>
2829

2930
#endif /* PISTACHE_USE_SSL */

src/common/transport.cc

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -323,12 +323,29 @@ namespace Pistache::Tcp
323323
bytes = SSL_read(reinterpret_cast<SSL*>(peer->ssl()),
324324
buffer + totalBytes,
325325
static_cast<int>(Const::MaxBuffer - totalBytes));
326-
if (bytes < 0)
326+
if (bytes <= 0)
327327
{
328328
int ssl_get_error_res = SSL_get_error(
329329
reinterpret_cast<SSL*>(peer->ssl()),
330330
static_cast<int>(bytes));
331-
retry = (ssl_get_error_res == SSL_ERROR_WANT_READ);
331+
PS_LOG_DEBUG_ARGS("SSL_read err %d, bytes %d", ssl_get_error_res, bytes);
332+
333+
switch (ssl_get_error_res)
334+
{
335+
case SSL_ERROR_SYSCALL:
336+
case SSL_ERROR_SSL:
337+
PS_LOG_DEBUG_ARGS("SSL_read clearing error queue, last 0x%08X", ERR_peek_last_error());
338+
ERR_clear_error();
339+
break;
340+
341+
case SSL_ERROR_WANT_READ:
342+
bytes = -1; // To force handling of reset in the logic that follows.
343+
retry = true;
344+
break;
345+
346+
default:
347+
break;
348+
}
332349
}
333350
}
334351
else
@@ -723,10 +740,12 @@ namespace Pistache::Tcp
723740
PS_LOG_DEBUG_ARGS("SSL_write, len %d", static_cast<int>(len));
724741

725742
bytesWritten = SSL_write(ssl_, buffer, static_cast<int>(len));
726-
if (bytesWritten < 0)
743+
if (bytesWritten <= 0)
727744
{
728745
int ssl_get_error_res = SSL_get_error(
729746
ssl_, static_cast<int>(bytesWritten));
747+
PS_LOG_DEBUG_ARGS("SSL_write err %d, bytes %d", ssl_get_error_res, bytesWritten);
748+
730749
switch (ssl_get_error_res)
731750
{
732751
case SSL_ERROR_WANT_WRITE:
@@ -744,6 +763,8 @@ namespace Pistache::Tcp
744763

745764
case SSL_ERROR_SYSCALL:
746765
case SSL_ERROR_SSL:
766+
PS_LOG_DEBUG_ARGS("SSL_write clearing error queue, last 0x%08X", ERR_peek_last_error());
767+
ERR_clear_error();
747768
errno = EBADF;
748769
break;
749770

tests/https_server_test.cc

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -505,6 +505,59 @@ TEST(https_server_test, basic_tls_request_with_password_cert)
505505
ASSERT_EQ(buffer, "Hello, World!");
506506
}
507507

508+
TEST(https_server_test, basic_tls_requests_with_no_shutdown_from_peer)
509+
{
510+
Http::Endpoint server(Address("localhost", Pistache::Port(0)));
511+
512+
const auto sslctxCallback = +[](CURL* /* curl */, void* sslctx, void* /* parm */) -> CURLcode {
513+
// Enable quiet shutdown so that we do not send any shutdown notification to server from peer.
514+
#if !defined(__APPLE__)
515+
// In macOS, setting this flag causes a seg fault intermittently,
516+
// seemingly because a bad sslctx is being passed in by
517+
// libcurl/openssl. See discussion in:
518+
// https://github.com/pistacheio/pistache/pull/1310
519+
// @May/2025.
520+
SSL_CTX_set_quiet_shutdown(reinterpret_cast<SSL_CTX*>(sslctx), 1);
521+
#else
522+
(void)sslctx;
523+
#endif
524+
return CURLE_OK;
525+
};
526+
527+
server.init(Http::Endpoint::options().flags(Tcp::Options::ReuseAddr));
528+
server.setHandler(Http::make_handler<HelloHandler>());
529+
server.useSSL("./certs/server.crt", "./certs/server.key");
530+
server.serveThreaded();
531+
532+
CURL* curl;
533+
CURLcode res;
534+
std::string buffer;
535+
536+
curl = curl_easy_init();
537+
ASSERT_NE(curl, nullptr);
538+
539+
const auto url = getServerUrl(server);
540+
curl_easy_setopt(curl, CURLOPT_URL, url.c_str());
541+
curl_easy_setopt(curl, CURLOPT_CAINFO, "./certs/rootCA.crt");
542+
curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 0L);
543+
curl_easy_setopt(curl, CURLOPT_SSL_CTX_FUNCTION, sslctxCallback);
544+
curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, &write_cb);
545+
curl_easy_setopt(curl, CURLOPT_WRITEDATA, &buffer);
546+
CSO_WIN_REVOKE_BEST_EFFORT;
547+
548+
// Perform multiple requests with quiet shutdown and ensure they are handled.
549+
for (std::size_t req_i = 0U; req_i < 5U; req_i++)
550+
{
551+
res = curl_easy_perform(curl);
552+
EXPECT_EQ(res, CURLE_OK);
553+
EXPECT_EQ(buffer, "Hello, World!");
554+
buffer.clear();
555+
}
556+
557+
curl_easy_cleanup(curl);
558+
server.shutdown();
559+
}
560+
508561
// MUST be LAST test
509562
TEST(https_server_test, last_curl_global_cleanup)
510563
{

version.txt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
0.5.6.20250328
1+
0.5.7.20250508

0 commit comments

Comments
 (0)