Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
addaleax authored and BethGriggs committed Aug 15, 2019
1 parent 156f2f3 commit a319168
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 16 deletions.
15 changes: 7 additions & 8 deletions src/node_http2.cc
Expand Up @@ -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:
Expand Down Expand Up @@ -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;
}


Expand Down
16 changes: 8 additions & 8 deletions src/node_http2.h
Expand Up @@ -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(
Expand Down
1 change: 1 addition & 0 deletions src/node_revert.h
Expand Up @@ -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.
Expand Down

0 comments on commit a319168

Please sign in to comment.