[Openvpn-devel,v3] reliable: add unit tests for ACK and backoff DoS hardening
Commit Message
From: Lev Stipakov <lev@openvpn.net>
Add regression coverage for the reliability-layer denial-of-service fix
to test_packet_id, which already links reliable.c:
- 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 <lev@openvpn.net>
Acked-by: Razvan Cojocaru <razvanc@mailbox.org>
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 3 of this Change.
Acked-by according to Gerrit (reflected above):
Razvan Cojocaru <razvanc@mailbox.org>
Comments
More unit tests are always good, especially when we fix things and want
to know if the fix is effective - BB confirms that these failed before
we merged the CVE 2026-84732 fixes, and now succeed. Great :-)
..
[ RUN ] test_copy_acks_to_lru
[ OK ] test_copy_acks_to_lru
[ RUN ] test_packet_id_window
[ OK ] test_packet_id_window
[ RUN ] test_reliable_backoff_is_bounded
[ OK ] test_reliable_backoff_is_bounded
[ RUN ] test_reliable_purge_ignores_forged_acks
[ OK ] test_reliable_purge_ignores_forged_acks
[ RUN ] test_reliable_purge_legitimate_ack
[ OK ] test_reliable_purge_legitimate_ack
[==========] packet_id tests: 13 test(s) run.
Also, thanks for rewriting to avoid creating an extra executable (which
loads the GHA test runs quite a bit, with copying these around for
windows UT runs).
Your patch has been applied to the master and release/2.7 branch.
commit c9b8ca1795957f2e22dad1589479bd32faea4fc1 (master)
commit acd9702b6cdf94b173e78a6e310e408ffe33aec2 (release/2.7)
Author: Lev Stipakov
Date: Fri Sep 4 18:08:42 2026 +0200
reliable: add unit tests for ACK and backoff DoS hardening
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Razvan Cojocaru <razvanc@mailbox.org>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1894
Message-Id: <20260904160848.5553-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg38948.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
@@ -372,6 +372,150 @@
}
+/* 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 inside the send window, so
+ * it passes the window check, but ids wrap around and 0xFFFFFFFF is really
+ * older than our pid 1. A plain "<" thinks it is newer and counts it; the
+ * wraparound-aware comparison must not. */
+ {
+ 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)
{
@@ -394,7 +538,10 @@
cmocka_unit_test(test_get_num_output_sequenced_available),
cmocka_unit_test(test_copy_acks_to_lru),
- cmocka_unit_test(test_packet_id_window)
+ cmocka_unit_test(test_packet_id_window),
+ 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)
};