Browse code

Revamped the script-security warning logging (version 2)

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.

This is a new rewritten version which simplifies the implementaion of the new
openvpn_run_script() function. It was considered to remove it completely, but
due to code clearity and easy of use it was decided to make this function a static
inline function instead. Anyhow, this function will enforce openvpn_execve_check()
to be called with the S_SCRIPT flag.

Patch ACKed on the developers meeting 2009-04-29.

Signed-off-by: David Sommerseth <dazo@users.sourceforge.net>
Acked-by: James Yonan <james@openvpn.net>

David Sommerseth authored on 2010/04/30 06:35:45
Showing 8 changed files
... ...
@@ -97,6 +97,6 @@ typedef unsigned long ptr_type;
97 97
 /*
98 98
  * Script security warning
99 99
  */
100
-#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."
100
+#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."
101 101
 
102 102
 #endif
... ...
@@ -1191,7 +1191,7 @@ do_route (const struct options *options,
1191 1191
       struct argv argv = argv_new ();
1192 1192
       setenv_str (es, "script_type", "route-up");
1193 1193
       argv_printf (&argv, "%sc", options->route_script);
1194
-      openvpn_execve_check (&argv, es, S_SCRIPT, "Route script failed");
1194
+      openvpn_run_script (&argv, es, 0, "--route-up");
1195 1195
       argv_reset (&argv);
1196 1196
     }
1197 1197
 
... ...
@@ -229,7 +229,7 @@ run_up_down (const char *command,
229 229
 		  ifconfig_local, ifconfig_remote,
230 230
 		  context);
231 231
       argv_msg (M_INFO, &argv);
232
-      openvpn_execve_check (&argv, es, S_SCRIPT|S_FATAL, "script failed");
232
+      openvpn_run_script (&argv, es, S_FATAL, "--up/--down");
233 233
       argv_reset (&argv);
234 234
     }
235 235
 
... ...
@@ -492,6 +492,7 @@ openvpn_execve_allowed (const unsigned int flags)
492 492
     return script_security >= SSEC_BUILT_IN;
493 493
 }
494 494
 
495
+
495 496
 #ifndef WIN32
496 497
 /*
497 498
  * Run execve() inside a fork().  Designed to replicate the semantics of system() but
... ...
@@ -503,6 +504,7 @@ openvpn_execve (const struct argv *a, const struct env_set *es, const unsigned i
503 503
 {
504 504
   struct gc_arena gc = gc_new ();
505 505
   int ret = -1;
506
+  static bool warn_shown = false;
506 507
 
507 508
   if (a && a->argv[0])
508 509
     {
... ...
@@ -539,9 +541,10 @@ openvpn_execve (const struct argv *a, const struct env_set *es, const unsigned i
539 539
 	      ASSERT (0);
540 540
 	    }
541 541
 	}
542
-      else
542
+      else if (!warn_shown && (script_security < SSEC_SCRIPTS))
543 543
 	{
544 544
 	  msg (M_WARN, SCRIPT_SECURITY_WARNING);
545
+          warn_shown = true;
545 546
 	}
546 547
 #else
547 548
       msg (M_WARN, "openvpn_execve: execve function not available");
... ...
@@ -136,6 +136,15 @@ bool openvpn_execve_check (const struct argv *a, const struct env_set *es, const
136 136
 bool openvpn_execve_allowed (const unsigned int flags);
137 137
 int openvpn_system (const char *command, const struct env_set *es, unsigned int flags);
138 138
 
139
+static inline bool
140
+openvpn_run_script (const struct argv *a, const struct env_set *es, const unsigned int flags, const char *hook)
141
+{
142
+  char msg[256];
143
+
144
+  openvpn_snprintf(msg, sizeof(msg), "WARNING: Failed running command (%s)", hook);
145
+  return openvpn_execve_check(a, es, flags | S_SCRIPT, msg);
146
+};
147
+
139 148
 #ifdef HAVE_STRERROR
140 149
 /* a thread-safe version of strerror */
141 150
 const char* strerror_ts (int errnum, struct gc_arena *gc);
... ...
@@ -308,6 +317,7 @@ void get_user_pass_auto_userid (struct user_pass *up, const char *tag);
308 308
 extern const char *iproute_path;
309 309
 #endif
310 310
 
311
+/* Script security */
311 312
 #define SSEC_NONE      0 /* strictly no calling of external programs */
312 313
 #define SSEC_BUILT_IN  1 /* only call built-in programs such as ifconfig, route, netsh, etc.*/
313 314
 #define SSEC_SCRIPTS   2 /* allow calling of built-in programs and user-defined scripts */
