[Openvpn-devel,07/10] Move env helper functions into their own module/file

Message ID 1512734870-17133-8-git-send-email-steffan.karger@fox-it.com
State Accepted
Headers show
Series Client-specific tls-crypt keys (--tls-crypt-v2) | expand

Commit Message

Steffan Karger Dec. 8, 2017, 1:07 a.m. UTC
To avoid a dependency on misc.c - which is a dependency mess - in the
tls-crypt unit tests, split the env_set functionality out into it's own
file.

Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
---
 src/openvpn/Makefile.am              |   1 +
 src/openvpn/argv.c                   |   1 +
 src/openvpn/env_set.c                | 459 +++++++++++++++++++++++++++++++++++
 src/openvpn/env_set.h                | 121 +++++++++
 src/openvpn/misc.c                   | 441 ---------------------------------
 src/openvpn/misc.h                   |  73 +-----
 tests/unit_tests/openvpn/Makefile.am |   1 +
 7 files changed, 584 insertions(+), 513 deletions(-)
 create mode 100644 src/openvpn/env_set.c
 create mode 100644 src/openvpn/env_set.h

Comments

Gert Doering March 7, 2018, 3:21 a.m. UTC | #1
Hi,

On Fri, Dec 08, 2017 at 01:07:47PM +0100, Steffan Karger wrote:
> To avoid a dependency on misc.c - which is a dependency mess - in the
> tls-crypt unit tests, split the env_set functionality out into it's own
> file.
> 
> Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
> Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>

*sigh*

Not exactly sure what to do with this one - I was about to ACK and merge
this, but it fails cmocka building

