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 0000000000000000000000000000000000000000..71b6255411612fc73eb9b4e180f2f2b4b6eed7bc --- /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 d66661b34862a20a678b8298915b79f91e8e8350..ee94af696a3334f6a2502e63af4b2bf740323407 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 ab0845e90ed1e5336535958d47052eb5dc9d0010..3d9b1883dfbbb9a8cf59cabb82b2e9d6c03c0fdc 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 d64f1bd18ecd44565879d86b96ca481bf2f6ef75..87d947c7f2028edc35d27ef0b21878ccfadacd6b 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 b9c99fc4ad376670e0f21c3e510e9b90e4477c1c..f92830b18fe482a4a11bfbc7a2d2db44ec5dc58d 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 3ec74a549d6a5f597030577e43dc4ad0871cfdca..7fc4161f6901aae7a2363a714de82d247c37712f 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 b8886cd1635e9df56689c670e21144ffab7f742b..9c22e6e3c08e98ebef1b27c2adeff0381a378813 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 5afbd18391eb77e4a71fc9279c0fc6d923afe448..577ec73b3da79fe029c79083c607dc908707fa41 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 bd5de2a3c416fc0ddfb12cef1726e9b4d5b57fba..f6db14219054aba4e2b1a97929ac112d3592fbbb 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 456560592c37c1fb54cdbfed11fd98b22af6493c..5b46503e8d4d1d04d6b7c2cb513b74be494972ed 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 585c8713cdc69add4e5c2b958e657a1d754e9e4c..1c45fa9d051c51995c7d78fbc5c59d298624ef04 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 0ef201217b06ba6e56e599e7467ea17057dfefb5..3feb83448388299df2be0778a9a5c13e9bd97c4e 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 4201c7e82d787e413ccf3a4b86bf6841dc844702..629f4403cc21c8ab06d0701be270298929e59251 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 1f29ee91c37c62abcd670ed5efd73a2b61181cc8..66c225ff3bf49afa862467b7a395e7949c5cf8af 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 0000000000000000000000000000000000000000..31a41b65fa3148bc74b1407364d60c2c587ec3db --- /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(); +}