Revamped the script-security warning logging
PATCH 1/1 Revamped the script-security warning logging
From: David Sommerseth <[email protected]...>
This is a first-cut of removing misleading warnings from the logs.
The main task of this patch is to avoid reporting the SCRIPT_SECURITY_WARNING over and over again, in addition to not show this warning when it should not be a problem. This general warning should now only appear once, and only when --script-security is not set, 0 or 1. In all other cases this warning should not appear.
In addition, this warning will come close to the script-hook which most probably will fail. It will also give a little bit more concrete hint on which script-hook which failed. If --script-security is 2 or 3, only the execve failure itself will be shown. This message will on the other hand be shown repeatedly.
--- common.h | 2 +- init.c | 2 +- misc.c | 25 ++++++++++++++++++++++--- misc.h | 3 +++ multi.c | 6 +++--- socket.c | 2 +- ssl.c | 4 ++-- win32.c | 5 ++++- 8 files changed, 37 insertions(+), 12 deletions(-)
common.h +1 -1
@@ -90,6 +90,6 @@ typedef unsigned long ptr_type;/* * Script security warning */ -#define SCRIPT_SECURITY_WARNING "openvpn_execve: external program may not be called unless '--script-security 2' or higher is enabled. Use '--script-security 3 system' for backward compatibility with 2.1_rc8 and earlier. See --help text or man page for detailed info." +#define SCRIPT_SECURITY_WARNING "WARNING: External program may not be called unless '--script-security 2' or higher is enabled. Use '--script-security 3 system' for backward compatibility with 2.1_rc8 and earlier. See --help text or man page for detailed info."#endif
init.c +1 -1
@@ -975,7 +975,7 @@ do_route (const struct options *options,struct argv argv =argv_new ();setenv_str (es,"script_type" ,"route-up" );argv_printf (&argv,"%sc" , options->route_script); -openvpn_execve_check (&argv, es,S_SCRIPT ,"Route script failed" ); +openvpn_run_script (&argv, es,S_SCRIPT ,"--route-up" );argv_reset (&argv); }
misc.c +22 -3
@@ -230,7 +230,7 @@ run_up_down (const char *command, ifconfig_local, ifconfig_remote, context);argv_msg (M_INFO , &argv); -openvpn_execve_check (&argv, es,S_SCRIPT |S_FATAL ,"script failed" ); +openvpn_run_script (&argv, es,S_SCRIPT |S_FATAL ,"--up/--down" );argv_reset (&argv); }
@@ -485,6 +485,22 @@ openvpn_execve_check (const struct argv *a, const struct env_set *es, const unsi }bool +openvpn_run_script (const struct argv *a,const struct env_set *es,const unsigned int flags,const char *hook) +{ +static bool script_warning = false; +char msg[256 ]; + +if (!script_warning && (script_security <SSEC_SCRIPTS ) ) + { +msg (M_WARN ,"[2] " SCRIPT_SECURITY_WARNING ); + script_warning = true; + } + +openvpn_snprintf (msg,255 ,"WARNING: Failed running command (%s)" , hook); +return openvpn_execve_check (a, es, flags |S_SCRIPT_NOWARN , msg); +} + +bool openvpn_execve_allowed (const unsigned int flags) {if (flags &S_SCRIPT )
@@ -493,6 +509,7 @@ openvpn_execve_allowed (const unsigned int flags)return script_security >=SSEC_BUILT_IN ; } +#ifndef WIN32 /* * Run execve() inside a fork(). Designed to replicate the semantics of system() but
@@ -504,6 +521,7 @@ openvpn_execve (const struct argv *a, const struct env_set *es, const unsigned i {struct gc_arena gc =gc_new ();int ret =-1 ; +static bool exec_warn = false;if (a && a->argv[0 ]) {
@@ -540,9 +558,10 @@ openvpn_execve (const struct argv *a, const struct env_set *es, const unsigned i ASSERT (0); } } -else +else if (!(flags &S_SCRIPT_NOWARN ) && !exec_warn && (script_security <SSEC_SCRIPTS )) { -msg (M_WARN ,SCRIPT_SECURITY_WARNING ); +msg (M_WARN ,"[1] " SCRIPT_SECURITY_WARNING ); + exec_warn = true; }#else msg (M_WARN ,"openvpn_execve: execve function not available" );
misc.h +3 -0
@@ -124,6 +124,7 @@ void warn_if_group_others_accessible(const char* filename);/* system flags */ #define S_SCRIPT (1<<0)#define S_FATAL (1<<1) +#define S_SCRIPT_NOWARN (1<<2)/* interpret the status code returned by system()/execve() */ bool system_ok (int );
@@ -135,6 +136,7 @@ int openvpn_execve (const struct argv *a, const struct env_set *es, const unsignbool openvpn_execve_check (const struct argv *a,const struct env_set *es,const unsigned int flags,const char *error_message);bool openvpn_execve_allowed (const unsigned int flags);int openvpn_system (const char *command,const struct env_set *es,unsigned int flags); +bool openvpn_run_script (const struct argv *a,const struct env_set *es,const unsigned int flags,const char *hook);#ifdef HAVE_STRERROR /* a thread-safe version of strerror */
@@ -303,6 +305,7 @@ void get_user_pass_auto_userid (struct user_pass *up, const char *tag);extern const char *iproute_path;#endif +/* Script security */ #define SSEC_NONE 0/* strictly no calling of external programs */ #define SSEC_BUILT_IN 1/* only call built-in programs such as ifconfig, route, netsh, etc.*/ #define SSEC_SCRIPTS 2/* allow calling of built-in programs and user-defined scripts */
multi.c +3 -3
@@ -109,7 +109,7 @@ learn_address_script (const struct multi_context *m, mroute_addr_print (addr, &gc));if (mi)argv_printf_cat (&argv,"%s" ,tls_common_name (mi->context.c2.tls_multi, false)); -if (!openvpn_execve_check (&argv, es,S_SCRIPT ,"WARNING: learn-address command failed" )) +if (!openvpn_run_script (&argv, es,S_SCRIPT ,"--learn-address" )) ret = false;argv_reset (&argv); }
@@ -480,7 +480,7 @@ multi_client_disconnect_script (struct multi_context *m,struct argv argv =argv_new ();setenv_str (mi->context.c2.es,"script_type" ,"client-disconnect" );argv_printf (&argv,"%sc" , mi->context.options.client_disconnect_script); -openvpn_execve_check (&argv, mi->context.c2.es,S_SCRIPT ,"client-disconnect command failed" ); +openvpn_run_script (&argv, mi->context.c2.es,S_SCRIPT ,"--client-disconnect" );argv_reset (&argv); }#ifdef MANAGEMENT_DEF_AUTH
@@ -1586,7 +1586,7 @@ multi_connection_established (struct multi_context *m, struct multi_instance *mi mi->context.options.client_connect_script, dc_file); -if (openvpn_execve_check (&argv, mi->context.c2.es,S_SCRIPT ,"client-connect command failed" )) +if (openvpn_run_script (&argv, mi->context.c2.es,S_SCRIPT ,"--client-connect" )) {multi_client_connect_post (m, mi, dc_file, option_permissions_mask, &option_types_found); ++cc_succeeded_count;
socket.c +1 -1
@@ -1663,7 +1663,7 @@ link_socket_connection_initiated (const struct buffer *buf,struct argv argv =argv_new ();setenv_str (es,"script_type" ,"ipchange" );ipchange_fmt (true, &argv, info, &gc); -openvpn_execve_check (&argv, es,S_SCRIPT ,"ip-change command failed" ); +openvpn_run_script (&argv, es,S_SCRIPT ,"--ipchange" );argv_reset (&argv); }
ssl.c +2 -2
@@ -948,7 +948,7 @@ verify_callback (int preverify_ok, X509_STORE_CTX * ctx) ctx->error_depth, subject);argv_msg_prefix (D_TLS_DEBUG , &argv,"TLS: executing verify command" ); - ret =openvpn_execve (&argv, opt->es,S_SCRIPT ); + ret =openvpn_run_script (&argv, opt->es,S_SCRIPT ,"--tls-verify script" );if (opt->verify_export_cert) {
@@ -3232,7 +3232,7 @@ verify_user_pass_script (struct tls_session *session, const struct user_pass *upargv_printf (&argv,"%sc %s" , session->opt->auth_user_pass_verify_script, tmp_file);/* call command */ - retval =openvpn_execve (&argv, session->opt->es,S_SCRIPT ); + retval =openvpn_run_script (&argv, session->opt->es,S_SCRIPT ,"--auth-user-pass-verify" );/* test return status of command */ if (system_ok (retval))
win32.c +4 -1
@@ -952,6 +952,8 @@ int openvpn_execve (const struct argv *a, const struct env_set *es, const unsigned int flags) { int ret = -1; +static bool exec_warn = false; +if (a && a->argv[0 ]) {if (openvpn_execve_allowed (flags))
@@ -1004,9 +1006,10 @@ openvpn_execve (const struct argv *a, const struct env_set *es, const unsigned iASSERT (0 ); } } -else +else if (!exec_warn && (script_security <SSEC_SCRIPTS )) {msg (M_WARN ,SCRIPT_SECURITY_WARNING ); + exec_warn = true; } }else
Signature · 1 line
1.6.6.1
Quoted 10 lines · David Sommerseth
From: David Sommerseth <[email protected]...>
This is a first-cut of removing misleading warnings from the logs.
The main task of this patch is to avoid reporting the
SCRIPT_SECURITY_WARNINGover and over again, in addition to not show this warning when it should not be a problem. This general warning should now only appear once, and only when --script-security is not set, 0 or 1. In all other cases this warning should not appear.
I haven't tested it yet (so please ignore this message if I'm wrong), but then IIUC you still get the warning even if /no/ script at all is defined in the configuration (ie, no "up", no "down", etc.). I imagine that getting a warning in that case would be even more misleading!
Signature · 1 line
D.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 12 lines · Davide Brini
Quoted 10 lines · David Sommerseth
From: David Sommerseth <[email protected]...>
This is a first-cut of removing misleading warnings from the logs.
The main task of this patch is to avoid reporting the
SCRIPT_SECURITY_WARNINGover and over again, in addition to not show this warning when it should not be a problem. This general warning should now only appear once, and only when --script-security is not set, 0 or 1. In all other cases this warning should not appear.
Quoted 5 lines
I haven't tested it yet (so please ignore this message if I'm wrong), but then IIUC you still get the warning even if /no/ script at all is defined in the configuration (ie, no "up", no "down", etc.). I imagine that getting a warning in that case would be even more misleading!
No problem. But I can't resist to answer ;-)
You should not get any warnings related to using --script-script security 2 or 3 at all if you are not using any of the 9 script hooks I've identified [1]. All those places in the code now will now use a new function called openvpn_run_script() (--up/--down scripts actually use the same entry point, so you'll find that function 8 places in the code now).
But you *might* still get that warning if your installation executes the 'route' command. All functions calling openvpn_execve() will print this warning the first time this function is called. And it should only be printed if --script-security is 0 or 1.
And I'm not perfect ... it might be I've overseen something in this patch. Anyhow I'm considering to rewrite it again, by not using the openvpn_run_script() function and rather just set a flag to openvpn_execve() those places where openvpn_run_script() is called now.
It struck me a few hours ago that such approach might be somewhat cleaner.
Anyhow, thank you for your comments! :) I like to get critical questions back on my patches :)
kind regards,
David Sommerseth
[1] Script hooks I've identified:
--up, --tls-verify, --ipchange, --client-connect, --route-up, --client-disconnect, --down, --learn-address, --auth-user-pass-verify
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAkvTHFIACgkQDC186MBRfrpLSQCfRIB8adtYuKXlk5A2zukhFnYO kKAAoJeNKO9vPSQlXybFv0K1+PeFb8fn
=OWf1
-----END PGP SIGNATURE-----
David Sommerseth
Applied-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 16 lines · David Sommerseth
From: David Sommerseth <[email protected]...>
This is a first-cut of removing misleading warnings from the logs.
The main task of this patch is to avoid reporting the
SCRIPT_SECURITY_WARNINGover and over again, in addition to not show this warning when it should not be a problem. This general warning should now only appear once, and only when --script-security is not set, 0 or 1. In all other cases this warning should not appear.In addition, this warning will come close to the script-hook which most probably will fail. It will also give a little bit more concrete hint on which script-hook which failed. If --script-security is 2 or 3, only the execve failure itself will be shown. This message will on the other hand be shown repeatedly.
Signed-off-by David Sommerseth <[email protected]...>
Quoted 10 lines
--- common.h | 2 +- init.c | 2 +- misc.c | 25 ++++++++++++++++++++++--- misc.h | 3 +++ multi.c | 6 +++--- socket.c | 2 +- ssl.c | 4 ++-- win32.c | 5 ++++- 8 files changed, 37 insertions(+), 12 deletions(-)
This patch received an ACK on the developers meeting 2010-04-29
<http://thread.gmane.org/gmane.network.openvpn.devel/3673>
Applied to feat_misc and merged into allmerged.
Commit 339f2a4d4b487afa53fa99d72c35b16f31e417d3
kind regards,
David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAkvbTWcACgkQDC186MBRfrqOcACgqZpBjyXOiCIsguEsXtKE1uVJ dQoAnRG2IuNETVCG+d52saKWSTlRzwap
=brqt
-----END PGP SIGNATURE-----