From 35352cd4baf774cb5c89406ed05a35d27f766295 Mon Sep 17 00:00:00 2001 From: Christian Aurich Zanettini Martins Date: Fri, 25 Sep 2026 17:01:36 -0300 Subject: [PATCH] quic: validate session close error codes `session.close()` and `session.destroy()` only checked that `options.code` was a bigint or a number. Numbers were then converted to `uint64_t` unchecked, so fractions were truncated and negative, non-finite or too large values were undefined behavior. Bigints were only checked to fit in 64 bits, but QUIC error codes are variable-length integers limited to 62 bits, so larger codes cannot be encoded in the `CONNECTION_CLOSE` frame. Reject codes that are not integers between `0` and `2n ** 62n - 1n`, the range `QuicError` already enforces, before any close or destroy side effects. Signed-off-by: Christian Aurich Zanettini Martins --- doc/api/quic.md | 6 +++-- lib/internal/quic/quic.js | 8 +++++++ ...-quic-session-destroy-validate-options.mjs | 23 ++++++++++++++++--- 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/doc/api/quic.md b/doc/api/quic.md index 5bdac8189fa..05de774dbd9 100644 --- a/doc/api/quic.md +++ b/doc/api/quic.md @@ -974,7 +974,8 @@ added: v23.8.0 * `options` {Object} * `code` {bigint|number} The error code to include in the `CONNECTION_CLOSE` - frame sent to the peer. **Default:** `0` (no error). + frame sent to the peer. Must be a non-negative 62-bit unsigned varint + (`0n <= code <= 2n ** 62n - 1n`). **Default:** `0` (no error). * `type` {string} Either `'transport'` or `'application'`. Determines the error code namespace used in the `CONNECTION_CLOSE` frame. When `'transport'` (the default), the frame type is `0x1c` and the code is interpreted as a QUIC @@ -1056,7 +1057,8 @@ added: v23.8.0 * `error` {any} * `options` {Object} * `code` {bigint|number} The error code to include in the `CONNECTION_CLOSE` - frame sent to the peer. **Default:** `0`. + frame sent to the peer. Must be a non-negative 62-bit unsigned varint + (`0n <= code <= 2n ** 62n - 1n`). **Default:** `0`. * `type` {string} Either `'transport'` or `'application'`. **Default:** `'transport'`. * `reason` {string} An optional human-readable reason string included in diff --git a/lib/internal/quic/quic.js b/lib/internal/quic/quic.js index 4922ce56275..afd436e02d5 100644 --- a/lib/internal/quic/quic.js +++ b/lib/internal/quic/quic.js @@ -13,6 +13,7 @@ const { ErrorCaptureStackTrace, FunctionPrototypeBind, FunctionPrototypeCall, + NumberIsInteger, ObjectDefineProperties, ObjectKeys, PromisePrototypeThen, @@ -5478,6 +5479,13 @@ function validateCloseOptions(options) { throw new ERR_INVALID_ARG_TYPE('options.code', ['bigint', 'number'], code); } + if (typeof code === 'number' && !NumberIsInteger(code)) { + throw new ERR_OUT_OF_RANGE('options.code', 'an integer', code); + } + if (code < 0 || code > kMaxQuicErrorCode) { + throw new ERR_OUT_OF_RANGE('options.code', + `>= 0 and <= ${kMaxQuicErrorCode}`, code); + } } validateOneOf(type, 'options.type', ['transport', 'application']); if (reason !== undefined) { diff --git a/test/parallel/test-quic-session-destroy-validate-options.mjs b/test/parallel/test-quic-session-destroy-validate-options.mjs index b188033f573..a57b718f812 100644 --- a/test/parallel/test-quic-session-destroy-validate-options.mjs +++ b/test/parallel/test-quic-session-destroy-validate-options.mjs @@ -97,6 +97,20 @@ assert.throws(() => clientSession.destroy(goodError, { reason: 42 }), { assert.strictEqual(clientSession.destroyed, false); assert.strictEqual(stream.destroyed, false); +// 5. options.code does not fit in a QUIC varint -> throws ERR_OUT_OF_RANGE, +// from both destroy() and close(). +for (const code of [-1, 1.5, NaN, Infinity, 2 ** 62, -1n, 2n ** 62n]) { + assert.throws(() => clientSession.destroy(goodError, { code }), { + code: 'ERR_OUT_OF_RANGE', + }); + assert.throws(() => clientSession.close({ code }), { + code: 'ERR_OUT_OF_RANGE', + }); +} +assert.strictEqual(clientSession.destroyed, false); +assert.strictEqual(clientSession.closing, false); +assert.strictEqual(stream.destroyed, false); + // Now switch the handlers to expect the real teardown so the final // destroy with valid options can run cleanly. diagnostics_channel.unsubscribe('quic.session.error', errSub); @@ -107,19 +121,22 @@ stream.onerror = mustCall((err) => { assert.strictEqual(err, goodError); }); // final destroy, so the rejections do not race ahead of any awaits in // the test body. The client rejects with the original `goodError`; // the server decodes the CONNECTION_CLOSE frame transport code into -// an `ERR_QUIC_TRANSPORT_ERROR`. +// an `ERR_QUIC_TRANSPORT_ERROR`. The code is the largest one that fits +// in a QUIC varint, so it must be accepted and arrive unchanged. +const maxCode = 2n ** 62n - 1n; const clientClosedAssertion = assert.rejects(clientSession.closed, goodError); const serverClosedAssertion = assert.rejects(serverSession.closed, mustCall((err) => { assert.strictEqual(err.code, 'ERR_QUIC_TRANSPORT_ERROR'); + assert.strictEqual(err.errorCode, maxCode); return true; })); -// 5. Valid options after the failed attempts -> session destroys +// 6. Valid options after the failed attempts -> session destroys // normally, the underlying handle sends CONNECTION_CLOSE with the // supplied transport code, and the local closed promise rejects // with the original error. clientSession.destroy(goodError, { - code: 1n, + code: maxCode, type: 'transport', reason: 'after validation throw', });