cron2 has uploaded a new patch set (#2) to the change originally created by flichtenheld. ( http://gerrit.openvpn.net/c/openvpn/+/1831?usp=email )
The following approvals got outdated and were removed: Code-Review+2 by plaisthos Change subject: ssl: Do not queue control ciphertext while a packet is still queued ...................................................................... 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 <[email protected]> Acked-by: Arne Schwabe <[email protected]> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1831 Message-Id: <[email protected]> URL: https://www.mail-archive.com/[email protected]/msg38133.html Signed-off-by: Gert Doering <[email protected]> --- M src/openvpn/ssl.c 1 file changed, 10 insertions(+), 3 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/31/1831/2 diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index d5100af..d539018 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -2769,8 +2769,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) { @@ -2950,7 +2957,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; } @@ -2962,7 +2969,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 */ -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/1831?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: Ib0e7c9d3a2f4e6b8c1d5a9f7e3b2c4d6a8f1e5b3 Gerrit-Change-Number: 1831 Gerrit-PatchSet: 2 Gerrit-Owner: flichtenheld <[email protected]> Gerrit-Reviewer: plaisthos <[email protected]> Gerrit-CC: openvpn-devel <[email protected]>
_______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
