From patchwork Thu Sep 3 19:34:39 2026 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gert Doering X-Patchwork-Id: 5314 Return-Path: Delivered-To: patchwork@openvpn.net Received: by 2002:a05:7000:2190:b0:892:1b45:3040 with SMTP id s16csp2585841mae; Thu, 3 Sep 2026 12:35:01 -0700 (PDT) X-Forwarded-Encrypted: i=2; AKwUvBzqsV3RPh1T4DYN2z/LVTOiFEGRxJTuqYSrl4dze7/2Np03L+/SdmjravWXf9DZmW5Dd2H2YekJ0iM=@openvpn.net X-Received: by 2002:a05:6808:23cd:b0:4b2:8d7b:390b with SMTP id 5614622812f47-4b963dc8ac3mr546459b6e.19.1788464101232; Thu, 03 Sep 2026 12:35:01 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1788464101; cv=none; d=google.com; s=arc-20260327; b=B4lhwfOJbEJNuj9dsCubh7fBsfexdavdjT0c0YDhEk/LznvhxvQ1qboG56csM8eSgx ZhJKZ/fNXtXC5lh9lbN0P87zbaG8R6HoagAuxPOeFgXeCetMCM8005diHl/ThD3YLXej s282/5CLoye1TGEzwUSwIvzwdNz19aKgF6DLYEp5kZ82C6ai6ksB2SwMJR/upmTI9/Sm tjcDpiXnt9AnxWeaX1JWqjt1GomBPFyjn5LScirxCeC/2dk4Chdqzti0coh3G0WzG1vj X+UQEByshhkhrsYnC/YdA/WSt/5iLrnF/UzTRB181vSR2p2EkHWvg7RSYA/rwg+B5ojW uOrA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20260327; h=errors-to:content-transfer-encoding:list-subscribe:list-help :list-post:list-archive:list-unsubscribe:list-id:precedence:subject :mime-version:references:in-reply-to:message-id:date:to:from :dkim-signature:dkim-signature:dkim-signature; bh=y+UEO9SI/LoGrypyMLvt2Ix/fyR4QSbUuWpOyunUgIs=; fh=4NbAC/LsuMLI0S0hprUlLSLCiHwg6SCAifhH718Jh0Q=; b=X+YofnUrxcR+EasVYjLM+TJstC8H3kM8AeXs50ah7KeR81jS1H2WCbty7sofp+661i 7iX0WOgiw+kZ0qjOUgvD8rizzVdgTufvguiUdLtNqF9MwDqo/74G6MTjd9GMFcSwLxbP E1h8X0X3m+92VPP4AW9KRdupSnPmaOzLAGepM3+TP0o1OCl8KIT9WAPpr4wVdtWb/Lj6 affCshM3gkRsJB0fXdN1Lha1/ZNHATi4Z0Tn78T4QauNXdhnaUu1qWBQuhxQyVpxp8HK BWiv8MQkl+JBgUEnYnRJnr7dUa8iU1MPoQ86QmUy8jJptW9nSL8OICs8ok/13Sho0rcc 3a2w==; dara=google.com ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@lists.sourceforge.net header.s=beta header.b=UBSVbzAY; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=hOTCUwDW; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=KoWcFp5d; spf=pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) smtp.mailfrom=openvpn-devel-bounces@lists.sourceforge.net; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=muc.de Received: from lists.sourceforge.net (lists.sourceforge.net. [216.105.38.7]) by mx.google.com with ESMTPS id 5614622812f47-4b96fbe3ad6si278883b6e.45.2026.09.03.12.35.00 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 03 Sep 2026 12:35:00 -0700 (PDT) Received-SPF: pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) client-ip=216.105.38.7; Authentication-Results: mx.google.com; dkim=pass header.i=@lists.sourceforge.net header.s=beta header.b=UBSVbzAY; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=hOTCUwDW; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=KoWcFp5d; spf=pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) smtp.mailfrom=openvpn-devel-bounces@lists.sourceforge.net; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=muc.de DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.sourceforge.net; s=beta; h=Content-Transfer-Encoding:Content-Type: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: Subject:MIME-Version:References:In-Reply-To:Message-ID:Date:To:From:Sender: Reply-To:Cc:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=y+UEO9SI/LoGrypyMLvt2Ix/fyR4QSbUuWpOyunUgIs=; b=UBSVbzAY08taqabvANkJ9O1tdz sewiuLgEN2jcfTETD06Ea8B8ND9NZqIoU+kxFMrIM0VfLZOM/DdXPtmcqzGObRDJYKJkck/TIzanc YpkhjCCwrmbJDZIT11qqRhXuyUq00vRVQizAwQMehT5ty0tOhEGu9e25R8Sl1eda151k=; Received: from [127.0.0.1] (helo=sfs-ml-4.v29.lw.sourceforge.com) by sfs-ml-4.v29.lw.sourceforge.com with esmtp (Exim 4.95) (envelope-from ) id 1x2DD7-0006Vp-12; Thu, 03 Sep 2026 19:34:57 +0000 Received: from [172.30.29.66] (helo=mx.sourceforge.net) by sfs-ml-4.v29.lw.sourceforge.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1x2DD5-0006Vf-LO for openvpn-devel@lists.sourceforge.net; Thu, 03 Sep 2026 19:34:55 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=Content-Transfer-Encoding:MIME-Version:References: In-Reply-To:Message-ID:Date:Subject:To:From:Sender:Reply-To:Cc:Content-Type: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=spsr6QKpzHhnAjbS8EnkYt3oNRzjFSzUdDk8KRm1jwA=; b=hOTCUwDWW/NiTH/JCERhIn5Qy0 VKunIWKvXmqdiW0r0/zG1HbJts9otPw01RxCJGoyp6nQEUT6HGIwfPiAMMMU4os6+lp+jhhQdqcBk ikhiUtBDVbO93i4wxv8Ez5anKPuTffEm9vFtnFiJyxNCEcfRw372QGCmRgdyy20sMR98=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=Content-Transfer-Encoding:MIME-Version:References:In-Reply-To:Message-ID: Date:Subject:To:From:Sender:Reply-To:Cc:Content-Type:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=spsr6QKpzHhnAjbS8EnkYt3oNRzjFSzUdDk8KRm1jwA=; b=KoWcFp5dGnGmEKuPgeMWxozHX+ CaXsshSmLBGjh+9D3fmj6vSINT7RoChtKsM6nBcvn2kH89tRLD+l8cbXBnqOKiRy0jCupW/+ZLWAN da6Pee8602kT3lcvLROBgbnZMgzppnE2rmVaUiKplhETO2IDcd36KhTNI6YRF42P3g/s=; Received: from [193.149.48.129] (helo=blue.greenie.muc.de) by sfi-mx-1.v28.lw.sourceforge.com with esmtps (TLS1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.95) id 1x2DD4-0003vQ-0q for openvpn-devel@lists.sourceforge.net; Thu, 03 Sep 2026 19:34:55 +0000 Received: from blue.greenie.muc.de (localhost [127.0.0.1]) by blue.greenie.muc.de (8.18.1/8.18.1) with ESMTP id 683JYk2d020293 for ; Thu, 3 Sep 2026 21:34:46 +0200 Received: (from gert@localhost) by blue.greenie.muc.de (8.18.2/8.18.1/Submit) id 683JYkt5020292 for openvpn-devel@lists.sourceforge.net; Thu, 3 Sep 2026 21:34:46 +0200 From: Gert Doering To: openvpn-devel@lists.sourceforge.net Date: Thu, 3 Sep 2026 21:34:39 +0200 Message-ID: <20260903193446.20274-1-gert@greenie.muc.de> X-Mailer: git-send-email 2.53.0 In-Reply-To: References: MIME-Version: 1.0 X-Spam-Score: 1.3 (+) X-Spam-Report: Spam detection software, running on the system "sfi-spamd-2.hosts.colo.sdot.me", has NOT identified this incoming email as spam. The original message has been attached to this so you can view it or label similar future email. If you have any questions, see the administrator of that system for details. Content preview: From: Lev Stipakov Add regression coverage for the reliability-layer denial-of-service fix: - the retransmission timeout must stay positive and bounded no matter how often the fast-retransmit path is forced, so it can no longer overflow; - reliable_send_purge() must ignore ACKs for packet I [...] Content analysis details: (1.3 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- 1.3 RDNS_NONE Delivered to internal network by a host with no rDNS X-Headers-End: 1x2DD4-0003vQ-0q Subject: [Openvpn-devel] [PATCH v2] reliable: add unit tests for ACK and backoff DoS hardening X-BeenThere: openvpn-devel@lists.sourceforge.net X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: openvpn-devel-bounces@lists.sourceforge.net X-getmail-retrieved-from-mailbox: Inbox X-GMAIL-THRID: 1875340533145813388 X-GMAIL-MSGID: 1875340533145813388 From: Lev Stipakov Add regression coverage for the reliability-layer denial-of-service fix: - the retransmission timeout must stay positive and bounded no matter how often the fast-retransmit path is forced, so it can no longer overflow; - reliable_send_purge() must ignore ACKs for packet IDs that could never have been in flight (out of the send window, or wrapped-around), so a peer cannot inflate n_acks and force early retransmits; - a legitimate ACK for a real higher packet ID still removes that entry and counts towards fast retransmit (positive control). These tests fail on the unfixed code and pass with the fix. Signed-off-by: Lev Stipakov Acked-by: Gert Doering Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1894 Change-Id: I3c6ad247614fb204a279b73e98f8c4704e16da2a --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1894 This mail reflects revision 2 of this Change. Acked-by according to Gerrit (reflected above): Gert Doering diff --git a/CMakeLists.txt b/CMakeLists.txt index 6eb5954..429f143 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -680,6 +680,7 @@ "test_options_parse" "test_packet_id" "test_pkt" + "test_reliable" "test_provider" "test_socket" "test_ssl" @@ -881,6 +882,14 @@ src/openvpn/session_id.c ) + target_sources(test_reliable PRIVATE + tests/unit_tests/openvpn/mock_get_random.c + src/openvpn/otime.c + src/openvpn/packet_id.c + src/openvpn/reliable.c + src/openvpn/session_id.c + ) + target_sources(test_pkt PRIVATE tests/unit_tests/openvpn/mock_win32_execve.c src/openvpn/argv.c diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am index 5954902..44614ae 100644 --- a/tests/unit_tests/openvpn/Makefile.am +++ b/tests/unit_tests/openvpn/Makefile.am @@ -17,6 +17,7 @@ options_parse_testdriver \ packet_id_testdriver \ pkt_testdriver \ + reliable_testdriver \ provider_testdriver \ push_update_msg_testdriver \ socket_testdriver \ @@ -153,6 +154,21 @@ $(top_srcdir)/src/openvpn/win32-util.c \ $(top_srcdir)/src/openvpn/session_id.c +reliable_testdriver_CFLAGS = \ + -I$(top_srcdir)/include -I$(top_srcdir)/src/compat -I$(top_srcdir)/src/openvpn \ + @TEST_CFLAGS@ +reliable_testdriver_LDFLAGS = @TEST_LDFLAGS@ +reliable_testdriver_SOURCES = test_reliable.c \ + mock_msg.c mock_msg.h test_common.h \ + mock_get_random.c \ + $(top_srcdir)/src/openvpn/buffer.c \ + $(top_srcdir)/src/openvpn/otime.c \ + $(top_srcdir)/src/openvpn/packet_id.c \ + $(top_srcdir)/src/openvpn/platform.c \ + $(top_srcdir)/src/openvpn/reliable.c \ + $(top_srcdir)/src/openvpn/win32-util.c \ + $(top_srcdir)/src/openvpn/session_id.c + pkt_testdriver_CFLAGS = \ -I$(top_srcdir)/include -I$(top_srcdir)/src/compat -I$(top_srcdir)/src/openvpn \ @TEST_CFLAGS@ diff --git a/tests/unit_tests/openvpn/test_reliable.c b/tests/unit_tests/openvpn/test_reliable.c new file mode 100644 index 0000000..fa85906 --- /dev/null +++ b/tests/unit_tests/openvpn/test_reliable.c @@ -0,0 +1,193 @@ +/* + * OpenVPN -- An application to securely tunnel IP networks + * over a single UDP port, with support for SSL/TLS-based + * session authentication and key exchange, + * packet encryption, packet authentication, and + * packet compression. + * + * Copyright (C) 2026 OpenVPN Inc + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program (see the file COPYING included with this + * distribution); if not, see . + */ + +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include "syshead.h" + +#include +#include +#include +#include + +#include "reliable.h" +#include "test_common.h" + +/* The fix keeps the timeout well below this; the broken code grows past it. + * A plain number, so the test builds with or without the fix. */ +#define SANE_TIMEOUT_BOUND (10 * 1000 * 1000) + +static struct reliable * +test_reliable_new(void) +{ + struct reliable *rel = malloc(sizeof(struct reliable)); + assert_non_null(rel); + /* reliable_init() zeroes everything, so each test only sets the + * fields it actually needs. */ + reliable_init(rel, 100, 50, 8, false); + rel->initial_timeout = 2; + return rel; +} + +/* + * Each retransmit doubles the timeout. If a peer keeps forcing retransmits, + * the broken code doubles it forever: after about 30 rounds it overflows and + * turns zero or negative, and then the packet is resent nonstop (the flood). + * The fix caps the doubling. This test forces many retransmits and checks the + * timeout never overflows or grows without limit. + */ +static void +test_reliable_backoff_is_bounded(void **state) +{ + (void)state; + now = 1000; + + struct reliable *rel = test_reliable_new(); + + struct reliable_entry *e = &rel->array[0]; + e->active = true; + e->packet_id = 1; + e->timeout = rel->initial_timeout; + rel->packet_id = 2; + + for (int i = 0; i < 40; ++i) + { + /* make the packet due for a fast retransmit */ + e->n_acks = N_ACK_RETRANSMIT; + + int opcode; + struct buffer *buf = reliable_send(rel, &opcode); + /* our one active packet is the one picked to send */ + assert_ptr_equal(buf, &e->buf); + + /* a zero or negative timeout would resend with no delay (the flood) */ + assert_true(e->timeout > 0); + /* the timeout must stop growing, not double forever */ + assert_true(e->timeout <= SANE_TIMEOUT_BOUND); + } + + reliable_free(rel); +} + +/* + * An ACK should only count if it is for a packet we actually sent. If the + * broken code accepts ACKs for packets that were never sent, a peer can force + * early retransmits at will (which then feeds the timeout overflow above). + * These two cases send such bogus ACKs and check they are ignored. + */ +static void +test_reliable_purge_ignores_forged_acks(void **state) +{ + (void)state; + + /* Case (a): an ACK for pid 0x40000000, which we never sent (we only sent + * 0 and 1). The old "e->packet_id < pid" check treats it as newer and + * counts it. It should be ignored. */ + { + struct reliable *rel = test_reliable_new(); + struct reliable_entry *e = &rel->array[0]; + e->active = true; + e->packet_id = 1; + rel->packet_id = 2; /* only pids 0 and 1 were ever sent */ + + struct reliable_ack ack = { .len = 1, .packet_id = { 0x40000000 } }; + reliable_send_purge(rel, &ack); + + /* the bogus ACK must not be counted */ + assert_int_equal(e->n_acks, 0); + /* and must not drop our real packet */ + assert_true(e->active); + reliable_free(rel); + } + + /* Case (b): an ACK for pid 0xFFFFFFFF. It is a big number, so a plain "<" + * thinks it is newer than our pid 1. But ids wrap around, and 0xFFFFFFFF + * is really older than 1, so it should be ignored. */ + { + struct reliable *rel = test_reliable_new(); + struct reliable_entry *e = &rel->array[0]; + e->active = true; + e->packet_id = 1; + rel->packet_id = 2; + + struct reliable_ack ack = { .len = 1, .packet_id = { 0xFFFFFFFF } }; + reliable_send_purge(rel, &ack); + + /* the bogus ACK must not be counted */ + assert_int_equal(e->n_acks, 0); + /* and must not drop our real packet */ + assert_true(e->active); + reliable_free(rel); + } +} + +/* + * Sanity check: a real ACK must still work. Acknowledging a higher packet + * should drop that packet and count once towards resending the older one. + * The fix must not break this. + */ +static void +test_reliable_purge_legitimate_ack(void **state) +{ + (void)state; + + struct reliable *rel = test_reliable_new(); + + struct reliable_entry *e0 = &rel->array[0]; + e0->active = true; + e0->packet_id = 1; + + struct reliable_entry *e1 = &rel->array[1]; + e1->active = true; + e1->packet_id = 2; + + rel->packet_id = 3; /* pids 0,1,2 sent; 1 and 2 still waiting */ + + struct reliable_ack ack = { .len = 1, .packet_id = { 2 } }; + reliable_send_purge(rel, &ack); + + /* packet 2 was acked, so it is dropped */ + assert_false(e1->active); + /* packet 1 is older, so it gets one ACK towards an early resend */ + assert_int_equal(e0->n_acks, 1); + /* packet 1 was not acked, so it stays */ + assert_true(e0->active); + + reliable_free(rel); +} + +int +main(void) +{ + openvpn_unit_test_setup(); + + const struct CMUnitTest tests[] = { + cmocka_unit_test(test_reliable_backoff_is_bounded), + cmocka_unit_test(test_reliable_purge_ignores_forged_acks), + cmocka_unit_test(test_reliable_purge_legitimate_ack), + }; + + return cmocka_run_group_tests_name("reliable tests", tests, NULL, NULL); +}