From patchwork Sat Oct 17 12:45:15 2026 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alejandro Pacheco X-Patchwork-Id: 5478 Return-Path: Delivered-To: patchwork@openvpn.net Received: by 2002:a05:7001:a68f:b0:8d7:ebbe:b250 with SMTP id wf15csp4369530mab; Sat, 17 Oct 2026 05:45:49 -0700 (PDT) X-Forwarded-Encrypted: i=3; AKwUvBxhv81ihEDoEnoGx8KrMoxcIhUcXbzH1veUD+w3ikTkcnE6uZ5G8zOAB9j3Jngmz5qAytjDmnlcyxA=@openvpn.net X-Received: by 2002:a05:6870:d14b:b0:4a6:e46e:fc7c with SMTP id 586e51a60fabf-4a757aaec9fmr2275298fac.56.1792241149442; Sat, 17 Oct 2026 05:45:49 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1792241149; cv=fail; d=google.com; s=arc-20260327; b=AIhwxjq+Em40/ZXjNCcCcHCLZHaRlPXlnCTWxAHRhCJKmCg8tDzJy4WztAtaqSvP/s sh82AwLbDdpXM78O4eO8/AgNo1b4c7Z4328LrUyCtDBfz+Gpm+ZCWhfsrPO0J6mkAoIB Pt3qpv+z0NZ5uqjNqabQXwBVmhQ5tR4Bt6kUf7bENxYe3yWYZMFZoEHw+92ppCtLy6wZ DDrrEhB4RKM2PKhOVw3+n6fnrTznOMf+Ru1yFwK/fAtgLcySZoI8nLbpSBZ99hlmr1fv /qQNEJ760HAfBdxeD4SCtmdJfWhO7NFnsQNWq6Du0shrfRxZCaAA3h3Oa54V2x2kLBSA HP9Q== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20260327; h=errors-to:list-subscribe:list-help:list-post:list-archive :list-unsubscribe:list-id:precedence:subject:to:message-id:date :mime-version:from:dkim-signature:dkim-signature:dkim-signature :dkim-signature; bh=H4oCLKgvsMC1rWO8l4r9cGkyHBWguDSXHoYKsMV+VmI=; fh=4NbAC/LsuMLI0S0hprUlLSLCiHwg6SCAifhH718Jh0Q=; b=ZKoNd+SXut+qIIfDHgWQ5hpi89FBCV2z9ZOsHF2eFV/tV8fs9EMiUaAwCG7mePqfV7 ChmJQSD4dKctaN0RBx6+WV4VemIsWmPdPmQDK0b25mZZJeE89rOwjA+0DYd4CogEyR2Z Jbm/KEzd4q94XzpwtU0W6BTDFPB8DazrXBifLw0oVIU8t/a6PRrOE9SSZwjyNZeBw7Xi ZMwLNdalq/+Ecl16HK+QYGqUpmFt3hIYpBdv8dfXHwwAI+6O3yYoPXNJE2Dhi4/yuiPA II2mDXU42AqqqDXdFwFLatkT/BrGES2dgIk1ivfhTCxZSKseRH/2IPqL82D/CDipOecV F/Hw==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@lists.sourceforge.net header.s=beta header.b=g721qAJv; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=BQ6Fy6tE; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=Q98JkHVr; dkim=neutral (body hash did not verify) header.i=@gmail.com header.s=20251104 header.b=gDsGp5bm; arc=fail (body hash mismatch); 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=QUARANTINE dis=NONE) header.from=gmail.com; dara=neutral header.i=@openvpn.net Received: from lists.sourceforge.net (lists.sourceforge.net. [216.105.38.7]) by mx.google.com with ESMTPS id 586e51a60fabf-4a73b6a9753si7034004fac.140.2026.10.17.05.45.49 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sat, 17 Oct 2026 05:45:49 -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=g721qAJv; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=BQ6Fy6tE; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=Q98JkHVr; dkim=neutral (body hash did not verify) header.i=@gmail.com header.s=20251104 header.b=gDsGp5bm; arc=fail (body hash mismatch); 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=QUARANTINE dis=NONE) header.from=gmail.com; dara=neutral header.i=@openvpn.net DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.sourceforge.net; s=beta; h=List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Subject:Content-Type:To:Message-ID:Date :MIME-Version:From:Sender:Reply-To:Cc:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:In-Reply-To:References:List-Owner; bh=H4oCLKgvsMC1rWO8l4r9cGkyHBWguDSXHoYKsMV+VmI=; b=g721qAJvmluzI16GXdfAe3I9Ab dCfIYisz6nhXUUUba6QqWC//JvkvYWNi/nkCQsd3kJM9Z6AlsfILInWXPJxaQ0lAd7Z4KFQJ61Sxu hJc9UeKcnKAmWugrwszQsTPoHol07eTFda5SLGHJMQ/knbuLGJ/E+NH5+ho2usRexpRc=; Received: from [127.0.0.1] (helo=sfs-ml-3.v29.lw.sourceforge.com) by sfs-ml-3.v29.lw.sourceforge.com with esmtp (Exim 4.95) (envelope-from ) id 1xI3mz-0004yD-Ce; Sat, 17 Oct 2026 12:45:30 +0000 Received: from [172.30.29.66] (helo=mx.sourceforge.net) by sfs-ml-3.v29.lw.sourceforge.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1xI3ms-0004y3-Dp for openvpn-devel@lists.sourceforge.net; Sat, 17 Oct 2026 12:45:23 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=Content-Type:To:Subject:Message-ID:Date: MIME-Version:From:Sender:Reply-To:Cc:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:In-Reply-To:References:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=/V3iIeDxJZWpjtSMZIjri41Ur5ENngxyni5wQY3hmAQ=; b=BQ6Fy6tERatfJNAaJ/v3JyPqXL AwVoFoqtQFvk1Pe6fl8HyXtPy2A54eZWO3vf56g3ZGZjvVCguXeU8XYUvN/sGbxgXTWMgbpAFmxTJ br7GHTy7EZu6SpHmAIqgYyWvNkbAwifpPqLnGDIL8Em/b2IPEQVnVWlMPrDYj939UQ7g=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=Content-Type:To:Subject:Message-ID:Date:MIME-Version:From:Sender:Reply-To :Cc:Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To: References:List-Id:List-Help:List-Unsubscribe:List-Subscribe:List-Post: List-Owner:List-Archive; bh=/V3iIeDxJZWpjtSMZIjri41Ur5ENngxyni5wQY3hmAQ=; b=Q 98JkHVrQMi7l9FO7p6wRPSTgNqPyXnSzf65j/VztJq3Wcss0TwfDNA/5s56qHK+ve3sD5fwBNOnyh sVD3kvLZQZ5hMKBi+OjUR4BDjC3bipD8thiksRfMlX1nIoZoDHHe539coKk9Tq6P7zoOPmgczXX67 mUUGl4iV2iDWt2kc=; Received: from mail-pj1-f53.google.com ([209.85.216.53]) by sfi-mx-1.v28.lw.sourceforge.com with esmtps (TLS1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.95) id 1xI3ms-0000ke-Gm for openvpn-devel@lists.sourceforge.net; Sat, 17 Oct 2026 12:45:23 +0000 Received: by mail-pj1-f53.google.com with SMTP id 98e67ed59e1d1-3a4cdc9025bso548785a91.0 for ; Sat, 17 Oct 2026 05:45:22 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1792241117; cv=none; d=google.com; s=arc-20260327; b=J9XEpk7gl8egm4O7H5C+Wq2sRN9Kb/Gno8tR5YEsQqy9Qc/iKnx0g9NNVsU3PpBATt Q9tsU/KTndKeZuHXbhz3hkCHvvV/0atuecuAt3qaHNOnQqJbF+OmXf2RW8bNUksNVwzo mEp/aiJl2cPNF4qd/Q+fyHXIp2cuA0BD/27xqb1t4v9PNnt4dOsMBM2aMe1VzbsSba/k NoegVPXtg7i/GSeryNM3I1VxJaINtwaE6tYrlT1+R2qvs5phFP767y0+hvD3GRlPEoKp LsoK5dD660c9h1007R8LpA5m0ec3gbGw64qQagNw551/iB7L2OTLE27hij7VZvD3Nb4x vLbw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20260327; h=to:subject:message-id:date:mime-version:from:dkim-signature; bh=/V3iIeDxJZWpjtSMZIjri41Ur5ENngxyni5wQY3hmAQ=; fh=4NbAC/LsuMLI0S0hprUlLSLCiHwg6SCAifhH718Jh0Q=; b=WlqVIBVFSew4wReu+CcZM6IHL6t351zFf3BolYKSjdkeTFMFKQHYqoDG6N+c+FWsIW cO+vDr47t25Y1I84cCvXUyNpIJiQQxW18+V0HBVJv/inEv610F9DUFNhgm2BdCvBp6iN hgtrKDNihAJB070DVF5kgicRJgkkVmGaTpWYPEhudP6UxRnLXfIfw+HDRYkuUZ0BAZAo gGnA6wegeckCFlX/IN+BHTF+trAdpcXM65VJkljt/A3M8fIdWK3DwUj8ywoJrEtFBdG1 baOmGZ/ClY9jTUL19uO1/QEmJS4SqRNJ7WUykhiHybsPHJj3V4ZxT/zkaBWhQFnnnBvu YKQw==; darn=lists.sourceforge.net ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1792241117; x=1792845917; darn=lists.sourceforge.net; h=content-type:to:subject:message-id:date:mime-version:from:from:to :cc:subject:date:message-id:reply-to:content-type; bh=/V3iIeDxJZWpjtSMZIjri41Ur5ENngxyni5wQY3hmAQ=; b=gDsGp5bms3r0dSbaVSdtJUKP9iJjsxH9j/7sJr9PEp0O3kQFV+M7GIITSJ/3O6QXyX sbFN7iKvzudhIbfa4wSHkRkQ0dYw3yuVQor1XV77aB1utM992mieg/BL7tDPvBJ3ilAA UyRqJiQYv+lVx04poxiurkZ6ujJQNI3EF2im01Zi0kSGX0qybxHY/uU7PUaHKLyX2mth WBQdnnVnzbgjAaHEzkaGEb/mfjHatBUUnlTstd2QqxFwMzjnvXBFLIbw/oJiH5R+Qn1E NP0TMNZyquUZaIpsBYx+8knlMQm0FTT8CsoFJQv5bqROk1dIXXzP+3rJLpv+Llvqrju6 5H5Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1792241117; x=1792845917; h=content-type:to:subject:message-id:date:mime-version:from:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=/V3iIeDxJZWpjtSMZIjri41Ur5ENngxyni5wQY3hmAQ=; b=sC7x+0B4UUU5WpOS8LXQ94MWHQ77e6StBDZfLf5dcg8KEh1NsJMDikLVGc2rFErqg1 RqF0BUnm0Y29Z9NXBAg8EpDOybfoZZhlccF3IJkTinAYOhbdMQAA+DE1bdpaPZgSQANr 9IJsHZnQl2oMEJvEZh6F9PDjNIULIabMx+r1A+zaJLi0OeAJe+vAscZXXxw7QlSm5U7O CjK/i+/pPn6B3qYxC38KVUk8FlA9uquY/cVnO8hJuXhzfhSJ9R2B7bn7bOzigy6Jfmay qdatourGRfmI/FdsD7UPFauy6gxe3eF2qWwuflstP4kEDyk83b3ZP0miIS/d1wlCJuI6 ysQQ== X-Gm-Message-State: AFq9FYIv5gv21swHyXeik23HYjzc3qipNpk16TVdSYwcPEZsjKqVZN1U lAhWx1NBwsb61UhdeiZp733o0j1kHkR/KacoyIw4pyBLb9x1FZNvJDe4H+jWpsRPrGxZH8C6Bqn DdC7gvOTnD7e8fkkZjISkftmYL3FrKqcwAq7y0fjZ60bA X-Gm-Gg: AYBFou2AO8dBvJ9+vPOcxu/I5LhuCPfdWTBp3eHzhHIUpwq3neHdwyyLwkeERAbHnXw VWyJ3YTaNk+bBoyzl80Tyglp5a+5g77FLoOk16mnpTeD9V846gC9idrXFajbYw9mMOMg6eWmwF9 V1ZvAMuacR0Gii4dKLztXF4ValIJogUk36P/q2InhYIBwf2ywS+RRiMN8WihUCHA+MVhq+GzcA0 U3ntEw1lpVAIukapVAc7w7MO6kBnmL+JOeoEoNdThduo3U58D8a65wifRap753s3krQy1zXBc38 eKzmaLqHLr7D4X4YSOd39V2S7dx5ECtlHYc8BQjPTu8h34LoZkhU/wBHZvxfvbFmdNsijUAD2R4 A6sGDe7fXrTUS5KnG6XACiAmnoImvKvn2qwYwipB/SA== X-Received: by 2002:a17:90b:4c4f:b0:3ac:d17c:2a6d with SMTP id 98e67ed59e1d1-3aceefbf462mr2308459a91.12.1792241116572; Sat, 17 Oct 2026 05:45:16 -0700 (PDT) Received: from 1077388689865 named unknown by gmailapi.google.com with HTTPREST; Sat, 17 Oct 2026 08:45:15 -0400 Received: from 1077388689865 named unknown by gmailapi.google.com with HTTPREST; Sat, 17 Oct 2026 08:45:15 -0400 From: Alejandro Pacheco MIME-Version: 1.0 Date: Sat, 17 Oct 2026 08:45:15 -0400 X-Gm-Features: AclHuK_GjFCI8nC_vQ51TIeEY68Qg6_4P5dQhxfNmE8URoUNDvFuQx79qRXh0ao Message-ID: To: openvpn-devel@lists.sourceforge.net X-Spam-Score: 0.1 (/) 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: tls_multi_process() returns the outgoing control packet in to_link. Until now to_link pointed into memory owned by a session or key state: the reliable send buffer, ks->ack_write_buf, or the tls_wrap [...] Content analysis details: (0.1 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain -0.1 DKIM_VALID_EF Message has a valid DKIM or DK signature from envelope-from domain 0.2 FREEMAIL_ENVFROM_END_DIGIT Envelope-from freemail username ends in digit [alepacheco99(at)gmail.com] 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider [alepacheco99(at)gmail.com] 0.0 RCVD_IN_MSPIKE_H3 RBL: Good reputation (+3) [209.85.216.53 listed in wl.mailspike.net] 0.0 RCVD_IN_MSPIKE_WL Mailspike good senders X-Headers-End: 1xI3ms-0000ke-Gm Subject: [Openvpn-devel] [PATCH RESEND] Keep the outgoing control packet in a buffer owned by tls_multi 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: 1879301055011366076 X-GMAIL-MSGID: 1879301055011366076 tls_multi_process() returns the outgoing control packet in to_link. Until now to_link pointed into memory owned by a session or key state: the reliable send buffer, ks->ack_write_buf, or the tls_wrap or tls_wrap_reneg work buffer. When that session or key state was freed or reset before the packet was written to the link, to_link was left pointing to freed memory. This caused CVE-2023-46850, CVE-2026-40215, CVE-2026-12996, CVE-2026-13117 and CVE-2026-84471. The fixes so far added check_session_buf_not_used() and check_keystate_buf_not_used(), which compare to_link against a list of the buffers of a session or key state before it is freed and drop the packet on a match. This only works as long as the list names every buffer a packet can end up in and every caller that frees a session or key state passes to_link down: key_state_soft_reset() is called without it from tls_pre_decrypt() and tls_session_soft_reset(). Fix the ownership instead. write_control_auth() now copies the finished packet into a buffer the caller owns, and tls_multi_process() returns it in multi->to_link_buf, which lives as long as the tls_multi, like multi->to_link_addr already does for the address. Freeing or moving sessions and key states no longer affects a pending packet, so the checks and the to_link parameters of key_state_free(), tls_session_free(), move_session(), reset_session() and key_state_soft_reset() are removed. A packet that is pending when its session is freed is now sent instead of dropped; the peer handles it like any late packet of that session. The copy costs one memcpy of a control channel packet. The new unit test checks that the packet written by write_control_auth() stays intact and valid after the buffers it was built from are overwritten, with no wrapping, tls-auth and tls-crypt. Signed-off-by: Alejandro Pacheco --- Resent: the first posting of this patch () reached the list with its long lines wrapped, so it does not apply and patchwork did not pick it up. The patch is unchanged. This time the diff is attached as plain text so that it arrives intact; git am takes this mail as it is. 64fae9d8 ("Ensure that buffer of freed session are not used") and 2a881d73 ("Move check_session_buf_not_used method into the method that free the buffer") call the check_*_buf_not_used() checks a band-aid that a real fix would replace with more refactoring. This patch does that for the to_link buffer only: it does not remove TM_INITIAL or change how sessions are moved. Tested on an ASan/UBSan build with OpenSSL 3.5.4: all unit tests, t_lpback.sh, and t_cltsrv.sh, whose loopback configs renegotiate every 10 seconds with tls-auth (22 renegotiations, no sanitizer reports). src/openvpn/ssl.c | 169 +++++----------------------- src/openvpn/ssl_common.h | 8 ++ src/openvpn/ssl_pkt.c | 20 +++- src/openvpn/ssl_pkt.h | 25 +++- tests/unit_tests/openvpn/test_pkt.c | 103 +++++++++++++++++ 5 files changed, 178 insertions(+), 147 deletions(-) diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 35948715..d52d539c 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -96,14 +96,6 @@ show_tls_performance_stats(void) #endif /* ifdef MEASURE_TLS_HANDSHAKE_STATS */ -/* forward decleration since tls_process needs this function prototype */ -static void -check_session_buf_not_used(struct buffer *to_link, struct tls_session *session); - -static void -check_keystate_buf_not_used(struct buffer *to_link, const struct key_state *ks); - - /** * Limit the reneg_bytes value when using a small-block (<128 bytes) cipher. * @@ -904,15 +896,10 @@ key_state_init(struct tls_session *session, struct key_state *ks) * cleaned up. * @param clear - Whether the memory allocated for the \a ks object * should be overwritten with 0s. - * - * @param to_link - if not NULL, check that the buffer does not contain - * any pointer to one of the internal structs of ks */ static void -key_state_free(struct key_state *ks, bool clear, struct buffer *to_link) +key_state_free(struct key_state *ks, bool clear) { - check_keystate_buf_not_used(to_link, ks); - ks->state = S_UNDEF; key_state_ssl_free(&ks->ks_ssl); @@ -1056,14 +1043,10 @@ tls_session_init(struct tls_multi *multi, struct tls_session *session) * object should be overwritten with 0s. This * implicitly sets many states to 0/false, * e.g. the validity of the keys in the structure - * @param to_link - if not NULL, check that the buffer does not contain - * any pointer to one of the internal structs of ks - * */ static void -tls_session_free(struct tls_session *session, bool clear, struct buffer *to_link) +tls_session_free(struct tls_session *session, bool clear) { - check_session_buf_not_used(to_link, session); tls_wrap_free(&session->tls_wrap); tls_wrap_free(&session->tls_wrap_reneg); @@ -1072,7 +1055,7 @@ tls_session_free(struct tls_session *session, bool clear, struct buffer *to_link /* we don't need clear=true for this call since * the structs are part of session and get cleared * as part of session */ - key_state_free(&session->key[i], false, to_link); + key_state_free(&session->key[i], false); } free(session->common_name); @@ -1091,16 +1074,14 @@ tls_session_free(struct tls_session *session, bool clear, struct buffer *to_link static void -move_session(struct tls_multi *multi, int dest, int src, bool reinit_src, - struct buffer *to_link) +move_session(struct tls_multi *multi, int dest, int src, bool reinit_src) { - check_session_buf_not_used(to_link, &multi->session[dest]); msg(D_TLS_DEBUG_LOW, "TLS: move_session: dest=%s src=%s reinit_src=%d", session_index_name(dest), session_index_name(src), reinit_src); ASSERT(src != dest); ASSERT(src >= 0 && src < TM_SIZE); ASSERT(dest >= 0 && dest < TM_SIZE); - tls_session_free(&multi->session[dest], false, to_link); + tls_session_free(&multi->session[dest], false); multi->session[dest] = multi->session[src]; if (reinit_src) @@ -1116,9 +1097,9 @@ move_session(struct tls_multi *multi, int dest, int src, bool reinit_src, } static void -reset_session(struct tls_multi *multi, struct tls_session *session, struct buffer *to_link) +reset_session(struct tls_multi *multi, struct tls_session *session) { - tls_session_free(session, false, to_link); + tls_session_free(session, false); tls_session_init(multi, session); } @@ -1192,6 +1173,8 @@ void tls_multi_init_finalize(struct tls_multi *multi, int tls_mtu) { tls_init_control_channel_frame_parameters(&multi->opt.frame, tls_mtu); + multi->to_link_buf = alloc_buf(BUF_SIZE(&multi->opt.frame)); + /* initialize the active and untrusted sessions */ tls_session_init(multi, &multi->session[TM_ACTIVE]); @@ -1283,9 +1266,11 @@ tls_multi_free(struct tls_multi *multi, bool clear) for (int i = 0; i < TM_SIZE; ++i) { - tls_session_free(&multi->session[i], false, NULL); + tls_session_free(&multi->session[i], false); } + free_buf(&multi->to_link_buf); + if (clear) { secure_memzero(multi, sizeof(*multi)); @@ -1816,13 +1801,13 @@ flush_payload_buffer(struct key_state *ks) * active key. */ static void -key_state_soft_reset(struct tls_session *session, struct buffer *to_link) +key_state_soft_reset(struct tls_session *session) { struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */ struct key_state *ks_lame = &session->key[KS_LAME_DUCK]; /* retiring key */ ks->must_die = now + session->opt->transition_window; /* remaining lifetime of old key */ - key_state_free(ks_lame, false, to_link); + key_state_free(ks_lame, false); *ks_lame = *ks; key_state_init(session, ks); @@ -1833,7 +1818,7 @@ key_state_soft_reset(struct tls_session *session, struct buffer *to_link) void tls_session_soft_reset(struct tls_multi *tls_multi) { - key_state_soft_reset(&tls_multi->session[TM_ACTIVE], NULL); + key_state_soft_reset(&tls_multi->session[TM_ACTIVE]); } /* @@ -2914,18 +2899,15 @@ tls_process_state(struct tls_multi *multi, struct tls_session *session, struct b const struct buffer *buf = reliable_send(ks->send_reliable, &opcode); ASSERT(buf); - struct buffer b = *buf; INCR_SENT; - write_control_auth(session, ks, &b, to_link_addr, opcode, CONTROL_SEND_ACK_MAX, true); - *to_link = b; + write_control_auth(session, ks, buf, &multi->to_link_buf, to_link_addr, opcode, + CONTROL_SEND_ACK_MAX, true); + *to_link = multi->to_link_buf; dmsg(D_TLS_DEBUG, "Reliable -> TCP/UDP"); - /* This changed the state of the outgoing buffer. In order to avoid - * running this function again/further and invalidating the key_state - * buffer and accessing the buffer that is now in to_link after it being - * freed for a potential error, we shortcircuit exiting of the outer - * process here. */ + /* This changed the state of the outgoing buffer. Stop this + * processing pass so that the event loop can send the packet. */ return false; } @@ -3165,13 +3147,13 @@ tls_process(struct tls_multi *multi, struct tls_session *session, struct buffer session->opt->aead_usage_limit, ks->crypto_options.key_ctx_bi.decrypt.plaintext_blocks + ks->n_packets, session->opt->aead_usage_limit); - key_state_soft_reset(session, to_link); + key_state_soft_reset(session); } /* Kill lame duck key transition_window seconds after primary key negotiation */ if (lame_duck_must_die(session, wakeup)) { - key_state_free(ks_lame, true, to_link); + key_state_free(ks_lame, true); msg(D_TLS_DEBUG_LOW, "TLS: tls_process: killed expiring key"); } @@ -3220,8 +3202,9 @@ tls_process(struct tls_multi *multi, struct tls_session *session, struct buffer { struct buffer buf = ks->ack_write_buf; ASSERT(buf_init(&buf, multi->opt.frame.buf.headroom)); - write_control_auth(session, ks, &buf, to_link_addr, P_ACK_V1, RELIABLE_ACK_SIZE, false); - *to_link = buf; + write_control_auth(session, ks, &buf, &multi->to_link_buf, to_link_addr, P_ACK_V1, + RELIABLE_ACK_SIZE, false); + *to_link = multi->to_link_buf; dmsg(D_TLS_DEBUG, "Dedicated ACK -> TCP/UDP"); } } @@ -3264,98 +3247,6 @@ tls_process(struct tls_multi *multi, struct tls_session *session, struct buffer return false; } -static void -check_keystate_buf_not_used(struct buffer *to_link, const struct key_state *ks) -{ - if (ks->state == S_UNDEF || !to_link || !to_link->data) - { - return; - } - - const uint8_t *dataptr = to_link->data; - - /* we don't expect send_reliable to be NULL when state is - * not S_UNDEF, but people have reported crashes nonetheless, - * therefore we better catch this event, report and exit. - */ - if (!ks->send_reliable) - { - msg(M_FATAL, - "ERROR: ks.send_reliable (key-id %d), is NULL " - "while key state is %s. Exiting.", - ks->key_id, state_name(ks->state)); - } - - for (int j = 0; j < ks->send_reliable->size; j++) - { - if (ks->send_reliable->array[j].buf.data == dataptr) - { - msg(M_INFO, - "Warning buffer of freed TLS session is still in" - " use (key-id %d, ks.send_reliable->array[%d])", - ks->key_id, j); - - goto used; - } - } - - if (ks->ack_write_buf.data == dataptr) - { - msg(M_INFO, "Warning buffer of freed TLS session is still in use " - "(ks.ack_write_buf, key-id %d)", - ks->key_id); - - goto used; - } - return; - -used: - to_link->len = 0; - to_link->data = 0; -} - -/** - * This is a safe guard function to double check that a buffer from a session is - * not used in a session to avoid a use after free. - * - * @param to_link - * @param session - */ -static void -check_session_buf_not_used(struct buffer *to_link, struct tls_session *session) -{ - if (!to_link || !to_link->data) - { - return; - } - const uint8_t *dataptr = to_link->data; - - /* Checks buffers in tls_wrap */ - if (session->tls_wrap.work.data == dataptr) - { - msg(M_INFO, "Warning buffer of freed TLS session is " - "still in use (tls_wrap.work.data)"); - goto used; - } - if (session->tls_wrap_reneg.work.data == dataptr) - { - msg(M_INFO, "Warning buffer of freed TLS session is " - "still in use (tls_wrap_reneg.work.data)"); - goto used; - } - - for (int i = 0; i < KS_SIZE; i++) - { - const struct key_state *ks = &session->key[i]; - check_keystate_buf_not_used(to_link, ks); - } - return; - -used: - to_link->len = 0; - to_link->data = 0; - /* for debugging, you can add an ASSERT(0); here to trigger an abort */ -} /* * Called by the top-level event loop. * @@ -3444,11 +3335,11 @@ tls_multi_process(struct tls_multi *multi, struct buffer *to_link, if (i == TM_ACTIVE && ks_lame->state >= S_GENERATED_KEYS && !multi->opt.single_session) { - move_session(multi, TM_LAME_DUCK, TM_ACTIVE, true, to_link); + move_session(multi, TM_LAME_DUCK, TM_ACTIVE, true); } else { - reset_session(multi, session, to_link); + reset_session(multi, session); } } } @@ -3511,7 +3402,7 @@ tls_multi_process(struct tls_multi *multi, struct buffer *to_link, */ if (lame_duck_must_die(&multi->session[TM_LAME_DUCK], wakeup)) { - tls_session_free(&multi->session[TM_LAME_DUCK], true, to_link); + tls_session_free(&multi->session[TM_LAME_DUCK], true); msg(D_TLS_DEBUG_LOW, "TLS: tls_multi_process: killed expiring key"); } @@ -3526,7 +3417,7 @@ tls_multi_process(struct tls_multi *multi, struct buffer *to_link, */ if (TLS_AUTHENTICATED(multi, &multi->session[TM_INITIAL].key[KS_PRIMARY])) { - move_session(multi, TM_ACTIVE, TM_INITIAL, true, to_link); + move_session(multi, TM_ACTIVE, TM_INITIAL, true); tas = tls_authentication_status(multi); msg(D_TLS_DEBUG_LOW, "TLS: tls_multi_process: initial untrusted " @@ -3938,7 +3829,7 @@ tls_pre_decrypt(struct tls_multi *multi, const struct link_socket_actual *from, goto error; } - key_state_soft_reset(session, NULL); + key_state_soft_reset(session); dmsg(D_TLS_DEBUG, "TLS: received P_CONTROL_SOFT_RESET_V1 s=%d sid=%s", i, session_id_print(&sid, &gc)); diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h index f6392f9a..a0f677ec 100644 --- a/src/openvpn/ssl_common.h +++ b/src/openvpn/ssl_common.h @@ -627,6 +627,14 @@ struct tls_multi */ struct link_socket_actual to_link_addr; + /** + * Holds the outgoing control packet returned by tls_multi_process. + * It belongs to the tls_multi and not to a session or key state, so + * it stays valid when these are freed or moved before the packet is + * written to the link. + */ + struct buffer to_link_buf; + int n_sessions; /**< Number of sessions negotiated thus * far. */ enum multi_status multi_state; diff --git a/src/openvpn/ssl_pkt.c b/src/openvpn/ssl_pkt.c index f78782fa..d8a1cefa 100644 --- a/src/openvpn/ssl_pkt.c +++ b/src/openvpn/ssl_pkt.c @@ -163,10 +163,14 @@ tls_wrap_control(struct tls_wrap_ctx *ctx, uint8_t header, struct buffer *buf, } void -write_control_auth(struct tls_session *session, struct key_state *ks, struct buffer *buf, - struct link_socket_actual **to_link_addr, int opcode, int max_ack, - bool prepend_ack) +write_control_auth(struct tls_session *session, struct key_state *ks, const struct buffer *payload, + struct buffer *out, struct link_socket_actual **to_link_addr, int opcode, + int max_ack, bool prepend_ack) { + /* Work on a copy so that tls_wrap_control can point it at another + * buffer without modifying the caller's buffer */ + struct buffer buf = *payload; + ASSERT(ks->key_id >= 0 && ks->key_id <= P_KEY_ID_MASK); ASSERT(opcode >= 0 && opcode <= P_LAST_OPCODE); uint8_t header = (uint8_t)(ks->key_id | (opcode << P_OPCODE_SHIFT)); @@ -181,14 +185,20 @@ write_control_auth(struct tls_session *session, struct key_state *ks, struct buf } ASSERT(link_socket_actual_defined(&ks->remote_addr)); - ASSERT(reliable_ack_write(ks->rec_ack, ks->lru_acks, buf, &ks->session_id_remote, max_ack, + ASSERT(reliable_ack_write(ks->rec_ack, ks->lru_acks, &buf, &ks->session_id_remote, max_ack, prepend_ack)); msg(D_TLS_DEBUG, "%s(): %s", __func__, packet_opcode_name(opcode)); - tls_wrap_control(tls_session_get_tls_wrap(session, ks->key_id), header, buf, + tls_wrap_control(tls_session_get_tls_wrap(session, ks->key_id), header, &buf, &session->session_id); + /* buf now points into memory owned by ks or session (the payload's + * buffer or the tls_wrap work buffer). Either can be freed before + * the packet is written to the link, so copy the packet to out. */ + ASSERT(buf_init(out, buf.offset)); + ASSERT(buf_copy(out, &buf)); + *to_link_addr = &ks->remote_addr; } diff --git a/src/openvpn/ssl_pkt.h b/src/openvpn/ssl_pkt.h index 8203e4ac..d02b4b09 100644 --- a/src/openvpn/ssl_pkt.h +++ b/src/openvpn/ssl_pkt.h @@ -185,10 +185,29 @@ struct session_id calculate_session_id_hmac(struct session_id client_sid, bool check_session_hmac_and_pkt_id(struct tls_pre_decrypt_state *state, const struct openvpn_sockaddr *from, const uint8_t *key, int handwindow, bool pkt_is_ack); -/* - * Write a control channel authentication record. +/** + * Write a control channel packet: add the ACKs, the opcode and session id + * header and the tls-auth/tls-crypt wrapping to \c payload. + * + * @param session the TLS session the packet belongs to + * @param ks the key state the packet belongs to + * @param payload the packet payload. Its data is left + * unchanged, but the space around it in its + * buffer is used to build the packet. + * @param out buffer the finished packet is written to, at + * the offset it was built at. It must be owned + * by the caller, not share memory with \c payload + * or the tls_wrap work buffers, and have room for + * the packet at that offset (a buffer of + * BUF_SIZE() of the session's frame does). + * @param to_link_addr set to the address the packet is sent to + * @param opcode opcode of the packet + * @param max_ack maximum number of ACKs to add + * @param prepend_ack add the ACKs in front of the payload instead + * of writing them into it */ -void write_control_auth(struct tls_session *session, struct key_state *ks, struct buffer *buf, +void write_control_auth(struct tls_session *session, struct key_state *ks, + const struct buffer *payload, struct buffer *out, struct link_socket_actual **to_link_addr, int opcode, int max_ack, bool prepend_ack); diff --git a/tests/unit_tests/openvpn/test_pkt.c b/tests/unit_tests/openvpn/test_pkt.c index 51c73d8e..120f98f6 100644 --- a/tests/unit_tests/openvpn/test_pkt.c +++ b/tests/unit_tests/openvpn/test_pkt.c @@ -741,6 +741,108 @@ test_wkc_not_appended_without_key(void **ut_state) free_tas(&tas); } +/* write_control_auth() writes the finished packet to the out buffer of the + * caller, so the packet stays valid when the buffers of the session and key + * state are reused or freed before it is sent. */ +static void +check_write_control_auth_out_buf(struct tls_wrap_ctx *wrap, struct tls_wrap_ctx *peer_wrap) +{ + struct tls_options opt = { 0 }; + opt.server = true; + + struct tls_session session = { 0 }; + session.opt = &opt; + session.tls_wrap = *wrap; + session.session_id = (struct session_id){ { 1, 2, 3, 4, 5, 6, 7, 8 } }; + + struct reliable_ack rec_ack = { .len = 1, .packet_id = { 7 } }; + struct reliable_ack lru_acks = { 0 }; + struct key_state ks = { 0 }; + ks.rec_ack = &rec_ack; + ks.lru_acks = &lru_acks; + ks.session_id_remote = (struct session_id){ { 8, 7, 6, 5, 4, 3, 2, 1 } }; + ks.remote_addr.dest.addr.sa.sa_family = AF_INET; + ks.remote_addr.dest.addr.in4.sin_addr.s_addr = htonl(0x01020304); + + const uint8_t data[] = "control channel payload"; + struct buffer payload = alloc_buf(1600); + struct buffer out = alloc_buf(1600); + + /* a control packet with the ACK in front of its payload, as sent from + * the reliability layer, and a dedicated ACK packet */ + for (int i = 0; i < 2; i++) + { + const bool dedicated_ack = (i == 1); + const size_t len = dedicated_ack ? 0 : sizeof(data); + + assert_true(buf_init(&payload, 200)); + assert_true(buf_write(&payload, data, len)); + + struct link_socket_actual *to_link_addr = NULL; + write_control_auth(&session, &ks, &payload, &out, &to_link_addr, + dedicated_ack ? P_ACK_V1 : P_CONTROL_V1, RELIABLE_ACK_SIZE, + !dedicated_ack); + + assert_ptr_equal(to_link_addr, &ks.remote_addr); + assert_int_equal(BLEN(&payload), len); + assert_memory_equal(BPTR(&payload), data, len); + assert_true(BLENZ(&out) > len); + + /* the packet is still intact after the session buffers are overwritten */ + memset(payload.data, 0x55, payload.capacity); + if (session.tls_wrap.work.data) + { + memset(session.tls_wrap.work.data, 0x55, session.tls_wrap.work.capacity); + } + + struct link_socket_actual from = { 0 }; + assert_true(read_control_auth(&out, peer_wrap, &from, &opt)); + + struct reliable_ack ack; + struct session_id sid; + assert_true(reliable_ack_parse(&out, &ack, &sid)); + assert_int_equal(ack.len, 1); + assert_int_equal(ack.packet_id[0], 7); + assert_memory_equal(&sid, &ks.session_id_remote, SID_SIZE); + assert_int_equal(BLEN(&out), len); + assert_memory_equal(BPTR(&out), data, len); + } + + free_buf(&out); + free_buf(&payload); +} + +static void +test_write_control_auth_out_buf(void **ut_state) +{ + now = 0x22446688; + + struct tls_wrap_ctx none = { .mode = TLS_WRAP_NONE }; + check_write_control_auth_out_buf(&none, &none); + + struct tls_auth_standalone tas_client = init_tas_auth(KEY_DIRECTION_INVERSE); + struct tls_auth_standalone tas_server = init_tas_auth(KEY_DIRECTION_NORMAL); + packet_id_init(&tas_client.tls_wrap.opt.packet_id, 5, 5, "UNITTEST", 0); + packet_id_init(&tas_server.tls_wrap.opt.packet_id, 5, 5, "UNITTEST", 0); + reset_packet_id_send(&tas_client.tls_wrap.opt.packet_id.send); + check_write_control_auth_out_buf(&tas_client.tls_wrap, &tas_server.tls_wrap); + packet_id_free(&tas_client.tls_wrap.opt.packet_id); + packet_id_free(&tas_server.tls_wrap.opt.packet_id); + free_tas(&tas_client); + free_tas(&tas_server); + + tas_client = init_tas_crypt(false); + tas_server = init_tas_crypt(true); + packet_id_init(&tas_client.tls_wrap.opt.packet_id, 5, 5, "UNITTEST", 0); + packet_id_init(&tas_server.tls_wrap.opt.packet_id, 5, 5, "UNITTEST", 0); + reset_packet_id_send(&tas_client.tls_wrap.opt.packet_id.send); + check_write_control_auth_out_buf(&tas_client.tls_wrap, &tas_server.tls_wrap); + packet_id_free(&tas_client.tls_wrap.opt.packet_id); + packet_id_free(&tas_server.tls_wrap.opt.packet_id); + free_tas(&tas_client); + free_tas(&tas_server); +} + static void test_extract_control_message(void **ut_state) { @@ -792,6 +894,7 @@ main(void) cmocka_unit_test(test_generate_reset_packet_plain), cmocka_unit_test(test_generate_reset_packet_tls_auth), cmocka_unit_test(test_wkc_not_appended_without_key), + cmocka_unit_test(test_write_control_auth_out_buf), cmocka_unit_test(test_extract_control_message) };