From 1c043272ea775621156bfa4e78718108803b3315 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 23 Mar 2021 15:48:41 +0100 Subject: [PATCH] http2: treat non-EOF empty frames like other invalid frames 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 695e38be69a780417, 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: https://github.com/nodejs/node/issues/37849 PR-URL: https://github.com/nodejs/node/pull/37875 Reviewed-By: Matteo Collina Reviewed-By: Colin Ihrig --- src/node_http2.cc | 6 ++- test/fixtures/emptyframe.http2 | Bin 0 -> 4233 bytes .../test-http2-empty-frame-without-eof.js | 39 ++++++++++++++++++ 3 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 test/fixtures/emptyframe.http2 create mode 100644 test/parallel/test-http2-empty-frame-without-eof.js diff --git a/src/node_http2.cc b/src/node_http2.cc index ccff6dc4ca60c1..004bc8df22a567 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1285,7 +1285,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; } diff --git a/test/fixtures/emptyframe.http2 b/test/fixtures/emptyframe.http2 new file mode 100644 index 0000000000000000000000000000000000000000..c4a095c4334529276c7b8435cd8fa6a86fb77fbb GIT binary patch literal 4233 zcma)8c{r497a!T8tYyhIV;TEc#x9dk#$J|2wlu~V!G40}uZTaj|4!t% zDm)tXw|xH_>Hq!rd5{O%8H@aT(PWC>arXa%gUd01>3i245zg6_RAICicxl$-ZvZlN|icI|7COIO3Ou>-QWKWzNfrOD~XZMBp zlF1Mvi9k7q0ZYKMC>~csa7M$Na0HY)@Dv^97M2Ki=aZPAhI(zk{fs) z`l6rgvG+ZK7L1NA(Z5DKC+{ad;X>$cghOj5zU+?du{2oPSnHu0xJq8fe{j)T&sj$TS7aEnha_^o5{{@ZjcZ6-apV9F9)ZQdWF$uEU zT*t~31*46mKjrLG$eSxv9}bpOGjs{e)f~}k&BhgPS^t8u*dk^~aP4BrWOr=9Xo)+V zmZ1hw%3TBNvPynrX3RFMfmKD;&wmc_4pNDOUTr+B7%3iu*Zuf-eyg1=y)-rFla{7^ zZcOMYDtx`*lZBKC{jRl)n`1zWEgbjE#e9qXEgqrv=-XWy;mVum|2wCWz%=1A3O;zRz9mJXgWMP zc%pk(D5gaC`9a0l57V0dT%QN7@ro+jAes=Sl+24M>EsD3DfhA!ND~V_M6&4e;^>Hi zbTiTh6Zs-NL)4_*2M>HV@7h$lZ`|k*8^hC!eRy$0Z=AnlsH1{|z%OK{U$UVGEqc*? z-#L+gah;h#Upd=G_B5Ir!4SnnL9=Ee9T@_mM;le2NWiS)hI%*;vIWDdoxiF^G@ok#LZIb0uCL3mjLPExr7T7Ayy+(8P%q(YMrXAhN;#F*i;x6m{ zz;lhqP0PBzKXmv}n{)PviBOa+x=O$sCmoWx@a5aFR$AIR^RZ{sOiE?A-s!Cb?+vN` z;Zb3cRNS~uY*kOEv}745YsgiQ(Y{n2|3eco+*IMWYqlm+jW(-!i<=j-`|5XqtCH3% zb?*whKRH~O{jN!GQ_$F>bQHa+e6Cpet550{uG37;bw>AN+D#eVi?sF_F7VDvYvu~C zk2h2^8#qY|e>PA}Z%s;E8uv~~J}C2E2dh$gKl}HH3ih_ckG^%W%D09*N*q0XD3&AT z`6mC*sfrBnx!PSqP$y$+Ws*MTXu?bi3m(a~;G9<+BoGwa&*-pgx!<3Mz{YLdM(q9s zDF*U>5oiYvpKoD8nsO`0qzTZVai7~TU~run|4ref^hk}% zI_X7XdWl4jtZ}&n@iF3V#;bnS(lr3!FN$!SwrJ4L`R6y_B=1jwX3%DV^j@9wQ2WXl z6=>&kP4T{xVb~hywg8dSnxoU{#pM3kT7c=fnS^EMqBp=z*6-ifY1UQN%~uS|!L*9g z>=+D5%_`sNneAIgpHe%IU59~_)2KbcB1h!afG&wIdWi+8X+Zeh$_UbJJ{Q>_`UNF! zEI$oBLP*==L+Lqgd$+!_5U8uz9je$^=5kmgmV1Y-PdbG8+vV)6PN&CNYhPN#3Uv|m z@L|i#z2{Dl%?eY~IJ)k*xq=>^{Fpx(dw>h}=4^M`8l9{}$E)slF;Uyr%GKuiWtl%v zd+cS|`$1;iF_xd}gdTogu9C*-gX@h*;IG;6lG}(8cfgGlF-0EbApd?#W!OPOV#SHB ztSm|T?h$qPJil;8fokT$6?*a-`EyOpJ>U18n;If1B_8#dH)?PL=j8xAvDImY#O=qM2~?GG z>$a9Z>^uBy&rq=}>4k)d&D*Bw%X&>QWJk5{FkVMeYj!ig~q* zkc#_~`6kP;ol;zy%0|&{nBP{8i_q<&1UT?UDm<_eUHq=7StNmN!9+X6SJSjOe#B+B z-c%BAPakXQXp0(PUv}D3J6EpRrj-tq zLTavrh>2au=tW-K%}=pob21Z_oO=f-$13yL((6y8i;I;66<@hjQwYYHLWSQCi8#~k zkJD;`Z6aQfWnSW{ZtA+%O*3d3B8|4L8N7jnEI7)Q{wOifovCZL4IJ}QU=Grm8sIT_ zY4@q~5#jQ5%aGltqF40qg85rIjP--5>9B*~zloB`f-aFi2=9^aMjGYeLx&8MxWjq_6q!<$!-67lw4Mt$&!2F`Z}EVx}I}~%?C}rCMC@i zy$;%bv`vfGVTP;f4iliPgz9l0*j}9?Cy~2xGQ$hyA62ThuDQ;#^<(ZNL+9Z$CEZ39 zik9K0I5=l)2le%C?qcv%o6rHC#Upf_-X8Gw4kke2S!{Z%e;$sO`lHgJE+xEHIQLbH zk5*F?LivM#CV6=aA=B(h^`@_X*BO$Se5PD5Z|lP2xMs5v(z@32`Ae{xQ6{?{U^~~F zNG^cjZE6gAe^FpwrMgc@X2>EHZSg7twl@2I)HdhAjf-s!wbbIu>(ZCZFtgQR3cPG% zB276<9s=Q+t-U*xg-M||s5*Bxnu9lRO#KYkp>Q<4`1+LmQJIKc=DKgG)!~n2$3OF0 ztF++y9G)$_CuXA#N;Ap1y3{^cZ3lx-%Hn<86zRMwzU=GGQQ&)4ezwL*gT@hW!t#UD zO@-Uj4sE5)<#fwWONw4KEjQa2swD>qSF1sM*f*1E3t43bA;ZO4f!{8!%Au-^Jqs#h zlnjbZ*1#(&;_?O9T{;Asrdp`49PQ8i-p-RSsufcIyxva9^|wtSRl zZP!II)xP&3AfPkaA)jxcB}p4`M{PF1Y4+e&J$3uZ;~|Jc^RueTnyd@++7T~|6Rjr)p8U`au$~|i%K)Q&}PFr1r z@5Myw>o^Z$61Of%^i=erkxnUfL0s_9)2|4hJY>v$3`wKM(azk7P)!7H>fq2m+gs? zJ<@yHcfS6j&Elzi1DI5*nQzXwUg_<%q%7x_Y(JAc6Qi|?@fA_&K9=jiASdgI1~+<)l9 LFADS*52E=OrzwSx literal 0 HcmV?d00001 diff --git a/test/parallel/test-http2-empty-frame-without-eof.js b/test/parallel/test-http2-empty-frame-without-eof.js new file mode 100644 index 00000000000000..02da78d940a92d --- /dev/null +++ b/test/parallel/test-http2-empty-frame-without-eof.js @@ -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());