http2: consider 0-length non-end DATA frames an error

This is intended to mitigate CVE-2019-9518. Backport-PR-URL: #29124 PR-URL: #29122 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Aug 15, 2019 · a319168 · a319168
1 parent 156f2f3
commit a319168
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 16 deletions.
diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -1031,8 +1031,7 @@ int Http2Session::OnFrameReceive(nghttp2_session* handle,
                       frame->hd.type);
   switch (frame->hd.type) {
     case NGHTTP2_DATA:
-      session->HandleDataFrame(frame);
-      break;
+      return session->HandleDataFrame(frame);
     case NGHTTP2_PUSH_PROMISE:
       // Intentional fall-through, handled just like headers frames
     case NGHTTP2_HEADERS:
@@ -1408,18 +1407,18 @@ void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) {
 // Called by OnFrameReceived when a complete DATA frame has been received.
 // If we know that this was the last DATA frame (because the END_STREAM flag
 // is set), then we'll terminate the readable side of the StreamBase.
-inline void Http2Session::HandleDataFrame(const nghttp2_frame* frame) {
+int Http2Session::HandleDataFrame(const nghttp2_frame* frame) {
   int32_t id = GetFrameID(frame);
   DEBUG_HTTP2SESSION2(this, "handling data frame for stream %d", id);
   Http2Stream* stream = FindStream(id);
 
-  // If the stream has already been destroyed, do nothing
-  if (stream->IsDestroyed())
-    return;
-
-  if (frame->hd.flags & NGHTTP2_FLAG_END_STREAM) {
+  if (!stream->IsDestroyed() && frame->hd.flags & NGHTTP2_FLAG_END_STREAM) {
     stream->EmitData(UV_EOF, Local<Object>(), Local<Object>());
+  } else if (frame->hd.length == 0 &&
+           !IsReverted(SECURITY_REVERT_CVE_2019_9518)) {
+    return 1;  // Consider 0-length frame without END_STREAM an error.
   }
+  return 0;
 }
 
 

diff --git a/src/node_http2.h b/src/node_http2.h
@@ -940,14 +940,14 @@ class Http2Session : public AsyncWrap {
                                    size_t maxPayloadLen);
 
   // Frame Handler
-  inline void HandleDataFrame(const nghttp2_frame* frame);
-  inline void HandleGoawayFrame(const nghttp2_frame* frame);
-  inline void HandleHeadersFrame(const nghttp2_frame* frame);
-  inline void HandlePriorityFrame(const nghttp2_frame* frame);
-  inline void HandleSettingsFrame(const nghttp2_frame* frame);
-  inline void HandlePingFrame(const nghttp2_frame* frame);
-  inline void HandleAltSvcFrame(const nghttp2_frame* frame);
-  inline void HandleOriginFrame(const nghttp2_frame* frame);
+  int HandleDataFrame(const nghttp2_frame* frame);
+  void HandleGoawayFrame(const nghttp2_frame* frame);
+  void HandleHeadersFrame(const nghttp2_frame* frame);
+  void HandlePriorityFrame(const nghttp2_frame* frame);
+  void HandleSettingsFrame(const nghttp2_frame* frame);
+  void HandlePingFrame(const nghttp2_frame* frame);
+  void HandleAltSvcFrame(const nghttp2_frame* frame);
+  void HandleOriginFrame(const nghttp2_frame* frame);
 
   // nghttp2 callbacks
   static inline int OnBeginHeadersCallback(

diff --git a/src/node_revert.h b/src/node_revert.h
@@ -19,6 +19,7 @@ namespace node {
   XX(CVE_2018_12116, "CVE-2018-12116", "HTTP request splitting")           \
   XX(CVE_2019_9514, "CVE-2019-9514", "HTTP/2 Reset Flood")                 \
   XX(CVE_2019_9516, "CVE-2019-9516", "HTTP/2 0-Length Headers Leak")       \
+  XX(CVE_2019_9518, "CVE-2019-9518", "HTTP/2 Empty DATA Frame Flooding")   \
 //  XX(CVE_2016_PEND, "CVE-2016-PEND", "Vulnerability Title")
   // TODO(addaleax): Remove all of the above before Node.js 13 as the comment
   // at the start of the file indicates.