From 7b8fd0022e6634ce24b8b00df7673af3827a0029 Mon Sep 17 00:00:00 2001 From: David Woodhouse Date: Thu, 30 Oct 2014 19:32:54 +0000 Subject: [PATCH] Fix thread-unsafe setenv() usage Since moving the tunnel handling into libopenconnect too, we've been unsafely using setenv() before spawning vpnc-script. We mustn't do that in a multi-threaded environment. So store up all the variables in a list, and apply them after fork(). Or on Windows, build up the environment block to pass to CreateProcess() manually. Signed-off-by: David Woodhouse --- compat.c | 37 ------- configure.ac | 2 - library.c | 5 +- openconnect-internal.h | 16 ++- script.c | 240 ++++++++++++++++++++++++++++++----------- tun.c | 5 +- 6 files changed, 188 insertions(+), 117 deletions(-) diff --git a/compat.c b/compat.c index 84510063..f2f1c6e3 100644 --- a/compat.c +++ b/compat.c @@ -183,43 +183,6 @@ char *openconnect__strndup(const char *s, size_t n) } #endif -#ifndef HAVE_SETENV -int openconnect__setenv(const char *name, const char *value, int overwrite) -{ - struct oc_text_buf *buf; - - if (!value) { - openconnect__unsetenv(name); - return 0; - } - - buf = buf_alloc(); - buf_append_utf16le(buf, name); - buf_append_utf16le(buf, "="); - buf_append_utf16le(buf, value); - if (buf_error(buf)) { - errno = -buf_free(buf); - return -1; - } - - /* Windows putenv() takes a copy of the string */ - _wputenv((wchar_t *)buf->data); - buf_free(buf); - - return 0; -} -#endif - -#ifndef HAVE_UNSETENV -void openconnect__unsetenv(const char *name) -{ - char *buf = alloca(strlen(name) + 2); - - sprintf(buf, "%s=", name); - putenv(buf); -} -#endif - #ifndef HAVE_INET_ATON int openconnect__inet_aton(const char *cp, struct in_addr *addr) { diff --git a/configure.ac b/configure.ac index 88ce3373..743c6ea7 100644 --- a/configure.ac +++ b/configure.ac @@ -177,8 +177,6 @@ fi AC_CHECK_FUNC(__android_log_vprint, [], AC_CHECK_LIB(log, __android_log_vprint, [], [])) -AC_CHECK_FUNCS(setenv unsetenv) - AC_ENABLE_SHARED AC_DISABLE_STATIC diff --git a/library.c b/library.c index 08fd6e4f..c0f2a1ce 100644 --- a/library.c +++ b/library.c @@ -178,6 +178,7 @@ void openconnect_vpninfo_free(struct openconnect_info *vpninfo) CloseHandle(vpninfo->dtls_event); #endif free(vpninfo->peer_addr); + free_optlist(vpninfo->script_env); free_optlist(vpninfo->cookies); free_optlist(vpninfo->cstp_options); free_optlist(vpninfo->dtls_options); @@ -657,7 +658,7 @@ int openconnect_setup_tun_device(struct openconnect_info *vpninfo, STRDUP(vpninfo->vpnc_script, vpnc_script); STRDUP(vpninfo->ifname, ifname); - set_script_env(vpninfo); + prepare_script_env(vpninfo); script_config_tun(vpninfo, "pre-init"); tun_fd = os_setup_tun(vpninfo); @@ -665,7 +666,7 @@ int openconnect_setup_tun_device(struct openconnect_info *vpninfo, return tun_fd; legacy_ifname = openconnect_utf8_to_legacy(vpninfo, vpninfo->ifname); - setenv("TUNDEV", legacy_ifname, 1); + script_setenv(vpninfo, "TUNDEV", legacy_ifname, 0); if (legacy_ifname != vpninfo->ifname) free(legacy_ifname); script_config_tun(vpninfo, "connect"); diff --git a/openconnect-internal.h b/openconnect-internal.h index 000eb171..4acbd53c 100644 --- a/openconnect-internal.h +++ b/openconnect-internal.h @@ -289,6 +289,8 @@ struct openconnect_info { struct oc_vpn_option *cstp_options; struct oc_vpn_option *dtls_options; + struct oc_vpn_option *script_env; + unsigned pfs; #if defined(OPENCONNECT_OPENSSL) X509 *cert_x509; @@ -493,14 +495,6 @@ char *openconnect__strcasestr(const char *haystack, const char *needle); #define strndup openconnect__strndup char *openconnect__strndup(const char *s, size_t n); #endif -#ifndef HAVE_SETENV -#define setenv openconnect__setenv -int openconnect__setenv(const char *name, const char *value, int overwrite); -#endif -#ifndef HAVE_UNSETENV -#define unsetenv openconnect__unsetenv -void openconnect__unsetenv(const char *name); -#endif #ifndef HAVE_INET_ATON #define inet_aton openconnect__inet_aton @@ -568,9 +562,11 @@ char *openconnect_legacy_to_utf8(struct openconnect_info *vpninfo, const char *l #endif /* script.c */ -int setenv_int(const char *opt, int value); -void set_script_env(struct openconnect_info *vpninfo); +int script_setenv(struct openconnect_info *vpninfo, const char *opt, const char *val, int append); +int script_setenv_int(struct openconnect_info *vpninfo, const char *opt, int value); +void prepare_script_env(struct openconnect_info *vpninfo); int script_config_tun(struct openconnect_info *vpninfo, const char *reason); +int apply_script_env(struct openconnect_info *vpninfo); /* tun.c / tun-win32.c */ void os_shutdown_tun(struct openconnect_info *vpninfo); diff --git a/script.c b/script.c index b1566dee..96744140 100644 --- a/script.c +++ b/script.c @@ -32,11 +32,40 @@ #include "openconnect-internal.h" -int setenv_int(const char *opt, int value) +int script_setenv(struct openconnect_info *vpninfo, + const char *opt, const char *val, int append) +{ + struct oc_vpn_option *p; + char *str; + + for (p = vpninfo->script_env; p; p = p->next) { + if (!strcmp(opt, p->option)) { + if (append) { + if (asprintf(&str, "%s %s", p->value, val) == -1) + return -ENOMEM; + } else + str = val ? strdup(val) : NULL; + + free (p->value); + p->value = str; + return 0; + } + } + p = malloc(sizeof(*p)); + if (!p) + return -ENOMEM; + p->next = vpninfo->script_env; + p->option = strdup(opt); + p->value = val ? strdup(val) : NULL; + vpninfo->script_env = p; + return 0; +} + +int script_setenv_int(struct openconnect_info *vpninfo, const char *opt, int value) { char buf[16]; sprintf(buf, "%d", value); - return setenv(opt, buf, 1); + return script_setenv(vpninfo, opt, buf, 0); } static int netmasklen(struct in_addr addr) @@ -78,11 +107,11 @@ static int process_split_xxclude(struct openconnect_info *vpninfo, if (strchr(route, ':')) { snprintf(envname, 79, "CISCO_IPV6_SPLIT_%sC_%d_ADDR", in_ex, *v6_incs); - setenv(envname, route, 1); + script_setenv(vpninfo, envname, route, 0); snprintf(envname, 79, "CISCO_IPV6_SPLIT_%sC_%d_MASKLEN", in_ex, *v6_incs); - setenv(envname, slash+1, 1); + script_setenv(vpninfo, envname, slash+1, 0); (*v6_incs)++; return 0; @@ -95,7 +124,7 @@ static int process_split_xxclude(struct openconnect_info *vpninfo, envname[79] = 0; snprintf(envname, 79, "CISCO_SPLIT_%sC_%d_ADDR", in_ex, *v4_incs); - setenv(envname, route, 1); + script_setenv(vpninfo, envname, route, 0); /* Put it back how we found it */ *slash = '/'; @@ -104,29 +133,15 @@ static int process_split_xxclude(struct openconnect_info *vpninfo, goto badinc; snprintf(envname, 79, "CISCO_SPLIT_%sC_%d_MASK", in_ex, *v4_incs); - setenv(envname, slash+1, 1); + script_setenv(vpninfo, envname, slash+1, 0); snprintf(envname, 79, "CISCO_SPLIT_%sC_%d_MASKLEN", in_ex, *v4_incs); - setenv_int(envname, netmasklen(addr)); + script_setenv_int(vpninfo, envname, netmasklen(addr)); (*v4_incs)++; return 0; } -static int appendenv(const char *opt, const char *new) -{ - char buf[1024]; - char *old = getenv(opt); - - buf[1023] = 0; - if (old) - snprintf(buf, 1023, "%s %s", old, new); - else - snprintf(buf, 1023, "%s", new); - - return setenv(opt, buf, 1); -} - static void setenv_cstp_opts(struct openconnect_info *vpninfo) { char *env_buf; @@ -147,7 +162,7 @@ static void setenv_cstp_opts(struct openconnect_info *vpninfo) bufofs += snprintf(env_buf + bufofs, buflen - bufofs, "%s=%s\n", opt->option, opt->value); - setenv("CISCO_CSTP_OPTIONS", env_buf, 1); + script_setenv(vpninfo, "CISCO_CSTP_OPTIONS", env_buf, 0); free(env_buf); } @@ -157,7 +172,7 @@ static void set_banner(struct openconnect_info *vpninfo) const char *p; if (!vpninfo->banner || !(banner = malloc(strlen(vpninfo->banner)+1))) { - unsetenv("CISCO_BANNER"); + script_setenv(vpninfo, "CISCO_BANNER", NULL, 0); return; } p = vpninfo->banner; @@ -173,29 +188,29 @@ static void set_banner(struct openconnect_info *vpninfo) } *q = 0; legacy_banner = openconnect_utf8_to_legacy(vpninfo, banner); - setenv("CISCO_BANNER", legacy_banner, 1); + script_setenv(vpninfo, "CISCO_BANNER", legacy_banner, 0); if (legacy_banner != banner) free(legacy_banner); free(banner); } -void set_script_env(struct openconnect_info *vpninfo) +void prepare_script_env(struct openconnect_info *vpninfo) { char host[80]; int ret = getnameinfo(vpninfo->peer_addr, vpninfo->peer_addrlen, host, sizeof(host), NULL, 0, NI_NUMERICHOST); if (!ret) - setenv("VPNGATEWAY", host, 1); + script_setenv(vpninfo, "VPNGATEWAY", host, 0); set_banner(vpninfo); - unsetenv("CISCO_SPLIT_INC"); - unsetenv("CISCO_SPLIT_EXC"); + script_setenv(vpninfo, "CISCO_SPLIT_INC", NULL, 0); + script_setenv(vpninfo, "CISCO_SPLIT_EXC", NULL, 0); - setenv_int("INTERNAL_IP4_MTU", vpninfo->ip_info.mtu); + script_setenv_int(vpninfo, "INTERNAL_IP4_MTU", vpninfo->ip_info.mtu); if (vpninfo->ip_info.addr) { - setenv("INTERNAL_IP4_ADDRESS", vpninfo->ip_info.addr, 1); + script_setenv(vpninfo, "INTERNAL_IP4_ADDRESS", vpninfo->ip_info.addr, 0); if (vpninfo->ip_info.netmask) { struct in_addr addr; struct in_addr mask; @@ -207,50 +222,50 @@ void set_script_env(struct openconnect_info *vpninfo) addr.s_addr &= mask.s_addr; netaddr = inet_ntoa(addr); - setenv("INTERNAL_IP4_NETADDR", netaddr, 1); - setenv("INTERNAL_IP4_NETMASK", vpninfo->ip_info.netmask, 1); - setenv_int("INTERNAL_IP4_NETMASKLEN", netmasklen(mask)); + script_setenv(vpninfo, "INTERNAL_IP4_NETADDR", netaddr, 0); + script_setenv(vpninfo, "INTERNAL_IP4_NETMASK", vpninfo->ip_info.netmask, 0); + script_setenv_int(vpninfo, "INTERNAL_IP4_NETMASKLEN", netmasklen(mask)); } } } if (vpninfo->ip_info.addr6) { - setenv("INTERNAL_IP6_ADDRESS", vpninfo->ip_info.addr6, 1); - setenv("INTERNAL_IP6_NETMASK", vpninfo->ip_info.netmask6, 1); + script_setenv(vpninfo, "INTERNAL_IP6_ADDRESS", vpninfo->ip_info.addr6, 0); + script_setenv(vpninfo, "INTERNAL_IP6_NETMASK", vpninfo->ip_info.netmask6, 0); } else if (vpninfo->ip_info.netmask6) { char *slash = strchr(vpninfo->ip_info.netmask6, '/'); - setenv("INTERNAL_IP6_NETMASK", vpninfo->ip_info.netmask6, 1); + script_setenv(vpninfo, "INTERNAL_IP6_NETMASK", vpninfo->ip_info.netmask6, 0); if (slash) { *slash = 0; - setenv("INTERNAL_IP6_ADDRESS", vpninfo->ip_info.netmask6, 1); + script_setenv(vpninfo, "INTERNAL_IP6_ADDRESS", vpninfo->ip_info.netmask6, 0); *slash = '/'; } } if (vpninfo->ip_info.dns[0]) - setenv("INTERNAL_IP4_DNS", vpninfo->ip_info.dns[0], 1); + script_setenv(vpninfo, "INTERNAL_IP4_DNS", vpninfo->ip_info.dns[0], 0); else - unsetenv("INTERNAL_IP4_DNS"); + script_setenv(vpninfo, "INTERNAL_IP4_DNS", NULL, 0); if (vpninfo->ip_info.dns[1]) - appendenv("INTERNAL_IP4_DNS", vpninfo->ip_info.dns[1]); + script_setenv(vpninfo, "INTERNAL_IP4_DNS", vpninfo->ip_info.dns[1], 1); if (vpninfo->ip_info.dns[2]) - appendenv("INTERNAL_IP4_DNS", vpninfo->ip_info.dns[2]); + script_setenv(vpninfo, "INTERNAL_IP4_DNS", vpninfo->ip_info.dns[2], 1); if (vpninfo->ip_info.nbns[0]) - setenv("INTERNAL_IP4_NBNS", vpninfo->ip_info.nbns[0], 1); + script_setenv(vpninfo, "INTERNAL_IP4_NBNS", vpninfo->ip_info.nbns[0], 0); else - unsetenv("INTERNAL_IP4_NBNS"); + script_setenv(vpninfo, "INTERNAL_IP4_NBNS", NULL, 0); if (vpninfo->ip_info.nbns[1]) - appendenv("INTERNAL_IP4_NBNS", vpninfo->ip_info.nbns[1]); + script_setenv(vpninfo, "INTERNAL_IP4_NBNS", vpninfo->ip_info.nbns[1], 1); if (vpninfo->ip_info.nbns[2]) - appendenv("INTERNAL_IP4_NBNS", vpninfo->ip_info.nbns[2]); + script_setenv(vpninfo, "INTERNAL_IP4_NBNS", vpninfo->ip_info.nbns[2], 1); if (vpninfo->ip_info.domain) - setenv("CISCO_DEF_DOMAIN", vpninfo->ip_info.domain, 1); + script_setenv(vpninfo, "CISCO_DEF_DOMAIN", vpninfo->ip_info.domain, 0); else - unsetenv("CISCO_DEF_DOMAIN"); + script_setenv(vpninfo, "CISCO_DEF_DOMAIN", NULL, 0); if (vpninfo->ip_info.proxy_pac) - setenv("CISCO_PROXY_PAC", vpninfo->ip_info.proxy_pac, 1); + script_setenv(vpninfo, "CISCO_PROXY_PAC", vpninfo->ip_info.proxy_pac, 0); if (vpninfo->ip_info.split_dns) { char *list; @@ -274,7 +289,7 @@ void set_script_env(struct openconnect_info *vpninfo) break; *(p++) = ','; } - setenv("CISCO_SPLIT_DNS", list, 1); + script_setenv(vpninfo, "CISCO_SPLIT_DNS", list, 0); free(list); } } @@ -290,9 +305,9 @@ void set_script_env(struct openconnect_info *vpninfo) this = this->next; } if (nr_split_includes) - setenv_int("CISCO_SPLIT_INC", nr_split_includes); + script_setenv_int(vpninfo, "CISCO_SPLIT_INC", nr_split_includes); if (nr_v6_split_includes) - setenv_int("CISCO_IPV6_SPLIT_INC", nr_v6_split_includes); + script_setenv_int(vpninfo, "CISCO_IPV6_SPLIT_INC", nr_v6_split_includes); } if (vpninfo->ip_info.split_excludes) { struct oc_split_include *this = vpninfo->ip_info.split_excludes; @@ -306,16 +321,89 @@ void set_script_env(struct openconnect_info *vpninfo) this = this->next; } if (nr_split_excludes) - setenv_int("CISCO_SPLIT_EXC", nr_split_excludes); + script_setenv_int(vpninfo, "CISCO_SPLIT_EXC", nr_split_excludes); if (nr_v6_split_excludes) - setenv_int("CISCO_IPV6_SPLIT_EXC", nr_v6_split_excludes); + script_setenv_int(vpninfo, "CISCO_IPV6_SPLIT_EXC", nr_v6_split_excludes); } setenv_cstp_opts(vpninfo); } + #ifdef _WIN32 +static wchar_t *create_script_env(struct openconnect_info *vpninfo) +{ + struct oc_vpn_option *opt; + struct oc_text_buf *envbuf; + wchar_t **oldenv, **p, *newenv = NULL; + int nr_envs = 0, i; + + /* _wenviron is NULL until we call _wgetenv() */ + (void)_wgetenv(L"PATH"); + + /* Take a copy of _wenviron (but not of its strings) */ + for (p = _wenviron; *p; p++) + nr_envs++; + + oldenv = malloc(nr_envs * sizeof(*oldenv)); + if (!oldenv) + return NULL; + memcpy(oldenv, _wenviron, nr_envs * sizeof(*oldenv)); + + envbuf = buf_alloc(); + + /* Add the script environment variables, prodding out any members of + oldenv which are obsoleted by them. */ + for (opt = vpninfo->script_env; opt && !buf_error(envbuf); opt = opt->next) { + struct oc_text_buf *buf; + + buf = buf_alloc(); + buf_append_utf16le(buf, opt->option); + buf_append_utf16le(buf, "="); + + if (buf_error(buf)) { + buf_free(buf); + goto err; + } + + /* See if we can find it in the existing environment */ + for (i = 0; i < nr_envs; i++) { + if (!wcsncmp((wchar_t *)buf->data, oldenv[i], buf->pos / 2)) { + oldenv[i] = NULL; + break; + } + } + + if (opt->value) { + buf_append_bytes(envbuf, buf->data, buf->pos); + buf_append_utf16le(envbuf, opt->value); + buf_append_bytes(envbuf, "\0\0", 2); + } + + buf_free(buf); + } + + for (i = 0; i < nr_envs && !buf_error(envbuf); i++) { + if (oldenv[i]) + buf_append_bytes(envbuf, oldenv[i], + (wcslen(oldenv[i]) + 1) * sizeof(wchar_t)); + } + + buf_append_bytes(envbuf, "\0\0", 2); + + if (!buf_error(envbuf)) { + newenv = (wchar_t *)envbuf->data; + envbuf->data = NULL; + } + + err: + free(oldenv); + buf_free(envbuf); + return newenv; +} + int script_config_tun(struct openconnect_info *vpninfo, const char *reason) { wchar_t *script_w; + wchar_t *script_env; int nr_chars; int ret; char *cmd; @@ -331,7 +419,7 @@ int script_config_tun(struct openconnect_info *vpninfo, const char *reason) si.dwFlags = STARTF_USESHOWWINDOW; si.wShowWindow = SW_HIDE; - setenv("reason", reason, 1); + script_setenv(vpninfo, "reason", reason, 0); if (asprintf(&cmd, "cscript.exe \"%s\"", vpninfo->vpnc_script) == -1) return 0; @@ -348,9 +436,11 @@ int script_config_tun(struct openconnect_info *vpninfo, const char *reason) free(cmd); - if(CreateProcessW(NULL, script_w, - NULL, NULL, FALSE, CREATE_NO_WINDOW, NULL, - NULL, &si, &pi)) { + script_env = create_script_env(vpninfo); + + if (CreateProcessW(NULL, script_w, NULL, NULL, FALSE, + CREATE_NO_WINDOW | CREATE_UNICODE_ENVIRONMENT, + script_env, NULL, &si, &pi)) { ret = WaitForSingleObject(pi.hProcess,10000); CloseHandle(pi.hThread); CloseHandle(pi.hProcess); @@ -362,6 +452,8 @@ int script_config_tun(struct openconnect_info *vpninfo, const char *reason) ret = -EIO; } + free(script_env); + if (ret < 0) { char *errstr = openconnect__win32_strerror(GetLastError()); vpn_progress(vpninfo, PRG_ERR, @@ -376,28 +468,48 @@ int script_config_tun(struct openconnect_info *vpninfo, const char *reason) return ret; } #else +/* Must only be run after fork(). */ +int apply_script_env(struct openconnect_info *vpninfo) +{ + struct oc_vpn_option *p = vpninfo->script_env; + + for (p = vpninfo->script_env; p; p = p->next) { + if (p->value) + setenv(p->option, p->value, 1); + else + unsetenv(p->option); + } + return 0; +} + int script_config_tun(struct openconnect_info *vpninfo, const char *reason) { int ret; - char *script; + pid_t pid; if (!vpninfo->vpnc_script || vpninfo->script_tun) return 0; - setenv("reason", reason, 1); + pid = fork(); + if (!pid) { + /* Child */ + char *script = openconnect_utf8_to_legacy(vpninfo, vpninfo->vpnc_script); - script = openconnect_utf8_to_legacy(vpninfo, vpninfo->vpnc_script); - ret = system(script); - if (script != vpninfo->vpnc_script) - free(script); + apply_script_env(vpninfo); - if (ret == -1) { + setenv("reason", reason, 1); + + execl("/bin/sh", "/bin/sh", "-c", script, NULL); + exit(127); + } + if (pid == -1 || waitpid(pid, &ret, 0) == -1) { int e = errno; vpn_progress(vpninfo, PRG_ERR, _("Failed to spawn script '%s' for %s: %s\n"), vpninfo->vpnc_script, reason, strerror(e)); return -e; } + if (!WIFEXITED(ret)) { vpn_progress(vpninfo, PRG_ERR, _("Script '%s' exited abnormally (%x)\n"), diff --git a/tun.c b/tun.c index b2d72f0b..d65d23bc 100644 --- a/tun.c +++ b/tun.c @@ -461,7 +461,7 @@ int openconnect_setup_tun_script(struct openconnect_info *vpninfo, STRDUP(vpninfo->vpnc_script, tun_script); vpninfo->script_tun = 1; - set_script_env(vpninfo); + prepare_script_env(vpninfo); if (socketpair(AF_UNIX, SOCK_DGRAM, 0, fds)) { vpn_progress(vpninfo, PRG_ERR, _("socketpair failed: %s\n"), strerror(errno)); return -EIO; @@ -474,7 +474,8 @@ int openconnect_setup_tun_script(struct openconnect_info *vpninfo, if (setpgid(0, getpid()) < 0) vpn_perror(vpninfo, _("setpgid")); close(fds[0]); - setenv_int("VPNFD", fds[1]); + script_setenv_int(vpninfo, "VPNFD", fds[1]); + apply_script_env(vpninfo); execl("/bin/sh", "/bin/sh", "-c", vpninfo->vpnc_script, NULL); vpn_perror(vpninfo, _("execl")); exit(1);