Browse code

down-root plugin: Replaced system() calls with execve()

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

Signed-off-by: David Sommerseth <davids@redhat.com>
Acked-by: Steffan Karger <steffan.karger@fox-it.com>
Message-Id: <1416148262-20978-1-git-send-email-openvpn.list@topphemmelig.net>
URL: http://article.gmane.org/gmane.network.openvpn.devel/9238
Signed-off-by: Gert Doering <gert@greenie.muc.de>

David Sommerseth authored on 2014/11/16 23:31:02
Showing 1 changed files
... ...
@@ -5,7 +5,8 @@
5 5
  *             packet encryption, packet authentication, and
6 6
  *             packet compression.
7 7
  *
8
- *  Copyright (C) 2002-2010 OpenVPN Technologies, Inc. <sales@openvpn.net>
8
+ *  Copyright (C) 2002-2013 OpenVPN Technologies, Inc. <sales@openvpn.net>
9
+ *  Copyright (C) 2013      David Sommerseth <davids@redhat.com>
9 10
  *
10 11
  *  This program is free software; you can redistribute it and/or modify
11 12
  *  it under the terms of the GNU General Public License version 2
... ...
@@ -46,8 +47,8 @@
46 46
 #define DEBUG(verb) ((verb) >= 7)
47 47
 
48 48
 /* Command codes for foreground -> background communication */
49
-#define COMMAND_RUN_SCRIPT 0
50
-#define COMMAND_EXIT       1
49
+#define COMMAND_RUN_SCRIPT 1
50
+#define COMMAND_EXIT       2
51 51
 
52 52
 /* Response codes for background -> foreground communication */
53 53
 #define RESPONSE_INIT_SUCCEEDED   10
... ...
@@ -56,7 +57,7 @@
56 56
 #define RESPONSE_SCRIPT_FAILED    13
57 57
 
58 58
 /* Background process function */
59
-static void down_root_server (const int fd, char *command, const char *argv[], const char *envp[], const int verb);
59
+static void down_root_server (const int fd, char * const * argv, char * const *envp, const int verb);
60 60
 
61 61
 /*
62 62
  * Plugin state, used by foreground
... ...
@@ -73,7 +74,7 @@ struct down_root_context
73 73
   int verb;
74 74
 
75 75
   /* down command */
76
-  char *command;
76
+  char **command;
77 77
 };
78 78
 
79 79
 /*
... ...
@@ -207,72 +208,48 @@ set_signals (void)
207 207
   signal (SIGPIPE, SIG_IGN);
208 208
 }
209 209
 
210
-/*
211
- * convert system() return into a success/failure value
212
- */
213
-int
214
-system_ok (int stat)
215
-{
216
-#ifdef WIN32
217
-  return stat == 0;
218
-#else
219
-  return stat != -1 && WIFEXITED (stat) && WEXITSTATUS (stat) == 0;
220
-#endif
221
-}
222
-
223
-static char *
224
-build_command_line (const char *argv[])
225
-{
226
-  int size = 0;
227
-  int n = 0;
228
-  int i;
229
-  char *string;
230
-
231
-  /* precompute size */
232
-  if (argv)
233
-    {
234
-      for (i = 0; argv[i]; ++i)
235
-	{
236
-	  size += (strlen (argv[i]) + 1); /* string length plus trailing space */
237
-	  ++n;
238
-	}
239
-    }
240
-  ++size;                                 /* for null terminator */
241
-
242
-  /* allocate memory */
243
-  string = (char *) malloc (size);
244
-  if (!string)
245
-    {
246
-      fprintf (stderr, "DOWN-ROOT: out of memory\n");
247
-      exit (1);
248
-    }
249
-  string[0] = '\0';
250
-
251
-  /* build string */
252
-  for (i = 0; i < n; ++i)
253
-    {
254
-      strcat (string, argv[i]);
255
-      if (i + 1 < n)
256
-	strcat (string, " ");
257
-    }
258
-  return string;
259
-}
260 210
 
261 211
 static void
262 212
 free_context (struct down_root_context *context)
263 213
 {
264 214
   if (context)
265 215
     {
266
-      if (context->command)
267
-	free (context->command);
216
+      free (context->command);
268 217
       free (context);
269 218
     }
270 219
 }
271 220
 
221
+/* Run the script using execve().  As execve() replaces the
222
+ * current process with the new one, do a fork first before
223
+ * calling execve()
224
+ */
225
+static int
226
+run_script(char * const *argv, char * const *envp) {
227
+  pid_t pid;
228
+  int ret = 0;
229
+
230
+  pid = fork();
231
+  if (pid == (pid_t)0) { /* child side */
232
+    execve(argv[0], argv, envp);
233
+    fprintf(stderr, "DOWN-ROOT: Failed execute: %s\n", argv[0]);
234
+    exit(127);  /* If execve() fails to run, exit child with exit code 127 */
235
+  } else if (pid < (pid_t)0 ) {
236
+    fprintf(stderr, "DOWN-ROOT: Failed to fork child to run %s\n", argv[0]);
237
+    return -1;
238
+  } else { /* parent side */
239
+    if( waitpid (pid, &ret, 0) != pid ) {
240
+      fprintf(stderr, "DOWN-ROOT: waitpid() failed, don't know exit code of child (%s)\n", argv[0]);
241
+      return -1;
242
+    }
243
+  }
244
+  return ret;
245
+}
246
+
272 247
 OPENVPN_EXPORT openvpn_plugin_handle_t
