From patchwork Wed Mar 15 15:04:20 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Arne Schwabe X-Patchwork-Id: 3136 Return-Path: Delivered-To: patchwork@openvpn.net Received: by 2002:a05:7300:2310:b0:9f:bfa4:120f with SMTP id r16csp3101007dye; Wed, 15 Mar 2023 08:05:31 -0700 (PDT) X-Google-Smtp-Source: AK7set+jejVAaOtz8+1ZrQB3sfKxwWnSHzPH//l2yJwoklmWBO7K6kAI/Eh9pxU5UTuCclUHGFT8 X-Received: by 2002:a05:6a20:69aa:b0:d5:6e91:f019 with SMTP id t42-20020a056a2069aa00b000d56e91f019mr83750pzk.33.1678892731639; Wed, 15 Mar 2023 08:05:31 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1678892731; cv=none; d=google.com; s=arc-20160816; b=O2G+JCY/rpDKwTV9je41ZcHlD0iHhRTydfHtoMCJotvcofDX596gN4Etn6yIcRHzsA 8YP+/SzTeznaDwc0wraRMe7wajyFjJS+FRp2NLl1t6u1sKbUOkixAevuHNlFgZdLzo6o b+dlwzIlAoyfbdFyo+cUb7vyoo2EUbdqTjImZAz6mtSmMRvCuruhN4yTv8soyxwCwiwM qfPbCZtFsTIHSqhTnJ43GUCczA6XI2//cjt0uiZkt2pDvsrRa++F1AlJiXQ5uFkM2Tyl 3Dgp/NDcnH+siKh/LNc+8QeMy+QxOyy9PPpjrjlxWjIoU5+uhauLJn5VCjoWt537kevJ ROpQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; 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; bh=tydP19Q/ChElWENpouRZ+FmsQB14UEwEI0rfRi3ytW8=; b=czxRri/r5ogJWJnmMGYL/EpVVBkkttpiUZd50C6f5kb+KMaBNDCJfjWDY2URV1naz0 ZuzUcPFihXW/W2a2Rm1YqDhJuLDzVJr7mgMWemrljwRCpjXmG584WVn3487U5n97c58E KQCoMakerHjhNsK+DI/KBuC/H2vqJld2Za/c23LGNva1zFiDp4C8IFA7uJUk2i4RWX5R ORSSqtryMhPrei+4BQWeHTBtri1GNVhYUBTaaUqxmf8NjBVGPoP1QlQwLosw6hdSHKuy kq7U649wUWCrGLN6XecFNLpRUOQ1Y94OcxMHpbRtN8r3SB+WCGiNdUZ9RGf0BroEjOlM DxLw== ARC-Authentication-Results: i=1; mx.google.com; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=L+usviUZ; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=JnPb+MrI; 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 Received: from lists.sourceforge.net (lists.sourceforge.net. [216.105.38.7]) by mx.google.com with ESMTPS id k3-20020a632403000000b004fb73b9a406si5314345pgk.636.2023.03.15.08.05.31 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Wed, 15 Mar 2023 08:05:31 -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=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=L+usviUZ; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=JnPb+MrI; 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 Received: from [127.0.0.1] (helo=sfs-ml-2.v29.lw.sourceforge.com) by sfs-ml-2.v29.lw.sourceforge.com with esmtp (Exim 4.95) (envelope-from ) id 1pcSg8-0006ws-J6; Wed, 15 Mar 2023 15:04:37 +0000 Received: from [172.30.20.202] (helo=mx.sourceforge.net) by sfs-ml-2.v29.lw.sourceforge.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1pcSg6-0006wj-70 for openvpn-devel@lists.sourceforge.net; Wed, 15 Mar 2023 15:04:34 +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=raHwnEO7cs216dQ8z5sT9vOpe8gJJDrh0eCBlSGuqro=; b=L+usviUZd+sYyXCy/mdIi8ZnYd JGDMRf59MR0YubAkG8zsHtuJdI68xtn6o/6AtdGqsrc9SYGWDEZh5ga2CP2VdUxT+zrAMHTw2k7tR Z9Ylc47T5IJu+tYp/Sjmos1IrNVvOQ2LQtkQwD9c/wFspm1sLLk2HerSFSZVH3Lp/bAA=; 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=raHwnEO7cs216dQ8z5sT9vOpe8gJJDrh0eCBlSGuqro=; b=JnPb+MrIkZv2t9NYgW4ptm8r64 NmEfHeBhnQV4xcehtGOnc11PfJl5Pd0++9GJXn0AzYXCFTVEiEl0V4D13bb0LWqG5G9zb9NgtlIep HDZAA5bus2BPFN9tMHeJuQEMx9JQoBjZ3fXZQTQvPAmYX7lOvrOCGy+o3ZVSSpo8Xb9Y=; Received: from mail.blinkt.de ([192.26.174.232]) by sfi-mx-2.v28.lw.sourceforge.com with esmtps (TLS1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.95) id 1pcSg4-00058X-Ct for openvpn-devel@lists.sourceforge.net; Wed, 15 Mar 2023 15:04:34 +0000 Received: from kamera.blinkt.de ([2001:638:502:390:20c:29ff:fec8:535c]) by mail.blinkt.de with smtp (Exim 4.95 (FreeBSD)) (envelope-from ) id 1pcSfs-000MTV-Gj for openvpn-devel@lists.sourceforge.net; Wed, 15 Mar 2023 16:04:20 +0100 Received: (nullmailer pid 299034 invoked by uid 10006); Wed, 15 Mar 2023 15:04:20 -0000 From: Arne Schwabe To: openvpn-devel@lists.sourceforge.net Date: Wed, 15 Mar 2023 16:04:20 +0100 Message-Id: <20230315150420.298988-1-arne@rfc2549.org> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20230314144854.182110-1-arne@rfc2549.org> References: <20230314144854.182110-1-arne@rfc2549.org> MIME-Version: 1.0 X-Spam-Score: 0.3 (/) X-Spam-Report: Spam detection software, running on the system "util-spamd-2.v13.lw.sourceforge.com", 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: The HMAC leaks are just forgotten frees/deinitialisations. tls_wrap_control will sometimes return the original buffer (non tls-crypt) and sometimes tls_wrap.work, handling this buffer lifetime is a bi [...] Content analysis details: (0.3 points, 6.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- 0.2 HEADER_FROM_DIFFERENT_DOMAINS From and EnvelopeFrom 2nd level mail domains are different 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record 0.0 SPF_NONE SPF: sender does not publish an SPF Record X-Headers-End: 1pcSg4-00058X-Ct Subject: [Openvpn-devel] [PATCH v2] Fix memory leaks in HMAC initial packet id 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?1760355021191894084?= X-GMAIL-MSGID: =?utf-8?q?1760446625174723451?= The HMAC leaks are just forgotten frees/deinitialisations. tls_wrap_control will sometimes return the original buffer (non tls-crypt) and sometimes tls_wrap.work, handling this buffer lifetime is a bit more complicated. Instead of further complicating that code just give our work buffer the same lifetime as the other one inside tls_wrap.work as that is also more consistent. Also packet_id_init will allocated a buffer with malloc and not using a gc_arena, so we need to manually also free it. Patch v2: add missing deallocations in unit tests of the new workbuf Found-By: clang with asan Signed-off-by: Arne Schwabe --- src/openvpn/init.c | 3 +++ src/openvpn/ssl.c | 12 +++++++++++ src/openvpn/ssl.h | 6 ++++++ src/openvpn/ssl_pkt.c | 8 +++++-- src/openvpn/ssl_pkt.h | 2 +- tests/unit_tests/openvpn/test_pkt.c | 33 +++++++++++++++++++---------- 6 files changed, 50 insertions(+), 14 deletions(-) diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 124ac76bd..fa2681dc7 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -3483,6 +3483,7 @@ do_init_frame_tls(struct context *c) frame_print(&c->c2.tls_auth_standalone->frame, D_MTU_INFO, "TLS-Auth MTU parms"); c->c2.tls_auth_standalone->tls_wrap.work = alloc_buf_gc(BUF_SIZE(&c->c2.frame), &c->c2.gc); + c->c2.tls_auth_standalone->workbuf = alloc_buf_gc(BUF_SIZE(&c->c2.frame), &c->c2.gc); } } @@ -3881,6 +3882,8 @@ do_close_tls(struct context *c) md_ctx_cleanup(c->c2.pulled_options_state); md_ctx_free(c->c2.pulled_options_state); } + + tls_auth_standalone_free(c->c2.tls_auth_standalone); } /* diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 78cec90a1..f2331f712 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1358,9 +1358,21 @@ tls_auth_standalone_init(struct tls_options *tls_options, packet_id_init(&tas->tls_wrap.opt.packet_id, tls_options->replay_window, tls_options->replay_time, "TAS", 0); + tas->workbuf = alloc_buf_gc(tas->frame.buf.payload_size, gc); return tas; } +void +tls_auth_standalone_free(struct tls_auth_standalone *tas) +{ + if (!tas) + { + return; + } + + packet_id_free(&tas->tls_wrap.opt.packet_id); +} + /* * Set local and remote option compatibility strings. * Used to verify compatibility of local and remote option diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h index 58ff4b9b4..a050cd5c9 100644 --- a/src/openvpn/ssl.h +++ b/src/openvpn/ssl.h @@ -180,6 +180,12 @@ void tls_multi_init_finalize(struct tls_multi *multi, int tls_mtu); struct tls_auth_standalone *tls_auth_standalone_init(struct tls_options *tls_options, struct gc_arena *gc); +/** + * Frees a standalone tls-auth verification object. + * @param tas the object to free. May be NULL. + */ +void tls_auth_standalone_free(struct tls_auth_standalone *tas); + /* * Setups the control channel frame size parameters from the data channel * parameters diff --git a/src/openvpn/ssl_pkt.c b/src/openvpn/ssl_pkt.c index 17a7f8917..8b3391e76 100644 --- a/src/openvpn/ssl_pkt.c +++ b/src/openvpn/ssl_pkt.c @@ -434,7 +434,10 @@ tls_reset_standalone(struct tls_wrap_ctx *ctx, uint8_t header, bool request_resend_wkc) { - struct buffer buf = alloc_buf(tas->frame.buf.payload_size); + /* Copy buffer here to point at the same data but allow tls_wrap_control + * to potentially change buf to point to another buffer without + * modifying the buffer in tas */ + struct buffer buf = tas->workbuf; ASSERT(buf_init(&buf, tas->frame.buf.headroom)); /* Reliable ACK structure */ @@ -461,7 +464,8 @@ tls_reset_standalone(struct tls_wrap_ctx *ctx, buf_write_u16(&buf, EARLY_NEG_FLAG_RESEND_WKC); } - /* Add tls-auth/tls-crypt wrapping, this might replace buf */ + /* Add tls-auth/tls-crypt wrapping, this might replace buf with + * ctx->work */ tls_wrap_control(ctx, header, &buf, own_sid); return buf; diff --git a/src/openvpn/ssl_pkt.h b/src/openvpn/ssl_pkt.h index ec7b48daf..ef4852e5d 100644 --- a/src/openvpn/ssl_pkt.h +++ b/src/openvpn/ssl_pkt.h @@ -77,6 +77,7 @@ struct tls_auth_standalone { struct tls_wrap_ctx tls_wrap; + struct buffer workbuf; struct frame frame; }; @@ -220,7 +221,6 @@ read_control_auth(struct buffer *buf, * This function creates a reset packet using the information * from the tls pre decrypt state. * - * The returned buf needs to be free with \c free_buf */ struct buffer tls_reset_standalone(struct tls_wrap_ctx *ctx, diff --git a/tests/unit_tests/openvpn/test_pkt.c b/tests/unit_tests/openvpn/test_pkt.c index f11e52a11..736f13178 100644 --- a/tests/unit_tests/openvpn/test_pkt.c +++ b/tests/unit_tests/openvpn/test_pkt.c @@ -203,6 +203,7 @@ init_tas_auth(int key_direction) static_key, true, key_direction, "Control Channel Authentication", "tls-auth", NULL); + tas.workbuf = alloc_buf(1600); return tas; } @@ -217,10 +218,22 @@ init_tas_crypt(bool server) tls_crypt_init_key(&tas.tls_wrap.opt.key_ctx_bi, &tas.tls_wrap.original_wrap_keydata, static_key, true, server); + tas.workbuf = alloc_buf(1600); + tas.tls_wrap.work = alloc_buf(1600); return tas; } +void +free_tas(struct tls_auth_standalone *tas) +{ + /* Not some of these might be null pointers but calling free on null + * pointers is a noop */ + free_key_ctx_bi(&tas->tls_wrap.opt.key_ctx_bi); + free_buf(&tas->workbuf); + free_buf(&tas->tls_wrap.work); +} + void test_tls_decrypt_lite_crypt(void **ut_state) { @@ -228,7 +241,6 @@ test_tls_decrypt_lite_crypt(void **ut_state) struct tls_pre_decrypt_state state = { 0 }; struct tls_auth_standalone tas = init_tas_crypt(true); - struct buffer buf = alloc_buf(1024); /* tls-auth should be invalid */ @@ -263,6 +275,7 @@ test_tls_decrypt_lite_crypt(void **ut_state) } free_key_ctx_bi(&tas.tls_wrap.opt.key_ctx_bi); + free_tas(&tas); free_buf(&buf); } @@ -318,6 +331,7 @@ test_tls_decrypt_lite_auth(void **ut_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); tas = init_tas_auth(KEY_DIRECTION_INVERSE); buf_reset_len(&buf); @@ -326,7 +340,7 @@ test_tls_decrypt_lite_auth(void **ut_state) assert_int_equal(verdict, VERDICT_INVALID); free_tls_pre_decrypt_state(&state); - free_key_ctx_bi(&tas.tls_wrap.opt.key_ctx_bi); + free_tas(&tas); free_buf(&buf); } @@ -371,6 +385,7 @@ test_tls_decrypt_lite_none(void **ut_state) free_tls_pre_decrypt_state(&state); free_buf(&buf); + free_tas(&tas); } static void @@ -442,10 +457,9 @@ test_verify_hmac_tls_auth(void **ut_state) bool valid = check_session_id_hmac(&state, &from.dest, hmac, 30); assert_false(valid); - free_key_ctx_bi(&tas.tls_wrap.opt.key_ctx_bi); - free_key_ctx(&tas.tls_wrap.tls_crypt_v2_server_key); free_tls_pre_decrypt_state(&state); free_buf(&buf); + free_tas(&tas); hmac_ctx_cleanup(hmac); hmac_ctx_free(hmac); } @@ -563,6 +577,7 @@ test_generate_reset_packet_plain(void **ut_state) tas.tls_wrap.mode = TLS_WRAP_NONE; struct frame frame = { {.headroom = 200, .payload_size = 1400}, 0}; tas.frame = frame; + tas.workbuf = alloc_buf(1600); uint8_t header = 0 | (P_CONTROL_HARD_RESET_CLIENT_V2 << P_OPCODE_SHIFT); @@ -577,10 +592,9 @@ test_generate_reset_packet_plain(void **ut_state) struct buffer buf2 = tls_reset_standalone(&tas.tls_wrap, &tas, &client_id, &server_id, header, false); assert_int_equal(BLEN(&buf), BLEN(&buf2)); assert_memory_equal(BPTR(&buf), BPTR(&buf2), BLEN(&buf)); - free_buf(&buf2); free_tls_pre_decrypt_state(&state); - free_buf(&buf); + free_buf(&tas.workbuf); } static void @@ -614,15 +628,12 @@ test_generate_reset_packet_tls_auth(void **ut_state) assert_int_equal(BLEN(&buf), BLEN(&buf2)); assert_memory_equal(BPTR(&buf), BPTR(&buf2), BLEN(&buf)); - free_buf(&buf2); free_tls_pre_decrypt_state(&state); packet_id_free(&tas_client.tls_wrap.opt.packet_id); - free_buf(&buf); - free_key_ctx_bi(&tas_server.tls_wrap.opt.key_ctx_bi); - free_key_ctx_bi(&tas_client.tls_wrap.opt.key_ctx_bi); - + free_tas(&tas_client); + free_tas(&tas_server); } int