Attention is currently required from: razvanc.
Hello plaisthos, razvanc,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1909?usp=email
to look at the new patch set (#2).
Change subject: Ignore acks for packets that cannot be outstanding
......................................................................
Ignore acks for packets that cannot be outstanding
This ignores acks for pids outside the send window, i.e. for packets
that have either not been sent out yet or already left the window.
Nothing outside that window can be outstanding, so such acks can only
be used to manipulate the state of the send buffer.
Comparing the pid of an outstanding packet against the acked pid needs
to be wraparound aware as well. Otherwise a peer can ack a pid from the
lower half of the id space, which passes the window check above, and
still increment n_acks on every outstanding packet, forcing an early
retransmit after N_ACK_RETRANSMIT such acks.
Github: OpenVPN/openvpn-private-issues#161
Reported-By: Mark Bregman (Fox-IT) <[email protected]>
CVE: 2026-84732
Change-Id: I944478767b52a1b9bf6a65373ce0544c69cb1012
Signed-off-by: Arne Schwabe <[email protected]>
Signed-off-by: Razvan Cojocaru <[email protected]>
Acked-by: MaxF <[email protected]>
(cherry picked from commit d988ef4508e63b28c8e3efcb9f62eb8c836907fe)
---
M dev-tools/uncrustify.conf
M src/openvpn/options.c
M src/openvpn/reliable.c
M src/openvpn/reliable.h
M tests/unit_tests/openvpn/test_packet_id.c
5 files changed, 102 insertions(+), 11 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/09/1909/2
diff --git a/dev-tools/uncrustify.conf b/dev-tools/uncrustify.conf
index 325f310..727ba86 100644
--- a/dev-tools/uncrustify.conf
+++ b/dev-tools/uncrustify.conf
@@ -51,7 +51,7 @@
# Always use scoping braces for conditionals
mod_full_brace_if=add
-mod_full_brace_if_chain=false
+mod_full_brace_if_chain=0
mod_full_brace_while=add
mod_full_brace_for=add
mod_full_brace_do=add
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 873efaf..56041bb 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -9185,7 +9185,7 @@
* timeout is already way too much anyway */
if (options->tls_timeout < 1 || options->tls_timeout >
RELIABLE_MAX_INITIAL_TIMEOUT)
{
- msg(msglevel, "tls_timeout: Must be an integer between %d and %d,
not %lld",
+ msg(msglevel, "tls_timeout: Must be an integer between %d and %d,
not %d",
1, RELIABLE_MAX_INITIAL_TIMEOUT, options->tls_timeout);
goto err;
}
diff --git a/src/openvpn/reliable.c b/src/openvpn/reliable.c
index babe4c8..d0ae3cc 100644
--- a/src/openvpn/reliable.c
+++ b/src/openvpn/reliable.c
@@ -404,15 +404,36 @@
return true;
}
+int
+validate_packet_id_window(struct reliable *rel, packet_id_type pid)
+{
+ return reliable_pid_min(pid, rel->packet_id)
+ && reliable_pid_min(subtract_pid(rel->packet_id,
RELIABLE_CAPACITY), pid);
+}
+
/* del acknowledged items from send buf */
void
reliable_send_purge(struct reliable *rel, const struct reliable_ack *ack)
{
- int i, j;
- for (i = 0; i < ack->len; ++i)
+ unsigned int out_of_window = 0;
+ packet_id_type first_out_of_window = 0;
+
+ for (int i = 0; i < ack->len; ++i)
{
packet_id_type pid = ack->packet_id[i];
- for (j = 0; j < rel->size; ++j)
+
+
+ if (!validate_packet_id_window(rel, pid))
+ {
+ if (out_of_window == 0)
+ {
+ first_out_of_window = pid;
+ }
+ out_of_window++;
+ continue;
+ }
+
+ for (int j = 0; j < rel->size; ++j)
{
struct reliable_entry *e = &rel->array[j];
if (e->active && e->packet_id == pid)
@@ -432,16 +453,27 @@
#endif
e->active = false;
}
- else if (e->active && e->packet_id < pid)
+
+ if (e->active && reliable_pid_min(e->packet_id, pid))
{
/* We have received an ACK for a packet with a higher PID.
Either
* we have received ACKs out of or order or the packet has been
* lost. We count the number of ACKs to determine if we should
- * resend it early. */
+ * resend it early. The comparison needs to be wraparound
aware,
+ * otherwise a peer can inflate n_acks with an ACK for a pid
from
+ * the lower half of the id space and force a retransmit. */
e->n_acks++;
}
}
}
+
+ if (out_of_window > 0)
+ {
+ (void)first_out_of_window; /* dmsg might not generate code */
+ dmsg(D_REL_LOW, "ACK contained %u ids outside the send window, "
+ "first was " packet_id_format,
+ out_of_window, (packet_id_print_type)first_out_of_window);
+ }
}
#ifdef ENABLE_DEBUG
diff --git a/src/openvpn/reliable.h b/src/openvpn/reliable.h
index 5877f20..cdd33c8 100644
--- a/src/openvpn/reliable.h
+++ b/src/openvpn/reliable.h
@@ -106,9 +106,9 @@
{
int size;
interval_t initial_timeout;
- packet_id_type packet_id;
- int offset; /**< Offset of the bufs in the reliable_entry array */
- bool hold; /* don't xmit until reliable_schedule_now is called */
+ packet_id_type packet_id; /**< Packet ID for the next packet to be sent
out. */
+ int offset; /**< Offset of the bufs in the reliable_entry
array */
+ bool hold; /* don't xmit until reliable_schedule_now is
called */
struct reliable_entry array[RELIABLE_CAPACITY];
};
@@ -193,6 +193,17 @@
}
/**
+ * check that pid is inside the window of possible outstanding packets
+ * of size RELIABLE_CAPACITY, ie inside the range
+ * [rel->packet_id - RELIABLE_CAPACITY, rel->packet_id).
+ *
+ * rel->packet is the *next* packet id to be sent out, so it is not
+ * included in the valid range.
+ */
+int
+validate_packet_id_window(struct reliable *rel, packet_id_type pid);
+
+/**
* Returns the number of packets that need to be acked.
*
* @param ack The acknowledgment structure to check.
diff --git a/tests/unit_tests/openvpn/test_packet_id.c
b/tests/unit_tests/openvpn/test_packet_id.c
index ff3f788..a27bd6d 100644
--- a/tests/unit_tests/openvpn/test_packet_id.c
+++ b/tests/unit_tests/openvpn/test_packet_id.c
@@ -271,6 +271,53 @@
assert_memory_equal(mru_ack.packet_id, expected_ack.packet_id,
sizeof(expected_ack.packet_id));
}
+static void
+test_packet_id_window(void **state)
+{
+ struct reliable rel = { 0 };
+ rel.packet_id = 1;
+
+ assert_true(validate_packet_id_window(&rel, 0));
+
+ /* packet id 1 is outside the window as it is the *next* packet id */
+ assert_false(validate_packet_id_window(&rel, 1));
+
+ /* wrapped around packet id, "-2" */
+ assert_true(validate_packet_id_window(&rel, 0xFFFFFFFD));
+
+ /* wrapped around packet id, "-10" */
+ assert_true(validate_packet_id_window(&rel, 0xFFFFFFF6));
+
+ /* wrapped around packet id, "-11" */
+ assert_false(validate_packet_id_window(&rel, 0xFFFFFFF5));
+ assert_false(validate_packet_id_window(&rel, 0x80000000));
+
+ rel.packet_id = 0x80000000;
+
+ /* near the signed/usigned integer area */
+ assert_false(validate_packet_id_window(&rel, 0x80000001));
+ assert_true(validate_packet_id_window(&rel, 0x7fffffff));
+ assert_true(validate_packet_id_window(&rel, 0x7ffffff5));
+ assert_false(validate_packet_id_window(&rel, 0x7ffffff4));
+
+ rel.packet_id = 0xFFFFFFFD;
+ assert_false(validate_packet_id_window(&rel, 0xFFFFFFFD));
+ assert_false(validate_packet_id_window(&rel, 0));
+ assert_false(validate_packet_id_window(&rel, 1));
+ assert_false(validate_packet_id_window(&rel, 0xFFFFFFFE));
+ assert_false(validate_packet_id_window(&rel, 0xFFFFFFFF));
+ assert_true(validate_packet_id_window(&rel, 0xFFFFFFF3));
+ assert_true(validate_packet_id_window(&rel, 0xFFFFFFF2));
+ assert_false(validate_packet_id_window(&rel, 0xFFFFFFF1));
+
+ rel.packet_id = 500;
+ assert_false(validate_packet_id_window(&rel, 501));
+ assert_true(validate_packet_id_window(&rel, 497));
+ assert_true(validate_packet_id_window(&rel, 500 - (RELIABLE_CAPACITY -
1)));
+ assert_false(validate_packet_id_window(&rel, 500 - RELIABLE_CAPACITY));
+}
+
+
int
main(void)
{
@@ -295,7 +342,8 @@
test_packet_id_write_setup,
test_packet_id_write_teardown),
cmocka_unit_test(test_get_num_output_sequenced_available),
- cmocka_unit_test(test_copy_acks_to_lru)
+ cmocka_unit_test(test_copy_acks_to_lru),
+ cmocka_unit_test(test_packet_id_window)
};
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1909?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: release/2.6
Gerrit-Change-Id: I944478767b52a1b9bf6a65373ce0544c69cb1012
Gerrit-Change-Number: 1909
Gerrit-PatchSet: 2
Gerrit-Owner: MaxF <[email protected]>
Gerrit-Reviewer: razvanc <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-CC: plaisthos <[email protected]>
Gerrit-Attention: razvanc <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