273 248
 openvpn_plugin_open_v1 (unsigned int *type_mask, const char *argv[], const char *envp[])
274 249
 {
275 250
   struct down_root_context *context;
251
+  int i = 0;
276 252
 
277 253
   /*
278 254
    * Allocate our context
... ...
@@ -298,9 +275,14 @@ openvpn_plugin_open_v1 (unsigned int *type_mask, const char *argv[], const char
298 298
     }
299 299
 
300 300
   /*
301
-   * Save our argument in context
301
+   * Save the arguments in our context
302 302
    */
303
-  context->command = build_command_line (&argv[1]);
303
+  context->command = calloc(string_array_len(argv), sizeof(char *));
304
+  /* Ignore argv[0], as it contains just the plug-in file name */
305
+  for (i = 1; i < string_array_len(argv); i++)
306
+    {
307
+      context->command[i-1] = (char *) argv[i];
308
+    }
304 309
 
305 310
   /*
306 311
    * Get verbosity level from environment
... ...
@@ -385,7 +367,7 @@ openvpn_plugin_func_v1 (openvpn_plugin_handle_t handle, const int type, const ch
385 385
 	  daemonize (envp);
386 386
 
387 387
 	  /* execute the event loop */
388
-	  down_root_server (fd[1], context->command, argv, envp, context->verb);
388
+	  down_root_server (fd[1], context->command, (char * const *) envp, context->verb);
389 389
 
390 390
 	  close (fd[1]);
391 391
 	  exit (0);
... ...
@@ -422,7 +404,7 @@ openvpn_plugin_close_v1 (openvpn_plugin_handle_t handle)
422 422
     {
423 423
       /* tell background process to exit */
424 424
       if (send_control (context->foreground_fd, COMMAND_EXIT) == -1)
425
-	fprintf (stderr, "DOWN-ROOT: Error signaling background process to exit\n");
425
+	fprintf (stderr, "DOWN-ROOT: Error signalling background process to exit\n");
426 426
 
427 427
       /* wait for background process to exit */
428 428
       if (context->background_pid > 0)
... ...
@@ -453,18 +435,16 @@ openvpn_plugin_abort_v1 (openvpn_plugin_handle_t handle)
453 453
  * Background process -- runs with privilege.
454 454
  */
455 455
 static void
456
-down_root_server (const int fd, char *command, const char *argv[], const char *envp[], const int verb)
456
+down_root_server (const int fd, char * const *argv, char * const *envp, const int verb)
457 457
 {
458 458
   const char *p[3];
459
-  char *command_line = NULL;
460
-  char *argv_cat = NULL;
461 459
   int i;
462 460
 
463 461
   /*
464 462
    * Do initialization
465 463
    */
466 464
   if (DEBUG (verb))
467
-    fprintf (stderr, "DOWN-ROOT: BACKGROUND: INIT command='%s'\n", command);
465
+    fprintf (stderr, "DOWN-ROOT: BACKGROUND: INIT command='%s'\n", argv[0]);
468 466
 
469 467
   /*
470 468
    * Tell foreground that we initialized successfully
... ...
@@ -476,32 +456,12 @@ down_root_server (const int fd, char *command, const char *argv[], const char *e
476 476
     }
477 477
 
478 478
   /*
479
-   * Build command line
480
-   */
481
-  if (string_array_len (argv) >= 2)
482
-    argv_cat = build_command_line (&argv[1]);
483
-  else
484
-    argv_cat = build_command_line (NULL);
485
-  p[0] = command;
486
-  p[1] = argv_cat;
487
-  p[2] = NULL;
488
-  command_line = build_command_line (p);
489
-
490
-  /*
491
-   * Save envp in environment
492
-   */
493
-  for (i = 0; envp[i]; ++i)
494
-    {
495
-      putenv ((char *)envp[i]);
496
-    }
497
-
498
-  /*
499 479
    * Event loop
500 480
    */
501 481
   while (1)
502 482
     {
503 483
       int command_code;
504
-      int status;
484
+      int exit_code = -1;
505 485
 
506 486
       /* get a command from foreground process */
507 487
       command_code = recv_control (fd);
... ...
@@ -512,8 +472,7 @@ down_root_server (const int fd, char *command, const char *argv[], const char *e
512 512
       switch (command_code)
513 513
 	{
514 514
 	case COMMAND_RUN_SCRIPT:
515
-	  status = system (command_line);
516
-	  if (system_ok (status)) /* Succeeded */
515
+	  if ( (exit_code = run_script(argv, envp)) == 0 ) /* Succeeded */
517 516
 	    {
518 517
 	      if (send_control (fd, RESPONSE_SCRIPT_SUCCEEDED) == -1)
519 518
 		{
... ...
@@ -523,6 +482,7 @@ down_root_server (const int fd, char *command, const char *argv[], const char *e
523 523
 	    }
524 524
 	  else /* Failed */
525 525
 	    {
526
+	      fprintf(stderr, "DOWN-ROOT: BACKGROUND: %s exited with exit code %i\n", argv[0], exit_code);
526 527
 	      if (send_control (fd, RESPONSE_SCRIPT_FAILED) == -1)
527 528
 		{
528 529
 		  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
546 546
     }
547 547
 
548 548
  done:
549
-  if (argv_cat)
550
-    free (argv_cat);
551
-  if (command_line)
552
-    free (command_line);
553 549
   if (DEBUG (verb))
554 550
     fprintf (stderr, "DOWN-ROOT: BACKGROUND: EXIT\n");
555 551