Browse code

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

- 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>
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>

Selva Nair authored on 2019/02/21 09:46:22
Showing 3 changed files
... ...
@@ -54,44 +54,57 @@ script_security_set(int level)
54 54
 }
55 55
 
56 56
 /*
57
- * Print an error message based on the status code returned by system().
57
+ * Generate an error message based on the status code returned by openvpn_execve().
58 58
  */
59 59
 static const char *
60 60
 system_error_message(int stat, struct gc_arena *gc)
61 61
 {
62 62
     struct buffer out = alloc_buf_gc(256, gc);
63
-#ifdef _WIN32
64
-    if (stat == -1)
63
+
64
+    switch (stat)
65 65
     {
66
-        buf_printf(&out, "external program did not execute -- ");
67
-    }
68
-    buf_printf(&out, "returned error code %d", stat);
66
+        case OPENVPN_EXECVE_NOT_ALLOWED:
67
+            buf_printf(&out, "disallowed by script-security setting");
68
+            break;
69
+
70
+#ifdef _WIN32
71
+        case OPENVPN_EXECVE_ERROR:
72
+            buf_printf(&out, "external program did not execute -- ");
73
+        /* fall through */
74
+
75
+        default:
76
+            buf_printf(&out, "returned error code %d", stat);
77
+            break;
69 78
 #else  /* ifdef _WIN32 */
70
-    if (stat == -1)
71
-    {
72
-        buf_printf(&out, "external program fork failed");
73
-    }
74
-    else if (!WIFEXITED(stat))
75
-    {
76
-        buf_printf(&out, "external program did not exit normally");
77
-    }
78
-    else
79
-    {
80
-        const int cmd_ret = WEXITSTATUS(stat);
81
-        if (!cmd_ret)
82
-        {
83
-            buf_printf(&out, "external program exited normally");
84
-        }
85
-        else if (cmd_ret == 127)
86
-        {
87
-            buf_printf(&out, "could not execute external program");
88
-        }
89
-        else
90
-        {
91
-            buf_printf(&out, "external program exited with error status: %d", cmd_ret);
92
-        }
93
-    }
79
+
80
+        case OPENVPN_EXECVE_ERROR:
81
+            buf_printf(&out, "external program fork failed");
82
+            break;
83
+
84
+        default:
85
+            if (!WIFEXITED(stat))
86
+            {
87
+                buf_printf(&out, "external program did not exit normally");
88
+            }
89
+            else
90
+            {
91
+                const int cmd_ret = WEXITSTATUS(stat);
92
+                if (!cmd_ret)
93
+                {
94
+                    buf_printf(&out, "external program exited normally");
95
+                }
96
+                else if (cmd_ret == OPENVPN_EXECVE_FAILURE)
97
+                {
98
+                    buf_printf(&out, "could not execute external program");
99
+                }
100
+                else
101
+                {
102
+                    buf_printf(&out, "external program exited with error status: %d", cmd_ret);
103
+                }
104
+            }
105
+            break;
94 106
 #endif /* ifdef _WIN32 */
107
+    }
95 108
     return (const char *)out.data;
96 109
 }
97 110
 
... ...
@@ -114,12 +127,14 @@ openvpn_execve_allowed(const unsigned int flags)
114 114
  * Run execve() inside a fork().  Designed to replicate the semantics of system() but
115 115
  * in a safer way that doesn't require the invocation of a shell or the risks
116 116
  * associated with formatting and parsing a command line.
117
+ * Returns the exit status of child, OPENVPN_EXECVE_NOT_ALLOWED if openvpn_execve_allowed()
118
+ * returns false, or OPENVPN_EXECVE_ERROR on other errors.
117 119
  */
118 120
 int
119 121
 openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned int flags)
