[M] Change in openvpn[master]: reliable: add unit tests for ACK and backoff DoS hardening

Attention is currently required from: plaisthos.

Hello plaisthos,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1894?usp=email

to review the following change.


Change subject: reliable: add unit tests for ACK and backoff DoS hardening
......................................................................

reliable: add unit tests for ACK and backoff DoS hardening

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 <[email protected]>
Change-Id: I3c6ad247614fb204a279b73e98f8c4704e16da2a
---
M CMakeLists.txt
M tests/unit_tests/openvpn/Makefile.am
A tests/unit_tests/openvpn/test_reliable.c
3 files changed, 218 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/94/1894/1

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 <[email protected]>
+ *
+ *  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 <https://www.gnu.org/licenses/>.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include "syshead.h"
+
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <cmocka.h>
+
+#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);
+}

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1894?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I3c6ad247614fb204a279b73e98f8c4704e16da2a
Gerrit-Change-Number: 1894
Gerrit-PatchSet: 1
Gerrit-Owner: cron2 <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel