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 |
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
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 <<a href="mailto:gert@greenie.muc.de">gert@greenie.muc.de</a>> 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 <<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>><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'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> "127" and "switch()", I assume he's totally fine with v2 now :-) )<br> <br> Your patch has been applied to the master branch - it won't apply to<br> 2.4 as we have no "run_command.*" 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'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>
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
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 --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