diff mbox series

[Openvpn-devel,v1] ssl: Do not queue control ciphertext while a packet is still queued

Message ID 20260805134336.163392-1-frank@lichtenheld.com
State New
Headers
Series [Openvpn-devel,v1] ssl: Do not queue control ciphertext while a packet is still queued |

Commit Message

Frank Lichtenheld Aug. 5, 2026, 1:43 p.m. UTC 
  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
---

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/+/1831
This mail reflects revision 1 of this Change.

Acked-by according to Gerrit (reflected above):
Arne Schwabe <arne-openvpn@rfc2549.org>
diff mbox series

Patch

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 60df7ce..7f2e850 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2714,8 +2714,15 @@ 
 
 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)
     {
@@ -2895,7 +2902,7 @@ 
             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;
     }
@@ -2907,7 +2914,7 @@ 
     /* 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 */