Browse code

Remove the support for using system() when executing external programs or scripts

This patch removes the support for the system() call, and enforces the
usage of execve() on the *nix platform and CreateProcessW() on Windows.
This is to enhance the overall security when calling external scripts.
Using system() is prone to shell expansions, which may lead to security
breaches. Which is also why the execve() approach has been the default
since commit a82813527551f0e79c6d6ed5a9c1162e3c171bcf which
re-introduced the system() in Nov. 2008.

After having asked on the mailing list and checked around on the IRC
channels, the genereal consensus is that very few uses system() these
days.

The only annoyance I've been made aware of is that this will now
require adding a full path to the script interpreter together with the
script, and not just put in the script name alone. But to just use the
script name in Windows, you had to configure --script-security with the
'system' flag earlier too. So my conclusion is that it's better to add
a full path to the script interpreter in Windows and raise the overal
security with OpenVPN, than to continue to have a possible potentially
risky OpenVPN configuration just to make life "easier" for Windows
script users.

Removal of the system() call, also solves a nasty bug related to the
usage of putenv() on the *nix platforms.

For more information please see:
http://thread.gmane.org/gmane.network.openvpn.devel/7090
https://community.openvpn.net/openvpn/ticket/228

Trac-ticket: 228
Signed-off-by: David Sommerseth <davids@redhat.com>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <1351539352-17371-1-git-send-email-dazo@users.sourceforge.net>
URL: http://article.gmane.org/gmane.network.openvpn.devel/7114

David Sommerseth authored on 2012/10/25 21:22:30
Showing 8 changed files
... ...
@@ -1886,7 +1886,7 @@ is a safety precaution to prevent a LD_PRELOAD style attack
1886 1886
 from a malicious or compromised server.
1887 1887
 .\"*********************************************************
1888 1888
 .TP
1889
-.B \-\-script-security level [method]
1889
+.B \-\-script-security level
1890 1890
 This directive offers policy-level control over OpenVPN's usage of external programs
1891 1891
 and scripts.  Lower
1892 1892
 .B level
... ...
@@ -1905,24 +1905,40 @@ Allow calling of built-in executables and user-defined scripts.
1905 1905
 .B 3 \-\-
1906 1906
 Allow passwords to be passed to scripts via environmental variables (potentially unsafe).
1907 1907
 
1908
-The
1908
+OpenVPN releases before v2.3 also supported a
1909 1909
 .B method
1910
-parameter indicates how OpenVPN should call external commands and scripts.
1911
-Settings for
1912
-.B method:
1910
+flag which indicated how OpenVPN should call external commands and scripts.  This
1911
+could be either
1912
+.B execve
1913
+or 
1914
+.B system. 
1915
+As of OpenVPN v2.3, this flag is no longer accepted.  In most *nix environments the execve()
1916
+approach has been used without any issues.
1917
+
1918
+To run scripts in Windows in earlier OpenVPN
1919
+versions you needed to either add a full path to the script interpreter which can parse the
1920
+script or use the
1921
+.B system
1922
+flag to run these scripts.  As of OpenVPN v2.3 it is now a strict requirement to have
1923
+full path to the script interpreter when running non-executables files.
1924
+This is not needed for executable files, such as .exe, .com, .bat or .cmd files.  For
1925
+example, if you have a Visual Basic script, you must use this syntax now:
1913 1926
 
1914
-.B execve \-\-
1915
-(default) Use execve() function on Unix family OSes and CreateProcess() on Windows.
1916
-.br
1917
-.B system \-\-
1918
-Use system() function (deprecated and less safe since the external program command
1919
-line is subject to shell expansion).
1927
+.nf
1928
+.ft 3
1929
+.in +4
1930
+\-\-up 'C:\\\\Windows\\\\System32\\\\wscript.exe C:\\\\Program\\ Files\\\\OpenVPN\\\\config\\\\my-up-script.vbs'
1931
+.in -4
1932
+.ft
1933
+.fi
1920 1934
 
