From 71ce4ef9814386a12a152bae97669a0e5b3b83ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 6 Jul 2021 12:39:43 +0200 Subject: [PATCH] Fix bug with UDP proxy not forwarding enough MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We previously introduced a safety check ensuring that if a datagram had already been dropped twice, it would no longer be dropped or delayed after that. This missed an edge case: if a datagram is dropped once, it can be delayed any number of times. Since "delay" is not defined in terms of time (x seconds) but in terms of ordering with respect to other messages (will be forwarded after the next message is forwarded), depending on the RNG results this could result in an endless loop where all messages are delayed until the next, which is itself delayed, etc. and no message is ever forwarded. The probability of this happening n times in a row is (1/d)^n, where d is the value passed as delay=d, so for delay=5 and n=5 it's around 0.03% which seems small but we still happened on such an occurrence in real life: tests/ssl-opt.sh --seed 1625061502 -f 'DTLS proxy: 3d, min handshake, resumption$' results (according to debug statements added for the investigation) in the ClientHello of the second handshake being dropped once then delayed 5 times, after which the client stops re-trying and the test fails for no interesting reason. Make sure this doesn't happen again by putting a cap on the number of times we fail to forward a given datagram immediately. Signed-off-by: Manuel Pégourié-Gonnard --- programs/test/udp_proxy.c | 40 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/programs/test/udp_proxy.c b/programs/test/udp_proxy.c index 307fc7426..6546e8fd0 100644 --- a/programs/test/udp_proxy.c +++ b/programs/test/udp_proxy.c @@ -681,26 +681,21 @@ int send_delayed() } /* - * Avoid dropping or delaying a packet that was already dropped twice: this - * only results in uninteresting timeouts. We can't rely on type to identify - * packets, since during renegotiation they're all encrypted. So, rely on - * size mod 2048 (which is usually just size). - */ -static unsigned char dropped[2048] = { 0 }; -#define DROP_MAX 2 - -/* We only drop packets at the level of entire datagrams, not at the level + * Avoid dropping or delaying a packet that was already dropped or delayed + * ("held") twice: this only results in uninteresting timeouts. We can't rely + * on type to identify packets, since during renegotiation they're all + * encrypted. So, rely on size mod 2048 (which is usually just size). + * + * We only hold packets at the level of entire datagrams, not at the level * of records. In particular, if the peer changes the way it packs multiple * records into a single datagram, we don't necessarily count the number of - * times a record has been dropped correctly. However, the only known reason + * times a record has been held correctly. However, the only known reason * why a peer would change datagram packing is disabling the latter on - * retransmission, in which case we'd drop involved records at most - * DROP_MAX + 1 times. */ -void update_dropped( const packet *p ) -{ - size_t id = p->len % sizeof( dropped ); - ++dropped[id]; -} + * retransmission, in which case we'd hold involved records at most + * HOLD_MAX + 1 times. + */ +static unsigned char held[2048] = { 0 }; +#define HOLD_MAX 2 int handle_message( const char *way, mbedtls_net_context *dst, @@ -727,7 +722,7 @@ int handle_message( const char *way, cur.dst = dst; print_packet( &cur, NULL ); - id = cur.len % sizeof( dropped ); + id = cur.len % sizeof( held ); if( strcmp( way, "S <- C" ) == 0 ) { @@ -769,10 +764,10 @@ int handle_message( const char *way, ! ( opt.protect_hvr && strcmp( cur.type, "HelloVerifyRequest" ) == 0 ) && cur.len != (size_t) opt.protect_len && - dropped[id] < DROP_MAX && + held[id] < HOLD_MAX && rand() % opt.drop == 0 ) ) { - update_dropped( &cur ); + ++held[id]; } else if( ( opt.delay_ccs == 1 && strcmp( cur.type, "ChangeCipherSpec" ) == 0 ) || @@ -782,9 +777,10 @@ int handle_message( const char *way, ! ( opt.protect_hvr && strcmp( cur.type, "HelloVerifyRequest" ) == 0 ) && cur.len != (size_t) opt.protect_len && - dropped[id] < DROP_MAX && + held[id] < HOLD_MAX && rand() % opt.delay == 0 ) ) { + ++held[id]; delay_packet( &cur ); } else @@ -895,7 +891,7 @@ accept: * 3. Forward packets forever (kill the process to terminate it) */ clear_pending(); - memset( dropped, 0, sizeof( dropped ) ); + memset( held, 0, sizeof( held ) ); nb_fds = client_fd.fd; if( nb_fds < server_fd.fd )