Skip to content

Commit d51e377

Browse files
efekrskladuh95
authored andcommitted
quic: fix crypto error classification
Signed-off-by: Efe Karasakal <hi@efe.dev> PR-URL: #66968 Reviewed-By: Tim Perry <pimterry@gmail.com>
1 parent 0971803 commit d51e377

4 files changed

Lines changed: 147 additions & 16 deletions

File tree

‎lib/internal/quic/quic.js‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -768,10 +768,11 @@ setCallbacks({
768768
* Called when the underlying session C++ handle is closed either normally
769769
* or with an error.
770770
* @param {number} errorType
771-
* @param {number} code
771+
* @param {bigint} code
772772
* @param {string} [reason]
773-
* @param {string} [errorName] Decoded TLS alert name when `code` is a
774-
* CRYPTO_ERROR; otherwise undefined.
773+
* @param {string} [errorName] Symbolic name for a recognized transport
774+
* error code, including a decoded TLS alert for CRYPTO_ERROR; otherwise
775+
* undefined.
775776
*/
776777
onSessionClose(errorType, code, reason, errorName) {
777778
debug('session close callback', errorType, code, reason, errorName);
@@ -3915,8 +3916,9 @@ class QuicSession {
39153916

39163917
/**
39173918
* @param {number} errorType
3918-
* @param {number} code
3919+
* @param {bigint} code
39193920
* @param {string} [reason]
3921+
* @param {string} [errorName]
39203922
*/
39213923
[kFinishClose](errorType, code, reason, errorName) {
39223924
// If code is zero, then we closed without an error. Yay! We can destroy

‎src/quic/data.cc‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -374,15 +374,20 @@ error_code QuicError::h3_liberr_to_code(int liberr) {
374374
}
375375

376376
bool QuicError::is_crypto_error() const {
377-
return code() & NGTCP2_CRYPTO_ERROR;
377+
const auto error_code = code();
378+
return type() == Type::TRANSPORT && error_code >= NGTCP2_CRYPTO_ERROR &&
379+
error_code < NGTCP2_CRYPTO_ERROR + 0x100U;
378380
}
379381

380382
std::optional<int> QuicError::get_crypto_error() const {
381383
if (!is_crypto_error()) return std::nullopt;
382-
return code() & ~NGTCP2_CRYPTO_ERROR;
384+
return static_cast<int>(code() - NGTCP2_CRYPTO_ERROR);
383385
}
384386

385387
const char* QuicError::name() const {
388+
// Application error codes are
389+
// opaque to QUIC, so we only decode for transport.
390+
if (type() != Type::TRANSPORT) return nullptr;
386391
// CRYPTO_ERROR carries a TLS alert in its low byte (RFC 9001 sec. 4.8).
387392
// OpenSSL's SSL_alert_desc_string_long owns a stable string for every
388393
// alert it knows about; we filter out the "unknown" placeholder so the
@@ -392,10 +397,8 @@ const char* QuicError::name() const {
392397
if (n != nullptr && std::string_view(n) != "unknown") return n;
393398
return nullptr;
394399
}
395-
// Named transport-layer error codes from RFC 9000 sec. 20.1 (and the
396-
// RFC 9368 version-negotiation extension). Application error codes are
397-
// opaque to QUIC, so we only decode for transport.
398-
if (type() != Type::TRANSPORT) return nullptr;
400+
// Named transport-layer error codes from RFC 9000 sec. 20.1 and the
401+
// RFC 9368 version-negotiation extension.
399402
switch (code()) {
400403
case NGTCP2_NO_ERROR:
401404
return "NO_ERROR";

‎src/quic/data.h‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,8 @@ class QuicError final : public MemoryRetainer {
267267
bool is_crypto_error() const;
268268
std::optional<int> get_crypto_error() const;
269269

270-
// Returns a human-readable name for this error if known, or nullptr
270+
// Returns a human-readable name for a recognized transport error, including
271+
// a decoded TLS alert for CRYPTO_ERROR, or nullptr if the name is unknown.
271272
const char* name() const;
272273

273274
// Note that since application errors are application-specific and we
@@ -284,11 +285,12 @@ class QuicError final : public MemoryRetainer {
284285

285286
std::string ToString() const;
286287

287-
// Returns an array containing [type, code, reason], where type is a string
288-
// representation of the error type, code is a bigint representation of the
289-
// error code, and reason is a string representation of the error reason, or
290-
// undefined if the reason is the empty string. This is expected to be used
291-
// to construct a JS error object from the information in JS.
288+
// Returns an array containing [type, code, reason, error_name], where type
289+
// is a string representation of the error type, code is a bigint
290+
// representation of the error code, and reason is a string representation
291+
// of the error reason, or undefined if the reason is empty. error_name is
292+
// the symbolic name of a recognized transport error, or undefined. This is
293+
// expected to be used to construct a JS error object.
292294
v8::MaybeLocal<v8::Value> ToV8Value(Environment* env) const;
293295
inline v8::MaybeLocal<v8::Value> ToV8Value(Realm* realm) const {
294296
return ToV8Value(realm->env());
Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
// Flags: --experimental-quic --experimental-stream-iter --no-warnings
2+
3+
// Verify that error names are decoded only within the QUIC transport error
4+
// namespace and the CRYPTO_ERROR range.
5+
6+
import { hasQuic, skip, mustCall, mustNotCall } from '../common/index.mjs';
7+
import assert from 'node:assert';
8+
import * as fixtures from '../common/fixtures.mjs';
9+
10+
if (!hasQuic) {
11+
skip('QUIC is not enabled');
12+
}
13+
14+
const { listen, connect } = await import('node:quic');
15+
const { createPrivateKey } = await import('node:crypto');
16+
const { bytes } = await import('stream/iter');
17+
18+
const key = createPrivateKey(fixtures.readKey('agent1-key.pem'));
19+
const cert = fixtures.readKey('agent1-cert.pem');
20+
const decoder = new TextDecoder();
21+
22+
async function receiveCloseError(closeOptions) {
23+
const serverDone = Promise.withResolvers();
24+
const serverEndpoint = await listen(mustCall((serverSession) => {
25+
serverSession.onstream = mustCall(async (stream) => {
26+
await stream.closed;
27+
serverSession.close(closeOptions);
28+
serverDone.resolve();
29+
});
30+
}), {
31+
alpn: ['h3'],
32+
sni: { '*': { keys: [key], certs: [cert] } },
33+
onheaders: mustCall(function() {
34+
this.sendHeaders({ ':status': '200' });
35+
this.writer.writeSync('ok');
36+
this.writer.endSync();
37+
}),
38+
});
39+
40+
const clientSession = await connect(serverEndpoint.address, {
41+
alpn: 'h3',
42+
servername: 'localhost',
43+
verifyPeer: 'manual',
44+
});
45+
const closed = clientSession.closed.then(
46+
mustNotCall('session close must not resolve'),
47+
mustCall((error) => error),
48+
);
49+
await clientSession.opened;
50+
51+
const stream = await clientSession.createBidirectionalStream({
52+
headers: {
53+
':method': 'GET',
54+
':path': '/',
55+
':scheme': 'https',
56+
':authority': 'localhost',
57+
},
58+
onheaders: mustCall(function(headers) {
59+
assert.strictEqual(headers[':status'], 200);
60+
}),
61+
});
62+
63+
const body = await bytes(stream);
64+
assert.strictEqual(decoder.decode(body), 'ok');
65+
await Promise.all([stream.closed, serverDone.promise]);
66+
67+
const error = await closed;
68+
await serverEndpoint.close();
69+
return error;
70+
}
71+
72+
{
73+
const error = await receiveCloseError({
74+
code: 0x10an,
75+
type: 'application',
76+
});
77+
assert.strictEqual(error.code, 'ERR_QUIC_APPLICATION_ERROR');
78+
assert.strictEqual(error.type, 'application');
79+
assert.strictEqual(error.errorCode, 0x10an);
80+
assert.strictEqual(error.errorName, undefined);
81+
assert.strictEqual(error.message, 'QUIC application error 266');
82+
}
83+
84+
{
85+
const error = await receiveCloseError({
86+
code: 0x0fn,
87+
type: 'transport',
88+
});
89+
assert.strictEqual(error.code, 'ERR_QUIC_TRANSPORT_ERROR');
90+
assert.strictEqual(error.type, 'transport');
91+
assert.strictEqual(error.errorCode, 0x0fn);
92+
assert.strictEqual(error.errorName, 'AEAD_LIMIT_REACHED');
93+
assert.strictEqual(
94+
error.message,
95+
'QUIC transport error AEAD_LIMIT_REACHED (15)',
96+
);
97+
}
98+
99+
{
100+
const error = await receiveCloseError({
101+
code: 0x300n,
102+
type: 'transport',
103+
});
104+
assert.strictEqual(error.code, 'ERR_QUIC_TRANSPORT_ERROR');
105+
assert.strictEqual(error.type, 'transport');
106+
assert.strictEqual(error.errorCode, 0x300n);
107+
assert.strictEqual(error.errorName, undefined);
108+
assert.strictEqual(error.message, 'QUIC transport error 768');
109+
}
110+
111+
{
112+
const error = await receiveCloseError({
113+
code: 0x10an,
114+
type: 'transport',
115+
});
116+
assert.strictEqual(error.code, 'ERR_QUIC_TRANSPORT_ERROR');
117+
assert.strictEqual(error.type, 'transport');
118+
assert.strictEqual(error.errorCode, 0x10an);
119+
assert.strictEqual(error.errorName, 'unexpected message');
120+
assert.strictEqual(
121+
error.message,
122+
'QUIC transport error unexpected message (266)',
123+
);
124+
}

0 commit comments

Comments
 (0)