From b86395ff60201cb21902e9423b26d9c403be1612 Mon Sep 17 00:00:00 2001
From: Serge Petrenko <sergepetrenko@tarantool.org>
Date: Thu, 13 Oct 2022 15:18:35 +0300
Subject: [PATCH] security: check size boundaries for getenv() returns
getenv() return values cannot be trusted, because an attacker might set
them. For instance, we shouldn't expect, that getenv() returns a value
of some sane size.
Another problem is that getenv() returns a pointer to one of
`char **environ` members, which might change upon next setenv().
Introduce a wrapper, getenv_safe(), which returns the value only when
it fits in a buffer of a specified size, and copies the value onto the
buffer. Use this wrapper everywhere in our code.
Below's a slightly decorated output of `grep -rwn getenv ./src --include
*.c --include *.h --include *.cc --include *.cpp --include *.hpp
--exclude *.lua.c` as of 2022-10-14.
`-` marks invalid occurences (comments, for example),
`*` marks the places that are already guarded before this patch,
`X` mars the places guarded in this patch, and
`^` marks places fixed in the next commit:
NO_WRAP
```
* ./src/lib/core/coio_file.c:509: const char *tmpdir = getenv("TMPDIR");
X ./src/lib/core/errinj.c:75: const char *env_value = getenv(inj->name);
- ./src/proc_title.c:202: * that might try to hang onto a getenv() result.)
- ./src/proc_title.c:241: * is mandatory to flush internal libc caches on getenv/setenv
X ./src/systemd.c:54: sd_unix_path = getenv("NOTIFY_SOCKET");
* ./src/box/module_cache.c:300: const char *tmpdir = getenv("TMPDIR");
X ./src/box/sql/os_unix.c:1441: azDirs[0] = getenv("SQL_TMPDIR");
X ./src/box/sql/os_unix.c:1446: azDirs[1] = getenv("TMPDIR");
* ./src/box/lua/console.c:394: const char *envvar = getenv("TT_CONSOLE_HIDE_SHOW_PROMPT");
^ ./src/box/lua/console.lua:771: local home_dir = os.getenv('HOME')
^ ./src/box/lua/load_cfg.lua:1007: local raw_value = os.getenv(env_var_name)
X ./src/lua/init.c:575: const char *path = getenv(envname);
X ./src/lua/init.c:592: const char *home = getenv("HOME");
* ./src/find_path.c:77: snprintf(buf, sizeof(buf) - 1, "%s", getenv("_"));
```
NO_WRAP
Part-of #7797
NO_DOC=security
---
.../gh-7797-untrusted-input-validation.md | 4 ++
src/box/lua/console.c | 4 +-
src/box/module_cache.c | 11 ++--
src/box/sql/os_unix.c | 6 +-
src/find_path.c | 3 +-
src/lib/core/coio_file.c | 14 +++--
src/lib/core/coio_file.h | 4 +-
src/lib/core/errinj.c | 9 ++-
src/lib/core/util.c | 41 ++++++++++++++
src/lua/init.c | 10 ++--
src/main.cc | 2 +-
src/systemd.c | 4 +-
src/trivia/util.h | 28 +++++++++-
test/unit/CMakeLists.txt | 9 ++-
test/unit/getenv_safe.c | 55 +++++++++++++++++++
15 files changed, 177 insertions(+), 27 deletions(-)
create mode 100644 changelogs/unreleased/gh-7797-untrusted-input-validation.md
create mode 100644 test/unit/getenv_safe.c
diff --git a/changelogs/unreleased/gh-7797-untrusted-input-validation.md b/changelogs/unreleased/gh-7797-untrusted-input-validation.md
new file mode 100644
index 0000000000..71b6255411
--- /dev/null
+++ b/changelogs/unreleased/gh-7797-untrusted-input-validation.md
@@ -0,0 +1,4 @@
+## bugfix/core
+
+* Added boundary checking for getenv() return values and started copying them
+ rather than using directly (gh-7797).
diff --git a/src/box/lua/console.c b/src/box/lua/console.c
index d66661b348..ee94af696a 100644
--- a/src/box/lua/console.c
+++ b/src/box/lua/console.c
@@ -391,7 +391,9 @@ lbox_console_show_prompt(struct lua_State *L)
static bool
console_hide_show_prompt_is_enabled(void)
{
- const char *envvar = getenv("TT_CONSOLE_HIDE_SHOW_PROMPT");
+ char var_buf[10];
+ const char *envvar = getenv_safe("TT_CONSOLE_HIDE_SHOW_PROMPT", var_buf,
+ sizeof(var_buf));
/* Enabled by default. */
if (envvar == NULL || *envvar == '\0')
diff --git a/src/box/module_cache.c b/src/box/module_cache.c
index ab0845e90e..3d9b1883df 100644
--- a/src/box/module_cache.c
+++ b/src/box/module_cache.c
@@ -296,14 +296,15 @@ module_new(const char *package, size_t package_len,
memcpy(m->package, package, package_len);
m->package[package_len] = 0;
-
- const char *tmpdir = getenv("TMPDIR");
- if (tmpdir == NULL)
- tmpdir = "/tmp";
+ char *tmpdir = getenv_safe("TMPDIR", NULL, 0);
+ char *print_dir = tmpdir;
+ if (print_dir == NULL)
+ print_dir = "/tmp";
char dir_name[PATH_MAX];
int rc = snprintf(dir_name, sizeof(dir_name),
- "%s/tntXXXXXX", tmpdir);
+ "%s/tntXXXXXX", print_dir);
+ free(tmpdir);
if (rc < 0 || (size_t)rc >= sizeof(dir_name)) {
diag_set(SystemError, "failed to generate path to tmp dir");
goto error;
diff --git a/src/box/sql/os_unix.c b/src/box/sql/os_unix.c
index d64f1bd18e..87d947c7f2 100644
--- a/src/box/sql/os_unix.c
+++ b/src/box/sql/os_unix.c
@@ -1433,14 +1433,16 @@ unixTempFileDir(void)
"/tmp",
"."
};
+ static char zbuf0[PATH_MAX];
+ static char zbuf1[PATH_MAX];
unsigned int i = 0;
struct stat buf;
const char *zDir = sql_temp_directory;
if (!azDirs[0])
- azDirs[0] = getenv("SQL_TMPDIR");
+ azDirs[0] = getenv_safe("SQL_TMPDIR", zbuf0, sizeof(zbuf0));
if (!azDirs[1])
- azDirs[1] = getenv("TMPDIR");
+ azDirs[1] = getenv_safe("TMPDIR", zbuf1, sizeof(zbuf1));
while (1) {
if (zDir != 0 && stat(zDir, &buf) == 0 &&
S_ISDIR(buf.st_mode) && access(zDir, 03) == 0)
diff --git a/src/find_path.c b/src/find_path.c
index b9c99fc4ad..f92830b18f 100644
--- a/src/find_path.c
+++ b/src/find_path.c
@@ -35,6 +35,7 @@
#include <stdint.h>
#include <stdbool.h>
#include <unistd.h>
+#include <trivia/util.h>
#if defined(__APPLE__)
#include <mach-o/dyld.h>
@@ -74,7 +75,7 @@ find_path(const char *argv0)
rc = _NSGetExecutablePath(buf, &usize);
#endif
if (rc == -1)
- snprintf(buf, sizeof(buf) - 1, "%s", getenv("_"));
+ getenv_safe("_", buf, sizeof(buf));
}
if (realpath(buf, path) == NULL)
snprintf(path, sizeof(path), "%s", buf);
diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
index 3ec74a549d..7fc4161f69 100644
--- a/src/lib/core/coio_file.c
+++ b/src/lib/core/coio_file.c
@@ -502,17 +502,19 @@ coio_do_tempdir(eio_req *req)
}
int
-coio_tempdir(char *path, size_t path_len)
+coio_tempdir(char *path, size_t path_size)
{
INIT_COEIO_FILE(eio);
- const char *tmpdir = getenv("TMPDIR");
- if (tmpdir == NULL)
- tmpdir = "/tmp";
- int rc = snprintf(path, path_len, "%s/XXXXXX", tmpdir);
+ char *tmpdir = getenv_safe("TMPDIR", NULL, 0);
+ const char *append_dir = tmpdir;
+ if (append_dir == NULL)
+ append_dir = "/tmp";
+ int rc = snprintf(path, path_size, "%s/XXXXXX", append_dir);
+ free(tmpdir);
if (rc < 0)
return -1;
- if ((size_t) rc >= path_len) {
+ if ((size_t)rc >= path_size) {
errno = ENOMEM;
return -1;
}
diff --git a/src/lib/core/coio_file.h b/src/lib/core/coio_file.h
index b8886cd163..9c22e6e3c0 100644
--- a/src/lib/core/coio_file.h
+++ b/src/lib/core/coio_file.h
@@ -80,7 +80,9 @@ int coio_sync(void);
int coio_fsync(int fd);
int coio_fdatasync(int fd);
-int coio_tempdir(char *path, size_t path_len);
+/** A wrapper around mkdtemp. */
+int
+coio_tempdir(char *path, size_t path_size);
int coio_readdir(const char *path, char **buf);
int coio_copyfile(const char *source, const char *dest);
diff --git a/src/lib/core/errinj.c b/src/lib/core/errinj.c
index 5afbd18391..577ec73b3d 100644
--- a/src/lib/core/errinj.c
+++ b/src/lib/core/errinj.c
@@ -72,9 +72,13 @@ int errinj_foreach(errinj_cb cb, void *cb_ctx) {
void errinj_set_with_environment_vars(void) {
for (enum errinj_id i = 0; i < errinj_id_MAX; i++) {
struct errinj *inj = &errinjs[i];
- const char *env_value = getenv(inj->name);
- if (env_value == NULL || *env_value == '\0')
+ char *env_value = getenv_safe(inj->name, NULL, 0);
+ if (env_value == NULL)
continue;
+ if (*env_value == '\0') {
+ free(env_value);
+ continue;
+ }
if (inj->type == ERRINJ_INT) {
char *end;
@@ -101,5 +105,6 @@ void errinj_set_with_environment_vars(void) {
panic("Incorrect value for double %s: %s",
inj->name, env_value);
}
+ free(env_value);
}
}
diff --git a/src/lib/core/util.c b/src/lib/core/util.c
index bd5de2a3c4..f6db142190 100644
--- a/src/lib/core/util.c
+++ b/src/lib/core/util.c
@@ -484,3 +484,44 @@ thread_sleep(double sec)
assert(rc == 0);
(void)rc;
}
+
+/*
+ * 32 4 kilobyte pages seems to be the limit for a single environment variable
+ * or argv element set by the kernel. We use that to limit maximum buffer size
+ * allocated for an env variable.
+ */
+#define MAX_ENV_VAR_SIZE 131072
+
+char *
+getenv_safe(const char *name, char *buf, size_t buf_size)
+{
+ assert(buf != NULL || buf_size == 0);
+ assert(name != NULL);
+
+ char *var = getenv(name);
+ if (var == NULL)
+ return NULL;
+ if (buf == NULL)
+ buf_size = MAX_ENV_VAR_SIZE;
+ size_t var_len = strnlen(var, buf_size);
+ if (var_len >= buf_size) {
+ say_warn("Ignoring environment variable %s because its value "
+ "is too long (>= %zu)", name, buf_size);
+ return NULL;
+ }
+ char *alloc_buf = NULL;
+ if (buf == NULL) {
+ alloc_buf = xmalloc(var_len + 1);
+ buf = alloc_buf;
+ }
+ memcpy(buf, var, var_len + 1);
+ /*
+ * The value could have changed during copying so we reset the
+ * terminating zero explicitly to avoid out-of-bounds access.
+ * The returned value may still be malformed, intentionally, for
+ * example, but there's nothing we can really do about it. The user must
+ * check that the returned value is valid for his use.
+ */
+ buf[var_len] = '\0';
+ return buf;
+}
diff --git a/src/lua/init.c b/src/lua/init.c
index 456560592c..5b46503e8d 100644
--- a/src/lua/init.c
+++ b/src/lua/init.c
@@ -621,11 +621,12 @@ fiber_backtracer(void *(*frame_writer)(int frame_no, void *addr))
static void
tarantool_lua_pushpath_env(struct lua_State *L, const char *envname)
{
- const char *path = getenv(envname);
+ char *path = getenv_safe(envname, NULL, 0);
if (path != NULL) {
const char *def = lua_tostring(L, -1);
- path = luaL_gsub(L, path, ";;", ";\1;");
- luaL_gsub(L, path, "\1", def);
+ const char *path_new = luaL_gsub(L, path, ";;", ";\1;");
+ free(path);
+ luaL_gsub(L, path_new, "\1", def);
lua_remove(L, -2);
lua_remove(L, -2);
}
@@ -638,7 +639,7 @@ tarantool_lua_pushpath_env(struct lua_State *L, const char *envname)
static void
tarantool_lua_setpaths(struct lua_State *L)
{
- const char *home = getenv("HOME");
+ char *home = getenv_safe("HOME", NULL, 0);
lua_getglobal(L, "package");
int top = lua_gettop(L);
@@ -663,6 +664,7 @@ tarantool_lua_setpaths(struct lua_State *L)
lua_pushliteral(L, "/.luarocks/lib/lua/5.1/?" MODULE_LIBSUFFIX ";");
lua_pushstring(L, home);
lua_pushliteral(L, "/.luarocks/lib/lua/?" MODULE_LIBSUFFIX ";");
+ free(home);
}
lua_pushliteral(L, MODULE_LIBPATH ";");
/* overwrite standard paths */
diff --git a/src/main.cc b/src/main.cc
index 585c8713cd..1c45fa9d05 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -719,7 +719,7 @@ main(int argc, char **argv)
* such options (in script line) don't work
*/
- char *tarantool_bin = find_path(argv[0]);
+ const char *tarantool_bin = find_path(argv[0]);
if (!tarantool_bin)
tarantool_bin = argv[0];
if (argc > 1) {
diff --git a/src/systemd.c b/src/systemd.c
index 0ef201217b..3feb834483 100644
--- a/src/systemd.c
+++ b/src/systemd.c
@@ -51,7 +51,7 @@ static int systemd_fd = -1;
static const char *sd_unix_path = NULL;
int systemd_init(void) {
- sd_unix_path = getenv("NOTIFY_SOCKET");
+ sd_unix_path = getenv_safe("NOTIFY_SOCKET", NULL, 0);
if (sd_unix_path == NULL) {
/* Do nothing if the path is not set. */
return 0;
@@ -100,6 +100,7 @@ int systemd_init(void) {
close(systemd_fd);
systemd_fd = -1;
}
+ free((char *)sd_unix_path);
sd_unix_path = NULL;
return -1;
}
@@ -107,6 +108,7 @@ int systemd_init(void) {
void systemd_free(void) {
if (systemd_fd > 0)
close(systemd_fd);
+ free((char *)sd_unix_path);
}
int systemd_notify(const char *message) {
diff --git a/src/trivia/util.h b/src/trivia/util.h
index 4201c7e82d..629f4403cc 100644
--- a/src/trivia/util.h
+++ b/src/trivia/util.h
@@ -456,7 +456,7 @@ gcov_flush(void)
ssize_t
fdprintf(int fd, const char *format, ...) __attribute__((format(printf, 2, 3)));
-char *
+const char *
find_path(const char *argv0);
char *
@@ -643,6 +643,32 @@ is_exp_of_two(unsigned n)
void
thread_sleep(double sec);
+/**
+ * Returns the value associated with an environment variable \a name. The value
+ * is copied onto the buffer, which's either user-provided (when \a buf != NULL)
+ * or dynamically allocated.
+ *
+ * \return buf in case \a buf != NULL, and strlen(value) < \a buf_size.
+ * ptr a pointer to dynamically allocated memory, which has to be freed
+ * manually, in case \a buf == NULL and strlen(value) < internal
+ * hard limit.
+ * NULL in case no value is found.
+ * in case buf != NULL and strlen(value) >= \a buf_size.
+ * in case buf == NULL and strlen(value) >= internal limit.
+ *
+ * When a non-null pointer is returned, it's guaranteed to contain a
+ * null-terminated string. The string is a copy of the corresponding environment
+ * variable in all cases, except when `getenv_safe` is run concurrently with
+ * `setenv`.
+ * In that case the buffer might contain:
+ * - an old variable value,
+ * - a new value, truncated to not exceed old value length,
+ * - garbage, truncated to not exceed old value length
+ * Hence the user has to validate the returns.
+ */
+char *
+getenv_safe(const char *name, char *buf, size_t buf_size);
+
#if !defined(__cplusplus) && !defined(static_assert)
# define static_assert _Static_assert
#endif
diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index 1f29ee91c3..66c225ff3b 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -305,9 +305,9 @@ create_unit_test(PREFIX crc32
)
create_unit_test(PREFIX find_path
- SOURCES find_path.c
+ SOURCES find_path.c core_test_utils.c
${PROJECT_SOURCE_DIR}/src/find_path.c
- LIBRARIES
+ LIBRARIES core
)
create_unit_test(PREFIX reflection_c
@@ -563,3 +563,8 @@ create_unit_test(PREFIX key_def
SOURCES key_def.c box_test_utils.c
LIBRARIES unit box core
)
+
+create_unit_test(PREFIX getenv_safe
+ SOURCES getenv_safe.c core_test_utils.c
+ LIBRARIES unit core
+)
diff --git a/test/unit/getenv_safe.c b/test/unit/getenv_safe.c
new file mode 100644
index 0000000000..31a41b65fa
--- /dev/null
+++ b/test/unit/getenv_safe.c
@@ -0,0 +1,55 @@
+#include <stdlib.h>
+#include <string.h>
+#include "trivia/util.h"
+
+#define UNIT_TAP_COMPATIBLE 1
+#include "unit.h"
+
+static void
+test_getenv_safe(void)
+{
+ header();
+ plan(10);
+
+ static const char str[] = "some env value";
+ static const char name[] = "TT_GH_7797_ENV_TEST";
+ size_t size = sizeof(str);
+ char buf[size];
+ bool exceeded = true;
+
+ is(getenv(name), NULL, "Getenv finds nothing initially");
+ is(getenv_safe(name, buf, size), NULL,
+ "Getenv_safe finds nothing");
+
+ is(setenv(name, str, 1), 0, "Setenv succeeeds");
+
+ char *ret1 = getenv(name);
+ isnt(ret1, NULL, "Getenv finds the value");
+ char *ret2 = getenv_safe(name, buf, size);
+ isnt(ret2, NULL, "Getenv_safe finds the value");
+ is(ret2, buf, "Getenv_safe returns pointer to passed buffer");
+ is(strcmp(ret1, ret2), 0, "Returns are the same");
+ char *ret3 = getenv_safe(name, buf, size - 1);
+ is(ret3, NULL, "Getenv_safe returns nothing when size doesn't fit");
+ char *ret4 = getenv_safe(name, NULL, 0);
+ isnt(ret4, NULL, "Getenv_safe returns allocated memory when not "
+ "provided with a buffer");
+ is(strcmp(ret4, ret1), 0, "Returns are the same");
+ free(ret4);
+
+ unsetenv(name);
+
+ footer();
+}
+
+int
+main(void)
+{
+ header();
+ plan(1);
+
+ test_getenv_safe();
+
+ footer();
+ return check_plan();
+}
--
GitLab