Le vendredi 06 janvier 2012 à 17:22 +0100, David Sommerseth a écrit :
> On 12/12/11 18:57, Frederic Crozat wrote:
> > Le lundi 31 octobre 2011 à 22:11 +0100, David Sommerseth a écrit :
> >> On 31/10/11 16:30, Frederic Crozat wrote:
> [...snip...]
> 
> Hey again, and thanks for this great rework!  I've looked through it, and
> it looks a lot better now.  However ...
> 
> >> e) Consider to make use of openvpn_execve_check() or
> >> openvpn_execve() instead of the popen() - or make a popen() variant
> >> which got similar error controls to popen() as openvpn_execve*()
> >> functions.  I also think that this execution should honour the
> >> --script-security setting (which is implicitly checked for with
> >> openvpn_execve())
> 
> I like the approach of the openvpn_popen().  However, there are a few things.
> 
> +              if (pipe(pipe_stdout) == 0) {
> +                   pid = fork ();
> +                   if (pid == (pid_t)0) /* child side */
> +                     {
> +                       close(pipe_stdout[0]);
> +                       dup2(pipe_stdout[1],1);
> +                       execve (cmd, argv, envp);
> +                       exit (127);
> +                     }
> +                   else if (pid < (pid_t)0) /* fork failed */
> +                     ;
>                        ^^^^^  No error message if fork() fails?
>                               And is the (pid_t) typecasting really needed?

Well, I stole the code from openvpn_execve function ;))
I'm ok to change it, but it would be better for both function to stay
similar, I think.

> +                   else /* parent side */
> +                     {
> +                            ret=pipe_stdout[0];
> +                         close(pipe_stdout[1]);
> +                     }
> +           }
> 
> And:
> 
> +  else
> +    {
> +      msg (M_WARN, "openvpn_popen: called with empty argv");
> +    }
> 
> This should probably be M_FATAL and not M_WARN.  If openvpn_popen()
> receives an empty argv array, it most likely is due to very bad
> programming.  So halting execution is quite appropriate in my eyes.

Again, I stole this part from openvpn_execve, but I'll change it to
M_FATAL.

> In get_console_input_systemd() you do:
> 
> +  *input = 0;
> 
> I think it would be better to do: CLEAR(*input) instead.  Just setting
> the first byte to 0, will not solve any leak possibilities.  An
> alternative is to use memset() directly, which the CLEAR() macro uses
> (see basic.h:47), but we prefer to use CLEAR() over memset() directly.

