From d32bdd14c2a764b6920afee84951cbd3886faf55 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 23 Mar 2021 15:44:21 +0100 Subject: [PATCH 1/2] http2: fix setting options before handle exists Currently, when a JS Http2Session object is created, we have to handle the situation in which the native object corresponding to it does not yet exist. As part of that, we create a typed array for storing options that are passed through the `AliasedStruct` mechanism, and up until now, we copied that typed array over the native one once the native one was available. This was not good, because it was overwriting the defaults that were set during construction of the native typed array with zeroes. In order to fix this, create a wrapper for the JS-created typed array that keeps track of which fields were changed, which enables us to only overwrite fields that were intentionally changed on the JS side. It is surprising that this behavior was not tested (which is, guessing from the commit history around these features, my fault). The subseqeuent commit introduces a test that would fail without this change. PR-URL: https://github.com/nodejs/node/pull/37875 Fixes: https://github.com/nodejs/node/issues/37849 Reviewed-By: Matteo Collina Reviewed-By: Colin Ihrig --- lib/internal/http2/core.js | 52 +++++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 7 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index cca254be3f8559..5e930ad216a1f5 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -13,7 +13,9 @@ const { ObjectPrototypeHasOwnProperty, Promise, ReflectApply, + ReflectGet, ReflectGetPrototypeOf, + ReflectSet, Set, Symbol, Uint32Array, @@ -933,6 +935,36 @@ const validateSettings = hideStackFrames((settings) => { } }); +// Wrap a typed array in a proxy, and allow selectively copying the entries +// that have explicitly been set to another typed array. +function trackAssignmentsTypedArray(typedArray) { + const typedArrayLength = typedArray.length; + const modifiedEntries = new Uint8Array(typedArrayLength); + + function copyAssigned(target) { + for (let i = 0; i < typedArrayLength; i++) { + if (modifiedEntries[i]) { + target[i] = typedArray[i]; + } + } + } + + return new Proxy(typedArray, { + get(obj, prop, receiver) { + if (prop === 'copyAssigned') { + return copyAssigned; + } + return ReflectGet(obj, prop, receiver); + }, + set(obj, prop, value) { + if (`${+prop}` === prop) { + modifiedEntries[prop] = 1; + } + return ReflectSet(obj, prop, value); + } + }); +} + // Creates the internal binding.Http2Session handle for an Http2Session // instance. This occurs only after the socket connection has been // established. Note: the binding.Http2Session will take over ownership @@ -963,10 +995,13 @@ function setupHandle(socket, type, options) { handle.consume(socket._handle); this[kHandle] = handle; - if (this[kNativeFields]) - handle.fields.set(this[kNativeFields]); - else - this[kNativeFields] = handle.fields; + if (this[kNativeFields]) { + // If some options have already been set before the handle existed, copy + // those (and only those) that have manually been set over. + this[kNativeFields].copyAssigned(handle.fields); + } + + this[kNativeFields] = handle.fields; if (socket.encrypted) { this[kAlpnProtocol] = socket.alpnProtocol; @@ -1018,7 +1053,8 @@ function cleanupSession(session) { session[kProxySocket] = undefined; session[kSocket] = undefined; session[kHandle] = undefined; - session[kNativeFields] = new Uint8Array(kSessionUint8FieldCount); + session[kNativeFields] = trackAssignmentsTypedArray( + new Uint8Array(kSessionUint8FieldCount)); if (handle) handle.ondone = null; if (socket) { @@ -1184,8 +1220,10 @@ class Http2Session extends EventEmitter { setupFn(); } - if (!this[kNativeFields]) - this[kNativeFields] = new Uint8Array(kSessionUint8FieldCount); + if (!this[kNativeFields]) { + this[kNativeFields] = trackAssignmentsTypedArray( + new Uint8Array(kSessionUint8FieldCount)); + } this.on('newListener', sessionListenerAdded); this.on('removeListener', sessionListenerRemoved); From 990ef981f71fcc179e87a26c1c56c439ab044c17 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 23 Mar 2021 15:48:41 +0100 Subject: [PATCH 2/2] 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 fb23cb0f51073c..5e7b484886f801 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -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; } 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());