Skip to content

Commit

Permalink
http2: treat non-EOF empty frames like other invalid frames
Browse files Browse the repository at this point in the history
Use the existing mechanism that we have to keep track of invalid frames
for treating this specific kind of invalid frame.

The commit that originally introduced this check was 695e38b,
which was supposed to proected against CVE-2019-9518, which in turn
was specifically about a *flood* of empty data frames. While these are
still invalid frames either way, it makes sense to be forgiving here
and just treat them like other invalid frames, i.e. to allow a small
(configurable) number of them.

Fixes: #37849

PR-URL: #37875
Backport-PR-URL: #38673
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
addaleax authored and targos committed Jun 11, 2021
1 parent c3bd0fd commit d8da265
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 1 deletion.
6 changes: 5 additions & 1 deletion src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1277,7 +1277,11 @@ int Http2Session::HandleDataFrame(const nghttp2_frame* frame) {
frame->hd.flags & NGHTTP2_FLAG_END_STREAM) {
stream->EmitRead(UV_EOF);
} else if (frame->hd.length == 0) {
return 1; // Consider 0-length frame without END_STREAM an error.
if (invalid_frame_count_++ > js_fields_->max_invalid_frames) {
Debug(this, "rejecting empty-frame-without-END_STREAM flood\n");
// Consider a flood of 0-length frames without END_STREAM an error.
return 1;
}
}
return 0;
}
Expand Down
Binary file added test/fixtures/emptyframe.http2
Binary file not shown.
39 changes: 39 additions & 0 deletions test/parallel/test-http2-empty-frame-without-eof.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
'use strict';
const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const { readSync } = require('../common/fixtures');
const net = require('net');
const http2 = require('http2');
const { once } = require('events');

async function main() {
const blobWithEmptyFrame = readSync('emptyframe.http2');
const server = net.createServer((socket) => {
socket.end(blobWithEmptyFrame);
}).listen(0);
await once(server, 'listening');

for (const maxSessionInvalidFrames of [0, 2]) {
const client = http2.connect(`http://localhost:${server.address().port}`, {
maxSessionInvalidFrames
});
const stream = client.request({
':method': 'GET',
':path': '/'
});
if (maxSessionInvalidFrames) {
stream.on('error', common.mustNotCall());
client.on('error', common.mustNotCall());
} else {
stream.on('error', common.mustCall());
client.on('error', common.mustCall());
}
stream.resume();
await once(stream, 'end');
client.close();
}
server.close();
}

main().then(common.mustCall());

0 comments on commit d8da265

Please sign in to comment.