tls_crypt_testdriver-env_set.o: In function `env_allowed':
/tmp/openvpn-236/tests/unit_tests/openvpn/../../../src/openvpn/env_set.c:417: undefined reference to `script_security'
/tmp/openvpn-236/tests/unit_tests/openvpn/../../../src/openvpn/env_set.c:417: undefined reference to `script_security'

... because *that* change is in 9/10...


So while the change itself is fine (besides the "%lld" to PRIi64 change
in setenv_long_long() that came in with 06ad53e067d9a8b) it will cause
lots of build fails for the foreseeable future, so I'm not sure it's 
a good plan.

gert
Gert Doering March 8, 2018, 5:02 a.m. UTC | #2
Acked-by: Gert Doering <gert@greenie.muc.de>

Does what it says on the lid: code is moved 1:1 with no code changes, 
and only #include lines are adjusted elsewhere (which is actually painful
to review in this case, as the "moved out" stuff was scattered along
misc.c and parts ended in env_set.h, so "plain diff" between old-misc.c 
and new-env_set.c didn't help much).

One small catch: this collided with 06ad53e067d9a8b and setenv_long_long()
being changed from "%lld" to PRIi64 - so I changed the patch accordingly
so that it still only moves lines from misc.c to env_set.c

Your patch has been applied to the master branch.

commit 0afe0ca5620d5ab7cec3380d40048dce2286f048
Author: Steffan Karger
Date:   Fri Dec 8 13:07:47 2017 +0100

     Move env helper functions into their own module/file

     Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
     Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <1512734870-17133-8-git-send-email-steffan.karger@fox-it.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16066.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering March 8, 2018, 5:03 a.m. UTC | #3
Acked-by: Gert Doering <gert@greenie.muc.de>

Does what it says on the lid: code is moved 1:1 with no code changes, 
and only #include lines are adjusted elsewhere (which is actually painful
to review in this case, as the "moved out" stuff was scattered along
misc.c and parts ended in env_set.h, so "plain diff" between old-misc.c 
and new-env_set.c didn't help much.  "git show --color-moved" helps, tho).

Two small catches: 

 - this collided with 06ad53e067d9a8b and setenv_long_long() being 
   changed from "%lld" to PRIi64 - so I changed the patch accordingly
   so that it still only moves lines from misc.c to env_set.c

 - the hunk that adds env_set.c to "some of the test modules" caused
   build fails here (because it was added to the wrong module, assuming
   test code that wasn't there yet) - so I've removed it.  I'm aware
   that this will cause build failures later on when the test driver in
   question shows up, but then I'll re-add it.  "Building test code"
   follows slightly easier rules on "changing patches" :-)

Your patch has been applied to the master branch.

commit 68b97b25e4c479156d697bf3df90a4b68fbbbcea
Author: Steffan Karger
Date:   Fri Dec 8 13:07:47 2017 +0100

     Move env helper functions into their own module/file

     Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
     Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <1512734870-17133-8-git-send-email-steffan.karger@fox-it.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16066.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering March 8, 2018, 5:05 a.m. UTC | #4
Hi,

*this* mail should have never gone out, it was the first draft yesterday
after which I noticed that it failed cmocka testing...  so, please ignore
it.

git head is

commit 68b97b25e4c479156d697bf3df90a4b68fbbbcea (HEAD -> master, stable/master, gitlab/master, github/master, delta2/master)
Author: Steffan Karger <steffan.karger@fox-it.com>
Date:   Fri Dec 8 13:07:47 2017 +0100

    Move env helper functions into their own module/file


On Thu, Mar 08, 2018 at 05:02:58PM +0100, Gert Doering wrote:
> Acked-by: Gert Doering <gert@greenie.muc.de>
> 
> Does what it says on the lid: code is moved 1:1 with no code changes, 
> and only #include lines are adjusted elsewhere (which is actually painful
> to review in this case, as the "moved out" stuff was scattered along
> misc.c and parts ended in env_set.h, so "plain diff" between old-misc.c 
> and new-env_set.c didn't help much).
> 
> One small catch: this collided with 06ad53e067d9a8b and setenv_long_long()
> being changed from "%lld" to PRIi64 - so I changed the patch accordingly
> so that it still only moves lines from misc.c to env_set.c
> 
> Your patch has been applied to the master branch.
> 
> commit 0afe0ca5620d5ab7cec3380d40048dce2286f048
> Author: Steffan Karger
> Date:   Fri Dec 8 13:07:47 2017 +0100
> 
>      Move env helper functions into their own module/file
> 
>      Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
>      Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
>      Acked-by: Gert Doering <gert@greenie.muc.de>
>      Message-Id: <1512734870-17133-8-git-send-email-steffan.karger@fox-it.com>
>      URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16066.html
>      Signed-off-by: Gert Doering <gert@greenie.muc.de>
> 
> 
> --
> kind regards,
> 
> Gert Doering
> 
> 
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>

Patch

diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am
index fcc22d6..441700a 100644
--- a/src/openvpn/Makefile.am
+++ b/src/openvpn/Makefile.am
@@ -51,6 +51,7 @@  openvpn_SOURCES = \
 	crypto_openssl.c crypto_openssl.h \
 	crypto_mbedtls.c crypto_mbedtls.h \
 	dhcp.c dhcp.h \
+	env_set.c env_set.h \
 	errlevel.h \
 	error.c error.h \
 	event.c event.h \
diff --git a/src/openvpn/argv.c b/src/openvpn/argv.c
index 95bdfea..7a41169 100644
--- a/src/openvpn/argv.c
+++ b/src/openvpn/argv.c
@@ -37,6 +37,7 @@ 
 
 #include "argv.h"
 #include "integer.h"
+#include "env_set.h"
 #include "options.h"
 
 static void
diff --git a/src/openvpn/env_set.c b/src/openvpn/env_set.c
new file mode 100644
index 0000000..2a22e0b
--- /dev/null
+++ b/src/openvpn/env_set.c
@@ -0,0 +1,459 @@ 
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ *             over a single TCP/UDP port, with support for SSL/TLS-based
+ *             session authentication and key exchange,
+ *             packet encryption, packet authentication, and
+ *             packet compression.
+ *
+ *  Copyright (C) 2002-2017 OpenVPN Technologies, Inc. <sales@openvpn.net>
+ *  Copyright (C) 2014-2015 David Sommerseth <davids@redhat.com>
+ *  Copyright (C) 2016-2017 David Sommerseth <davids@openvpn.net>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program (see the file COPYING included with this
+ *  distribution); if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include "syshead.h"
+
+#include "misc.h"
+
+#include "env_set.h"
+
+/*
+ * Set environmental variable (int or string).
+ *
+ * On Posix, we use putenv for portability,
+ * and put up with its painful semantics
+ * that require all the support code below.
+ */
+
+/* General-purpose environmental variable set functions */
+
+static char *
+construct_name_value(const char *name, const char *value, struct gc_arena *gc)
+{
+    struct buffer out;
+
+    ASSERT(name);
+    if (!value)
+    {
+        value = "";
+    }
+    out = alloc_buf_gc(strlen(name) + strlen(value) + 2, gc);
+    buf_printf(&out, "%s=%s", name, value);
+    return BSTR(&out);
+}
+
+static bool
+env_string_equal(const char *s1, const char *s2)
+{
+    int c1, c2;
+    ASSERT(s1);
+    ASSERT(s2);
+
+    while (true)
+    {
+        c1 = *s1++;
+        c2 = *s2++;
+        if (c1 == '=')
+        {
+            c1 = 0;
+        }
+        if (c2 == '=')
+        {
+            c2 = 0;
+        }
+        if (!c1 && !c2)
+        {
+            return true;
+        }
+        if (c1 != c2)
+        {
+            break;
+        }
+    }
+    return false;
+}
+
+static bool
+remove_env_item(const char *str, const bool do_free, struct env_item **list)
+{
+    struct env_item *current, *prev;
+
+    ASSERT(str);
+    ASSERT(list);
+
+    for (current = *list, prev = NULL; current != NULL; current = current->next)
+    {
+        if (env_string_equal(current->string, str))
+        {
+            if (prev)
+            {
+                prev->next = current->next;
+            }
+            else
+            {
+                *list = current->next;
+            }
+            if (do_free)
+            {
+                secure_memzero(current->string, strlen(current->string));
+                free(current->string);
+                free(current);
+            }
+            return true;
+        }
+        prev = current;
+    }
+    return false;
+}
+
+static void
+add_env_item(char *str, const bool do_alloc, struct env_item **list, struct gc_arena *gc)
+{
+    struct env_item *item;
+
+    ASSERT(str);
+    ASSERT(list);
+
+    ALLOC_OBJ_GC(item, struct env_item, gc);
+    item->string = do_alloc ? string_alloc(str, gc) : str;
+    item->next = *list;
+    *list = item;
+}
+
+/* struct env_set functions */
+
+static bool
+env_set_del_nolock(struct env_set *es, const char *str)
+{
+    return remove_env_item(str, es->gc == NULL, &es->list);
+}
+
+static void
+env_set_add_nolock(struct env_set *es, const char *str)
+{
+    remove_env_item(str, es->gc == NULL, &es->list);
+    add_env_item((char *)str, true, &es->list, es->gc);
+}
+
+struct env_set *
+env_set_create(struct gc_arena *gc)
+{
+    struct env_set *es;
+    ALLOC_OBJ_CLEAR_GC(es, struct env_set, gc);
+    es->list = NULL;
+    es->gc = gc;
+    return es;
+}
+
+void
+env_set_destroy(struct env_set *es)
+{
+    if (es && es->gc == NULL)
+    {
+        struct env_item *e = es->list;
+        while (e)
+        {
+            struct env_item *next = e->next;
+            free(e->string);
+            free(e);
+            e = next;
+        }
+        free(es);
+    }
+}
+
+bool
+env_set_del(struct env_set *es, const char *str)
+{
+    bool ret;
+    ASSERT(es);
+    ASSERT(str);
+    ret = env_set_del_nolock(es, str);
+    return ret;
+}
+
+void
+env_set_add(struct env_set *es, const char *str)
+{
+    ASSERT(es);
+    ASSERT(str);
+    env_set_add_nolock(es, str);
+}
+
+const char *
+env_set_get(const struct env_set *es, const char *name)
+{
+    const struct env_item *item = es->list;
+    while (item && !env_string_equal(item->string, name))
+    {
+        item = item->next;
+    }
+    return item ? item->string : NULL;
+}
+
+void
+env_set_print(int msglevel, const struct env_set *es)
+{
+    if (check_debug_level(msglevel))
+    {
+        const struct env_item *e;
+        int i;
+
+        if (es)
+        {
+            e = es->list;
+            i = 0;
+
+            while (e)
+            {
+                if (env_safe_to_print(e->string))
+                {
+                    msg(msglevel, "ENV [%d] '%s'", i, e->string);
+                }
+                ++i;
+                e = e->next;
+            }
+        }
+    }
+}
+
+void
+env_set_inherit(struct env_set *es, const struct env_set *src)
+{
+    const struct env_item *e;
+
+    ASSERT(es);
+
+    if (src)
+    {
+        e = src->list;
+        while (e)
+        {
+            env_set_add_nolock(es, e->string);
+            e = e->next;
+        }
+    }
+}
+
+
+/* add/modify/delete environmental strings */
+
+void
+setenv_counter(struct env_set *es, const char *name, counter_type value)
+{
+    char buf[64];
+    openvpn_snprintf(buf, sizeof(buf), counter_format, value);
+    setenv_str(es, name, buf);
+}
+
+void
+setenv_int(struct env_set *es, const char *name, int value)
+{
+    char buf[64];
+    openvpn_snprintf(buf, sizeof(buf), "%d", value);
+    setenv_str(es, name, buf);
+}
+
+void
+setenv_long_long(struct env_set *es, const char *name, long long value)
+{
+    char buf[64];
+    openvpn_snprintf(buf, sizeof(buf), "%lld", value);
+    setenv_str(es, name, buf);
+}
+
+void
+setenv_str(struct env_set *es, const char *name, const char *value)
+{
+    setenv_str_ex(es, name, value, CC_NAME, 0, 0, CC_PRINT, 0, 0);
+}
+
+void
+setenv_str_safe(struct env_set *es, const char *name, const char *value)
+{
+    uint8_t b[64];
+    struct buffer buf;
+    buf_set_write(&buf, b, sizeof(b));
+    if (buf_printf(&buf, "OPENVPN_%s", name))
+    {
+        setenv_str(es, BSTR(&buf), value);
+    }
+    else
+    {
+        msg(M_WARN, "setenv_str_safe: name overflow");
+    }
+}
+
+void
+setenv_str_incr(struct env_set *es, const char *name, const char *value)
+{
+    unsigned int counter = 1;
+    const size_t tmpname_len = strlen(name) + 5; /* 3 digits counter max */
+    char *tmpname = gc_malloc(tmpname_len, true, NULL);
+    strcpy(tmpname, name);
+    while (NULL != env_set_get(es, tmpname) && counter < 1000)
+    {
+        ASSERT(openvpn_snprintf(tmpname, tmpname_len, "%s_%u", name, counter));
+        counter++;
+    }
+    if (counter < 1000)
+    {
+        setenv_str(es, tmpname, value);
+    }
+    else
+    {
+        msg(D_TLS_DEBUG_MED, "Too many same-name env variables, ignoring: %s", name);
+    }
+    free(tmpname);
+}
+
+void
+setenv_del(struct env_set *es, const char *name)
+{
+    ASSERT(name);
+    setenv_str(es, name, NULL);
+}
+
+void
+setenv_str_ex(struct env_set *es,
+              const char *name,
+              const char *value,
+              const unsigned int name_include,
+              const unsigned int name_exclude,
+              const char name_replace,
+              const unsigned int value_include,
+              const unsigned int value_exclude,
+              const char value_replace)
+{
+    struct gc_arena gc = gc_new();
+    const char *name_tmp;
+    const char *val_tmp = NULL;
+
+    ASSERT(name && strlen(name) > 1);
+
+    name_tmp = string_mod_const(name, name_include, name_exclude, name_replace, &gc);
+
+    if (value)
+    {
+        val_tmp = string_mod_const(value, value_include, value_exclude, value_replace, &gc);
+    }
+
+    ASSERT(es);
+
+    if (val_tmp)
+    {
+        const char *str = construct_name_value(name_tmp, val_tmp, &gc);
+        env_set_add(es, str);
+#if DEBUG_VERBOSE_SETENV
+        msg(M_INFO, "SETENV_ES '%s'", str);
+#endif
+    }
+    else
+    {
+        env_set_del(es, name_tmp);
+    }
+
+    gc_free(&gc);
+}
+
+/*
+ * Setenv functions that append an integer index to the name
+ */
+static const char *
+setenv_format_indexed_name(const char *name, const int i, struct gc_arena *gc)
+{
+    struct buffer out = alloc_buf_gc(strlen(name) + 16, gc);
+    if (i >= 0)
+    {
+        buf_printf(&out, "%s_%d", name, i);
+    }
+    else
+    {
+        buf_printf(&out, "%s", name);
+    }
+    return BSTR(&out);
+}
+
+void
+setenv_int_i(struct env_set *es, const char *name, const int value, const int i)
+{
+    struct gc_arena gc = gc_new();
+    const char *name_str = setenv_format_indexed_name(name, i, &gc);
+    setenv_int(es, name_str, value);
+    gc_free(&gc);
+}
+
+void
+setenv_str_i(struct env_set *es, const char *name, const char *value, const int i)
+{
+    struct gc_arena gc = gc_new();
+    const char *name_str = setenv_format_indexed_name(name, i, &gc);
+    setenv_str(es, name_str, value);
+    gc_free(&gc);
+}
+
+bool
+env_allowed(const char *str)
+{
+    return (script_security >= SSEC_PW_ENV || !is_password_env_var(str));
+}
+
+/* Make arrays of strings */
+
+const char **
+make_env_array(const struct env_set *es,
+               const bool check_allowed,
+               struct gc_arena *gc)
+{
+    char **ret = NULL;
+    struct env_item *e = NULL;
+    int i = 0, n = 0;
+
+    /* figure length of es */
+    if (es)
+    {
+        for (e = es->list; e != NULL; e = e->next)
+        {
+            ++n;
+        }
+    }
+
+    /* alloc return array */
+    ALLOC_ARRAY_CLEAR_GC(ret, char *, n+1, gc);
+
+    /* fill return array */
+    if (es)
+    {
+        i = 0;
+        for (e = es->list; e != NULL; e = e->next)
+        {
+            if (!check_allowed || env_allowed(e->string))
+            {
+                ASSERT(i < n);
+                ret[i++] = e->string;
+            }
+        }
+    }
+
+    ret[i] = NULL;
+    return (const char **)ret;
+}
diff --git a/src/openvpn/env_set.h b/src/openvpn/env_set.h
new file mode 100644
index 0000000..5dc3348
--- /dev/null
+++ b/src/openvpn/env_set.h
@@ -0,0 +1,121 @@ 
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ *             over a single TCP/UDP port, with support for SSL/TLS-based
+ *             session authentication and key exchange,
+ *             packet encryption, packet authentication, and
+ *             packet compression.
+ *
+ *  Copyright (C) 2002-2017 OpenVPN Technologies, Inc. <sales@openvpn.net>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program (see the file COPYING included with this
+ *  distribution); if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#ifndef ENV_SET_H
+#define ENV_SET_H
+
+#include "argv.h"
+#include "basic.h"
+#include "buffer.h"
+#include "common.h"
+
+/*
+ * Handle environmental variable lists
+ */
+
+struct env_item {
+    char *string;
+    struct env_item *next;
+};
+
+struct env_set {
+    struct gc_arena *gc;
+    struct env_item *list;
+};
+
+/* set/delete environmental variable */
+void setenv_str_ex(struct env_set *es,
+                   const char *name,
+                   const char *value,
+                   const unsigned int name_include,
+                   const unsigned int name_exclude,
+                   const char name_replace,
+                   const unsigned int value_include,
+                   const unsigned int value_exclude,
+                   const char value_replace);
+
+void setenv_counter(struct env_set *es, const char *name, counter_type value);
+
+void setenv_int(struct env_set *es, const char *name, int value);
+
+void setenv_long_long(struct env_set *es, const char *name, long long value);
+
+void setenv_str(struct env_set *es, const char *name, const char *value);
+
+void setenv_str_safe(struct env_set *es, const char *name, const char *value);
+
+void setenv_del(struct env_set *es, const char *name);
+
+/**
+ * Store the supplied name value pair in the env_set.  If the variable with the
+ * supplied name  already exists, append _N to the name, starting at N=1.
+ */
+void setenv_str_incr(struct env_set *es, const char *name, const char *value);
+
+void setenv_int_i(struct env_set *es, const char *name, const int value, const int i);
+
+void setenv_str_i(struct env_set *es, const char *name, const char *value, const int i);
+
+/* struct env_set functions */
+
+struct env_set *env_set_create(struct gc_arena *gc);
+
+void env_set_destroy(struct env_set *es);
+
+bool env_set_del(struct env_set *es, const char *str);
+
+void env_set_add(struct env_set *es, const char *str);
+
+const char *env_set_get(const struct env_set *es, const char *name);
+
+void env_set_print(int msglevel, const struct env_set *es);
+
+void env_set_inherit(struct env_set *es, const struct env_set *src);
+
+/* returns true if environmental variable name starts with 'password' */
+static inline bool is_password_env_var(const char *str)
+{
+    return (strncmp(str, "password", 8) == 0);
+}
+
+/* returns true if environmental variable safe to print to log */
+static inline bool env_safe_to_print(const char *str)
+{
+#ifndef UNSAFE_DEBUG
+    if (is_password_env_var(str))
+    {
+        return false;
+    }
+#endif
+    return true;
+}
+
+/* returns true if environmental variable may be passed to an external program */
+bool env_allowed(const char *str);
+
+const char **make_env_array(const struct env_set *es,
+                            const bool check_allowed,
+                            struct gc_arena *gc);
+
+#endif /* ifndef ENV_SET_H */
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 76b592f..ad26f3d 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -314,382 +314,6 @@  openvpn_popen(const struct argv *a,  const struct env_set *es)
 }
 
 
-
-/*
- * Set environmental variable (int or string).
- *
- * On Posix, we use putenv for portability,
- * and put up with its painful semantics
- * that require all the support code below.
- */
-
-/* General-purpose environmental variable set functions */
-
-static char *
-construct_name_value(const char *name, const char *value, struct gc_arena *gc)
-{
-    struct buffer out;
-
-    ASSERT(name);
-    if (!value)
-    {
-        value = "";
-    }
-    out = alloc_buf_gc(strlen(name) + strlen(value) + 2, gc);
-    buf_printf(&out, "%s=%s", name, value);
-    return BSTR(&out);
-}
-
-static bool
-env_string_equal(const char *s1, const char *s2)
-{
-    int c1, c2;
-    ASSERT(s1);
-    ASSERT(s2);
-
-    while (true)
-    {
-        c1 = *s1++;
-        c2 = *s2++;
-        if (c1 == '=')
-        {
-            c1 = 0;
-        }
-        if (c2 == '=')
-        {
-            c2 = 0;
-        }
-        if (!c1 && !c2)
-        {
-            return true;
-        }
-        if (c1 != c2)
-        {
-            break;
-        }
-    }
-    return false;
-}
-
-static bool
-remove_env_item(const char *str, const bool do_free, struct env_item **list)
-{
-    struct env_item *current, *prev;
-
-    ASSERT(str);
-    ASSERT(list);
-
-    for (current = *list, prev = NULL; current != NULL; current = current->next)
-    {
-        if (env_string_equal(current->string, str))
-        {
-            if (prev)
-            {
-                prev->next = current->next;
-            }
-            else
-            {
-                *list = current->next;
-            }
-            if (do_free)
-            {
-                secure_memzero(current->string, strlen(current->string));
-                free(current->string);
-                free(current);
-            }
-            return true;
-        }
-        prev = current;
-    }
-    return false;
-}
-
-static void
-add_env_item(char *str, const bool do_alloc, struct env_item **list, struct gc_arena *gc)
-{
-    struct env_item *item;
-
-    ASSERT(str);
-    ASSERT(list);
-
-    ALLOC_OBJ_GC(item, struct env_item, gc);
-    item->string = do_alloc ? string_alloc(str, gc) : str;
-    item->next = *list;
-    *list = item;
-}
-
-/* struct env_set functions */
-
-static bool
-env_set_del_nolock(struct env_set *es, const char *str)
-{
-    return remove_env_item(str, es->gc == NULL, &es->list);
-}
-
-static void
-env_set_add_nolock(struct env_set *es, const char *str)
-{
-    remove_env_item(str, es->gc == NULL, &es->list);
-    add_env_item((char *)str, true, &es->list, es->gc);
-}
-
-struct env_set *
-env_set_create(struct gc_arena *gc)
-{
-    struct env_set *es;
-    ALLOC_OBJ_CLEAR_GC(es, struct env_set, gc);
-    es->list = NULL;
-    es->gc = gc;
-    return es;
-}
-
-void
-env_set_destroy(struct env_set *es)
-{
-    if (es && es->gc == NULL)
-    {
-        struct env_item *e = es->list;
-        while (e)
-        {
-            struct env_item *next = e->next;
-            free(e->string);
-            free(e);
-            e = next;
-        }
-        free(es);
-    }
-}
-
-bool
-env_set_del(struct env_set *es, const char *str)
-{
-    bool ret;
-    ASSERT(es);
-    ASSERT(str);
-    ret = env_set_del_nolock(es, str);
-    return ret;
-}
-
-void
-env_set_add(struct env_set *es, const char *str)
-{
-    ASSERT(es);
-    ASSERT(str);
-    env_set_add_nolock(es, str);
-}
-
-const char *
-env_set_get(const struct env_set *es, const char *name)
-{
-    const struct env_item *item = es->list;
-    while (item && !env_string_equal(item->string, name))
-    {
-        item = item->next;
-    }
-    return item ? item->string : NULL;
-}
-
-void
-env_set_print(int msglevel, const struct env_set *es)
-{
-    if (check_debug_level(msglevel))
-    {
-        const struct env_item *e;
-        int i;
-
-        if (es)
-        {
-            e = es->list;
-            i = 0;
-
-            while (e)
-            {
-                if (env_safe_to_print(e->string))
-                {
-                    msg(msglevel, "ENV [%d] '%s'", i, e->string);
-                }
-                ++i;
-                e = e->next;
-            }
-        }
-    }
-}
-
-void
-env_set_inherit(struct env_set *es, const struct env_set *src)
-{
-    const struct env_item *e;
-
-    ASSERT(es);
-
-    if (src)
-    {
-        e = src->list;
-        while (e)
-        {
-            env_set_add_nolock(es, e->string);
-            e = e->next;
-        }
-    }
-}
-
-
-/* add/modify/delete environmental strings */
-
-void
-setenv_counter(struct env_set *es, const char *name, counter_type value)
-{
-    char buf[64];
-    openvpn_snprintf(buf, sizeof(buf), counter_format, value);
-    setenv_str(es, name, buf);
-}
-
-void
-setenv_int(struct env_set *es, const char *name, int value)
-{
-    char buf[64];
-    openvpn_snprintf(buf, sizeof(buf), "%d", value);
-    setenv_str(es, name, buf);
-}
-
-void
-setenv_long_long(struct env_set *es, const char *name, long long value)
-{
-    char buf[64];
-    openvpn_snprintf(buf, sizeof(buf), "%lld", value);
-    setenv_str(es, name, buf);
-}
-
-void
-setenv_str(struct env_set *es, const char *name, const char *value)
-{
-    setenv_str_ex(es, name, value, CC_NAME, 0, 0, CC_PRINT, 0, 0);
-}
-
-void
-setenv_str_safe(struct env_set *es, const char *name, const char *value)
-{
-    uint8_t b[64];
-    struct buffer buf;
-    buf_set_write(&buf, b, sizeof(b));
-    if (buf_printf(&buf, "OPENVPN_%s", name))
-    {
-        setenv_str(es, BSTR(&buf), value);
-    }
-    else
-    {
-        msg(M_WARN, "setenv_str_safe: name overflow");
-    }
-}
-
-void
-setenv_str_incr(struct env_set *es, const char *name, const char *value)
-{
-    unsigned int counter = 1;
-    const size_t tmpname_len = strlen(name) + 5; /* 3 digits counter max */
-    char *tmpname = gc_malloc(tmpname_len, true, NULL);
-    strcpy(tmpname, name);
-    while (NULL != env_set_get(es, tmpname) && counter < 1000)
-    {
-        ASSERT(openvpn_snprintf(tmpname, tmpname_len, "%s_%u", name, counter));
-        counter++;
-    }
-    if (counter < 1000)
-    {
-        setenv_str(es, tmpname, value);
-    }
-    else
-    {
-        msg(D_TLS_DEBUG_MED, "Too many same-name env variables, ignoring: %s", name);
-    }
-    free(tmpname);
-}
-
-void
-setenv_del(struct env_set *es, const char *name)
-{
-    ASSERT(name);
-    setenv_str(es, name, NULL);
-}
-
-void
-setenv_str_ex(struct env_set *es,
-              const char *name,
-              const char *value,
-              const unsigned int name_include,
-              const unsigned int name_exclude,
-              const char name_replace,
-              const unsigned int value_include,
-              const unsigned int value_exclude,
-              const char value_replace)
-{
-    struct gc_arena gc = gc_new();
-    const char *name_tmp;
-    const char *val_tmp = NULL;
-
-    ASSERT(name && strlen(name) > 1);
-
-    name_tmp = string_mod_const(name, name_include, name_exclude, name_replace, &gc);
-
-    if (value)
-    {
-        val_tmp = string_mod_const(value, value_include, value_exclude, value_replace, &gc);
-    }
-
-    ASSERT(es);
-
-    if (val_tmp)
-    {
-        const char *str = construct_name_value(name_tmp, val_tmp, &gc);
-        env_set_add(es, str);
-#if DEBUG_VERBOSE_SETENV
-        msg(M_INFO, "SETENV_ES '%s'", str);
-#endif
-    }
-    else
-    {
-        env_set_del(es, name_tmp);
-    }
-
-    gc_free(&gc);
-}
-
-/*
- * Setenv functions that append an integer index to the name
- */
-static const char *
-setenv_format_indexed_name(const char *name, const int i, struct gc_arena *gc)
-{
-    struct buffer out = alloc_buf_gc(strlen(name) + 16, gc);
-    if (i >= 0)
-    {
-        buf_printf(&out, "%s_%d", name, i);
-    }
-    else
-    {
-        buf_printf(&out, "%s", name);
-    }
-    return BSTR(&out);
-}
-
-void
-setenv_int_i(struct env_set *es, const char *name, const int value, const int i)
-{
-    struct gc_arena gc = gc_new();
-    const char *name_str = setenv_format_indexed_name(name, i, &gc);
-    setenv_int(es, name_str, value);
-    gc_free(&gc);
-}
-
-void
-setenv_str_i(struct env_set *es, const char *name, const char *value, const int i)
-{
-    struct gc_arena gc = gc_new();
-    const char *name_str = setenv_format_indexed_name(name, i, &gc);
-    setenv_str(es, name_str, value);
-    gc_free(&gc);
-}
-
 /* return true if filename can be opened for read */
 bool
 test_file(const char *filename)
@@ -1288,71 +912,6 @@  safe_print(const char *str, struct gc_arena *gc)
     return string_mod_const(str, CC_PRINT, CC_CRLF, '.', gc);
 }
 
-static bool
-is_password_env_var(const char *str)
-{
-    return (strncmp(str, "password", 8) == 0);
-}
-
-bool
-env_allowed(const char *str)
-{
-    return (script_security >= SSEC_PW_ENV || !is_password_env_var(str));
-}
-
-bool
-env_safe_to_print(const char *str)
-{
-#ifndef UNSAFE_DEBUG
-    if (is_password_env_var(str))
-    {
-        return false;
-    }
-#endif
-    return true;
-}
-
-/* Make arrays of strings */
-
-const char **
-make_env_array(const struct env_set *es,
-               const bool check_allowed,
-               struct gc_arena *gc)
-{
-    char **ret = NULL;
-    struct env_item *e = NULL;
-    int i = 0, n = 0;
-
-    /* figure length of es */
-    if (es)
-    {
-        for (e = es->list; e != NULL; e = e->next)
-        {
-            ++n;
-        }
-    }
-
-    /* alloc return array */
-    ALLOC_ARRAY_CLEAR_GC(ret, char *, n+1, gc);
-
-    /* fill return array */
-    if (es)
-    {
-        i = 0;
-        for (e = es->list; e != NULL; e = e->next)
-        {
-            if (!check_allowed || env_allowed(e->string))
-            {
-                ASSERT(i < n);
-                ret[i++] = e->string;
-            }
-        }
-    }
-
-    ret[i] = NULL;
-    return (const char **)ret;
-}
-
 const char **
 make_arg_array(const char *first, const char *parms, struct gc_arena *gc)
 {
diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
index ec20ee7..3725fcf 100644
--- a/src/openvpn/misc.h
+++ b/src/openvpn/misc.h
@@ -27,6 +27,7 @@ 
 #include "argv.h"
 #include "basic.h"
 #include "common.h"
+#include "env_set.h"
 #include "integer.h"
 #include "buffer.h"
 #include "platform.h"
@@ -37,20 +38,6 @@ 
 /* forward declarations */
 struct plugin_list;
 
-/*
- * Handle environmental variable lists
- */
-
-struct env_item {
-    char *string;
-    struct env_item *next;
-};
-
-struct env_set {
-    struct gc_arena *gc;
-    struct env_item *list;
-};
-
 /* system flags */
 #define S_SCRIPT (1<<0)
 #define S_FATAL  (1<<1)
@@ -83,61 +70,8 @@  void set_std_files_to_null(bool stdin_only);
 extern int inetd_socket_descriptor;
 void save_inetd_socket_descriptor(void);
 
-/* set/delete environmental variable */
-void setenv_str_ex(struct env_set *es,
-                   const char *name,
-                   const char *value,
-                   const unsigned int name_include,
-                   const unsigned int name_exclude,
-                   const char name_replace,
-                   const unsigned int value_include,
-                   const unsigned int value_exclude,
-                   const char value_replace);
-
-void setenv_counter(struct env_set *es, const char *name, counter_type value);
-
-void setenv_int(struct env_set *es, const char *name, int value);
-
-void setenv_long_long(struct env_set *es, const char *name, long long value);
-
-void setenv_str(struct env_set *es, const char *name, const char *value);
-
-void setenv_str_safe(struct env_set *es, const char *name, const char *value);
-
-void setenv_del(struct env_set *es, const char *name);
-
-/**
- * Store the supplied name value pair in the env_set.  If the variable with the
- * supplied name  already exists, append _N to the name, starting at N=1.
- */
-void setenv_str_incr(struct env_set *es, const char *name, const char *value);
-
-void setenv_int_i(struct env_set *es, const char *name, const int value, const int i);
-
-void setenv_str_i(struct env_set *es, const char *name, const char *value, const int i);
-
-/* struct env_set functions */
-
-struct env_set *env_set_create(struct gc_arena *gc);
-
-void env_set_destroy(struct env_set *es);
-
-bool env_set_del(struct env_set *es, const char *str);
-
-void env_set_add(struct env_set *es, const char *str);
-
-const char *env_set_get(const struct env_set *es, const char *name);
-
-void env_set_print(int msglevel, const struct env_set *es);
-
-void env_set_inherit(struct env_set *es, const struct env_set *src);
-
 /* Make arrays of strings */
 
-const char **make_env_array(const struct env_set *es,
-                            const bool check_allowed,
-                            struct gc_arena *gc);
-
 const char **make_arg_array(const char *first, const char *parms, struct gc_arena *gc);
 
 const char **make_extended_arg_array(char **p, struct gc_arena *gc);
@@ -259,11 +193,6 @@  void set_auth_token(struct user_pass *up, const char *token);
  */
 const char *safe_print(const char *str, struct gc_arena *gc);
 
-/* returns true if environmental variable safe to print to log */
-bool env_safe_to_print(const char *str);
-
-/* returns true if environmental variable may be passed to an external program */
-bool env_allowed(const char *str);
 
 void configure_path(void);
 
diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am
index c457ef9..7b41363 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -39,6 +39,7 @@  crypto_testdriver_SOURCES = test_crypto.c mock_msg.c \
 	$(openvpn_srcdir)/crypto.c \
 	$(openvpn_srcdir)/crypto_mbedtls.c \
 	$(openvpn_srcdir)/crypto_openssl.c \
+	$(openvpn_srcdir)/env_set.c \
 	$(openvpn_srcdir)/otime.c \
 	$(openvpn_srcdir)/packet_id.c \
 	$(openvpn_srcdir)/platform.c