From patchwork Tue Aug 19 21:22:09 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gert Doering X-Patchwork-Id: 4366 Return-Path: Delivered-To: patchwork@openvpn.net Received: by 2002:a05:7000:c414:b0:671:5a2c:6455 with SMTP id jt20csp2272397mab; Tue, 19 Aug 2025 14:22:33 -0700 (PDT) X-Forwarded-Encrypted: i=2; AJvYcCV2rCVHkcUgkVRqWsERmC0tpzssRg+OVy+hEYxwSsugHJ0QtRVsHIi5/olZ+yr9Um7Y2l5NoiQGMyM=@openvpn.net X-Google-Smtp-Source: AGHT+IEF+8t3whCqufMJOoMcuK112+nGWGe2y0x0f1LIEt+QtXnEA/jG5dU7NEeQP4SXNIz8/Tvy X-Received: by 2002:a05:6820:823:b0:61b:7d22:d55e with SMTP id 006d021491bc7-61d9b4e6df1mr575631eaf.2.1755638553688; Tue, 19 Aug 2025 14:22:33 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1755638553; cv=none; d=google.com; s=arc-20240605; b=RJ9msT2g5gsjgHtrcnYHw3Ieuif7UYoBG2KD4Gmvnotb50b61vaaXOIliovQFCbGD1 EGyJLebvirp1E4I7w1BfltAFN+H4AMRp8CuvQqVzvjLXmIk6S/J6nN6chNXWVm1/tewz +aYLT/5y7h8hlfEKOGqnLO47ANkFwJrh4qDUiyvW5dHii57QwOkMjuk9FUOMfLcXitvD ggroO82cWFVbLvV/GhVhg/tl2bxgpd1uapIjmPqPpx8ArjNO2ZLD+YJs/OF44EQJqm41 tvsG585hrRPNFe5pFim0MZv12vuPyjJ8f8S9FweBy8Fjav8XY8NXloNhfcgjoJnXFb5u PnFQ== 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=7K+WLoZJ8kmvWUFO1VQjboXbszOq/T/wiFhGgtHJVoQ=; fh=4NbAC/LsuMLI0S0hprUlLSLCiHwg6SCAifhH718Jh0Q=; b=Wtx3x1ymWSLygePy6sfVMemQJN6gD8CNzkYPZm3kWpwiwGmKu57pTJ7bWkGfj06Bz7 wIGIIIx1pDagOZMb8q7SITs8lyCVpVp4exjTjPvvAakuL1PTGjTCRbQgTNtF36qPMbqL nu1a6wVAQFbw/M5+8g6hf3n/k7fxQgYEh4ykcfeVUSgKIcqh64SjK8ZxGVD6bohEvOOP Vle4aL16L1rovto4bAd1BHy6ok2kUHmIZsbTKaohTjdcdtNUGPlkAQ9FYfwSMIpfaLSa aNJB0wkIEp1ZFFqHJ+QLvWzI5V4Snb3IGLQA6n3DfZiMKgtJ7sZ/dUbZBD748B1FEaFT ClLA==; dara=google.com ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@lists.sourceforge.net header.s=beta header.b=dTcNIW0B; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=gg1+DZbt; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=f0tYXnZ+; 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 006d021491bc7-61bebfc1d21si1970960eaf.19.2025.08.19.14.22.32 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 19 Aug 2025 14:22:33 -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=dTcNIW0B; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=gg1+DZbt; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=f0tYXnZ+; 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=7K+WLoZJ8kmvWUFO1VQjboXbszOq/T/wiFhGgtHJVoQ=; b=dTcNIW0BssNpL9k5q4mSB+l4Lv mHhMzGH3aR6u4nfMGx/FjFyHQFCo3adPmHWg6cPM5/q20EB8VDse0TH6MXfOaaLYreWAoxyo4DW2R 9dwcxiwSt6B/JX+aCWPZfuyWTqm7zezP40mFgtLz2tRiYc9DF0h099vvL6zyMY7WVJ2c=; Received: from [127.0.0.1] (helo=sfs-ml-1.v29.lw.sourceforge.com) by sfs-ml-1.v29.lw.sourceforge.com with esmtp (Exim 4.95) (envelope-from ) id 1uoTmj-0000QX-9B; Tue, 19 Aug 2025 21:22:25 +0000 Received: from [172.30.29.66] (helo=mx.sourceforge.net) by sfs-ml-1.v29.lw.sourceforge.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1uoTmh-0000QF-LM for openvpn-devel@lists.sourceforge.net; Tue, 19 Aug 2025 21:22:23 +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=H2xF/zF+VQfmEjXv0atbn/dQ7hhWlRc1Y/y5P5CMZCc=; b=gg1+DZbtStRkRakanEOMmly9Tm DuM4tmR9hcC9HAN1nBx9ppgmuP6eZToBKRce+yVIH2WB0O6hc68gz3Oz6H/Or5x9atkGZnwlqpul6 ciqalX02zV4e2siEPR5wcTRZUnG+QBArgqaSW6DDLqHw0MjNGRgMMbir8MYwX8AM1QE0=; 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=H2xF/zF+VQfmEjXv0atbn/dQ7hhWlRc1Y/y5P5CMZCc=; b=f0tYXnZ+ZKnO9ITbNCq7GpKbTF vIjpxmXVZnTYYDkJQ0GskRgtSzFu5NfvLqiYk/wHHCsRcLflEhRGLL2yf/m8riRPhg2U5i0mJval5 hErR9DTMcd0m6xaMvcn9D+4CNKlC2Tcm5PSbX1hXA4y6nXfI0kmq17ir5iKuj0rg/9GM=; Received: from [193.149.48.143] (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 1uoTmg-0007J0-6e for openvpn-devel@lists.sourceforge.net; Tue, 19 Aug 2025 21:22:23 +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 57JLMFd2016240 for ; Tue, 19 Aug 2025 23:22:15 +0200 Received: (from gert@localhost) by blue.greenie.muc.de (8.18.1/8.18.1/Submit) id 57JLMFk2016239 for openvpn-devel@lists.sourceforge.net; Tue, 19 Aug 2025 23:22:15 +0200 From: Gert Doering To: openvpn-devel@lists.sourceforge.net Date: Tue, 19 Aug 2025 23:22:09 +0200 Message-ID: <20250819212214.16218-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-1.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.143 listed in list.dnswl.org] 1.3 RDNS_NONE Delivered to internal network by a host with no rDNS X-Headers-End: 1uoTmg-0007J0-6e Subject: [Openvpn-devel] [PATCH v6] 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?1840920452212096343?= X-GMAIL-MSGID: =?utf-8?q?1840920452212096343?= 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 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 --- 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/+/1067 This mail reflects revision 6 of this Change. Acked-by according to Gerrit (reflected above): MaxF diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c index 7259a4b..31134be 100644 --- a/src/openvpn/mudp.c +++ b/src/openvpn/mudp.c @@ -151,7 +151,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); @@ -159,7 +160,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 b901f87..6ec05a7 100644 --- a/src/openvpn/ssl_pkt.c +++ b/src/openvpn/ssl_pkt.c @@ -496,8 +496,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) { @@ -512,6 +515,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 8fe4880..96cdd68 100644 --- a/src/openvpn/ssl_pkt.h +++ b/src/openvpn/ssl_pkt.h @@ -178,14 +178,20 @@ /** * Checks if a control packet has a correct HMAC server session id * + * 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 65b31e7..5122766 100644 --- a/tests/unit_tests/openvpn/test_pkt.c +++ b/tests/unit_tests/openvpn/test_pkt.c @@ -139,6 +139,27 @@ 0xc8, 0x01, 0x00, 0x00, 0x00, 0x00, 0xdd, 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) { @@ -256,12 +277,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)); @@ -279,7 +298,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); @@ -319,15 +337,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)); @@ -405,7 +420,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); @@ -436,7 +451,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); @@ -445,6 +460,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) { @@ -634,16 +694,20 @@ main(void) { openvpn_unit_test_setup(); - const struct CMUnitTest tests[] = { cmocka_unit_test(test_tls_decrypt_lite_none), - cmocka_unit_test(test_tls_decrypt_lite_auth), - cmocka_unit_test(test_tls_decrypt_lite_crypt), - cmocka_unit_test(test_parse_ack), - 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_generate_reset_packet_plain), - cmocka_unit_test(test_generate_reset_packet_tls_auth), - cmocka_unit_test(test_extract_control_message) }; + + const struct CMUnitTest tests[] = { + cmocka_unit_test(test_tls_decrypt_lite_none), + cmocka_unit_test(test_tls_decrypt_lite_auth), + cmocka_unit_test(test_tls_decrypt_lite_crypt), + cmocka_unit_test(test_parse_ack), + 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) + }; #if defined(ENABLE_CRYPTO_OPENSSL) OpenSSL_add_all_algorithms();