1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
|
From ce385d3f55a4b76da976b3bdf71fe2deddf315ba Mon Sep 17 00:00:00 2001
From: Tatsuhiro Tsujikawa <tatsuhiro.t@gmail.com>
Date: Mon, 4 Sep 2023 06:48:30 +0000
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
CVE: CVE-2023-35945
Upstream-Status: Backport [https://github.com/nghttp2/nghttp2/commit/ce385d3f55a4b76da976b3bdf71fe2deddf315ba]
Signed-off-by: Yogita Urade <yogita.urade@windriver.com>
---
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 93f3f07..9bb32b2 100644
--- a/lib/nghttp2_session.c
+++ b/lib/nghttp2_session.c
@@ -3300,6 +3300,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));
@@ -3342,19 +3343,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 08152d4..14ab132 100644
--- a/tests/nghttp2_session_test.c
+++ b/tests/nghttp2_session_test.c
@@ -585,6 +585,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) {
@@ -4297,6 +4306,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;
@@ -4338,6 +4349,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.5
|
