From ce385d3f55a4b76da976b3bdf71fe2deddf315ba Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Fri, 14 Jul 2023 20:52:03 +0900 Subject: [PATCH] Fix memory leak This commit fixes memory leak that happens when PUSH_PROMISE or HEADERS frame cannot be sent, and nghttp2_on_stream_close_callback fails with a fatal error. For example, if GOAWAY frame has been received, a HEADERS frame that opens new stream cannot be sent. This issue has already been made public via CVE-2023-35945 [1] issued by envoyproxy/envoy project. During embargo period, the patch to fix this bug was accidentally submitted to nghttp2/nghttp2 repository [2]. And they decided to disclose CVE early. I was notified just 1.5 hours before disclosure. I had no time to respond. PoC described in [1] is quite simple, but I think it is not enough to trigger this bug. While it is true that receiving GOAWAY prevents a client from opening new stream, and nghttp2 enters error handling branch, in order to cause the memory leak, nghttp2_session_close_stream function must return a fatal error. nghttp2 defines 2 fatal error codes: - NGHTTP2_ERR_NOMEM - NGHTTP2_ERR_CALLBACK_FAILURE NGHTTP2_ERR_NOMEM, as its name suggests, indicates out of memory. It is unlikely that a process gets short of memory with this simple PoC scenario unless application does something memory heavy processing. NGHTTP2_ERR_CALLBACK_FAILURE is returned from application defined callback function (nghttp2_on_stream_close_callback, in this case), which indicates something fatal happened inside a callback, and a connection must be closed immediately without any further action. As nghttp2_on_stream_close_error_callback documentation says, any error code other than 0 or NGHTTP2_ERR_CALLBACK_FAILURE is treated as fatal error code. More specifically, it is treated as if NGHTTP2_ERR_CALLBACK_FAILURE is returned. I guess that envoy returns NGHTTP2_ERR_CALLBACK_FAILURE or other error code which is translated into NGHTTP2_ERR_CALLBACK_FAILURE. [1] https://github.com/envoyproxy/envoy/security/advisories/GHSA-jfxv-29pc-x22r [2] https://github.com/nghttp2/nghttp2/pull/1929 --- lib/nghttp2_session.c | 10 +++++----- tests/nghttp2_session_test.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 7509ceb5..71858a39 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -3296,6 +3296,7 @@ static ssize_t nghttp2_session_mem_send_internal(nghttp2_session *session, if (rv < 0) { int32_t opened_stream_id = 0; uint32_t error_code = NGHTTP2_INTERNAL_ERROR; + int rv2 = 0; DEBUGF("send: frame preparation failed with %s\n", nghttp2_strerror(rv)); @@ -3338,19 +3339,18 @@ static ssize_t nghttp2_session_mem_send_internal(nghttp2_session *session, } if (opened_stream_id) { /* careful not to override rv */ - int rv2; rv2 = nghttp2_session_close_stream(session, opened_stream_id, error_code); - - if (nghttp2_is_fatal(rv2)) { - return rv2; - } } nghttp2_outbound_item_free(item, mem); nghttp2_mem_free(mem, item); active_outbound_item_reset(aob, mem); + if (nghttp2_is_fatal(rv2)) { + return rv2; + } + if (rv == NGHTTP2_ERR_HEADER_COMP) { /* If header compression error occurred, should terminiate connection. */ diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index b55ff534..74352426 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -584,6 +584,15 @@ static int on_stream_close_callback(nghttp2_session *session, int32_t stream_id, return 0; } +static int fatal_error_on_stream_close_callback(nghttp2_session *session, + int32_t stream_id, + uint32_t error_code, + void *user_data) { + on_stream_close_callback(session, stream_id, error_code, user_data); + + return NGHTTP2_ERR_CALLBACK_FAILURE; +} + static ssize_t pack_extension_callback(nghttp2_session *session, uint8_t *buf, size_t len, const nghttp2_frame *frame, void *user_data) { @@ -4296,6 +4305,8 @@ void test_nghttp2_session_on_goaway_received(void) { nghttp2_frame frame; int i; nghttp2_mem *mem; + const uint8_t *data; + ssize_t datalen; mem = nghttp2_mem_default(); user_data.frame_recv_cb_called = 0; @@ -4337,6 +4348,29 @@ void test_nghttp2_session_on_goaway_received(void) { nghttp2_frame_goaway_free(&frame.goaway, mem); nghttp2_session_del(session); + + /* Make sure that no memory leak when stream_close callback fails + with a fatal error */ + memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); + callbacks.on_stream_close_callback = fatal_error_on_stream_close_callback; + + memset(&user_data, 0, sizeof(user_data)); + + nghttp2_session_client_new(&session, &callbacks, &user_data); + + nghttp2_frame_goaway_init(&frame.goaway, 0, NGHTTP2_NO_ERROR, NULL, 0); + + CU_ASSERT(0 == nghttp2_session_on_goaway_received(session, &frame)); + + nghttp2_submit_request(session, NULL, reqnv, ARRLEN(reqnv), NULL, NULL); + + datalen = nghttp2_session_mem_send(session, &data); + + CU_ASSERT(NGHTTP2_ERR_CALLBACK_FAILURE == datalen); + CU_ASSERT(1 == user_data.stream_close_cb_called); + + nghttp2_frame_goaway_free(&frame.goaway, mem); + nghttp2_session_del(session); } void test_nghttp2_session_on_window_update_received(void) { -- 2.35.3