| 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. | expand |
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>
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
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");
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(-)