Skip to contentSkip to conversation

Revamped the script-security warning logging

PATCH Applied David Sommerseth 2 participants 4 messages Apr 24, 2010 – Apr 30, 2010

lore
Share & tools
lore
#0 lore raw

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
index 94da09e..3ee3fc8 100644
@@ -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
index 19ac032..20a846a 100644
@@ -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
index 33e6762..79a28b0 100644
@@ -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
index bf51e89..fec5e1e 100644
@@ -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 unsign
 bool 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
index 342871a..054a9f1 100644
@@ -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
index e42ccb9..106df6f 100644
@@ -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
index d5230f7..5aa43e7 100644
@@ -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 *up
       argv_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
index eb94eb8..4de4139 100644
@@ -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 i
 	      ASSERT (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
#1 lore raw
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_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.

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.
#2 lore raw

-----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_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.

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-----

author Applied
#3 lore raw
In reply to David Sommerseth

-----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_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.

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-----