120 122
 {
121 123
     struct gc_arena gc = gc_new();
122
-    int ret = -1;
124
+    int ret = OPENVPN_EXECVE_ERROR;
123 125
     static bool warn_shown = false;
124 126
 
125 127
     if (a && a->argv[0])
... ...
@@ -136,7 +151,7 @@ openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned in
136 136
             if (pid == (pid_t)0) /* child side */
137 137
             {
138 138
                 execve(cmd, argv, envp);
139
-                exit(127);
139
+                exit(OPENVPN_EXECVE_FAILURE);
140 140
             }
141 141
             else if (pid < (pid_t)0) /* fork failed */
142 142
             {
... ...
@@ -146,14 +161,18 @@ openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned in
146 146
             {
147 147
                 if (waitpid(pid, &ret, 0) != pid)
148 148
                 {
149
-                    ret = -1;
149
+                    ret = OPENVPN_EXECVE_ERROR;
150 150
                 }
151 151
             }
152 152
         }
153
-        else if (!warn_shown && (script_security() < SSEC_SCRIPTS))
153
+        else
154 154
         {
155
-            msg(M_WARN, SCRIPT_SECURITY_WARNING);
156
-            warn_shown = true;
155
+            ret = OPENVPN_EXECVE_NOT_ALLOWED;
156
+            if (!warn_shown && (script_security() < SSEC_SCRIPTS))
157
+            {
158
+                msg(M_WARN, SCRIPT_SECURITY_WARNING);
159
+                warn_shown = true;
160
+            }
157 161
         }
158 162
 #else  /* if defined(ENABLE_FEATURE_EXECVE) */
159 163
         msg(M_WARN, "openvpn_execve: execve function not available");
... ...
@@ -227,7 +246,7 @@ openvpn_popen(const struct argv *a,  const struct env_set *es)
227 227
                     close(pipe_stdout[0]);         /* Close read end */
228 228
                     dup2(pipe_stdout[1],1);
229 229
                     execve(cmd, argv, envp);
230
-                    exit(127);
230
+                    exit(OPENVPN_EXECVE_FAILURE);
231 231
                 }
232 232
                 else if (pid > (pid_t)0)       /* parent side */
233 233
                 {
... ...
@@ -33,6 +33,10 @@
33 33
 #define SSEC_SCRIPTS   2 /* allow calling of built-in programs and user-defined scripts */
34 34
 #define SSEC_PW_ENV    3 /* allow calling of built-in programs and user-defined scripts that may receive a password as an environmental variable */
35 35
 
36
+#define OPENVPN_EXECVE_ERROR       -1 /* generic error while forking to run an external program */
37
+#define OPENVPN_EXECVE_NOT_ALLOWED -2 /* external program not run due to script security */
38
+#define OPENVPN_EXECVE_FAILURE    127 /* exit code passed back from child when execve fails */
39
+
36 40
 int script_security(void);
37 41
 
38 42
 void script_security_set(int level);
... ...
@@ -1088,7 +1088,7 @@ wide_cmd_line(const struct argv *a, struct gc_arena *gc)
1088 1088
 int
1089 1089
 openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned int flags)
1090 1090
 {
1091
-    int ret = -1;
1091
+    int ret = OPENVPN_EXECVE_ERROR;
1092 1092
     static bool exec_warn = false;
1093 1093
 
1094 1094
     if (a && a->argv[0])
... ...
@@ -1137,10 +1137,14 @@ openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned in
1137 1137
             free(env);
1138 1138
             gc_free(&gc);
1139 1139
         }
1140
-        else if (!exec_warn && (script_security() < SSEC_SCRIPTS))
1140
+        else
1141 1141
         {
1142
-            msg(M_WARN, SCRIPT_SECURITY_WARNING);
1143
-            exec_warn = true;
1142
+            ret = OPENVPN_EXECVE_NOT_ALLOWED;
1143
+            if (!exec_warn && (script_security() < SSEC_SCRIPTS))
1144
+            {
1145
+                msg(M_WARN, SCRIPT_SECURITY_WARNING);
1146
+                exec_warn = true;
1147
+            }
1144 1148
         }
1145 1149
     }
1146 1150
     else