[Openvpn-devel] Avoid passing NULL to argv_printf_cat() in temp_file error case.

Message ID 20201013204758.2472-1-gert@greenie.muc.de
State Accepted
Headers show
Series
  • [Openvpn-devel] Avoid passing NULL to argv_printf_cat() in temp_file error case.
Related show

Commit Message

Gert Doering Oct. 13, 2020, 8:47 p.m.
To pass username + password to verify_user_pass_script(), OpenVPN
can either put both into environment, or create a temp file, and
pass that file name to the "user-pass-verify" script.  The file
name is initialized as "", so if no file is desired, it's well
defined - but if the file can not be created, the pointer is NULL
afterwards.

Change the sequence of events, setting up the argv before the
"if (file)" conditional, and add the file name only inside that
clause, if creating the temp file succeeded.

commit a4eeef17b2 did not create the problem, but modified the
code enough so that the static analyzer in gcc 9.2.0 *now* noticed
and issued a warning.

 ssl_verify.c:1132:5: warning: '%s' directive argument is null
              1132 |     argv_printf_cat(&argv, "%s", tmp_file);

Signed-off-by: Gert Doering <gert@greenie.muc.de>
---
 src/openvpn/ssl_verify.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

David Sommerseth Oct. 15, 2020, 12:01 p.m. | #1
On 13/10/2020 22:47, Gert Doering wrote:
> To pass username + password to verify_user_pass_script(), OpenVPN
> can either put both into environment, or create a temp file, and
> pass that file name to the "user-pass-verify" script.  The file
> name is initialized as "", so if no file is desired, it's well
> defined - but if the file can not be created, the pointer is NULL
> afterwards.
> 
> Change the sequence of events, setting up the argv before the
> "if (file)" conditional, and add the file name only inside that
> clause, if creating the temp file succeeded.
> 
> commit a4eeef17b2 did not create the problem, but modified the
> code enough so that the static analyzer in gcc 9.2.0 *now* noticed
> and issued a warning.
> 
>  ssl_verify.c:1132:5: warning: '%s' directive argument is null
>               1132 |     argv_printf_cat(&argv, "%s", tmp_file);
> 
> Signed-off-by: Gert Doering <gert@greenie.muc.de>
> ---
>  src/openvpn/ssl_verify.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
I've glared at the code and related code paths, plus (only) compile
tested this change.  And it looks reasonable.

There are also at least two other ways to fix this issue, where this
approach is a bit more intrusive.  One would be to add an "if
(tmp_file)" wrapping around the argv_printf_cat() call, another would be
to set tmp_file to an empty string in the else block with the error
message at line 1122.

But after all, the chosen approach gives a reasonable code execution
flow and I consider it cleaner.  I don't see any reasons why it would be
beneficial to format the command line only after creating the temp file.

So ...

Acked-By: David Sommerseth <davids@openvpn.net>
Gert Doering Oct. 15, 2020, 1:37 p.m. | #2
Patch has been applied to the master and release/2.5 branch.

I have beaten this somewhat on the server torture bench, having
a config with plugin-auth-pam (deferred), auth-user-pass-verify *and*
auth-gen-token, testing both "via-env" (script-security 3 needed!)
and "via-file", testing script success and failure.

There might be a race between deferred plugin-auth and a failing
script - I saw something weird, but could not reproduce it (it might
have been "restarted the client too often, so the server log was
unclear on 'which message belongs to what instance'").

commit bbcada8abb410d077f7bc13b8157198b4bf6a3d1 (master)
commit 20965a18e52e60966ed754e6171e08ebe67432d6 (release/2.5)
Author: Gert Doering
Date:   Tue Oct 13 22:47:58 2020 +0200

     Avoid passing NULL to argv_printf_cat() in temp_file error case.

     Signed-off-by: Gert Doering <gert@greenie.muc.de>
     Acked-by: David Sommerseth <davids@openvpn.net>
     Message-Id: <20201013204758.2472-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21204.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index e19644c8..5c0aa6da 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -1098,6 +1098,9 @@  verify_user_pass_script(struct tls_session *session, struct tls_multi *multi,
     /* Set environmental variables prior to calling script */
     setenv_str(session->opt->es, "script_type", "user-pass-verify");
 
+    /* format command line */
+    argv_parse_cmd(&argv, session->opt->auth_user_pass_verify_script);
+
     if (session->opt->auth_user_pass_verify_script_via_file)
     {
         struct status_output *so;
@@ -1115,6 +1118,8 @@  verify_user_pass_script(struct tls_session *session, struct tls_multi *multi,
                     tmp_file);
                 goto done;
             }
+            /* pass temp file name to script */
+            argv_printf_cat(&argv, "%s", tmp_file);
         }
         else
         {
@@ -1127,10 +1132,6 @@  verify_user_pass_script(struct tls_session *session, struct tls_multi *multi,
         setenv_str(session->opt->es, "password", up->password);
     }
 
-    /* format command line */
-    argv_parse_cmd(&argv, session->opt->auth_user_pass_verify_script);
-    argv_printf_cat(&argv, "%s", tmp_file);
-
     /* call command */
     ret = openvpn_run_script(&argv, session->opt->es, 0,
                              "--auth-user-pass-verify");