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 <[email protected]> Acked-by: Arne Schwabe <[email protected]> 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 <[email protected]> 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 */ _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
