|
| 1 | +From 4f5c7b360f31f31ac160b171861c5f52f48e367e Mon Sep 17 00:00:00 2001 |
| 2 | +From: Kanishk-Bansal <kbkanishk975@gmail.com> |
| 3 | +Date: Tue, 11 Feb 2025 15:53:59 +0000 |
| 4 | +Subject: [PATCH] Fix CVE-2025-23085 |
| 5 | + |
| 6 | +--- |
| 7 | + lib/internal/http2/core.js | 15 ++++++-- |
| 8 | + src/node_http2.cc | 36 ++++++++++++++++--- |
| 9 | + ...2-connect-method-extended-cant-turn-off.js | 6 ++++ |
| 10 | + ...-http2-options-max-headers-block-length.js | 4 ++- |
| 11 | + ...tp2-options-max-headers-exceeds-nghttp2.js | 4 ++- |
| 12 | + 5 files changed, 55 insertions(+), 10 deletions(-) |
| 13 | + |
| 14 | +diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js |
| 15 | +index 92ce193b..38844d30 100644 |
| 16 | +--- a/lib/internal/http2/core.js |
| 17 | ++++ b/lib/internal/http2/core.js |
| 18 | +@@ -608,11 +608,20 @@ function onFrameError(id, type, code) { |
| 19 | + return; |
| 20 | + debugSessionObj(session, 'error sending frame type %d on stream %d, code: %d', |
| 21 | + type, id, code); |
| 22 | +- const emitter = session[kState].streams.get(id) || session; |
| 23 | ++ |
| 24 | ++ const stream = session[kState].streams.get(id); |
| 25 | ++ const emitter = stream || session; |
| 26 | + emitter[kUpdateTimer](); |
| 27 | + emitter.emit('frameError', type, code, id); |
| 28 | +- session[kState].streams.get(id).close(code); |
| 29 | +- session.close(); |
| 30 | ++ |
| 31 | ++ // When a frameError happens is not uncommon that a pending GOAWAY |
| 32 | ++ // package from nghttp2 is on flight with a correct error code. |
| 33 | ++ // We schedule it using setImmediate to give some time for that |
| 34 | ++ // package to arrive. |
| 35 | ++ setImmediate(() => { |
| 36 | ++ stream?.close(code); |
| 37 | ++ session.close(); |
| 38 | ++ }); |
| 39 | + } |
| 40 | + |
| 41 | + function onAltSvc(stream, origin, alt) { |
| 42 | +diff --git a/src/node_http2.cc b/src/node_http2.cc |
| 43 | +index eb3506ff..38d47f0c 100644 |
| 44 | +--- a/src/node_http2.cc |
| 45 | ++++ b/src/node_http2.cc |
| 46 | +@@ -750,6 +750,7 @@ bool Http2Session::CanAddStream() { |
| 47 | + } |
| 48 | + |
| 49 | + void Http2Session::AddStream(Http2Stream* stream) { |
| 50 | ++ Debug(this, "Adding stream: %d", stream->id()); |
| 51 | + CHECK_GE(++statistics_.stream_count, 0); |
| 52 | + streams_[stream->id()] = BaseObjectPtr<Http2Stream>(stream); |
| 53 | + size_t size = streams_.size(); |
| 54 | +@@ -760,6 +761,7 @@ void Http2Session::AddStream(Http2Stream* stream) { |
| 55 | + |
| 56 | + |
| 57 | + BaseObjectPtr<Http2Stream> Http2Session::RemoveStream(int32_t id) { |
| 58 | ++ Debug(this, "Removing stream: %d", id); |
| 59 | + BaseObjectPtr<Http2Stream> stream; |
| 60 | + if (streams_.empty()) |
| 61 | + return stream; |
| 62 | +@@ -936,6 +938,7 @@ int Http2Session::OnHeaderCallback(nghttp2_session* handle, |
| 63 | + if (UNLIKELY(!stream)) |
| 64 | + return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; |
| 65 | + |
| 66 | ++ Debug(session, "handling header key/pair for stream %d", id); |
| 67 | + // If the stream has already been destroyed, ignore. |
| 68 | + if (!stream->is_destroyed() && !stream->AddHeader(name, value, flags)) { |
| 69 | + // This will only happen if the connected peer sends us more |
| 70 | +@@ -1005,9 +1008,21 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle, |
| 71 | + return 1; |
| 72 | + } |
| 73 | + |
| 74 | +- // If the error is fatal or if error code is ERR_STREAM_CLOSED... emit error |
| 75 | ++ // If the error is fatal or if error code is one of the following |
| 76 | ++ // we emit and error: |
| 77 | ++ // |
| 78 | ++ // ERR_STREAM_CLOSED: An invalid frame has been received in a closed stream. |
| 79 | ++ // |
| 80 | ++ // ERR_PROTO: The RFC 7540 specifies: |
| 81 | ++ // "An endpoint that encounters a connection error SHOULD first send a GOAWAY |
| 82 | ++ // frame (Section 6.8) with the stream identifier of the last stream that it |
| 83 | ++ // successfully received from its peer. |
| 84 | ++ // The GOAWAY frame includes an error code that indicates the type of error" |
| 85 | ++ // The GOAWAY frame is already sent by nghttp2. We emit the error |
| 86 | ++ // to liberate the Http2Session to destroy. |
| 87 | + if (nghttp2_is_fatal(lib_error_code) || |
| 88 | +- lib_error_code == NGHTTP2_ERR_STREAM_CLOSED) { |
| 89 | ++ lib_error_code == NGHTTP2_ERR_STREAM_CLOSED || |
| 90 | ++ lib_error_code == NGHTTP2_ERR_PROTO) { |
| 91 | + Environment* env = session->env(); |
| 92 | + Isolate* isolate = env->isolate(); |
| 93 | + HandleScope scope(isolate); |
| 94 | +@@ -1070,7 +1085,6 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle, |
| 95 | + Debug(session, "frame type %d was not sent, code: %d", |
| 96 | + frame->hd.type, error_code); |
| 97 | + |
| 98 | +- // Do not report if the frame was not sent due to the session closing |
| 99 | + if (error_code == NGHTTP2_ERR_SESSION_CLOSING || |
| 100 | + error_code == NGHTTP2_ERR_STREAM_CLOSED || |
| 101 | + error_code == NGHTTP2_ERR_STREAM_CLOSING) { |
| 102 | +@@ -1079,7 +1093,15 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle, |
| 103 | + // to destroy the session completely. |
| 104 | + // Further information see: https://github.com/nodejs/node/issues/35233 |
| 105 | + session->DecrefHeaders(frame); |
| 106 | +- return 0; |
| 107 | ++ // Currently, nghttp2 doesn't not inform us when is the best |
| 108 | ++ // time to call session.close(). It relies on a closing connection |
| 109 | ++ // from peer. If that doesn't happen, the nghttp2_session will be |
| 110 | ++ // closed but the Http2Session will still be up causing a memory leak. |
| 111 | ++ // Therefore, if the GOAWAY frame couldn't be send due to |
| 112 | ++ // ERR_SESSION_CLOSING we should force close from our side. |
| 113 | ++ if (frame->hd.type != 0x03) { |
| 114 | ++ return 0; |
| 115 | ++ } |
| 116 | + } |
| 117 | + |
| 118 | + Isolate* isolate = env->isolate(); |
| 119 | +@@ -1145,12 +1167,15 @@ int Http2Session::OnStreamClose(nghttp2_session* handle, |
| 120 | + // ignore these. If this callback was not provided, nghttp2 would handle |
| 121 | + // invalid headers strictly and would shut down the stream. We are intentionally |
| 122 | + // being more lenient here although we may want to revisit this choice later. |
| 123 | +-int Http2Session::OnInvalidHeader(nghttp2_session* session, |
| 124 | ++int Http2Session::OnInvalidHeader(nghttp2_session* handle, |
| 125 | + const nghttp2_frame* frame, |
| 126 | + nghttp2_rcbuf* name, |
| 127 | + nghttp2_rcbuf* value, |
| 128 | + uint8_t flags, |
| 129 | + void* user_data) { |
| 130 | ++ Http2Session* session = static_cast<Http2Session*>(user_data); |
| 131 | ++ int32_t id = GetFrameID(frame); |
| 132 | ++ Debug(session, "invalid header received for stream %d", id); |
| 133 | + // Ignore invalid header fields by default. |
| 134 | + return 0; |
| 135 | + } |
| 136 | +@@ -1544,6 +1569,7 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { |
| 137 | + |
| 138 | + // Called by OnFrameReceived when a complete SETTINGS frame has been received. |
| 139 | + void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) { |
| 140 | ++ Debug(this, "handling settings frame"); |
| 141 | + bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK; |
| 142 | + if (!ack) { |
| 143 | + js_fields_->bitfield &= ~(1 << kSessionRemoteSettingsIsUpToDate); |
| 144 | +diff --git a/test/parallel/test-http2-connect-method-extended-cant-turn-off.js b/test/parallel/test-http2-connect-method-extended-cant-turn-off.js |
| 145 | +index f4d033ef..456aa1ce 100644 |
| 146 | +--- a/test/parallel/test-http2-connect-method-extended-cant-turn-off.js |
| 147 | ++++ b/test/parallel/test-http2-connect-method-extended-cant-turn-off.js |
| 148 | +@@ -27,4 +27,10 @@ server.listen(0, common.mustCall(() => { |
| 149 | + server.close(); |
| 150 | + })); |
| 151 | + })); |
| 152 | ++ |
| 153 | ++ client.on('error', common.expectsError({ |
| 154 | ++ code: 'ERR_HTTP2_ERROR', |
| 155 | ++ name: 'Error', |
| 156 | ++ message: 'Protocol error' |
| 157 | ++ })); |
| 158 | + })); |
| 159 | +diff --git a/test/parallel/test-http2-options-max-headers-block-length.js b/test/parallel/test-http2-options-max-headers-block-length.js |
| 160 | +index af1cc6f9..15b142ac 100644 |
| 161 | +--- a/test/parallel/test-http2-options-max-headers-block-length.js |
| 162 | ++++ b/test/parallel/test-http2-options-max-headers-block-length.js |
| 163 | +@@ -35,9 +35,11 @@ server.listen(0, common.mustCall(() => { |
| 164 | + assert.strictEqual(code, h2.constants.NGHTTP2_FRAME_SIZE_ERROR); |
| 165 | + })); |
| 166 | + |
| 167 | ++ // NGHTTP2 will automatically send the NGHTTP2_REFUSED_STREAM with |
| 168 | ++ // the GOAWAY frame. |
| 169 | + req.on('error', common.expectsError({ |
| 170 | + code: 'ERR_HTTP2_STREAM_ERROR', |
| 171 | + name: 'Error', |
| 172 | +- message: 'Stream closed with error code NGHTTP2_FRAME_SIZE_ERROR' |
| 173 | ++ message: 'Stream closed with error code NGHTTP2_REFUSED_STREAM' |
| 174 | + })); |
| 175 | + })); |
| 176 | +diff --git a/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js b/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js |
| 177 | +index df3aefff..7767dbbc 100644 |
| 178 | +--- a/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js |
| 179 | ++++ b/test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js |
| 180 | +@@ -59,6 +59,9 @@ server.listen(0, common.mustCall(() => { |
| 181 | + 'session', |
| 182 | + common.mustCall((session) => { |
| 183 | + assert.strictEqual(session instanceof ServerHttp2Session, true); |
| 184 | ++ session.on('close', common.mustCall(() => { |
| 185 | ++ server.close(); |
| 186 | ++ })); |
| 187 | + }), |
| 188 | + ); |
| 189 | + server.on( |
| 190 | +@@ -80,7 +83,6 @@ server.listen(0, common.mustCall(() => { |
| 191 | + assert.strictEqual(err.name, 'Error'); |
| 192 | + assert.strictEqual(err.message, 'Session closed with error code 9'); |
| 193 | + assert.strictEqual(session instanceof ServerHttp2Session, true); |
| 194 | +- server.close(); |
| 195 | + }), |
| 196 | + ); |
| 197 | + |
| 198 | +-- |
| 199 | +2.45.2 |
| 200 | + |
0 commit comments