Message-ID: <508F8268.4020808@...515...> Date: Tue, 30 Oct 2012 09:31:52 +0200 From: =?ISO-8859-1?Q?Samuli_Sepp=E4nen?= MIME-Version: 1.0 References: <1351539352-17371-1-git-send-email-dazo@...384...> In-Reply-To: <1351539352-17371-1-git-send-email-dazo@...384...> Subject: Re: [Openvpn-devel] [PATCH] Remove the support for using system() when executing external programs or scripts List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: quoted-printable To: David Sommerseth Cc: "openvpn-devel@lists.sourceforge.net" I did some tests yesterday and this syntax works with arbitrary interpreters and scripts: script-security 2 up 'c:\\Windows\\System32\\wscript.exe c:\\Program\ Files\\OpenVPN\\config\\test.vbs' For test details, look here: --=20 Samuli Sepp=E4nen Community Manager OpenVPN Technologies, Inc irc freenode net: mattock > From: David Sommerseth > > This patch removes the support for the system() call, and enforces the us= age 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() appr= oach has > been the default since commit a82813527551f0e79c6d6ed5a9c1162e3c171bcf wh= ich > re-introduced the system() in Nov. 2008. > > After having asked on the mailing list and checked around on the IRC chan= nels, 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 t= o configure > --script-security with the 'system' flag earlier too. So my conclusion i= s 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 ri= sky > OpenVPN configuration just to make life "easier" for Windows script users. > > Removal of the system() call, also solves a nasty bug related to the usag= e 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 > --- > doc/openvpn.8 | 48 ++++++++++++------ > src/openvpn/init.c | 3 -- > src/openvpn/misc.c | 98 ++++++++----------------------------- > src/openvpn/misc.h | 5 -- > src/openvpn/options.c | 16 +----- > src/openvpn/platform.c | 27 +--------- > src/openvpn/platform.h | 4 +- > src/openvpn/win32.c | 127 +++++++++++++-----------------------------= ------ > 8 files changed, 89 insertions(+), 239 deletions(-) > > diff --git a/doc/openvpn.8 b/doc/openvpn.8 > index aa653ec..2ed5201 100644 > --- a/doc/openvpn.8 > +++ b/doc/openvpn.8 > @@ -1886,7 +1886,7 @@ is a safety precaution to prevent a LD_PRELOAD styl= e attack > from a malicious or compromised server. > .\"********************************************************* > .TP > -.B \-\-script-security level [method] > +.B \-\-script-security level > This directive offers policy-level control over OpenVPN's usage of exter= nal programs > and scripts. Lower > .B level > @@ -1905,24 +1905,40 @@ Allow calling of built-in executables and user-de= fined scripts. > .B 3 \-\- > Allow passwords to be passed to scripts via environmental variables (pot= entially unsafe). > > -The > +OpenVPN releases before v2.3 also supported a > .B method > -parameter indicates how OpenVPN should call external commands and script= s. > -Settings for > -.B method: > +flag which indicated how OpenVPN should call external commands and scrip= ts. This > +could be either > +.B execve > +or > +.B system. > +As of OpenVPN v2.3, this flag is no longer accepted. In most *nix envir= onments the execve() > +approach has been used without any issues. > + > +To run scripts in Windows in earlier OpenVPN > +versions you needed to either add a full path to the script interpreter = which can parse the > +script or use the > +.B system > +flag to run these scripts. As of OpenVPN v2.3 it is now a strict requir= ement to have > +full path to the script interpreter when running non-executables files. > +This is not needed for executable files, such as .exe, .com, .bat or .cm= d files. For > +example, if you have a Visual Basic script, you must use this syntax now: > > -.B execve \-\- > -(default) Use execve() function on Unix family OSes and CreateProcess() = on Windows. > -.br > -.B system \-\- > -Use system() function (deprecated and less safe since the external progr= am command > -line is subject to shell expansion). > +.nf > +.ft 3 > +.in +4 > +\-\-up 'C:\\\\Windows\\\\System32\\\\wscript.exe C:\\\\Program\\ Files\\= \\OpenVPN\\\\config\\\\my-up-script.vbs' > +.in -4 > +.ft > +.fi > > -The > -.B \-\-script-security > -option was introduced in OpenVPN 2.1_rc9. For configuration file compat= ibility > -with previous OpenVPN versions, use: > -.B \-\-script-security 3 system > +Please note the single quote marks and the escaping of the backslashes (= \\) and > +the space character. > + > +The reason the support for the > +.B system > +flag was removed is due to the security implications with shell expansio= ns > +when executing scripts via the system() call. > .\"********************************************************* > .TP > .B \-\-disable-occ > diff --git a/src/openvpn/init.c b/src/openvpn/init.c > index 270ee6a..25d8225 100644 > --- a/src/openvpn/init.c > +++ b/src/openvpn/init.c > @@ -2487,9 +2487,6 @@ do_option_warnings (struct context *c) > msg (M_WARN, "WARNING: the current --script-security setting may all= ow passwords to be passed to scripts via environmental variables"); > else > msg (M_WARN, "NOTE: " PACKAGE_NAME " 2.1 requires '--script-security= 2' or higher to call user-defined scripts or executables"); > - > - if (script_method =3D=3D SM_SYSTEM) > - msg (M_WARN, "NOTE: --script-security method=3D'system' is deprecate= d due to the fact that passed parameters will be subject to shell expansion= "); > } > > static void > diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c > index d2882d8..53f6d03 100644 > --- a/src/openvpn/misc.c > +++ b/src/openvpn/misc.c > @@ -53,9 +53,6 @@ const char *iproute_path =3D IPROUTE_PATH; /* GLOBAL */ > /* contains an SSEC_x value defined in misc.h */ > int script_security =3D SSEC_BUILT_IN; /* GLOBAL */ > > -/* contains SM_x value defined in misc.h */ > -int script_method =3D SM_EXECVE; /* GLOBAL */ > - > /* > * Pass tunnel endpoint and MTU parms to a user-supplied script. > * 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 > #if defined(ENABLE_FEATURE_EXECVE) > if (openvpn_execve_allowed (flags)) > { > - if (script_method =3D=3D SM_EXECVE) > - { > - const char *cmd =3D a->argv[0]; > - char *const *argv =3D a->argv; > - char *const *envp =3D (char *const *)make_env_array (es, tr= ue, &gc); > - pid_t pid; > - > - pid =3D fork (); > - if (pid =3D=3D (pid_t)0) /* child side */ > - { > - execve (cmd, argv, envp); > - exit (127); > - } > - else if (pid < (pid_t)0) /* fork failed */ > - msg (M_ERR, "openvpn_execve: unable to fork"); > - else /* parent side */ > - { > - if (waitpid (pid, &ret, 0) !=3D pid) > - ret =3D -1; > - } > - } > - else if (script_method =3D=3D SM_SYSTEM) > - { > - ret =3D openvpn_system (argv_system_str (a), es, flags); > - } > - else > - { > - ASSERT (0); > - } > - } > + const char *cmd =3D a->argv[0]; > + char *const *argv =3D a->argv; > + char *const *envp =3D (char *const *)make_env_array (es, true,= &gc); > + pid_t pid; > + > + pid =3D fork (); > + if (pid =3D=3D (pid_t)0) /* child side */ > + { > + execve (cmd, argv, envp); > + exit (127); > + } > + else if (pid < (pid_t)0) /* fork failed */ > + msg (M_ERR, "openvpn_execve: unable to fork"); > + else /* parent side */ > + { > + if (waitpid (pid, &ret, 0) !=3D pid) > + ret =3D -1; > + } > + } > else if (!warn_shown && (script_security < SSEC_SCRIPTS)) > { > msg (M_WARN, SCRIPT_SECURITY_WARNING); > @@ -353,52 +339,6 @@ openvpn_execve (const struct argv *a, const struct e= nv_set *es, const unsigned i > #endif > > /* > - * Wrapper around the system() call. > - */ > -int > -openvpn_system (const char *command, const struct env_set *es, unsigned = int flags) > -{ > -#ifdef HAVE_SYSTEM > - int ret; > - > - perf_push (PERF_SCRIPT); > - > - /* > - * add env_set to environment. > - */ > - if (flags & S_SCRIPT) > - env_set_add_to_environment (es); > - > - > - /* debugging */ > - dmsg (D_SCRIPT, "SYSTEM[%u] '%s'", flags, command); > - if (flags & S_SCRIPT) > - env_set_print (D_SCRIPT, es); > - > - /* > - * execute the command > - */ > - ret =3D platform_system(command); > - > - /* debugging */ > - dmsg (D_SCRIPT, "SYSTEM return=3D%u", ret); > - > - /* > - * remove env_set from environment > - */ > - if (flags & S_SCRIPT) > - env_set_remove_from_environment (es); > - > - perf_pop (); > - return ret; > - > -#else > - msg (M_FATAL, "Sorry but I can't execute the shell command '%s' becaus= e this operating system doesn't appear to support the system() call", comma= nd); > - return -1; /* NOTREACHED */ > -#endif > -} > - > -/* > * Run execve() inside a fork(), duping stdout. Designed to replicate t= he semantics of popen() but > * in a safer way that doesn't require the invocation of a shell or the = risks > * assocated with formatting and parsing a command line. > diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h > index b6da3f4..183898e 100644 > --- a/src/openvpn/misc.h > +++ b/src/openvpn/misc.h > @@ -96,7 +96,6 @@ int openvpn_popen (const struct argv *a, const struct = env_set *es); > int openvpn_execve (const struct argv *a, const struct env_set *es, cons= t unsigned int flags); > bool openvpn_execve_check (const struct argv *a, const struct env_set *e= s, 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, unsig= ned int flags); > > static inline bool > openvpn_run_script (const struct argv *a, const struct env_set *es, cons= t unsigned int flags, const char *hook) > @@ -322,10 +321,6 @@ extern const char *iproute_path; > #define SSEC_PW_ENV 3 /* allow calling of built-in programs and user-= defined scripts that may receive a password as an environmental variable */ > extern int script_security; /* GLOBAL */ > > -#define SM_EXECVE 0 /* call external programs with execve() or Crea= teProcess() */ > -#define SM_SYSTEM 1 /* call external programs with system() */ > -extern int script_method; /* GLOBAL */ > - > /* return the next largest power of 2 */ > size_t adjust_power_of_2 (size_t u); > > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index edc9195..9baa4ff 100644 > --- a/src/openvpn/options.c > +++ b/src/openvpn/options.c > @@ -248,7 +248,7 @@ static const char usage_message[] =3D > "--setenv name value : Set a custom environmental variable to pass to = script.\n" > "--setenv FORWARD_COMPATIBLE 1 : Relax config file syntax checking to = allow\n" > " directives for future OpenVPN versions to be ignore= d.\n" > - "--script-security level mode : mode=3D'execve' (default) or 'system',= level=3D\n" > + "--script-security level: Where level can be:\n" > " 0 -- strictly no calling of external programs\n" > " 1 -- (default) only call built-ins such as ifconfig= \n" > " 2 -- allow calling of built-ins and scripts\n" > @@ -5293,20 +5293,6 @@ add_option (struct options *options, > { > VERIFY_PERMISSION (OPT_P_GENERAL); > script_security =3D atoi (p[1]); > - if (p[2]) > - { > - if (streq (p[2], "execve")) > - script_method =3D SM_EXECVE; > - else if (streq (p[2], "system")) > - script_method =3D SM_SYSTEM; > - else > - { > - msg (msglevel, "unknown --script-security method: %s", p[2]= ); > - goto err; > - } > - } > - else > - script_method =3D SM_EXECVE; > } > else if (streq (p[0], "mssfix")) > { > diff --git a/src/openvpn/platform.c b/src/openvpn/platform.c > index c79f680..e79de7a 100644 > --- a/src/openvpn/platform.c > +++ b/src/openvpn/platform.c > @@ -205,7 +205,7 @@ platform_chdir (const char* dir) > } > > /* > - * convert system() return into a success/failure value > + * convert execve() return into a success/failure value > */ > bool > platform_system_ok (int stat) > @@ -217,19 +217,6 @@ platform_system_ok (int stat) > #endif > } > > -/* > - * did system() call execute the given command? > - */ > -bool > -platform_system_executed (int stat) > -{ > -#ifdef WIN32 > - return stat !=3D -1; > -#else > - return stat !=3D -1 && WEXITSTATUS (stat) !=3D 127; > -#endif > -} > - > int > platform_access (const char *path, int mode) > { > @@ -288,18 +275,6 @@ platform_unlink (const char *filename) > #endif > } > > -int platform_system(const char *command) { > - int ret; > -#ifdef WIN32 > - struct gc_arena gc =3D gc_new (); > - ret =3D _wsystem (wide_string (command, &gc)); > - gc_free (&gc); > -#else > - ret =3D system (command); > -#endif > - return ret; > -} > - > int platform_putenv(char *string) > { > int status; > diff --git a/src/openvpn/platform.h b/src/openvpn/platform.h > index 7bd2067..7c0a4d7 100644 > --- a/src/openvpn/platform.h > +++ b/src/openvpn/platform.h > @@ -113,10 +113,8 @@ void platform_mlockall (bool print_msg); /* Disable = paging */ > > int platform_chdir (const char* dir); > > -/* interpret the status code returned by system()/execve() */ > +/* interpret the status code returned by execve() */ > bool platform_system_ok (int stat); > -bool platform_system_executed (int stat); > -int platform_system(const char *command); > > int platform_access (const char *path, int mode); > > diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c > index d00088e..2db96a8 100644 > --- a/src/openvpn/win32.c > +++ b/src/openvpn/win32.c > @@ -82,51 +82,6 @@ struct semaphore netcmd_semaphore; /* GLOBAL */ > */ > static char *win_sys_path =3D NULL; /* GLOBAL */ > > -/* > - * Configure PATH. On Windows, sometimes PATH is not set correctly > - * by default. > - */ > -static void > -configure_win_path (void) > -{ > - static bool done =3D false; /* GLOBAL */ > - if (!done) > - { > - FILE *fp; > - fp =3D fopen ("c:\\windows\\system32\\route.exe", "rb"); > - if (fp) > - { > - const int bufsiz =3D 4096; > - struct gc_arena gc =3D gc_new (); > - struct buffer oldpath =3D alloc_buf_gc (bufsiz, &gc); > - struct buffer newpath =3D alloc_buf_gc (bufsiz, &gc); > - const char* delim =3D ";"; > - DWORD status; > - fclose (fp); > - status =3D GetEnvironmentVariable ("PATH", BPTR(&oldpath), (DWO= RD)BCAP(&oldpath)); > -#if 0 > - status =3D 0; > -#endif > - if (!status) > - { > - *BPTR(&oldpath) =3D '\0'; > - delim =3D ""; > - } > - buf_printf (&newpath, "C:\\WINDOWS\\System32;C:\\WINDOWS;C:\\WI= NDOWS\\System32\\Wbem%s%s", > - delim, > - BSTR(&oldpath)); > - SetEnvironmentVariable ("PATH", BSTR(&newpath)); > -#if 0 > - status =3D GetEnvironmentVariable ("PATH", BPTR(&oldpath), (DWO= RD)BCAP(&oldpath)); > - if (status > 0) > - printf ("PATH: %s\n", BSTR(&oldpath)); > -#endif > - gc_free (&gc); > - done =3D true; > - } > - } > -} > - > void > init_win32 (void) > { > @@ -907,53 +862,41 @@ openvpn_execve (const struct argv *a, const struct = env_set *es, const unsigned i > { > if (openvpn_execve_allowed (flags)) > { > - if (script_method =3D=3D SM_EXECVE) > - { > - struct gc_arena gc =3D gc_new (); > - STARTUPINFOW start_info; > - PROCESS_INFORMATION proc_info; > - > - char *env =3D env_block (es); > - WCHAR *cl =3D wide_cmd_line (a, &gc); > - WCHAR *cmd =3D wide_string (a->argv[0], &gc); > - > - CLEAR (start_info); > - CLEAR (proc_info); > - > - /* fill in STARTUPINFO struct */ > - GetStartupInfoW(&start_info); > - start_info.cb =3D sizeof(start_info); > - start_info.dwFlags =3D STARTF_USESHOWWINDOW; > - start_info.wShowWindow =3D SW_HIDE; > - > - if (CreateProcessW (cmd, cl, NULL, NULL, FALSE, 0, env, NUL= L, &start_info, &proc_info)) > - { > - DWORD exit_status =3D 0; > - CloseHandle (proc_info.hThread); > - WaitForSingleObject (proc_info.hProcess, INFINITE); > - if (GetExitCodeProcess (proc_info.hProcess, &exit_statu= s)) > - ret =3D (int)exit_status; > - else > - msg (M_WARN|M_ERRNO, "openvpn_execve: GetExitCodeProc= ess %S failed", cmd); > - CloseHandle (proc_info.hProcess); > - } > - else > - { > - msg (M_WARN|M_ERRNO, "openvpn_execve: CreateProcess %S = failed", cmd); > - } > - free (env); > - gc_free (&gc); > - } > - else if (script_method =3D=3D SM_SYSTEM) > - { > - configure_win_path (); > - ret =3D openvpn_system (argv_system_str (a), es, flags); > - } > - else > - { > - ASSERT (0); > - } > - } > + struct gc_arena gc =3D gc_new (); > + STARTUPINFOW start_info; > + PROCESS_INFORMATION proc_info; > + > + char *env =3D env_block (es); > + WCHAR *cl =3D wide_cmd_line (a, &gc); > + WCHAR *cmd =3D wide_string (a->argv[0], &gc); > + > + CLEAR (start_info); > + CLEAR (proc_info); > + > + /* fill in STARTUPINFO struct */ > + GetStartupInfoW(&start_info); > + start_info.cb =3D sizeof(start_info); > + start_info.dwFlags =3D STARTF_USESHOWWINDOW; > + start_info.wShowWindow =3D SW_HIDE; > + > + if (CreateProcessW (cmd, cl, NULL, NULL, FALSE, 0, env, NULL, = &start_info, &proc_info)) > + { > + DWORD exit_status =3D 0; > + CloseHandle (proc_info.hThread); > + WaitForSingleObject (proc_info.hProcess, INFINITE); > + if (GetExitCodeProcess (proc_info.hProcess, &exit_status)) > + ret =3D (int)exit_status; > + else > + msg (M_WARN|M_ERRNO, "openvpn_execve: GetExitCodeProcess= %S failed", cmd); > + CloseHandle (proc_info.hProcess); > + } > + else > + { > + msg (M_WARN|M_ERRNO, "openvpn_execve: CreateProcess %S fai= led", cmd); > + } > + free (env); > + gc_free (&gc); > + } > else if (!exec_warn && (script_security < SSEC_SCRIPTS)) > { > msg (M_WARN, SCRIPT_SECURITY_WARNING); > -- > 1.7.10.2 > > > -------------------------------------------------------------------------= ----- > The Windows 8 Center - In partnership with Sourceforge > Your idea - your app - 30 days. > Get started! > http://windows8center.sourceforge.net/ > what-html-developers-need-to-know-about-coding-windows-8-metro-style-apps/ > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel