From patchwork Wed Mar 15 19:55:12 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Arne Schwabe X-Patchwork-Id: 3137 Return-Path: Delivered-To: patchwork@openvpn.net Received: by 2002:a05:7300:2310:b0:9f:bfa4:120f with SMTP id r16csp3290621dye; Wed, 15 Mar 2023 12:56:18 -0700 (PDT) X-Google-Smtp-Source: AK7set/Zhzf1H6fi4PwoB7LvBCfDIGQKq+qrbQizNn4DcQzY9lGzODkRK7vwaQ9xcXKmRpQi1Zdv X-Received: by 2002:aa7:8bca:0:b0:625:ce95:f2ba with SMTP id s10-20020aa78bca000000b00625ce95f2bamr570584pfd.22.1678910178426; Wed, 15 Mar 2023 12:56:18 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1678910178; cv=none; d=google.com; s=arc-20160816; b=i9PH6WYIhBish4O/0qj1Ij9nYEGZr+ZeLBbWQ7kSVYXEG/RMy1QFcNnc3CZ+hYYipP jStWh4uTt7wSGlBXJ25Jabf7+SHjWtxj4Oft2D1ok7YsLkktUyJ+Qq2BXu+5mYQ39Ea5 Y4sqEIINa/uKjVA0GqwpYfCQRtlqFC2eItG4oc9cgAOSRtqr6qnCIw/PuvhzcT6x0SeZ 9oaSRxOMjw3TsQ0osrQSGzNVjEzkIESBY3BoHxQgVjzgyUc4Mwfh87n84Gsv1a8Y9Nbn AanETes6VrkCXJKwFTTfA7D7AUNw3TA0dscpKoPxHdzROqrA/2khZMfBYNTwHAAP/gAk Oe9g== 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=U1NQqXedgzlfgaE1wW68eiYSrFGlwlqtJSqbgr2lkk4=; b=PntQ6Yt1syjldXzYzndjwlAaz1fFl+6zy4qo8SE6FC59yReuNMDFT8z0V++aliWn7+ gAkn9sA5+sqr4vX7ofXnMqMLrM+z8N5qJ/szso/UEIHBQYC2uBoXxLDo7+eUYbH1rRzM YlTmVpvH064a4ehgVnz1ibYqE/EXyqodPyF7BH2FvWj3BDvjMpMuvWGDSyKIv4bZh1xV fZRPaLVGmqxYzyc2eG4Uj5Nxp77sU5O1mRMTJBMIWhr5ZZwfvLAKyylbuSuSjMvEua6s fAEiD5bn3w19cBX8HEy7d7r3U7g6uYd+t/1AbixfQ+tUYi4kaoRnBiVmQPgELOzEkApW xnzw== ARC-Authentication-Results: i=1; mx.google.com; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=RTjyc4Df; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b="YRT1bhc/"; 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 v64-20020a626143000000b006254c97a3dasi5424366pfb.301.2023.03.15.12.56.17 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Wed, 15 Mar 2023 12:56:18 -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=RTjyc4Df; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b="YRT1bhc/"; 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-1.v29.lw.sourceforge.com) by sfs-ml-1.v29.lw.sourceforge.com with esmtp (Exim 4.95) (envelope-from ) id 1pcXDX-0004PV-3t; Wed, 15 Mar 2023 19:55:23 +0000 Received: from [172.30.20.202] (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 1pcXDV-0004PO-Mn for openvpn-devel@lists.sourceforge.net; Wed, 15 Mar 2023 19:55:22 +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=sWt35nDWhN9UlSNb/rNOR6rM59EpMQoLoItafHlim/8=; b=RTjyc4DfOBOo1X5g/LrykCvAjn gxNj0PagFLqT2gnzbf7SDhx49mNGsdu9yraMUIunH1FJeaA6FjaRsXiB1GJp+5qjqF4d0tdnMJT3x icej61nsQZsfSH+ln6lLoWGfprGhJXSdyHqYkpvnogFqGRoFzthMMRrFYlvJ/KZ6woEk=; 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=sWt35nDWhN9UlSNb/rNOR6rM59EpMQoLoItafHlim/8=; b=YRT1bhc/UzSdQRiAiNoANl1shh W2vWVou9iIHO3QND4IxmgkivjrKYvmvmJjaoonQw2ere0JwUyslaVYN6l7U+9PpbY0OPa1jE484S6 YWD/Zcz+royE3iUULkwMQMMkHRHaPH+THxrF6x1I5vCKnl6RjzjvLNJGJ6Tkd8M3m9D4=; 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 1pcXDT-0006EM-HX for openvpn-devel@lists.sourceforge.net; Wed, 15 Mar 2023 19:55:21 +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 1pcXDM-000Ns4-5Z for openvpn-devel@lists.sourceforge.net; Wed, 15 Mar 2023 20:55:12 +0100 Received: (nullmailer pid 323116 invoked by uid 10006); Wed, 15 Mar 2023 19:55:12 -0000 From: Arne Schwabe To: openvpn-devel@lists.sourceforge.net Date: Wed, 15 Mar 2023 20:55:12 +0100 Message-Id: <20230315195512.323070-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-1.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.0 SPF_NONE SPF: sender does not publish an SPF Record 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record 0.2 HEADER_FROM_DIFFERENT_DOMAINS From and EnvelopeFrom 2nd level mail domains are different X-Headers-End: 1pcXDT-0006EM-HX Subject: [Openvpn-devel] [PATCH v3] 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?1760464919660176056?= 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 Patch v3: remove useless allocation of 0 size buffer in tls_auth_standalone_init Found-By: clang with asan Change-Id: I0cff44f79ee7e3bcf7b5981fc94f469c15f21af3 Signed-off-by: Arne Schwabe Acked-by: Gert Doering --- src/openvpn/init.c | 3 +++ src/openvpn/ssl.c | 11 ++++++++++ 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, 49 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..fe6390fad 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1361,6 +1361,17 @@ tls_auth_standalone_init(struct tls_options *tls_options, 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