From fd15502a10033d1a13d66dadd3485e23ec7e5450 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 23 Mar 2021 15:44:21 +0100 Subject: [PATCH 1/4] 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. --- lib/internal/http2/core.js | 52 ++++++++++++++++++++++++++++++++------ 1 file changed, 44 insertions(+), 8 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index abcc460c423c78..2e9f11460b6459 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -19,14 +19,15 @@ const { Promise, PromisePrototypeCatch, ReflectApply, + ReflectGet, ReflectGetPrototypeOf, + ReflectSet, RegExpPrototypeTest, SafeArrayIterator, SafeMap, SafeSet, StringPrototypeSlice, Symbol, - TypedArrayPrototypeSet, Uint32Array, Uint8Array, } = primordials; @@ -960,6 +961,35 @@ 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 modifiedEntries = new Uint8Array(typedArray.length); + + function copyAssigned(target) { + for (let i = 0; i < typedArray.length; 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 @@ -990,10 +1020,13 @@ function setupHandle(socket, type, options) { handle.consume(socket._handle); this[kHandle] = handle; - if (this[kNativeFields]) - TypedArrayPrototypeSet(handle.fields, 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; @@ -1045,7 +1078,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) { @@ -1213,8 +1247,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 515cab2b8e2004d129dbda9014ed3965fb10f009 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 23 Mar 2021 15:48:41 +0100 Subject: [PATCH 2/4] 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 --- 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 112a3a336e0297..275284a147d583 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1335,7 +1335,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()); From 752cb857760e92882e65f5726272471caa178fb1 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 23 Mar 2021 18:05:00 +0100 Subject: [PATCH 3/4] fixup! http2: fix setting options before handle exists Co-authored-by: Antoine du Hamel --- lib/internal/http2/core.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 2e9f11460b6459..1d97822b4b087a 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -964,10 +964,11 @@ 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 modifiedEntries = new Uint8Array(typedArray.length); + const typedArrayLength = TypedArrayPrototypeGetLength(typedArray); + const modifiedEntries = new Uint8Array(typedArrayLength); function copyAssigned(target) { - for (let i = 0; i < typedArray.length; i++) { + for (let i = 0; i < typedArrayLength; i++) { if (modifiedEntries[i]) { target[i] = typedArray[i]; } From a087881a248663e89461f1c9f87252a440ff70e6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 23 Mar 2021 19:05:49 +0100 Subject: [PATCH 4/4] fixup! http2: fix setting options before handle exists --- lib/internal/http2/core.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 1d97822b4b087a..620e28a1524aa8 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -28,6 +28,7 @@ const { SafeSet, StringPrototypeSlice, Symbol, + TypedArrayPrototypeGetLength, Uint32Array, Uint8Array, } = primordials;