done (and I've also fixed some missing spaces in various location).

See the new version attached.
-- 
Frederic Crozat <fcro...@suse.com>
SUSE
>From b3eb16e24ff354ce417fee4e8f48b8a8244e8b0c Mon Sep 17 00:00:00 2001
From: Frederic Crozat <fcro...@suse.com>
List-Post: openvpn-devel@lists.sourceforge.net
Date: Mon, 31 Oct 2011 15:51:53 +0100
Subject: [PATCH] Add support to forward console query to systemd

Systemd requires console query to be forwarded using its own
tool.

Signed-off-by: Frederic Crozat <fcro...@suse.com>
---
 configure.ac |   11 +++++
 misc.c       |  121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 132 insertions(+), 0 deletions(-)

diff --git a/configure.ac b/configure.ac
index a4d68e6..7143cc5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -223,6 +223,12 @@ AC_ARG_ENABLE(selinux,
    [SELINUX="yes"]
 )

+AC_ARG_ENABLE(systemd,
+   [  --enable-systemd        Enable systemd suppport],
+   [SYSTEMD="$enableval"],
+   [SYSTEMD="no"]
+)
+
 AC_ARG_WITH(ssl-headers,
    [  --with-ssl-headers=DIR  Crypto/SSL Include files location],
    [CS_HDR_DIR="$withval"]
@@ -930,6 +936,11 @@ if test "$PASSWORD_SAVE" = "yes"; then
    AC_DEFINE(ENABLE_PASSWORD_SAVE, 1, [Allow --askpass and --auth-user-pass passwords to be read from a file])
 fi

+dnl enable systemd support
+if test "$SYSTEMD" = "yes"; then
+   AC_DEFINE(ENABLE_SYSTEMD, 1, [Enable systemd support])
+fi
+
 dnl
 dnl check for SELinux library and headers
 dnl
diff --git a/misc.c b/misc.c
index 99e5bc5..b38553f 100644
--- a/misc.c
+++ b/misc.c
@@ -36,6 +36,7 @@
 #include "crypto.h"
 #include "route.h"
 #include "win32.h"
+#include <unistd.h>

 #include "memdbg.h"

@@ -610,6 +611,71 @@ openvpn_system (const char *command, const struct env_set *es, unsigned int flag
 }

 /*
+ * Run execve() inside a fork(), duping stdout.  Designed to replicate the 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.
+ */
+int
+openvpn_popen (const struct argv *a,  const struct env_set *es)
+{
+  struct gc_arena gc = gc_new ();
+  int ret = -1;
+  static bool warn_shown = false;
+
+  if (a && a->argv[0])
+    {
+#if defined(ENABLE_EXECVE)
+      if (script_security >= SSEC_BUILT_IN)
+	{
+	      const char *cmd = a->argv[0];
+	      char *const *argv = a->argv;
+	      char *const *envp = (char *const *)make_env_array (es, true, &gc);
+	      pid_t pid;
+	      int pipe_stdout[2];
+
+              if (pipe (pipe_stdout) == 0) {
+		      pid = fork ();
+		      if (pid == (pid_t)0) /* child side */
+			{
+			  close (pipe_stdout[0]);
+			  dup2 (pipe_stdout[1],1);
+			  execve (cmd, argv, envp);
+			  exit (127);
+			}
+		      else if (pid < (pid_t)0) /* fork failed */
+			;
+		      else /* parent side */
+			{
+                            ret=pipe_stdout[0];
+			    close (pipe_stdout[1]);
+			}
+	      }
+	      else {
+		      msg (M_WARN, "openvpn_popen: unable to create stdout pipe");
+		      ret = -1;
+	      }
+	}
+      else if (!warn_shown && (script_security < SSEC_SCRIPTS))
+	{
+	  msg (M_WARN, SCRIPT_SECURITY_WARNING);
+          warn_shown = true;
+	}
+#else
+      msg (M_WARN, "openvpn_popen: execve function not available");
+#endif
+    }
+  else
+    {
+      msg (M_FATAL, "openvpn_popen: called with empty argv");
+    }
+
+  gc_free (&gc);
+  return ret;
+}
+
+
+
+/*
  * Initialize random number seed.  random() is only used
  * when "weak" random numbers are acceptable.
  * OpenSSL routines are always used when cryptographically
@@ -1328,6 +1394,56 @@ close_tty (FILE *fp)
 #endif

 /*
+ * is systemd running
+ */
+
+#if defined(TARGET_LINUX) && defined(ENABLE_SYSTEMD)
+bool
+check_systemd_running ()
+{
+  struct stat a, b;
+
+  /* We simply test whether the systemd cgroup hierarchy is
+   * mounted */
+
+  return (lstat("/sys/fs/cgroup", &a) == 0)
+	  && (lstat("/sys/fs/cgroup/systemd", &b) == 0)
+	  && (a.st_dev != b.st_dev);
+
+}
+
+bool
+get_console_input_systemd (const char *prompt, const bool echo, char *input, const int capacity)
+{
+  int std_out;
+  char cmd[256];
+  bool ret = false;
+  struct argv argv;
+
+  argv_init (&argv);
+  argv_printf (&argv, "/bin/systemd-ask-password");
+  argv_printf_cat (&argv, "%s", prompt);
+
+  if ((std_out = openvpn_popen (&argv, NULL)) < 0) {
+	  return false;
+  }
+  CLEAR (*input);
+  if (read (std_out, input, capacity) != 0)
+    {
+       chomp (input);
+       ret = true;
+    }
+  close (std_out);
+
+  argv_reset (&argv);
+
+  return ret;
+}
+
+
+#endif
+
+/*
  * Get input from console
  */
 bool
@@ -1339,6 +1455,11 @@ get_console_input (const char *prompt, const bool echo, char *input, const int c
   ASSERT (capacity > 0);
   input[0] = '\0';

+#if defined(TARGET_LINUX) && defined(ENABLE_SYSTEMD)
+  if (check_systemd_running ())
+    return get_console_input_systemd (prompt, echo, input, capacity);
+#endif
+
 #if defined(WIN32)
   return get_console_input_win32 (prompt, echo, input, capacity);
 #elif defined(HAVE_GETPASS)
-- 
1.7.7

Reply via email to