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

Hello plaisthos, razvanc, 

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#4).

The following approvals got outdated and were removed:
Code-Review+2 by razvanc


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
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 <[email protected]>
Acked-by: Razvan Cojocaru <[email protected]>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1894
Change-Id: I3c6ad247614fb204a279b73e98f8c4704e16da2a
Message-Id: <[email protected]>
URL: https://www.mail-archive.com/[email protected]/msg38948.html
Signed-off-by: Gert Doering <[email protected]>
---
M tests/unit_tests/openvpn/test_packet_id.c
1 file changed, 148 insertions(+), 1 deletion(-)


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

diff --git a/tests/unit_tests/openvpn/test_packet_id.c b/tests/unit_tests/openvpn/test_packet_id.c
index a5d50de..dc77553 100644
--- a/tests/unit_tests/openvpn/test_packet_id.c
+++ b/tests/unit_tests/openvpn/test_packet_id.c
@@ -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)
 
     };
 

-- 
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: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I3c6ad247614fb204a279b73e98f8c4704e16da2a
Gerrit-Change-Number: 1894
Gerrit-PatchSet: 4
Gerrit-Owner: cron2 <[email protected]>
Gerrit-Reviewer: cron2 <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-Reviewer: razvanc <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>

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