ssl: Do not queue control ciphertext while a packet is still queued

An outgoing control channel packet is handed to the link layer as a
buffer descriptor pointing into the reliable send buffer it was built
from, and the packet id sits in that buffer's headroom, right in front
of the payload.  If the entry is reused before the packet has been
written out, buf_copy_n() writes the new payload and
reliable_mark_active_outgoing() prepends the new packet id exactly over
the packet id of the queued packet, while its opcode, ACK array and
length stay untouched.  The queued packet then goes out with somebody
else's packet id.

Observed in a TCP p2p handshake: both peers reset simultaneously, the
peer's two HARD_RESET packets arrive back to back, so io_wait_dowork()
takes the residual data shortcut (event_set_status = SOCKET_READ) and
does not write out our already queued HARD_RESET retransmit.  The ACK
in the second peer reset then purges our reset from the send window,
tls_process_state() moves to S_START and queues the ClientHello into
the very same (now inactive) entry.  Result on the wire: a HARD_RESET
with the ClientHello's packet id 1, followed by the ClientHello with
the same id 1.  The receiver consumes the reset, advances its receive
window, and drops the real ClientHello as a replay - the handshake
deadlocks until it times out.

The send path in tls_process_state() and the dedicated ACK path in
tls_process() are already guarded by to_link->len, only the ciphertext
queueing was not.  Guard it as well.  A pending to_link makes
tls_process() report itself as active, so we are called again as soon
as the packet has been written out.  In the error path this can drop a
TLS alert that we would have queued, which is in line with that path
not ensuring delivery anyway.

Change-Id: Ib0e7c9d3a2f4e6b8c1d5a9f7e3b2c4d6a8f1e5b3
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1831
Message-Id: <20260805134336.163392-1-frank@lichtenheld.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg38133.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index d5100af76b2e953c2f2845a0af55a8cd27f6dfc0..d53901881db2fbffbcc65985989a0fc4b8527f56 100644 (file)
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2769,8 +2769,15 @@ write_outgoing_tls_ciphertext(struct tls_session *session, bool *continue_tls_pr
 
 static bool
 check_outgoing_ciphertext(struct key_state *ks, struct tls_session *session,
-                          bool *continue_tls_process)
+                          struct buffer *to_link, bool *continue_tls_process)
 {
+    if (to_link->len)
+    {
+        dmsg(D_TLS_DEBUG,
+             "Deferring outgoing ciphertext, previous packet not written out yet");
+        return true;
+    }
+
     /* Outgoing Ciphertext to reliable buffer */
     if (ks->state >= S_START)
     {
@@ -2950,7 +2957,7 @@ tls_process_state(struct tls_multi *multi, struct tls_session *session, struct b
             dmsg(D_TLS_DEBUG, "Outgoing Plaintext -> TLS");
         }
     }
-    if (!check_outgoing_ciphertext(ks, session, &continue_tls_process))
+    if (!check_outgoing_ciphertext(ks, session, to_link, &continue_tls_process))
     {
         goto error;
     }
@@ -2962,7 +2969,7 @@ error:
     /* Shut down the TLS session but do a last read from the TLS
      * object to be able to read potential TLS alerts */
     key_state_ssl_shutdown(&ks->ks_ssl);
-    check_outgoing_ciphertext(ks, session, &continue_tls_process);
+    check_outgoing_ciphertext(ks, session, to_link, &continue_tls_process);
 
     /* Put ourselves in the pre error state that will only send out the
      * control channel packets but nothing else */