From 5b6cd6fa1a0b54393d96ee0b34bd5d9334a9faec Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 3 Aug 2020 16:32:53 -0700 Subject: [PATCH] quic: use the SocketAddressLRU to track validation status PR-URL: https://github.com/nodejs/node/pull/34618 Reviewed-By: Anna Henningsen Reviewed-By: Rich Trott --- doc/api/quic.md | 5 ---- lib/internal/quic/core.js | 6 ---- lib/internal/quic/util.js | 5 +--- src/quic/node_quic.cc | 1 - src/quic/node_quic_socket.cc | 28 +++++++++++-------- src/quic/node_quic_socket.h | 4 +-- .../test-quic-errors-quicsocket-create.js | 7 ----- 7 files changed, 19 insertions(+), 37 deletions(-) diff --git a/doc/api/quic.md b/doc/api/quic.md index 6969a43ec9f716..4feba4d2bb595f 100644 --- a/doc/api/quic.md +++ b/doc/api/quic.md @@ -288,11 +288,6 @@ added: REPLACEME * `validateAddress` {boolean} When `true`, the `QuicSocket` will use explicit address validation using a QUIC `RETRY` frame when listening for new server sessions. Default: `false`. - * `validateAddressLRU` {boolean} When `true`, validation will be skipped if - the address has been recently validated. Currently, only the 10 most - recently validated addresses are remembered. Setting `validateAddressLRU` - to `true`, will enable the `validateAddress` option as well. Default: - `false`. The `net.createQuicSocket()` function is used to create new `QuicSocket` instances associated with a local UDP address. diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js index b0e8688a64ba84..11515ebf5fa476 100644 --- a/lib/internal/quic/core.js +++ b/lib/internal/quic/core.js @@ -179,7 +179,6 @@ const { QUICCLIENTSESSION_OPTION_REQUEST_OCSP, QUICCLIENTSESSION_OPTION_VERIFY_HOSTNAME_IDENTITY, QUICSOCKET_OPTIONS_VALIDATE_ADDRESS, - QUICSOCKET_OPTIONS_VALIDATE_ADDRESS_LRU, QUICSTREAM_HEADERS_KIND_NONE, QUICSTREAM_HEADERS_KIND_INFORMATIONAL, QUICSTREAM_HEADERS_KIND_INITIAL, @@ -941,9 +940,6 @@ class QuicSocket extends EventEmitter { // True if address verification should be used. validateAddress, - // True if an LRU should be used for add validation - validateAddressLRU, - // Whether qlog should be enabled for sessions qlog, @@ -964,8 +960,6 @@ class QuicSocket extends EventEmitter { let socketOptions = 0; if (validateAddress) socketOptions |= (1 << QUICSOCKET_OPTIONS_VALIDATE_ADDRESS); - if (validateAddressLRU) - socketOptions |= (1 << QUICSOCKET_OPTIONS_VALIDATE_ADDRESS_LRU); this[kSetHandle]( new QuicSocketHandle( diff --git a/lib/internal/quic/util.js b/lib/internal/quic/util.js index 1d742b5da7a5b9..77ca135a272178 100644 --- a/lib/internal/quic/util.js +++ b/lib/internal/quic/util.js @@ -539,7 +539,6 @@ function validateQuicSocketOptions(options = {}) { retryTokenTimeout = DEFAULT_RETRYTOKEN_EXPIRATION, server = {}, statelessResetSecret, - validateAddressLRU = false, validateAddress = false, } = options; @@ -548,7 +547,6 @@ function validateQuicSocketOptions(options = {}) { validateObject(server, 'options.server'); validateLookup(lookup); validateBoolean(validateAddress, 'options.validateAddress'); - validateBoolean(validateAddressLRU, 'options.validateAddressLRU'); validateBoolean(qlog, 'options.qlog'); validateBoolean(disableStatelessReset, 'options.disableStatelessReset'); @@ -597,8 +595,7 @@ function validateQuicSocketOptions(options = {}) { retryTokenTimeout, server, type, - validateAddress: validateAddress || validateAddressLRU, - validateAddressLRU, + validateAddress, qlog, statelessResetSecret, disableStatelessReset, diff --git a/src/quic/node_quic.cc b/src/quic/node_quic.cc index 29473492198d61..48ab6ac2fdc56f 100644 --- a/src/quic/node_quic.cc +++ b/src/quic/node_quic.cc @@ -189,7 +189,6 @@ void Initialize(Local target, V(QUICSERVERSESSION_OPTION_REJECT_UNAUTHORIZED) \ V(QUICSERVERSESSION_OPTION_REQUEST_CERT) \ V(QUICSOCKET_OPTIONS_VALIDATE_ADDRESS) \ - V(QUICSOCKET_OPTIONS_VALIDATE_ADDRESS_LRU) \ V(QUICSTREAM_HEADER_FLAGS_NONE) \ V(QUICSTREAM_HEADER_FLAGS_TERMINAL) \ V(QUICSTREAM_HEADERS_KIND_NONE) \ diff --git a/src/quic/node_quic_socket.cc b/src/quic/node_quic_socket.cc index a0c137998a3483..0e054619df899a 100644 --- a/src/quic/node_quic_socket.cc +++ b/src/quic/node_quic_socket.cc @@ -675,23 +675,27 @@ bool QuicSocket::SendStatelessReset( // peer must termination it's initial attempt to // establish a connection and start a new attempt. // -// TODO(@jasnell): Retry packets will only ever be -// generated by QUIC servers, and only if the QuicSocket -// is configured for explicit path validation. There is -// no way for a client to force a retry packet to be created. -// However, once a client determines that explicit -// path validation is enabled, it could attempt to -// DOS by sending a large number of malicious -// initial packets to intentionally ellicit retry -// packets (It can do so by intentionally sending -// initial packets that ignore the retry token). -// To help mitigate that risk, we should limit the number -// of retries we send to a given remote endpoint. +// Retry packets will only ever be generated by QUIC servers, +// and only if the QuicSocket is configured for explicit path +// validation. There is no way for a client to force a retry +// packet to be created. However, once a client determines that +// explicit path validation is enabled, it could attempt to +// DOS by sending a large number of malicious initial packets +// to intentionally ellicit retry packets (It can do so by +// intentionally sending initial packets that ignore the retry +// token). To help mitigate that risk, we limit the number of +// retries we send to a given remote endpoint. bool QuicSocket::SendRetry( const QuicCID& dcid, const QuicCID& scid, const SocketAddress& local_addr, const SocketAddress& remote_addr) { + auto info = addrLRU_.Upsert(remote_addr); + // Do not send a retry if the retry count is greater + // than the retry limit. + // TODO(@jasnell): Make the retry limit configurable. + if (++(info->retry_count) > DEFAULT_MAX_RETRY_LIMIT) + return true; std::unique_ptr packet = GenerateRetryPacket(token_secret_, dcid, scid, local_addr, remote_addr); return packet ? diff --git a/src/quic/node_quic_socket.h b/src/quic/node_quic_socket.h index 9df0235050f15a..15ce987f582b67 100644 --- a/src/quic/node_quic_socket.h +++ b/src/quic/node_quic_socket.h @@ -36,10 +36,10 @@ class QuicSocket; class QuicEndpoint; constexpr size_t DEFAULT_MAX_SOCKETADDRESS_LRU_SIZE = 1000; +constexpr size_t DEFAULT_MAX_RETRY_LIMIT = 10; #define QUICSOCKET_OPTIONS(V) \ - V(VALIDATE_ADDRESS, validate_address) \ - V(VALIDATE_ADDRESS_LRU, validate_address_lru) + V(VALIDATE_ADDRESS, validate_address) #define V(id, _) QUICSOCKET_OPTIONS_##id, enum QuicSocketOptions : uint32_t { diff --git a/test/parallel/test-quic-errors-quicsocket-create.js b/test/parallel/test-quic-errors-quicsocket-create.js index 570f02b1a95661..e7e47339fe34cb 100644 --- a/test/parallel/test-quic-errors-quicsocket-create.js +++ b/test/parallel/test-quic-errors-quicsocket-create.js @@ -68,13 +68,6 @@ const { createQuicSocket } = require('net'); }); }); -// Test invalid QuicSocket validateAddressLRU argument option -[1, NaN, 1n, null, {}, []].forEach((validateAddressLRU) => { - assert.throws(() => createQuicSocket({ validateAddressLRU }), { - code: 'ERR_INVALID_ARG_TYPE' - }); -}); - // Test invalid QuicSocket qlog argument option [1, NaN, 1n, null, {}, []].forEach((qlog) => { assert.throws(() => createQuicSocket({ qlog }), {