From 1d6373335c4a75565ea78ed11cf6c37bd011d76c Mon Sep 17 00:00:00 2001 From: Benjamin Sergeant Date: Fri, 20 Mar 2020 16:57:27 -0700 Subject: [PATCH] (websocket+tls) fix hang in tls handshake which could lead to ANR, discovered through unittesting. --- docs/CHANGELOG.md | 4 ++++ ixwebsocket/IXSocketAppleSSL.cpp | 35 ++++++++++++++++++---------- ixwebsocket/IXSocketAppleSSL.h | 4 ++++ ixwebsocket/IXSocketMbedTLS.cpp | 13 +++++++++-- ixwebsocket/IXSocketOpenSSL.cpp | 13 +++++++++-- ixwebsocket/IXSocketOpenSSL.h | 4 +++- ixwebsocket/IXWebSocketVersion.h | 2 +- test/IXCobraMetricsPublisherTest.cpp | 2 +- 8 files changed, 58 insertions(+), 19 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index bbe6d4bb..af615508 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -1,6 +1,10 @@ # Changelog All changes to this project will be documented in this file. +## [8.3.2] - 2020-03-20 + +(websocket+tls) fix hang in tls handshake which could lead to ANR, discovered through unittesting. + ## [8.3.1] - 2020-03-20 (cobra) CobraMetricsPublisher can be configure with an ix::CobraConfig + more unittest use SSL in server + client diff --git a/ixwebsocket/IXSocketAppleSSL.cpp b/ixwebsocket/IXSocketAppleSSL.cpp index ad557583..522d7819 100644 --- a/ixwebsocket/IXSocketAppleSSL.cpp +++ b/ixwebsocket/IXSocketAppleSSL.cpp @@ -164,6 +164,26 @@ namespace ix return false; } + OSStatus SocketAppleSSL::tlsHandShake(std::string& errMsg, + const CancellationRequest& isCancellationRequested) + { + OSStatus status; + + do + { + status = SSLHandshake(_sslContext); + + // Interrupt the handshake + if (isCancellationRequested()) + { + errMsg = "Cancellation requested"; + return errSSLInternal; + } + } while (status == errSSLWouldBlock || status == errSSLServerAuthCompleted); + + return status; + } + // No wait support bool SocketAppleSSL::connect(const std::string& host, int port, @@ -190,26 +210,17 @@ namespace ix Boolean option(1); SSLSetSessionOption(_sslContext, kSSLSessionOptionBreakOnServerAuth, option); - do - { - status = SSLHandshake(_sslContext); - } while (status == errSSLWouldBlock || status == errSSLServerAuthCompleted); + status = tlsHandShake(errMsg, isCancellationRequested); if (status == errSSLServerAuthCompleted) { // proceed with the handshake - do - { - status = SSLHandshake(_sslContext); - } while (status == errSSLWouldBlock || status == errSSLServerAuthCompleted); + status = tlsHandShake(errMsg, isCancellationRequested); } } else { - do - { - status = SSLHandshake(_sslContext); - } while (status == errSSLWouldBlock || status == errSSLServerAuthCompleted); + status = tlsHandShake(errMsg, isCancellationRequested); } } diff --git a/ixwebsocket/IXSocketAppleSSL.h b/ixwebsocket/IXSocketAppleSSL.h index ef81b09a..8b80ef8f 100644 --- a/ixwebsocket/IXSocketAppleSSL.h +++ b/ixwebsocket/IXSocketAppleSSL.h @@ -37,6 +37,10 @@ namespace ix static OSStatus writeToSocket(SSLConnectionRef connection, const void* data, size_t* len); static OSStatus readFromSocket(SSLConnectionRef connection, void* data, size_t* len); + OSStatus tlsHandShake( + std::string& errMsg, + const CancellationRequest& isCancellationRequested); + SSLContextRef _sslContext; mutable std::mutex _mutex; // AppleSSL routines are not thread-safe diff --git a/ixwebsocket/IXSocketMbedTLS.cpp b/ixwebsocket/IXSocketMbedTLS.cpp index 0077dba7..a3e1bc9a 100644 --- a/ixwebsocket/IXSocketMbedTLS.cpp +++ b/ixwebsocket/IXSocketMbedTLS.cpp @@ -195,8 +195,17 @@ namespace ix int res; do { - std::lock_guard lock(_mutex); - res = mbedtls_ssl_handshake(&_ssl); + { + std::lock_guard lock(_mutex); + res = mbedtls_ssl_handshake(&_ssl); + } + + if (isCancellationRequested()) + { + errMsg = "Cancellation requested"; + close(); + return false; + } } while (res == MBEDTLS_ERR_SSL_WANT_READ || res == MBEDTLS_ERR_SSL_WANT_WRITE); if (res != 0) diff --git a/ixwebsocket/IXSocketOpenSSL.cpp b/ixwebsocket/IXSocketOpenSSL.cpp index ed7df04e..a6e473cc 100644 --- a/ixwebsocket/IXSocketOpenSSL.cpp +++ b/ixwebsocket/IXSocketOpenSSL.cpp @@ -224,7 +224,10 @@ namespace ix return true; } - bool SocketOpenSSL::openSSLClientHandshake(const std::string& host, std::string& errMsg) + bool SocketOpenSSL::openSSLClientHandshake( + const std::string& host, + std::string& errMsg, + const CancellationRequest& isCancellationRequested) { while (true) { @@ -233,6 +236,12 @@ namespace ix return false; } + if (isCancellationRequested()) + { + errMsg = "Cancellation requested"; + return false; + } + ERR_clear_error(); int connect_result = SSL_connect(_ssl_connection); if (connect_result == 1) @@ -577,7 +586,7 @@ namespace ix X509_VERIFY_PARAM* param = SSL_get0_param(_ssl_connection); X509_VERIFY_PARAM_set1_host(param, host.c_str(), 0); #endif - handshakeSuccessful = openSSLClientHandshake(host, errMsg); + handshakeSuccessful = openSSLClientHandshake(host, errMsg, isCancellationRequested); } if (!handshakeSuccessful) diff --git a/ixwebsocket/IXSocketOpenSSL.h b/ixwebsocket/IXSocketOpenSSL.h index 905a5a90..a78e2fe6 100644 --- a/ixwebsocket/IXSocketOpenSSL.h +++ b/ixwebsocket/IXSocketOpenSSL.h @@ -39,7 +39,9 @@ namespace ix void openSSLInitialize(); std::string getSSLError(int ret); SSL_CTX* openSSLCreateContext(std::string& errMsg); - bool openSSLClientHandshake(const std::string& hostname, std::string& errMsg); + bool openSSLClientHandshake(const std::string& hostname, + std::string& errMsg, + const CancellationRequest& isCancellationRequested); bool openSSLCheckServerCert(SSL* ssl, const std::string& hostname, std::string& errMsg); bool checkHost(const std::string& host, const char* pattern); bool handleTLSOptions(std::string& errMsg); diff --git a/ixwebsocket/IXWebSocketVersion.h b/ixwebsocket/IXWebSocketVersion.h index 0ca674e3..100d4583 100644 --- a/ixwebsocket/IXWebSocketVersion.h +++ b/ixwebsocket/IXWebSocketVersion.h @@ -6,4 +6,4 @@ #pragma once -#define IX_WEBSOCKET_VERSION "8.3.1" +#define IX_WEBSOCKET_VERSION "8.3.2" diff --git a/test/IXCobraMetricsPublisherTest.cpp b/test/IXCobraMetricsPublisherTest.cpp index d273ad5d..dd5dc1ed 100644 --- a/test/IXCobraMetricsPublisherTest.cpp +++ b/test/IXCobraMetricsPublisherTest.cpp @@ -147,7 +147,7 @@ namespace TEST_CASE("Cobra_Metrics_Publisher", "[cobra]") { int port = getFreePort(); - bool preferTLS = false; + bool preferTLS = true; snake::AppConfig appConfig = makeSnakeServerConfig(port, preferTLS); // Start a redis server