... ...
@@ -109,7 +109,7 @@ learn_address_script (const struct multi_context *m,
109 109
 		   mroute_addr_print (addr, &gc));
110 110
       if (mi)
111 111
 	argv_printf_cat (&argv, "%s", tls_common_name (mi->context.c2.tls_multi, false));
112
-      if (!openvpn_execve_check (&argv, es, S_SCRIPT, "WARNING: learn-address command failed"))
112
+      if (!openvpn_run_script (&argv, es, 0, "--learn-address"))
113 113
 	ret = false;
114 114
       argv_reset (&argv);
115 115
     }
... ...
@@ -480,7 +480,7 @@ multi_client_disconnect_script (struct multi_context *m,
480 480
 	  struct argv argv = argv_new ();
481 481
 	  setenv_str (mi->context.c2.es, "script_type", "client-disconnect");
482 482
 	  argv_printf (&argv, "%sc", mi->context.options.client_disconnect_script);
483
-	  openvpn_execve_check (&argv, mi->context.c2.es, S_SCRIPT, "client-disconnect command failed");
483
+	  openvpn_run_script (&argv, mi->context.c2.es, 0, "--client-disconnect");
484 484
 	  argv_reset (&argv);
485 485
 	}
486 486
 #ifdef MANAGEMENT_DEF_AUTH
... ...
@@ -1594,7 +1594,7 @@ multi_connection_established (struct multi_context *m, struct multi_instance *mi
1594 1594
 		       mi->context.options.client_connect_script,
1595 1595
 		       dc_file);
1596 1596
 
1597
-	  if (openvpn_execve_check (&argv, mi->context.c2.es, S_SCRIPT, "client-connect command failed"))
1597
+	  if (openvpn_run_script (&argv, mi->context.c2.es, 0, "--client-connect"))
1598 1598
 	    {
1599 1599
 	      multi_client_connect_post (m, mi, dc_file, option_permissions_mask, &option_types_found);
1600 1600
 	      ++cc_succeeded_count;
... ...
@@ -1695,7 +1695,7 @@ link_socket_connection_initiated (const struct buffer *buf,
1695 1695
       struct argv argv = argv_new ();
1696 1696
       setenv_str (es, "script_type", "ipchange");
1697 1697
       ipchange_fmt (true, &argv, info, &gc);
1698
-      openvpn_execve_check (&argv, es, S_SCRIPT, "ip-change command failed");
1698
+      openvpn_run_script (&argv, es, 0, "--ipchange");
1699 1699
       argv_reset (&argv);
1700 1700
     }
1701 1701
 
... ...
@@ -983,7 +983,7 @@ verify_callback (int preverify_ok, X509_STORE_CTX * ctx)
983 983
 		   ctx->error_depth,
984 984
 		   subject);
985 985
       argv_msg_prefix (D_TLS_DEBUG, &argv, "TLS: executing verify command");
986
-      ret = openvpn_execve (&argv, opt->es, S_SCRIPT);
986
+      ret = openvpn_run_script (&argv, opt->es, 0, "--tls-verify script");
987 987
 
988 988
       if (opt->verify_export_cert)
989 989
         {
... ...
@@ -3344,7 +3344,7 @@ verify_user_pass_script (struct tls_session *session, const struct user_pass *up
3344 3344
       argv_printf (&argv, "%sc %s", session->opt->auth_user_pass_verify_script, tmp_file);
3345 3345
       
3346 3346
       /* call command */
3347
-      retval = openvpn_execve (&argv, session->opt->es, S_SCRIPT);
3347
+      retval = openvpn_run_script (&argv, session->opt->es, 0, "--auth-user-pass-verify");
3348 3348
 
3349 3349
       /* test return status of command */
3350 3350
       if (system_ok (retval))
... ...
@@ -952,6 +952,8 @@ int
952 952
 openvpn_execve (const struct argv *a, const struct env_set *es, const unsigned int flags)
953 953
 {
954 954
   int ret = -1;
955
+  static bool exec_warn = false;
956
+
955 957
   if (a && a->argv[0])
956 958
     {
957 959
       if (openvpn_execve_allowed (flags))
... ...
@@ -1002,9 +1004,10 @@ openvpn_execve (const struct argv *a, const struct env_set *es, const unsigned i
1002 1002
 	      ASSERT (0);
1003 1003
 	    }
1004 1004
 	}
1005
-      else
1005
+      else if (!exec_warn && (script_security < SSEC_SCRIPTS))
1006 1006
 	{
1007 1007
 	  msg (M_WARN, SCRIPT_SECURITY_WARNING);
1008
+          exec_warn = true;
1008 1009
 	}
1009 1010
     }
1010 1011
   else