From 2008c9722fcf7591e39013691f303934b622df7b Mon Sep 17 00:00:00 2001 From: Akshay K Date: Fri, 30 Jul 2021 18:46:45 -0400 Subject: [PATCH] http2: update handling of rst_stream with error code NGHTTP2_CANCEL The PR updates the handling of rst_stream frames and adds all streams to the pending list on receiving rst frames with the error code NGHTTP2_CANCEL. The changes will remove dependency on the stream state that may allow bypassing the checks in certain cases. I think a better solution is to delay streams in all cases if rst_stream is received for the cancel events. The rst_stream frames can be received for protocol/connection error as well it should be handled immediately. Adding streams to the pending list in such cases may cause errors. CVE-ID: CVE-2021-22930 Refs: https://nvd.nist.gov/vuln/detail/CVE-2021-22930 Backport-PR-URL: https://github.com/nodejs/node/pull/39659 PR-URL: https://github.com/nodejs/node/pull/39622 Refs: https://github.com/nodejs/node/pull/39423 Reviewed-By: Matteo Collina Reviewed-By: James M Snell Reviewed-By: Beth Griggs --- src/node_http2.cc | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index cc21373521e4c7..5156aa34df55a1 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -2151,18 +2151,19 @@ void Http2Stream::SubmitRstStream(const uint32_t code) { CHECK(!this->IsDestroyed()); code_ = code; - // If RST_STREAM frame is received and stream is not writable - // because it is busy reading data, don't try force purging it. - // Instead add the stream to pending stream list and process - // the pending data when it is safe to do so. This is to avoid - // double free error due to unwanted behavior of nghttp2. - // Ref:https://github.com/nodejs/node/issues/38964 - - // Add stream to the pending list if it is received with scope + auto is_stream_cancel = [](const uint32_t code) { + return code == NGHTTP2_CANCEL; + }; + + // If RST_STREAM frame is received with error code NGHTTP2_CANCEL, + // add it to the pending list and don't force purge the data. It is + // to avoids the double free error due to unwanted behavior of nghttp2. + + // Add stream to the pending list only if it is received with scope // below in the stack. The pending list may not get processed // if RST_STREAM received is not in scope and added to the list // causing endpoint to hang. - if (session_->is_in_scope() && IsReading()) { + if (session_->is_in_scope() && is_stream_cancel(code)) { session_->AddPendingRstStream(id_); return; }