1921
-The
1922
-.B \-\-script-security
1923
-option was introduced in OpenVPN 2.1_rc9.  For configuration file compatibility
1924
-with previous OpenVPN versions, use:
1925
-.B \-\-script-security 3 system
1935
+Please note the single quote marks and the escaping of the backslashes (\\) and
1936
+the space character.
1937
+
1938
+The reason the support for the
1939
+.B system
1940
+flag was removed is due to the security implications with shell expansions
1941
+when executing scripts via the system() call.
1926 1942
 .\"*********************************************************
1927 1943
 .TP
1928 1944
 .B \-\-disable-occ
... ...
@@ -2487,9 +2487,6 @@ do_option_warnings (struct context *c)
2487 2487
     msg (M_WARN, "WARNING: the current --script-security setting may allow passwords to be passed to scripts via environmental variables");
2488 2488
   else
2489 2489
     msg (M_WARN, "NOTE: " PACKAGE_NAME " 2.1 requires '--script-security 2' or higher to call user-defined scripts or executables");
2490
-
2491
-  if (script_method == SM_SYSTEM)
2492
-    msg (M_WARN, "NOTE: --script-security method='system' is deprecated due to the fact that passed parameters will be subject to shell expansion");
2493 2490
 }
2494 2491
 
2495 2492
 static void
... ...
@@ -53,9 +53,6 @@ const char *iproute_path = IPROUTE_PATH; /* GLOBAL */
53 53
 /* contains an SSEC_x value defined in misc.h */
54 54
 int script_security = SSEC_BUILT_IN; /* GLOBAL */
55 55
 
56
-/* contains SM_x value defined in misc.h */
57
-int script_method = SM_EXECVE; /* GLOBAL */
58
-
59 56
 /*
60 57
  * Pass tunnel endpoint and MTU parms to a user-supplied script.
61 58
  * Used to execute the up/down script/plugins.
... ...
@@ -303,36 +300,25 @@ openvpn_execve (const struct argv *a, const struct env_set *es, const unsigned i
303 303
 #if defined(ENABLE_FEATURE_EXECVE)
304 304
       if (openvpn_execve_allowed (flags))
305 305
 	{
306
-	  if (script_method == SM_EXECVE)
307
-	    {
308
-	      const char *cmd = a->argv[0];
309
-	      char *const *argv = a->argv;
310
-	      char *const *envp = (char *const *)make_env_array (es, true, &gc);
311
-	      pid_t pid;
312
-
313
-	      pid = fork ();
314
-	      if (pid == (pid_t)0) /* child side */
315
-		{
316
-		  execve (cmd, argv, envp);
317
-		  exit (127);
318
-		}
319
-	      else if (pid < (pid_t)0) /* fork failed */
320
-		msg (M_ERR, "openvpn_execve: unable to fork");
321
-	      else /* parent side */
322
-		{
323
-		  if (waitpid (pid, &ret, 0) != pid)
324
-		    ret = -1;
325
-		}
326
-	    }
327
-	  else if (script_method == SM_SYSTEM)
328
-	    {
329
-	      ret = openvpn_system (argv_system_str (a), es, flags);
330
-	    }
331
-	  else
332
-	    {
333
-	      ASSERT (0);
334
-	    }
335
-	}
306
+          const char *cmd = a->argv[0];
307
+          char *const *argv = a->argv;
308
+          char *const *envp = (char *const *)make_env_array (es, true, &gc);
309
+          pid_t pid;
310
+
311
+          pid = fork ();
312
+          if (pid == (pid_t)0) /* child side */
313
+            {
314
+              execve (cmd, argv, envp);
315
+              exit (127);
316
+            }
317
+          else if (pid < (pid_t)0) /* fork failed */
318
+            msg (M_ERR, "openvpn_execve: unable to fork");
319
+          else /* parent side */
320
+            {
321
+              if (waitpid (pid, &ret, 0) != pid)
322
+                ret = -1;
323
+            }
324
+        }
336 325
       else if (!warn_shown && (script_security < SSEC_SCRIPTS))
