From patchwork Tue Sep 16 15:52:50 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gert Doering X-Patchwork-Id: 4419 Return-Path: Delivered-To: patchwork@openvpn.net Received: by 2002:a05:7000:6d10:b0:671:5a2c:6455 with SMTP id e16csp449628may; Tue, 16 Sep 2025 08:53:20 -0700 (PDT) X-Forwarded-Encrypted: i=2; AJvYcCVb+nde6E1pGbMXRhO46fQBoQ+3lnks+xKriQ+xT7AQBvedfKtWG9Ebxwn5YLx3iPSGPoRiFqUv/7U=@openvpn.net X-Google-Smtp-Source: AGHT+IH7mQq641MCwrYZDhIHkCH39mJC7R3aS3wQXRchVvN+T61ffK+F0c5oWxjqNhJUdPJ15Vwb X-Received: by 2002:a05:6870:c085:b0:30b:ba5e:3472 with SMTP id 586e51a60fabf-32e552aa8bfmr7461337fac.12.1758037999794; Tue, 16 Sep 2025 08:53:19 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1758037999; cv=none; d=google.com; s=arc-20240605; b=PfYnjW6cJ6daODmbULQjRJUWU7JOfl+LDS3Ih9Rjpimeo9tgKrA/K5yZm+KZ86gAAp GrzEx6kbXwV8lQGtcHh7CoOC0O7RsBn8/UXPZiugn5rfH08tD/+zE4MY4adSfR0pnXtH F6YqssT7bgnaI8OYvrJz67smxP2KTIyAt1NGGZgPZvCVyX2aiBdhlBGheKkChHD8V9Yo /qljMW4r1lQBThwo794tPucpSC+Km6oGaaeirwRnV/+WQyUOA8x8hqWxt/Y6Lo5Cw5hp ShRG1YyVQUUa+q3tEV9a9+VdnjBtA8zJBU6pTrNu3RLlteB4UeCwBZAbKf3Rcw6Orc0x FI1w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=errors-to:content-transfer-encoding:list-subscribe:list-help :list-post:list-archive:list-unsubscribe:list-id:precedence:subject :mime-version:references:in-reply-to:message-id:date:to:from :dkim-signature:dkim-signature:dkim-signature; bh=2Ua3cZspk/rG3lLb73nVGowbiLegb5c/ZQPhf1RB9wg=; fh=4NbAC/LsuMLI0S0hprUlLSLCiHwg6SCAifhH718Jh0Q=; b=h3iFBmWVekQT+xYecWRB5MpbmxgW2FbWxC31JH7gBOUzzFmAVZ6VMeGLecz418Mbor 1DYAeT7XW0OjmmDpzquBkzIDw0t5+IxrpGnApB+HdvIOIhtBzIjIsXdEAclEDJfsVXqb 3pt2MeNxyYU/r8KntML8G+eIamwU/zMZSLjGd8EG39b1CmEXlSgpH8BRMkhbcbb+gVYb 8SQvxymUZNb8q3HEqsWD2TNJcZv7TQvKBFfvMlwoXRVKb7KOlOEByDuMPcGBb1mi3OJO U7czAM/9hEmSrHOsI0WGME+HD3qRrpc6JvqFwLT7gKCZK24h4p3ddBqUq0fwBC0AerdS gnRg==; dara=google.com ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@lists.sourceforge.net header.s=beta header.b="dpQXU/IJ"; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=j3eTe5jp; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=WmxpeNYE; spf=pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) smtp.mailfrom=openvpn-devel-bounces@lists.sourceforge.net; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=muc.de Received: from lists.sourceforge.net (lists.sourceforge.net. [216.105.38.7]) by mx.google.com with ESMTPS id 586e51a60fabf-3325bbfc009si1520402fac.297.2025.09.16.08.53.19 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 16 Sep 2025 08:53:19 -0700 (PDT) Received-SPF: pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) client-ip=216.105.38.7; Authentication-Results: mx.google.com; dkim=pass header.i=@lists.sourceforge.net header.s=beta header.b="dpQXU/IJ"; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=j3eTe5jp; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=WmxpeNYE; spf=pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) smtp.mailfrom=openvpn-devel-bounces@lists.sourceforge.net; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=muc.de DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.sourceforge.net; s=beta; h=Content-Transfer-Encoding:Content-Type: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: Subject:MIME-Version:References:In-Reply-To:Message-ID:Date:To:From:Sender: Reply-To:Cc:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=2Ua3cZspk/rG3lLb73nVGowbiLegb5c/ZQPhf1RB9wg=; b=dpQXU/IJn3rRLkYrcxKQ0H368I rjUxdBNnDEijFp2GWZkepxm8mDF7rmZ65DJu3zopvGa0Fva4BcPU3+CdEKXN+6+bo7n9IWAT5WmNC krPomM/bO0KSqYTn122IYTJ1PcBzwr6S5KHS27UbYhKcIbCWmreQ+41GA7ixXIe3UDEg=; Received: from [127.0.0.1] (helo=sfs-ml-4.v29.lw.sourceforge.com) by sfs-ml-4.v29.lw.sourceforge.com with esmtp (Exim 4.95) (envelope-from ) id 1uyXzX-0006Cz-VJ; Tue, 16 Sep 2025 15:53:15 +0000 Received: from [172.30.29.66] (helo=mx.sourceforge.net) by sfs-ml-4.v29.lw.sourceforge.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1uyXzW-0006Cs-3K for openvpn-devel@lists.sourceforge.net; Tue, 16 Sep 2025 15:53:14 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=Content-Transfer-Encoding:MIME-Version:References: In-Reply-To:Message-ID:Date:Subject:To:From:Sender:Reply-To:Cc:Content-Type: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=zOuguRaVY56kXjQFGA7UzSTo/6+TSbxqV/lU6tyjjvc=; b=j3eTe5jplEHR5591u6xJCeAqqz PjkO3NV064sVi9/dDaxEid42CYX9nOmp9/IUI9nta4MSPyScjCaZ84KQ1ETRkbKq7XcwL9EpnwT/+ SkVQMuWGQ2CYAqOzqYaX+SHuCsuqf7zruVWzaM0xgrpLHpOEw46GOJ/JbKxfFvv5Idfc=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=Content-Transfer-Encoding:MIME-Version:References:In-Reply-To:Message-ID: Date:Subject:To:From:Sender:Reply-To:Cc:Content-Type:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=zOuguRaVY56kXjQFGA7UzSTo/6+TSbxqV/lU6tyjjvc=; b=WmxpeNYEoxphDqhAFFJlCpDHKv mk4fkHJdiZS6z15OZSOEasPSGIicSMnIUrwR10NqGahrKIo4mwaypHOE5fumIp7DkG1bLIR3K6nAF +CunaZ/JelhFvSCSwHE/pyrZDPMFj5WehUrwMW7xCYwCzQ6ENBSLJISK3i6RQytU4nD8=; Received: from [193.149.48.134] (helo=blue.greenie.muc.de) by sfi-mx-2.v28.lw.sourceforge.com with esmtps (TLS1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.95) id 1uyXzU-0002qQ-C7 for openvpn-devel@lists.sourceforge.net; Tue, 16 Sep 2025 15:53:13 +0000 Received: from blue.greenie.muc.de (localhost [127.0.0.1]) by blue.greenie.muc.de (8.18.1/8.18.1) with ESMTP id 58GFr0vR006880 for ; Tue, 16 Sep 2025 17:53:00 +0200 Received: (from gert@localhost) by blue.greenie.muc.de (8.18.1/8.18.1/Submit) id 58GFr09A006879 for openvpn-devel@lists.sourceforge.net; Tue, 16 Sep 2025 17:53:00 +0200 From: Gert Doering To: openvpn-devel@lists.sourceforge.net Date: Tue, 16 Sep 2025 17:52:50 +0200 Message-ID: <20250916155258.6864-1-gert@greenie.muc.de> X-Mailer: git-send-email 2.49.1 In-Reply-To: References: MIME-Version: 1.0 X-Spam-Score: 1.3 (+) X-Spam-Report: Spam detection software, running on the system "sfi-spamd-2.hosts.colo.sdot.me", has NOT identified this incoming email as spam. The original message has been attached to this so you can view it or label similar future email. If you have any questions, see the administrator of that system for details. Content preview: From: Arne Schwabe This fixes that control packets on a floating client can trigger creating a new session in special circumstances: To trigger this circumstance a connection needs to Content analysis details: (1.3 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- 0.0 RCVD_IN_DNSWL_BLOCKED RBL: ADMINISTRATOR NOTICE: The query to DNSWL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#DnsBlocklists-dnsbl-block for more information. [193.149.48.134 listed in list.dnswl.org] 1.3 RDNS_NONE Delivered to internal network by a host with no rDNS X-Headers-End: 1uyXzU-0002qQ-C7 Subject: [Openvpn-devel] [PATCH v1] Check message id/acked ids too when doing sessionid cookie checks X-BeenThere: openvpn-devel@lists.sourceforge.net X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: openvpn-devel-bounces@lists.sourceforge.net X-getmail-retrieved-from-mailbox: Inbox X-GMAIL-THRID: =?utf-8?q?1843436453976013128?= X-GMAIL-MSGID: =?utf-8?q?1843436453976013128?= From: Arne Schwabe This fixes that control packets on a floating client can trigger creating a new session in special circumstances: To trigger this circumstance a connection needs to - starts on IP A - successfully floats to IP B by data packet - then has a control packet from IP A before any data packet can trigger the float back to IP A and all of this needs to happen in the 60s time that hmac cookie is valid in the default configuration. In this scenario we would trigger a new connection as the HMAC session id would be valid. This patch adds checking also of the message-id and acked ids to discern packet from the initial three-way handshake where these ids are 0 or 1 from any later packet. This will now trigger (at verb 4 or higher) a messaged like: Packet (P_ACK_V1) with invalid or missing SID instead. Also remove a few duplicated free_tls_pre_decrypt_state in test_ssl. Reported-By: Walter Doekes Tested-By: Walter Doekes Change-Id: I6752dcd5aff3e5cea2b439366479e86751a1c403 Signed-off-by: Arne Schwabe Acked-by: MaxF Message-Id: <20250819212214.16218-1-gert@greenie.muc.de> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg32626.html Signed-off-by: Gert Doering Acked-by: MaxF --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to release/2.6. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1184 This mail reflects revision 1 of this Change. Acked-by according to Gerrit (reflected above): MaxF diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c index cdde0b5..0492311 100644 --- a/src/openvpn/mudp.c +++ b/src/openvpn/mudp.c @@ -153,7 +153,8 @@ * need to contain the peer id */ struct gc_arena gc = gc_new(); - bool ret = check_session_id_hmac(state, from, hmac, handwindow); + bool pkt_is_ack = (verdict == VERDICT_VALID_ACK_V1); + bool ret = check_session_hmac_and_pkt_id(state, from, hmac, handwindow, pkt_is_ack); const char *peer = print_link_socket_actual(&m->top.c2.from, &gc); uint8_t pkt_firstbyte = *BPTR( &m->top.c2.buf); @@ -161,7 +162,8 @@ if (!ret) { - msg(D_MULTI_MEDIUM, "Packet (%s) with invalid or missing SID from %s", + msg(D_MULTI_MEDIUM, "Packet (%s) with invalid or missing SID from" + " %s or wrong packet id", packet_opcode_name(op), peer); } else diff --git a/src/openvpn/ssl_pkt.c b/src/openvpn/ssl_pkt.c index 41299f4..432bb8b 100644 --- a/src/openvpn/ssl_pkt.c +++ b/src/openvpn/ssl_pkt.c @@ -527,10 +527,11 @@ } bool -check_session_id_hmac(struct tls_pre_decrypt_state *state, - const struct openvpn_sockaddr *from, - hmac_ctx_t *hmac, - int handwindow) +check_session_hmac_and_pkt_id(struct tls_pre_decrypt_state *state, + const struct openvpn_sockaddr *from, + hmac_ctx_t *hmac, + int handwindow, + bool pkt_is_ack) { if (!from) { @@ -545,6 +546,36 @@ return false; } + /* Check if the packet ID of the packet or ACKED packet is <= 1 */ + for (int i = 0; i < ack.len; i++) + { + /* This packet ACKs a packet that has a higher packet id than the + * ones expected in the three-way handshake, consider it as invalid + * for the session */ + if (ack.packet_id[i] > 1) + { + return false; + } + } + + if (!pkt_is_ack) + { + packet_id_type message_id; + /* Extract the packet ID from the packet */ + if (!reliable_ack_read_packet_id(&buf, &message_id)) + { + return false; + } + + /* similar check. Anything larger than 1 is not considered part of the + * three-way handshake */ + if (message_id > 1) + { + return false; + } + } + + /* check adjacent timestamps too */ for (int offset = -2; offset <= 1; offset++) { diff --git a/src/openvpn/ssl_pkt.h b/src/openvpn/ssl_pkt.h index 2033da6..1df691c 100644 --- a/src/openvpn/ssl_pkt.h +++ b/src/openvpn/ssl_pkt.h @@ -182,17 +182,20 @@ /** * Checks if a control packet has a correct HMAC server session id * - * @param client_sid session id of the client + * This will also consider packets that have a packet id higher + * than 1 or ack packets higher than 1 to be invalid as they are + * not part of the initial three way handshake of OpenVPN and should + * not create a new connection. + * + * @param state session information * @param from link_socket from the client * @param hmac the hmac context to use for the calculation * @param handwindow the quantisation of the current time + * @param pkt_is_ack the packet being checked is a P_ACK_V1 * @return the expected server session id */ -bool -check_session_id_hmac(struct tls_pre_decrypt_state *state, - const struct openvpn_sockaddr *from, - hmac_ctx_t *hmac, - int handwindow); +bool check_session_hmac_and_pkt_id(struct tls_pre_decrypt_state *state, const struct openvpn_sockaddr *from, + hmac_ctx_t *hmac, int handwindow, bool pkt_is_ack); /* * Write a control channel authentication record. diff --git a/tests/unit_tests/openvpn/test_pkt.c b/tests/unit_tests/openvpn/test_pkt.c index 74d7311..27f52cf 100644 --- a/tests/unit_tests/openvpn/test_pkt.c +++ b/tests/unit_tests/openvpn/test_pkt.c @@ -174,6 +174,27 @@ 0x85, 0xdb, 0x53, 0x56, 0x23, 0xb0, 0x2e }; +/* no tls-auth, P_ACK_V1, acks 0,1, and 2 */ +const uint8_t client_ack_123_none_random_id[] = { + 0x28, + 0xae, 0xb9, 0xaf, 0xe1, 0xf0, 0x1d, 0x79, 0xc8, + 0x03, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x01, + 0x00, 0x00, 0x00, 0x02, + 0xdd, 0x85, 0xdb, 0x53, 0x56, 0x23, 0xb0, 0x2e +}; + +/* no tls-auth, P_CONTROL_V1, acks 0, msg-id 2 */ +const uint8_t client_control_none_random_id[] = { + 0x20, + 0xae, 0xb9, 0xaf, 0xe1, 0xf0, 0x1d, 0x79, 0xc8, + 0x01, + 0x00, 0x00, 0x00, 0x00, + 0x02 +}; + + struct tls_auth_standalone init_tas_auth(int key_direction) { @@ -294,12 +315,10 @@ assert_int_equal(verdict, VERDICT_VALID_RESET_V2); free_tls_pre_decrypt_state(&state); - free_tls_pre_decrypt_state(&state); /* The pre decrypt function should not modify the buffer, so calling it * again should have the same result */ verdict = tls_pre_decrypt_lite(&tas, &state, &from, &buf); assert_int_equal(verdict, VERDICT_VALID_RESET_V2); - free_tls_pre_decrypt_state(&state); /* and buf memory should be equal */ assert_memory_equal(BPTR(&buf), client_reset_v2_tls_auth, sizeof(client_reset_v2_tls_auth)); @@ -317,7 +336,6 @@ assert_int_equal(verdict, VERDICT_INVALID); free_tls_pre_decrypt_state(&state); - free_tls_pre_decrypt_state(&state); /* Wrong key direction gives a wrong hmac key and should not validate */ free_key_ctx_bi(&tas.tls_wrap.opt.key_ctx_bi); free_tas(&tas); @@ -357,15 +375,12 @@ assert_int_equal(verdict, VERDICT_VALID_RESET_V2); free_tls_pre_decrypt_state(&state); - free_tls_pre_decrypt_state(&state); buf_reset_len(&buf); buf_write(&buf, client_reset_v2_tls_crypt, sizeof(client_reset_v2_none)); verdict = tls_pre_decrypt_lite(&tas, &state, &from, &buf); assert_int_equal(verdict, VERDICT_VALID_RESET_V2); free_tls_pre_decrypt_state(&state); - free_tls_pre_decrypt_state(&state); - /* This is not a reset packet and should trigger the other response */ buf_reset_len(&buf); buf_write(&buf, client_ack_tls_auth_randomid, sizeof(client_ack_tls_auth_randomid)); @@ -443,7 +458,7 @@ assert_int_equal(verdict, VERDICT_VALID_CONTROL_V1); /* This is a valid packet but containing a random id instead of an HMAC id*/ - bool valid = check_session_id_hmac(&state, &from.dest, hmac, 30); + bool valid = check_session_hmac_and_pkt_id(&state, &from.dest, hmac, 30, false); assert_false(valid); free_tls_pre_decrypt_state(&state); @@ -474,7 +489,7 @@ verdict = tls_pre_decrypt_lite(&tas, &state, &from, &buf); assert_int_equal(verdict, VERDICT_VALID_ACK_V1); - bool valid = check_session_id_hmac(&state, &from.dest, hmac, 30); + bool valid = check_session_hmac_and_pkt_id(&state, &from.dest, hmac, 30, true); assert_true(valid); free_tls_pre_decrypt_state(&state); @@ -483,6 +498,51 @@ hmac_ctx_free(hmac); } +static void +test_verify_hmac_none_out_of_range_ack(void **ut_state) +{ + hmac_ctx_t *hmac = session_id_hmac_init(); + + struct link_socket_actual from = { 0 }; + from.dest.addr.sa.sa_family = AF_INET; + + struct tls_auth_standalone tas = { 0 }; + struct tls_pre_decrypt_state state = { 0 }; + + struct buffer buf = alloc_buf(1024); + enum first_packet_verdict verdict; + + tas.tls_wrap.mode = TLS_WRAP_NONE; + + buf_reset_len(&buf); + buf_write(&buf, client_ack_123_none_random_id, sizeof(client_ack_123_none_random_id)); + + + verdict = tls_pre_decrypt_lite(&tas, &state, &from, &buf); + assert_int_equal(verdict, VERDICT_VALID_ACK_V1); + + /* should fail because it acks 2 */ + bool valid = check_session_hmac_and_pkt_id(&state, &from.dest, hmac, 30, true); + assert_false(valid); + free_tls_pre_decrypt_state(&state); + + /* Try test with the control with a too high message id now */ + buf_reset_len(&buf); + buf_write(&buf, client_control_none_random_id, sizeof(client_control_none_random_id)); + + verdict = tls_pre_decrypt_lite(&tas, &state, &from, &buf); + assert_int_equal(verdict, VERDICT_VALID_CONTROL_V1); + + /* should fail because it has message id 2 */ + valid = check_session_hmac_and_pkt_id(&state, &from.dest, hmac, 30, true); + assert_false(valid); + + free_tls_pre_decrypt_state(&state); + free_buf(&buf); + hmac_ctx_cleanup(hmac); + hmac_ctx_free(hmac); +} + static hmac_ctx_t * init_static_hmac(void) { @@ -670,6 +730,7 @@ cmocka_unit_test(test_calc_session_id_hmac_static), cmocka_unit_test(test_verify_hmac_none), cmocka_unit_test(test_verify_hmac_tls_auth), + cmocka_unit_test(test_verify_hmac_none_out_of_range_ack), cmocka_unit_test(test_generate_reset_packet_plain), cmocka_unit_test(test_generate_reset_packet_tls_auth), cmocka_unit_test(test_extract_control_message)