diff mbox series

[Openvpn-devel,v2] Better error message when script fails due to script-security setting

Message ID 1550709982-19319-1-git-send-email-selva.nair@gmail.com
State Accepted
Headers show
Series [Openvpn-devel,v2] Better error message when script fails due to script-security setting | expand

Commit Message

Selva Nair Feb. 20, 2019, 1:46 p.m. UTC 
From: Selva Nair <selva.nair@gmail.com>

- Add a new return value (-2) for openvpn_execve() when external
  program execution is not allowed due to a low script-security
  setting.

- Add a corresponding error message

Errors and warnings in such cases will now display as
"WARNING: failed running command (<cmd>) :" followed by

"disallowed by script-security setting" on all platforms

instead of the current

"external program did not execute -- returned error code -1"
on Windows and
"external program fork failed" on other platforms.

The error is FATAL for some scripts and that behaviour is unchanged.

This helps the Windows GUI to detect when a connection failure
results from a safer script-security setting enforced by the GUI,
and show a relevant message.

v2 changes as suggested by <davds@openvpn.net>

- define macros for return values of openvpn_execve()
- replace if/else by switch() in system_error_message()

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/run_command.c | 93 ++++++++++++++++++++++++++++-------------------
 src/openvpn/run_command.h |  4 ++
 src/openvpn/win32.c       | 12 ++++--
 3 files changed, 68 insertions(+), 41 deletions(-)

Comments

Gert Doering Feb. 28, 2019, 6:25 a.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

Thanks.  I have not tested the various error conditions, but the code
makes sense to me and it passes compile and t_client tests (which use
--up here), so it's not breaking stuff in fundamental and non-obvious
ways.