337 326
 	{
338 327
 	  msg (M_WARN, SCRIPT_SECURITY_WARNING);
... ...
@@ -353,52 +339,6 @@ openvpn_execve (const struct argv *a, const struct env_set *es, const unsigned i
353 353
 #endif
354 354
 
355 355
 /*
356
- * Wrapper around the system() call.
357
- */
358
-int
359
-openvpn_system (const char *command, const struct env_set *es, unsigned int flags)
360
-{
361
-#ifdef HAVE_SYSTEM
362
-  int ret;
363
-
364
-  perf_push (PERF_SCRIPT);
365
-
366
-  /*
367
-   * add env_set to environment.
368
-   */
369
-  if (flags & S_SCRIPT)
370
-    env_set_add_to_environment (es);
371
-
372
-
373
-  /* debugging */
374
-  dmsg (D_SCRIPT, "SYSTEM[%u] '%s'", flags, command);
375
-  if (flags & S_SCRIPT)
376
-    env_set_print (D_SCRIPT, es);
377
-
378
-  /*
379
-   * execute the command
380
-   */
381
-  ret = platform_system(command);
382
-
383
-  /* debugging */
384
-  dmsg (D_SCRIPT, "SYSTEM return=%u", ret);
385
-
386
-  /*
387
-   * remove env_set from environment
388
-   */
389
-  if (flags & S_SCRIPT)
390
-    env_set_remove_from_environment (es);
391
-
392
-  perf_pop ();
393
-  return ret;
394
-
395
-#else
396
-  msg (M_FATAL, "Sorry but I can't execute the shell command '%s' because this operating system doesn't appear to support the system() call", command);
397
-  return -1; /* NOTREACHED */
398
-#endif
399
-}
400
-
401
-/*
402 356
  * Run execve() inside a fork(), duping stdout.  Designed to replicate the semantics of popen() but
403 357
  * in a safer way that doesn't require the invocation of a shell or the risks
404 358
  * assocated with formatting and parsing a command line.
... ...
@@ -96,7 +96,6 @@ int openvpn_popen (const struct argv *a,  const struct env_set *es);
96 96
 int openvpn_execve (const struct argv *a, const struct env_set *es, const unsigned int flags);
97 97
 bool openvpn_execve_check (const struct argv *a, const struct env_set *es, const unsigned int flags, const char *error_message);
98 98
 bool openvpn_execve_allowed (const unsigned int flags);
99
-int openvpn_system (const char *command, const struct env_set *es, unsigned int flags);
100 99
 
101 100
 static inline bool
102 101
 openvpn_run_script (const struct argv *a, const struct env_set *es, const unsigned int flags, const char *hook)
... ...
@@ -322,10 +321,6 @@ extern const char *iproute_path;
322 322
 #define SSEC_PW_ENV    3 /* allow calling of built-in programs and user-defined scripts that may receive a password as an environmental variable */
323 323
 extern int script_security; /* GLOBAL */
324 324
 
325
-#define SM_EXECVE 0      /* call external programs with execve() or CreateProcess() */
326
-#define SM_SYSTEM 1      /* call external programs with system() */
327
-extern int script_method; /* GLOBAL */
328
-
329 325
 /* return the next largest power of 2 */
330 326
 size_t adjust_power_of_2 (size_t u);
331 327
 
... ...
@@ -248,7 +248,7 @@ static const char usage_message[] =
248 248
   "--setenv name value : Set a custom environmental variable to pass to script.\n"
249 249
   "--setenv FORWARD_COMPATIBLE 1 : Relax config file syntax checking to allow\n"
250 250
   "                  directives for future OpenVPN versions to be ignored.\n"
251
-  "--script-security level mode : mode='execve' (default) or 'system', level=\n"
251
+  "--script-security level: Where level can be:\n"
252 252
   "                  0 -- strictly no calling of external programs\n"
253 253
   "                  1 -- (default) only call built-ins such as ifconfig\n"
254 254
   "                  2 -- allow calling of built-ins and scripts\n"
... ...
@@ -5293,20 +5293,6 @@ add_option (struct options *options,
5293 5293
     {
5294 5294
       VERIFY_PERMISSION (OPT_P_GENERAL);
5295 5295
       script_security = atoi (p[1]);
5296
-      if (p[2])
5297
-	{
5298
-	  if (streq (p[2], "execve"))
5299
-	    script_method = SM_EXECVE;
5300
-	  else if (streq (p[2], "system"))
5301
-	    script_method = SM_SYSTEM;
5302
-	  else
5303
-	    {
5304
-	      msg (msglevel, "unknown --script-security method: %s", p[2]);
5305
-	      goto err;
5306
-	    }
5307
-	}
5308
-      else
5309
-	script_method = SM_EXECVE;
5310 5296
     }
5311 5297
   else if (streq (p[0], "mssfix"))
5312 5298
     {
... ...
@@ -205,7 +205,7 @@ platform_chdir (const char* dir)
205 205
 }
206 206
 
207 207
 /*
208
- * convert system() return into a success/failure value
208
+ * convert execve() return into a success/failure value
209 209
  */
210 210
 bool
211 211
 platform_system_ok (int stat)
... ...
@@ -217,19 +217,6 @@ platform_system_ok (int stat)
217 217
 #endif
218 218
 }
219 219
 
220
-/*
221
- * did system() call execute the given command?
222
- */
223
-bool
224
-platform_system_executed (int stat)
225
-{
226
-#ifdef WIN32
227
-  return stat != -1;
228
-#else
229
-  return stat != -1 && WEXITSTATUS (stat) != 127;
230
-#endif
231
-}
232
-
233 220
 int
234 221
 platform_access (const char *path, int mode)
235 222
 {
... ...
@@ -288,18 +275,6 @@ platform_unlink (const char *filename)
288 288
 #endif
289 289
 }
290 290
 
291
-int platform_system(const char *command) {
292
-  int ret;
293
-#ifdef WIN32
294
-  struct gc_arena gc = gc_new ();
295
-  ret = _wsystem (wide_string (command, &gc));
296
-  gc_free (&gc);
297
-#else
298
-  ret = system (command);
299
-#endif
300
-  return ret;
301
-}
302
-
303 291
 int platform_putenv(char *string)
304 292
 {
305 293
   int status;
... ...
@@ -113,10 +113,8 @@ void platform_mlockall (bool print_msg); /* Disable paging */
113 113
 
114 114
 int platform_chdir (const char* dir);
115 115
 
116
-/* interpret the status code returned by system()/execve() */
116
+/* interpret the status code returned by execve() */
117 117
 bool platform_system_ok (int stat);
118
-bool platform_system_executed (int stat);
119
-int platform_system(const char *command);
120 118
 
121 119
 int platform_access (const char *path, int mode);
122 120
 
... ...
@@ -82,51 +82,6 @@ struct semaphore netcmd_semaphore; /* GLOBAL */
82 82
  */
83 83
 static char *win_sys_path = NULL; /* GLOBAL */
84 84
 
85
-/*
86
- * Configure PATH.  On Windows, sometimes PATH is not set correctly
87
- * by default.
88
- */
89
-static void
90
-configure_win_path (void)
91
-{
92
-  static bool done = false; /* GLOBAL */
93
-  if (!done)
94
-    {
95
-      FILE *fp;
96
-      fp = fopen ("c:\\windows\\system32\\route.exe", "rb");
97
-      if (fp)
98
-	{
99
-	  const int bufsiz = 4096;
100
-	  struct gc_arena gc = gc_new ();
101
-	  struct buffer oldpath = alloc_buf_gc (bufsiz, &gc);
102
-	  struct buffer newpath = alloc_buf_gc (bufsiz, &gc);
103
-	  const char* delim = ";";
104
-	  DWORD status;
105
-	  fclose (fp);
106
-	  status = GetEnvironmentVariable ("PATH", BPTR(&oldpath), (DWORD)BCAP(&oldpath));
107
-#if 0
108
-	  status = 0;
109
-#endif
110
-	  if (!status)
111
-	    {
112
-	      *BPTR(&oldpath) = '\0';
113
-	      delim = "";
114
-	    }
115
-	  buf_printf (&newpath, "C:\\WINDOWS\\System32;C:\\WINDOWS;C:\\WINDOWS\\System32\\Wbem%s%s",
116
-		      delim,
117
-		      BSTR(&oldpath));
118
-	  SetEnvironmentVariable ("PATH", BSTR(&newpath));
119
-#if 0
120
-	  status = GetEnvironmentVariable ("PATH", BPTR(&oldpath), (DWORD)BCAP(&oldpath));
121
-	  if (status > 0)
122
-	    printf ("PATH: %s\n", BSTR(&oldpath));
123
-#endif
124
-	  gc_free (&gc);
125
-	  done = true;
126
-	}
127
-    }
128
-}
129
-
130 85
 void
131 86
 init_win32 (void)
132 87
 {
... ...
@@ -907,53 +862,41 @@ openvpn_execve (const struct argv *a, const struct env_set *es, const unsigned i
907 907
     {
908 908
       if (openvpn_execve_allowed (flags))
909 909
 	{
910
-	  if (script_method == SM_EXECVE)
911
-	    {
912
-	      struct gc_arena gc = gc_new ();
913
-	      STARTUPINFOW start_info;
914
-	      PROCESS_INFORMATION proc_info;
915
-
916
-	      char *env = env_block (es);
917
-	      WCHAR *cl = wide_cmd_line (a, &gc);
918
-	      WCHAR *cmd = wide_string (a->argv[0], &gc);
919
-
920
-	      CLEAR (start_info);
921
-	      CLEAR (proc_info);
922
-
923
-	      /* fill in STARTUPINFO struct */
924
-	      GetStartupInfoW(&start_info);
925
-	      start_info.cb = sizeof(start_info);
926
-	      start_info.dwFlags = STARTF_USESHOWWINDOW;
927
-	      start_info.wShowWindow = SW_HIDE;
928
-
929
-	      if (CreateProcessW (cmd, cl, NULL, NULL, FALSE, 0, env, NULL, &start_info, &proc_info))
930
-		{
931
-		  DWORD exit_status = 0;
932
-		  CloseHandle (proc_info.hThread);
933
-		  WaitForSingleObject (proc_info.hProcess, INFINITE);
934
-		  if (GetExitCodeProcess (proc_info.hProcess, &exit_status))
935
-		    ret = (int)exit_status;
936
-		  else
937
-		    msg (M_WARN|M_ERRNO, "openvpn_execve: GetExitCodeProcess %S failed", cmd);
938
-		  CloseHandle (proc_info.hProcess);
939
-		}
940
-	      else
941
-		{
942
-		  msg (M_WARN|M_ERRNO, "openvpn_execve: CreateProcess %S failed", cmd);
943
-		}
944
-	      free (env);
945
-	      gc_free (&gc);
946
-	    }
947
-	  else if (script_method == SM_SYSTEM)
948
-	    {
949
-	      configure_win_path ();
950
-	      ret = openvpn_system (argv_system_str (a), es, flags);
951
-	    }
952
-	  else
953
-	    {
954
-	      ASSERT (0);
955
-	    }
956
-	}
910
+          struct gc_arena gc = gc_new ();
911
+          STARTUPINFOW start_info;
912
+          PROCESS_INFORMATION proc_info;
913
+
914
+          char *env = env_block (es);
915
+          WCHAR *cl = wide_cmd_line (a, &gc);
916
+          WCHAR *cmd = wide_string (a->argv[0], &gc);
917
+
918
+          CLEAR (start_info);
919
+          CLEAR (proc_info);
920
+
921
+          /* fill in STARTUPINFO struct */
922
+          GetStartupInfoW(&start_info);
923
+          start_info.cb = sizeof(start_info);
924
+          start_info.dwFlags = STARTF_USESHOWWINDOW;
925
+          start_info.wShowWindow = SW_HIDE;
926
+
927
+          if (CreateProcessW (cmd, cl, NULL, NULL, FALSE, 0, env, NULL, &start_info, &proc_info))
928
+            {
929
+              DWORD exit_status = 0;
930
+              CloseHandle (proc_info.hThread);
931
+              WaitForSingleObject (proc_info.hProcess, INFINITE);
932
+              if (GetExitCodeProcess (proc_info.hProcess, &exit_status))
933
+                ret = (int)exit_status;
934
+              else
935
+                msg (M_WARN|M_ERRNO, "openvpn_execve: GetExitCodeProcess %S failed", cmd);
936
+              CloseHandle (proc_info.hProcess);
937
+            }
938
+          else
939
+            {
940
+              msg (M_WARN|M_ERRNO, "openvpn_execve: CreateProcess %S failed", cmd);
941
+            }
942
+          free (env);
943
+          gc_free (&gc);
944
+        }
957 945
       else if (!exec_warn && (script_security < SSEC_SCRIPTS))
958 946
 	{
959 947
 	  msg (M_WARN, SCRIPT_SECURITY_WARNING);