[PATCH v1] ssl: Ignore hard reset packets with a non-zero packet id

From: Frank Lichtenheld <[email protected]>

A hard reset is always the first packet of a session, so it always
carries reliable packet id 0. tls_process_state() relies on that when it
treats a received reset as the early negotiation packet only for packet
id 0, and the stateless three-way handshake relies on it as well (see
the comment in session_skip_to_pre_start()).

A reset claiming a different id is therefore bogus.

We had a bug that could cause hard reset replays with packet id 1 in
specific scenarios (P2P TCP). In that case we accepted the packet id
at face value and then ignored the control packet that actually had
id 1 as an replay. This caused a difficult to diagnose dead connection
that was stuck just before TLS negotiation. The check added handles
this specific scenario well in that we just ignore the bogus reset
but do not abort the connection attempt. Starting fresh might retrigger
the bug. If there would be a separate bug where the client only sends
hard resets with packet id 1 we will still get logging on the server
side now.

Change-Id: I3c7d1f9e5b2a4c6d8e1f3a5b7c9d2e4f6a8b1c3d
Signed-off-by: Frank Lichtenheld <[email protected]>
Acked-by: Arne Schwabe <[email protected]>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1832
---

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/+/1832
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 7f2e850..46ccc12 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -3903,8 +3903,22 @@
         /* Extract the packet ID from the packet */
         if (reliable_ack_read_packet_id(buf, &id))
         {
+            /* A hard reset always is the first packet of a session, so it
+             * always must use packet id 0. Ignore it if it claims another id.
+             * In a specific existing bug these packets were replays of an
+             * already handled reset, so ignoring it is better than aborting
+             * the connection attempt.
+             */
+            if (is_hard_reset_method2(op) && id != 0)
+            {
+                msg(D_TLS_ERRORS,
+                    "TLS Error: received %s with packet id " packet_id_format
+                    " from %s -- 0 was expected, ignoring packet",
+                    packet_opcode_name(op), (packet_id_print_type)id,
+                    print_link_socket_actual(from, &gc));
+            }
             /* Avoid deadlock by rejecting packet that would de-sequentialize receive buffer */
-            if (reliable_wont_break_sequentiality(ks->rec_reliable, id))
+            else if (reliable_wont_break_sequentiality(ks->rec_reliable, id))
             {
                 if (reliable_not_replay(ks->rec_reliable, id))
                 {