(Since David was mostly happy with the v1 approach except for the
"127" and "switch()", I assume he's totally fine with v2 now :-) )

Your patch has been applied to the master branch - it won't apply to
2.4 as we have no "run_command.*" there yet.  Shall we cherrypick
bf97c00f7dba4 and 253f015558 to get there?  Or do you want to do a
2.4 version of the patch?

commit 01a3c876d4911ff7ea82b04edeb43defdebc69d7 (master)
Author: Selva Nair
Date:   Wed Feb 20 19:46:22 2019 -0500

     Better error message when script fails due to script-security setting

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <1550709982-19319-1-git-send-email-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg18223.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Selva Nair Feb. 28, 2019, 6:33 a.m. UTC | #2 
Hi,

On Thu, Feb 28, 2019 at 12:25 PM Gert Doering <gert@greenie.muc.de> wrote:

> Acked-by: Gert Doering <gert@greenie.muc.de>
>
> Thanks.  I have not tested the various error conditions, but the code
> makes sense to me and it passes compile and t_client tests (which use
> --up here), so it's not breaking stuff in fundamental and non-obvious
> ways.
>
> (Since David was mostly happy with the v1 approach except for the
> "127" and "switch()", I assume he's totally fine with v2 now :-) )
>
> Your patch has been applied to the master branch - it won't apply to
> 2.4 as we have no "run_command.*" there yet.  Shall we cherrypick
> bf97c00f7dba4 and 253f015558 to get there?  Or do you want to do a
> 2.4 version of the patch?
>

Would require more cherry-picks than that, or some manual edits, isn't it?

I can send a patch for 2.4 -- would you prefer the same as this --
i.e with switch(stat) in misc.c and and preprocessor macros in misc.h
or a simpler, return code = -2 and error message as in version 1?

Selva
<div dir="ltr"><div>Hi,<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Feb 28, 2019 at 12:25 PM Gert Doering &lt;<a href="mailto:gert@greenie.muc.de">gert@greenie.muc.de</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Acked-by: Gert Doering &lt;<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>&gt;<br>
<br>
Thanks.  I have not tested the various error conditions, but the code<br>
makes sense to me and it passes compile and t_client tests (which use<br>
--up here), so it&#39;s not breaking stuff in fundamental and non-obvious<br>
ways.<br>
<br>
(Since David was mostly happy with the v1 approach except for the<br>
&quot;127&quot; and &quot;switch()&quot;, I assume he&#39;s totally fine with v2 now :-) )<br>
<br>
Your patch has been applied to the master branch - it won&#39;t apply to<br>
2.4 as we have no &quot;run_command.*&quot; there yet.  Shall we cherrypick<br>
bf97c00f7dba4 and 253f015558 to get there?  Or do you want to do a<br>
2.4 version of the patch?<br></blockquote><div><br></div><div>Would require more cherry-picks than that, or some manual edits, isn&#39;t it?<br><br></div><div>I can send a patch for 2.4 -- would you prefer the same as this --<br>i.e with switch(stat) in misc.c and and preprocessor macros in misc.h<br>or a simpler, return code = -2 and error message as in version 1? <br><br></div><div>Selva <br></div></div></div>
Gert Doering Feb. 28, 2019, 7:25 a.m. UTC | #3 
Hi,

On Thu, Feb 28, 2019 at 12:33:46PM -0500, Selva Nair wrote:
> On Thu, Feb 28, 2019 at 12:25 PM Gert Doering <gert@greenie.muc.de> wrote:
> 
> > Your patch has been applied to the master branch - it won't apply to
> > 2.4 as we have no "run_command.*" there yet.  Shall we cherrypick
> > bf97c00f7dba4 and 253f015558 to get there?  Or do you want to do a
> > 2.4 version of the patch?
> 
> Would require more cherry-picks than that, or some manual edits, isn't it?

Yes, as we also have uncrustify patches that affect neighboring code.

> I can send a patch for 2.4 -- would you prefer the same as this --
> i.e with switch(stat) in misc.c and and preprocessor macros in misc.h
> or a simpler, return code = -2 and error message as in version 1?

I think we should have the same code (so, with the switch()), just in
misc.c - when we come here next time, at least it will be the same code,
not "something completely different".

thanks,

gert
David Sommerseth March 7, 2019, 4:59 a.m. UTC | #4 
On 28/02/2019 18:25, Gert Doering wrote:
> 
> (Since David was mostly happy with the v1 approach except for the
> "127" and "switch()", I assume he's totally fine with v2 now :-) )

Yeah, sorry I didn't manage to do the second review.  I've done it post-apply,
and it looks much better.  It would get an ACK from me without much fuzz.  Thanks!
diff mbox series

Patch

diff --git a/src/openvpn/run_command.c b/src/openvpn/run_command.c
index 2d75a3e..4c4adf9 100644
--- a/src/openvpn/run_command.c
+++ b/src/openvpn/run_command.c
@@ -54,44 +54,57 @@  script_security_set(int level)
 }
 
 /*
- * Print an error message based on the status code returned by system().
+ * Generate an error message based on the status code returned by openvpn_execve().
  */
 static const char *
 system_error_message(int stat, struct gc_arena *gc)
 {
     struct buffer out = alloc_buf_gc(256, gc);
-#ifdef _WIN32
-    if (stat == -1)
+
+    switch (stat)
     {
-        buf_printf(&out, "external program did not execute -- ");
-    }
-    buf_printf(&out, "returned error code %d", stat);
+        case OPENVPN_EXECVE_NOT_ALLOWED:
+            buf_printf(&out, "disallowed by script-security setting");
+            break;
+
+#ifdef _WIN32
+        case OPENVPN_EXECVE_ERROR:
+            buf_printf(&out, "external program did not execute -- ");
+        /* fall through */
+
+        default:
+            buf_printf(&out, "returned error code %d", stat);
+            break;
 #else  /* ifdef _WIN32 */
-    if (stat == -1)
-    {
-        buf_printf(&out, "external program fork failed");
-    }
-    else if (!WIFEXITED(stat))
-    {
-        buf_printf(&out, "external program did not exit normally");
-    }
-    else
-    {
-        const int cmd_ret = WEXITSTATUS(stat);
-        if (!cmd_ret)
-        {
-            buf_printf(&out, "external program exited normally");
-        }
-        else if (cmd_ret == 127)
-        {
-            buf_printf(&out, "could not execute external program");
-        }
-        else
-        {
-            buf_printf(&out, "external program exited with error status: %d", cmd_ret);
-        }
-    }
+
+        case OPENVPN_EXECVE_ERROR:
+            buf_printf(&out, "external program fork failed");
+            break;
+
+        default:
+            if (!WIFEXITED(stat))
+            {
+                buf_printf(&out, "external program did not exit normally");
+            }
+            else
+            {
+                const int cmd_ret = WEXITSTATUS(stat);
+                if (!cmd_ret)
+                {
+                    buf_printf(&out, "external program exited normally");
+                }
+                else if (cmd_ret == OPENVPN_EXECVE_FAILURE)
+                {
+                    buf_printf(&out, "could not execute external program");
+                }
+                else
+                {
+                    buf_printf(&out, "external program exited with error status: %d", cmd_ret);
+                }
+            }
+            break;
 #endif /* ifdef _WIN32 */
+    }
     return (const char *)out.data;
 }
 
@@ -114,12 +127,14 @@  openvpn_execve_allowed(const unsigned int flags)
  * Run execve() inside a fork().  Designed to replicate the semantics of system() but
  * in a safer way that doesn't require the invocation of a shell or the risks
  * associated with formatting and parsing a command line.
+ * Returns the exit status of child, OPENVPN_EXECVE_NOT_ALLOWED if openvpn_execve_allowed()
+ * returns false, or OPENVPN_EXECVE_ERROR on other errors.
  */
 int
 openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned int flags)
 {
     struct gc_arena gc = gc_new();
-    int ret = -1;
+    int ret = OPENVPN_EXECVE_ERROR;
     static bool warn_shown = false;
 
     if (a && a->argv[0])
@@ -136,7 +151,7 @@  openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned in
             if (pid == (pid_t)0) /* child side */
             {
                 execve(cmd, argv, envp);
-                exit(127);
+                exit(OPENVPN_EXECVE_FAILURE);
             }
             else if (pid < (pid_t)0) /* fork failed */
             {
@@ -146,14 +161,18 @@  openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned in
             {
                 if (waitpid(pid, &ret, 0) != pid)
                 {
-                    ret = -1;
+                    ret = OPENVPN_EXECVE_ERROR;
                 }
             }
         }
-        else if (!warn_shown && (script_security() < SSEC_SCRIPTS))
+        else
         {
-            msg(M_WARN, SCRIPT_SECURITY_WARNING);
-            warn_shown = true;
+            ret = OPENVPN_EXECVE_NOT_ALLOWED;
+            if (!warn_shown && (script_security() < SSEC_SCRIPTS))
+            {
+                msg(M_WARN, SCRIPT_SECURITY_WARNING);
+                warn_shown = true;
+            }
         }
 #else  /* if defined(ENABLE_FEATURE_EXECVE) */
         msg(M_WARN, "openvpn_execve: execve function not available");
@@ -227,7 +246,7 @@  openvpn_popen(const struct argv *a,  const struct env_set *es)
                     close(pipe_stdout[0]);         /* Close read end */
                     dup2(pipe_stdout[1],1);
                     execve(cmd, argv, envp);
-                    exit(127);
+                    exit(OPENVPN_EXECVE_FAILURE);
                 }
                 else if (pid > (pid_t)0)       /* parent side */
                 {
diff --git a/src/openvpn/run_command.h b/src/openvpn/run_command.h
index 4501a5c..7ccb13c 100644
--- a/src/openvpn/run_command.h
+++ b/src/openvpn/run_command.h
@@ -33,6 +33,10 @@ 
 #define SSEC_SCRIPTS   2 /* allow calling of built-in programs and user-defined scripts */
 #define SSEC_PW_ENV    3 /* allow calling of built-in programs and user-defined scripts that may receive a password as an environmental variable */
 
+#define OPENVPN_EXECVE_ERROR       -1 /* generic error while forking to run an external program */
+#define OPENVPN_EXECVE_NOT_ALLOWED -2 /* external program not run due to script security */
+#define OPENVPN_EXECVE_FAILURE    127 /* exit code passed back from child when execve fails */
+
 int script_security(void);
 
 void script_security_set(int level);
diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index 463ac07..55d9843 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -1088,7 +1088,7 @@  wide_cmd_line(const struct argv *a, struct gc_arena *gc)
 int
 openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned int flags)
 {
-    int ret = -1;
+    int ret = OPENVPN_EXECVE_ERROR;
     static bool exec_warn = false;
 
     if (a && a->argv[0])
@@ -1137,10 +1137,14 @@  openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned in
             free(env);
             gc_free(&gc);
         }
-        else if (!exec_warn && (script_security() < SSEC_SCRIPTS))
+        else
         {
-            msg(M_WARN, SCRIPT_SECURITY_WARNING);
-            exec_warn = true;
+            ret = OPENVPN_EXECVE_NOT_ALLOWED;
+            if (!exec_warn && (script_security() < SSEC_SCRIPTS))
+            {
+                msg(M_WARN, SCRIPT_SECURITY_WARNING);
+                exec_warn = true;
+            }
         }
     }
     else