down-root plugin: Replaced system() calls with execve()
PATCH 1/1 down-root plugin: Replaced system() calls with execve()
From: David Sommerseth <[email protected]...>
The system() call is prone to shell expansions and provides far more environments variables to the executable run than what is usually preferred. By moving over to exevce() shell expansions are far more difficult to achieve and only the OpenVPN provided environment variables are available.
This is a response to the patch submitted to openvpn-devel ML:
http://article.gmane.org/gmane.network.openvpn.devel/7919
v2 - Pulling it up again, fixing a few whitespace and spelling issues
--- src/plugins/down-root/down-root.c | 140 +++++++++++++------------------------- 1 file changed, 48 insertions(+), 92 deletions(-)
src/plugins/down-root/down-root.c +48 -92
@@ -5,7 +5,8 @@* packet encryption, packet authentication, and * packet compression. * -* Copyright (C) 2002-2010 OpenVPN Technologies, Inc. <[email protected]...> +* Copyright (C) 2002-2013 OpenVPN Technologies, Inc. <[email protected]...> +* Copyright (C) 2013 David Sommerseth <[email protected]...> * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2
@@ -46,8 +47,8 @@#define DEBUG (verb) ((verb) >= 7)/* Command codes for foreground -> background communication */ -#define COMMAND_RUN_SCRIPT 0 -#define COMMAND_EXIT 1 +#define COMMAND_RUN_SCRIPT 1 +#define COMMAND_EXIT 2/* Response codes for background -> foreground communication */ #define RESPONSE_INIT_SUCCEEDED 10
@@ -56,7 +57,7 @@#define RESPONSE_SCRIPT_FAILED 13/* Background process function */ -static void down_root_server (const int fd,char *command,const char *argv[],const char *envp[],const int verb); +static void down_root_server (const int fd,char *const * argv,char *const *envp,const int verb);/* * Plugin state, used by foreground
@@ -73,7 +74,7 @@ struct down_root_contextint verb;/* down command */ -char *command; +char **command; };/*
@@ -207,72 +208,48 @@ set_signals (void)signal (SIGPIPE ,SIG_IGN ); } -/* -* convert system() return into a success/failure value -*/ -int -system_ok (int stat) -{ -#ifdef WIN32 -return stat ==0 ; -#else -return stat !=-1 &&WIFEXITED (stat) &&WEXITSTATUS (stat) ==0 ; -#endif -} - -static char * -build_command_line (const char *argv[]) -{ -int size =0 ; -int n =0 ; -int i; -char *string; - -/* precompute size */ -if (argv) - { -for (i =0 ; argv[i]; ++i) - { - size += (strlen (argv[i]) +1 );/* string length plus trailing space */ - ++n; - } - } - ++size;/* for null terminator */ - -/* allocate memory */ - string = (char *)malloc (size); -if (!string) - { -fprintf (stderr,"DOWN-ROOT: out of memory\n" ); -exit (1 ); - } - string[0 ] ='\0' ; - -/* build string */ -for (i =0 ; i < n; ++i) - { -strcat (string, argv[i]); -if (i +1 < n) -strcat (string," " ); - } -return string; -}static void free_context (struct down_root_context *context) {if (context) { -if (context->command) -free (context->command); +free (context->command);free (context); } } +/* Run the script using execve(). As execve() replaces the +* current process with the new one, do a fork first before +* calling execve() +*/ +static int +run_script (char *const *argv,char *const *envp) { +pid_t pid; +int ret =0 ; + + pid =fork (); +if (pid == (pid_t )0 ) {/* child side */ +execve (argv[0 ], argv, envp); +fprintf (stderr,"DOWN-ROOT: Failed execute: %s\n" , argv[0 ]); +exit (127 );/* If execve() fails to run, exit child with exit code 127 */ + }else if (pid < (pid_t )0 ) { +fprintf (stderr,"DOWN-ROOT: Failed to fork child to run %s\n" , argv[0 ]); +return -1 ; + }else {/* parent side */ +if (waitpid (pid, &ret,0 ) != pid ) { +fprintf (stderr,"DOWN-ROOT: waitpid() failed, don't know exit code of child (%s)\n" , argv[0 ]); +return -1 ; + } + } +return ret; +} +OPENVPN_EXPORT openvpn_plugin_handle_t openvpn_plugin_open_v1 (unsigned int *type_mask, const char *argv[], const char *envp[]) {struct down_root_context *context; +int i =0 ;/* * Allocate our context
@@ -298,9 +275,14 @@ openvpn_plugin_open_v1 (unsigned int *type_mask, const char *argv[], const char }/* -* Save our argument in context +* Save the arguments in our context */ - context->command =build_command_line (&argv[1 ]); + context->command =calloc (string_array_len (argv),sizeof (char *)); +/* Ignore argv[0], as it contains just the plug-in file name */ +for (i =1 ; i <string_array_len (argv); i++) + { + context->command[i-1 ] = (char *) argv[i]; + }/* * Get verbosity level from environment
@@ -385,7 +367,7 @@ openvpn_plugin_func_v1 (openvpn_plugin_handle_t handle, const int type, const chdaemonize (envp);/* execute the event loop */ -down_root_server (fd[1 ], context->command, argv, envp, context->verb); +down_root_server (fd[1 ], context->command, (char *const *) envp, context->verb);close (fd[1 ]);exit (0 );
@@ -422,7 +404,7 @@ openvpn_plugin_close_v1 (openvpn_plugin_handle_t handle) {/* tell background process to exit */ if (send_control (context->foreground_fd,COMMAND_EXIT ) ==-1 ) -fprintf (stderr,"DOWN-ROOT: Error signaling background process to exit\n" ); +fprintf (stderr,"DOWN-ROOT: Error signalling background process to exit\n" );/* wait for background process to exit */ if (context->background_pid >0 )
@@ -453,18 +435,16 @@ openvpn_plugin_abort_v1 (openvpn_plugin_handle_t handle)* Background process -- runs with privilege. */ static void -down_root_server (const int fd,char *command,const char *argv[],const char *envp[],const int verb) +down_root_server (const int fd,char *const *argv,char *const *envp,const int verb) {const char *p[3 ]; -char *command_line =NULL ; -char *argv_cat =NULL ;int i;/* * Do initialization */ if (DEBUG (verb)) -fprintf (stderr,"DOWN-ROOT: BACKGROUND: INIT command='%s'\n" , command); +fprintf (stderr,"DOWN-ROOT: BACKGROUND: INIT command='%s'\n" , argv[0 ]);/* * Tell foreground that we initialized successfully
@@ -476,32 +456,12 @@ down_root_server (const int fd, char *command, const char *argv[], const char *e }/* -* Build command line -*/ -if (string_array_len (argv) >=2 ) - argv_cat =build_command_line (&argv[1 ]); -else - argv_cat =build_command_line (NULL ); - p[0 ] = command; - p[1 ] = argv_cat; - p[2 ] =NULL ; - command_line =build_command_line (p); - -/* -* Save envp in environment -*/ -for (i =0 ; envp[i]; ++i) - { -putenv ((char *)envp[i]); - } - -/* * Event loop */ while (1 ) {int command_code; -int status; +int exit_code =-1 ;/* get a command from foreground process */ command_code =recv_control (fd);
@@ -512,8 +472,7 @@ down_root_server (const int fd, char *command, const char *argv[], const char *eswitch (command_code) {case COMMAND_RUN_SCRIPT : - status =system (command_line); -if (system_ok (status))/* Succeeded */ +if ( (exit_code =run_script (argv, envp)) ==0 )/* Succeeded */ {if (send_control (fd,RESPONSE_SCRIPT_SUCCEEDED ) ==-1 ) {
@@ -523,6 +482,7 @@ down_root_server (const int fd, char *command, const char *argv[], const char *e }else /* Failed */ { +fprintf (stderr,"DOWN-ROOT: BACKGROUND: %s exited with exit code %i\n" , argv[0 ], exit_code);if (send_control (fd,RESPONSE_SCRIPT_FAILED ) ==-1 ) {fprintf (stderr,"DOWN-ROOT: BACKGROUND: write error on response socket [3]\n" );
@@ -546,10 +506,6 @@ down_root_server (const int fd, char *command, const char *argv[], const char *e } done: -if (argv_cat) -free (argv_cat); -if (command_line) -free (command_line);if (DEBUG (verb))fprintf (stderr,"DOWN-ROOT: BACKGROUND: EXIT\n" );
Signature · 1 line
1.8.3.1
Quoted 8 lines · David Sommerseth
From: David Sommerseth <[email protected]...>
The
system()call is prone to shell expansions and provides far more environments variables to the executable run than what is usually preferred. By moving over toexevce()shell expansions are far more difficult to achieve and only the OpenVPN provided environment variables are available.
Quoted 6 lines
This is a response to the patch submitted to openvpn-devel ML:
http://article.gmane.org/gmane.network.openvpn.devel/7919v2 - Pulling it up again, fixing a few whitespace and spelling issues
Signed-off-by David Sommerseth <[email protected]...>
Just a general comment on the error handling: Instead of printing generic error messages when syscalls fail, wouldn't it be better to look up the actual error description using strerror(3)? This would include calls to malloc, fork, execve, and possibly others.
Hi,
Quoted 5 lines · David Sommerseth
The
system()call is prone to shell expansions and provides far more environments variables to the executable run than what is usually preferred. By moving over toexevce()shell expansions are far more difficult to achieve and only the OpenVPN provided environment variables are available.
My compiler has some nitpicks (see below), but otherwise ACK. Code looks sane and works for me.
src/plugins/down-root/down-root.c openvpn_plugin_abort_v1 (openvpn_plugin_handle_t handle) · line 441
Quoted 11 lines
src/plugins/down-root/down-root.c +1
-3@@ -453,18 +435,16 @@ openvpn_plugin_abort_v1 (openvpn_plugin_handle_t handle)* Background process -- runs with privilege. */ static void -down_root_server (const int fd,char *command,const char *argv[],const char *envp[],const int verb) +down_root_server (const int fd,char *const *argv,char *const *envp,const int verb) {const char *p[3 ]; -char *command_line =NULL ; -char *argv_cat =NULL ;int i;
You can remove even more code:
down-root.c:440:15: warning: unused variable 'p' [-Wunused-variable]
const char *p[3];
^
down-root.c:441:7: warning: unused variable 'i' [-Wunused-variable]
int i;
^-Steffan
Your patch has been applied to the master and release/2.3 branch.
commit f87b1beccb817e1633bc95bd5dd19deec35c7edc (master) commit 07d61fee2047686e5bec237a6cd7467244f91e13 (release/2.3)
Author: David Sommerseth Date: Sun Nov 16 15:31:02 2014 +0100
down-root plugin: Replaced system() calls with execve()
Signed-off-by: David Sommerseth <[email protected]...>
Acked-by: Steffan Karger <[email protected]...>
Message-Id: <[email protected]...>
URL: http://article.gmane.org/gmane.network.openvpn.devel/9238
Signed-off-by: Gert Doering <[email protected]...>--
kind regards,
Gert Doering
Hi,
Quoted 7 lines · David Sommerseth
From: David Sommerseth <[email protected]...>
The
system()call is prone to shell expansions and provides far more environments variables to the executable run than what is usually preferred. By moving over toexevce()shell expansions are far more difficult to achieve and only the OpenVPN provided environment variables are available.
Since Steffan has ACKed it, I've committed it, but it would be nice if you could send a followup patch addressing the remaining warnings in down-root that Steffan pointed out - and maybe reformat these bits to conform to whatever coding style we agreed to (which this is not :-))...
src/plugins/down-root/down-root.c set_signals (void) · line 238
Quoted 13 lines
src/plugins/down-root/down-root.c +13
-0+static int +run_script (char *const *argv,char *const *envp) { +pid_t pid; +int ret =0 ; + + pid =fork (); +if (pid == (pid_t )0 ) {/* child side */ +execve (argv[0 ], argv, envp); +fprintf (stderr,"DOWN-ROOT: Failed execute: %s\n" , argv[0 ]); +exit (127 );/* If execve() fails to run, exit child with exit code 127 */ + }else if (pid < (pid_t )0 ) { +fprintf (stderr,"DOWN-ROOT: Failed to fork child to run %s\n" , argv[0 ]); +return -1 ;
I think this is called "all-mashed-together" style or so :-)
thanks,
gert
Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany [email protected]...
fax: +49-89-35655025 [email protected]...