From 9bf2184b33313f6c3cfb1bfb6686156e729e9203 Mon Sep 17 00:00:00 2001 From: Mohammad Nejati Date: Tue, 28 May 2024 09:31:17 +0000 Subject: [PATCH] Fix error handling in SSL shutdown operations in examples --- .../http_client_async_ssl_system_executor.cpp | 27 ++++++++++++------ .../async-ssl/http_client_async_ssl.cpp | 27 ++++++++++++------ .../http_client_awaitable_ssl.cpp | 28 +++++++++++++------ .../client/coro-ssl/http_client_coro_ssl.cpp | 28 +++++++++++++------ .../client/sync-ssl/http_client_sync_ssl.cpp | 28 +++++++++++++------ 5 files changed, 93 insertions(+), 45 deletions(-) diff --git a/example/http/client/async-ssl-system-executor/http_client_async_ssl_system_executor.cpp b/example/http/client/async-ssl-system-executor/http_client_async_ssl_system_executor.cpp index bb390dc6..74af2bf5 100644 --- a/example/http/client/async-ssl-system-executor/http_client_async_ssl_system_executor.cpp +++ b/example/http/client/async-ssl-system-executor/http_client_async_ssl_system_executor.cpp @@ -192,16 +192,25 @@ public: void on_shutdown(beast::error_code ec) { - if(ec == net::error::eof) - { - // Rationale: - // http://stackoverflow.com/questions/25587403/boost-asio-ssl-async-shutdown-always-finishes-with-an-error - ec = {}; - } - if(ec) - return fail(ec, "shutdown"); + // ssl::error::stream_truncated, also known as an SSL "short read", + // indicates the peer closed the connection without performing the + // required closing handshake (for example, Google does this to + // improve performance). Generally this can be a security issue, + // but if your communication protocol is self-terminated (as + // it is with both HTTP and WebSocket) then you may simply + // ignore the lack of close_notify. + // + // https://github.com/boostorg/beast/issues/38 + // + // https://security.stackexchange.com/questions/91435/how-to-handle-a-malicious-ssl-tls-shutdown + // + // When a short read would cut off the end of an HTTP message, + // Beast returns the error beast::http::error::partial_message. + // Therefore, if we see a short read here, it has occurred + // after the message has been completed, so it is safe to ignore it. - // If we get here then the connection is closed gracefully + if(ec != net::ssl::error::stream_truncated) + return fail(ec, "shutdown"); } }; diff --git a/example/http/client/async-ssl/http_client_async_ssl.cpp b/example/http/client/async-ssl/http_client_async_ssl.cpp index 436af1fd..3c0d6ce2 100644 --- a/example/http/client/async-ssl/http_client_async_ssl.cpp +++ b/example/http/client/async-ssl/http_client_async_ssl.cpp @@ -184,16 +184,25 @@ public: void on_shutdown(beast::error_code ec) { - if(ec == net::error::eof) - { - // Rationale: - // http://stackoverflow.com/questions/25587403/boost-asio-ssl-async-shutdown-always-finishes-with-an-error - ec = {}; - } - if(ec) - return fail(ec, "shutdown"); + // ssl::error::stream_truncated, also known as an SSL "short read", + // indicates the peer closed the connection without performing the + // required closing handshake (for example, Google does this to + // improve performance). Generally this can be a security issue, + // but if your communication protocol is self-terminated (as + // it is with both HTTP and WebSocket) then you may simply + // ignore the lack of close_notify. + // + // https://github.com/boostorg/beast/issues/38 + // + // https://security.stackexchange.com/questions/91435/how-to-handle-a-malicious-ssl-tls-shutdown + // + // When a short read would cut off the end of an HTTP message, + // Beast returns the error beast::http::error::partial_message. + // Therefore, if we see a short read here, it has occurred + // after the message has been completed, so it is safe to ignore it. - // If we get here then the connection is closed gracefully + if(ec != net::ssl::error::stream_truncated) + return fail(ec, "shutdown"); } }; diff --git a/example/http/client/awaitable-ssl/http_client_awaitable_ssl.cpp b/example/http/client/awaitable-ssl/http_client_awaitable_ssl.cpp index 8ace8b93..5c1d9fb8 100644 --- a/example/http/client/awaitable-ssl/http_client_awaitable_ssl.cpp +++ b/example/http/client/awaitable-ssl/http_client_awaitable_ssl.cpp @@ -109,16 +109,26 @@ do_session( // Gracefully close the stream - do not threat every error as an exception! auto [ec] = co_await stream.async_shutdown(net::as_tuple(net::use_awaitable)); - if (ec == net::error::eof) - { - // Rationale: - // http://stackoverflow.com/questions/25587403/boost-asio-ssl-async-shutdown-always-finishes-with-an-error - ec = {}; - } - if (ec) - throw boost::system::system_error(ec, "shutdown"); - // If we get here then the connection is closed gracefully + // ssl::error::stream_truncated, also known as an SSL "short read", + // indicates the peer closed the connection without performing the + // required closing handshake (for example, Google does this to + // improve performance). Generally this can be a security issue, + // but if your communication protocol is self-terminated (as + // it is with both HTTP and WebSocket) then you may simply + // ignore the lack of close_notify. + // + // https://github.com/boostorg/beast/issues/38 + // + // https://security.stackexchange.com/questions/91435/how-to-handle-a-malicious-ssl-tls-shutdown + // + // When a short read would cut off the end of an HTTP message, + // Beast returns the error beast::http::error::partial_message. + // Therefore, if we see a short read here, it has occurred + // after the message has been completed, so it is safe to ignore it. + + if(ec != net::ssl::error::stream_truncated) + throw boost::system::system_error(ec, "shutdown"); } //------------------------------------------------------------------------------ diff --git a/example/http/client/coro-ssl/http_client_coro_ssl.cpp b/example/http/client/coro-ssl/http_client_coro_ssl.cpp index adc6c05d..29b6e9e5 100644 --- a/example/http/client/coro-ssl/http_client_coro_ssl.cpp +++ b/example/http/client/coro-ssl/http_client_coro_ssl.cpp @@ -118,16 +118,26 @@ do_session( // Gracefully close the stream stream.async_shutdown(yield[ec]); - if(ec == net::error::eof) - { - // Rationale: - // http://stackoverflow.com/questions/25587403/boost-asio-ssl-async-shutdown-always-finishes-with-an-error - ec = {}; - } - if(ec) - return fail(ec, "shutdown"); - // If we get here then the connection is closed gracefully + // ssl::error::stream_truncated, also known as an SSL "short read", + // indicates the peer closed the connection without performing the + // required closing handshake (for example, Google does this to + // improve performance). Generally this can be a security issue, + // but if your communication protocol is self-terminated (as + // it is with both HTTP and WebSocket) then you may simply + // ignore the lack of close_notify. + // + // https://github.com/boostorg/beast/issues/38 + // + // https://security.stackexchange.com/questions/91435/how-to-handle-a-malicious-ssl-tls-shutdown + // + // When a short read would cut off the end of an HTTP message, + // Beast returns the error beast::http::error::partial_message. + // Therefore, if we see a short read here, it has occurred + // after the message has been completed, so it is safe to ignore it. + + if(ec != net::ssl::error::stream_truncated) + return fail(ec, "shutdown"); } //------------------------------------------------------------------------------ diff --git a/example/http/client/sync-ssl/http_client_sync_ssl.cpp b/example/http/client/sync-ssl/http_client_sync_ssl.cpp index 726c8420..95e99794 100644 --- a/example/http/client/sync-ssl/http_client_sync_ssl.cpp +++ b/example/http/client/sync-ssl/http_client_sync_ssl.cpp @@ -108,16 +108,26 @@ int main(int argc, char** argv) // Gracefully close the stream beast::error_code ec; stream.shutdown(ec); - if(ec == net::error::eof) - { - // Rationale: - // http://stackoverflow.com/questions/25587403/boost-asio-ssl-async-shutdown-always-finishes-with-an-error - ec = {}; - } - if(ec) - throw beast::system_error{ec}; - // If we get here then the connection is closed gracefully + // ssl::error::stream_truncated, also known as an SSL "short read", + // indicates the peer closed the connection without performing the + // required closing handshake (for example, Google does this to + // improve performance). Generally this can be a security issue, + // but if your communication protocol is self-terminated (as + // it is with both HTTP and WebSocket) then you may simply + // ignore the lack of close_notify. + // + // https://github.com/boostorg/beast/issues/38 + // + // https://security.stackexchange.com/questions/91435/how-to-handle-a-malicious-ssl-tls-shutdown + // + // When a short read would cut off the end of an HTTP message, + // Beast returns the error beast::http::error::partial_message. + // Therefore, if we see a short read here, it has occurred + // after the message has been completed, so it is safe to ignore it. + + if(ec != net::ssl::error::stream_truncated) + throw beast::system_error{ec}; } catch(std::exception const& e) {