diff mbox series

[Openvpn-devel,2/8] ssl_verify: Fix memleak if creating deferred auth control files fails

Message ID 20221215190143.2107896-3-arne@rfc2549.org
State Accepted
Headers show
Series Improvement/fixes based on Trail of Bits audit | expand

Commit Message

Arne Schwabe Dec. 15, 2022, 7:01 p.m. UTC 
From: David Sommerseth <davids@openvpn.net>

If the key_state_gen_auth_control_files() call fails, the code would
just return without freeing the argv container.  Instead the code should
jump to an appropriate exit point where memory is being released.

Also adjust the related comment, to indicate that these deferred auth
control files are really pre-created.

Signed-off-by: David Sommerseth <davids@openvpn.net>

Reported-by: Trial of Bits (TOB-OVPN-2)
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/ssl_verify.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Arne Schwabe Dec. 15, 2022, 10:52 p.m. UTC | #1 
Am 15.12.2022 um 20:01 schrieb Arne Schwabe:
> From: David Sommerseth <davids@openvpn.net>
>
> If the key_state_gen_auth_control_files() call fails, the code would
> just return without freeing the argv container.  Instead the code should
> jump to an appropriate exit point where memory is being released.
>
> Also adjust the related comment, to indicate that these deferred auth
> control files are really pre-created.
>
> Signed-off-by: David Sommerseth <davids@openvpn.net>
>
> Reported-by: Trial of Bits (TOB-OVPN-2)

Typo here: Should be Trail of Bits (my mistake)
Gert Doering Dec. 16, 2022, 8:47 a.m. UTC | #2 
Acked-by: Gert Doering <gert@greenie.muc.de>

As agreed on the security@ list - this covers all possible leaks, and
is fully "normal OpenVPN style".  I didn't test actual file creation
failures, but I *did* test regular server operation with plugins and
scripts, and that all still works fine.

As instructed, I've fixed the "Trial" to read "Trail of Bits" :-)

Your patch has been applied to the master and release/2.6 branch.

release/2.5 and older do not contain this code (no async/deferred 
--verify-auth-user-pass scripts yet) - that was only added in 2021
via commit 28e6103096ae8.

commit 0567da5377704cf64bd2599f2d49aa478d386941 (master)
commit cdfdfb3da0ce714f43b23f679a8ef9b36ab9f370 (release/2.6)
Author: David Sommerseth
Date:   Thu Dec 15 20:01:37 2022 +0100

     ssl_verify: Fix memleak if creating deferred auth control files fails

     Signed-off-by: David Sommerseth <davids@openvpn.net>
     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20221215190143.2107896-3-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25737.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 76cb9f19b..228cf16e1 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -1358,12 +1358,13 @@  verify_user_pass_script(struct tls_session *session, struct tls_multi *multi,
         setenv_str(session->opt->es, "password", up->password);
     }
 
-    /* generate filename for deferred auth control file */
+    /* pre-create files for deferred auth control */
     if (!key_state_gen_auth_control_files(&ks->script_auth, session->opt))
     {
         msg(D_TLS_ERRORS, "TLS Auth Error (%s): "
             "could not create deferred auth control file", __func__);
-        return OPENVPN_PLUGIN_FUNC_ERROR;
+        retval = OPENVPN_PLUGIN_FUNC_ERROR;
+        goto error;
     }
 
     /* call command */
@@ -1412,6 +1413,7 @@  done:
         platform_unlink(tmp_file);
     }
 
+error:
     argv_free(&argv);
     gc_free(&gc);
     return retval;