From 188fe5f47f9f9e8a4f67bf4e4a840ce84d80641c Mon Sep 17 00:00:00 2001 From: Jan Kiszka Date: Mon, 24 Jul 2023 08:00:34 +0200 Subject: [PATCH 5/9] Introduce validation of bgenv prior to its usage The parsing of user variables assumes sane input so far and can be mislead to out-of-bounds accesses, including writes. Address this by always validating a bgenv after reading it from a partition or a file. If an invalid bgenv is found, it is cleared to zero internally so that the existing code will always operate against a sane state. Include the CRC32 validation in the new helper as well which also ensures that the checksum is tested when operating against a specific file. Reported by Code Intelligence. Addresses CVE-2023-39950 Signed-off-by: Jan Kiszka --- env/env_api_fat.c | 44 ++++++++++++++++++++++++++++++++++---------- env/uservars.c | 29 +++++++++++++++++++++++++++++ include/env_api.h | 2 ++ include/uservars.h | 3 +++ tools/bg_envtools.c | 6 +++++- 5 files changed, 73 insertions(+), 11 deletions(-) diff --git a/env/env_api_fat.c b/env/env_api_fat.c index 0f4f474..b7540bb 100644 --- a/env/env_api_fat.c +++ b/env/env_api_fat.c @@ -51,6 +51,33 @@ void bgenv_be_verbose(bool v) ebgpart_beverbose(v); } +static void clear_envdata(BG_ENVDATA *data) +{ + memset(data, 0, sizeof(BG_ENVDATA)); + data->crc32 = crc32(0, (Bytef *)data, + sizeof(BG_ENVDATA) - sizeof(data->crc32)); +} + +bool validate_envdata(BG_ENVDATA *data) +{ + uint32_t sum = crc32(0, (Bytef *)data, + sizeof(BG_ENVDATA) - sizeof(data->crc32)); + + if (data->crc32 != sum) { + VERBOSE(stderr, "Invalid CRC32!\n"); + /* clear invalid environment */ + clear_envdata(data); + return false; + } + if (!bgenv_validate_uservars(data->userdata)) { + VERBOSE(stderr, "Corrupt uservars!\n"); + /* clear invalid environment */ + clear_envdata(data); + return false; + } + return true; +} + bool read_env(CONFIG_PART *part, BG_ENVDATA *env) { if (!part) { @@ -86,10 +113,16 @@ bool read_env(CONFIG_PART *part, BG_ENVDATA *env) if (part->not_mounted) { unmount_partition(part); } + if (result == false) { + clear_envdata(env); + return false; + } + /* enforce NULL-termination of strings */ env->kernelfile[ENV_STRING_LENGTH - 1] = 0; env->kernelparams[ENV_STRING_LENGTH - 1] = 0; - return result; + + return validate_envdata(env); } bool write_env(CONFIG_PART *part, BG_ENVDATA *env) @@ -147,15 +180,6 @@ bool bgenv_init(void) } for (int i = 0; i < ENV_NUM_CONFIG_PARTS; i++) { read_env(&config_parts[i], &envdata[i]); - uint32_t sum = crc32(0, (Bytef *)&envdata[i], - sizeof(BG_ENVDATA) - sizeof(envdata[i].crc32)); - if (envdata[i].crc32 != sum) { - VERBOSE(stderr, "Invalid CRC32!\n"); - /* clear invalid environment */ - memset(&envdata[i], 0, sizeof(BG_ENVDATA)); - envdata[i].crc32 = crc32(0, (Bytef *)&envdata[i], - sizeof(BG_ENVDATA) - sizeof(envdata[i].crc32)); - } } initialized = true; return true; diff --git a/env/uservars.c b/env/uservars.c index f251f24..23a6920 100644 --- a/env/uservars.c +++ b/env/uservars.c @@ -72,6 +72,35 @@ void bgenv_map_uservar(uint8_t *udata, char **key, uint64_t *type, uint8_t **val } } +bool bgenv_validate_uservars(uint8_t *udata) +{ + uint32_t spaceleft = ENV_MEM_USERVARS; + + while (*udata) { + uint32_t key_len = strnlen((char *)udata, spaceleft); + + /* we need space for the key string + null termination + + * the payload size field */ + if (key_len + 1 + sizeof(uint32_t) >= spaceleft) { + return false; + } + + spaceleft -= key_len + 1; + udata += key_len + 1; + + uint32_t payload_size = *(uint32_t *)udata; + + /* the payload must leave at least one byte free */ + if (payload_size >= spaceleft) { + return false; + } + + spaceleft -= payload_size; + udata += payload_size; + } + return true; +} + void bgenv_serialize_uservar(uint8_t *p, char *key, uint64_t type, void *data, uint32_t record_size) { diff --git a/include/env_api.h b/include/env_api.h index b796682..02b4167 100644 --- a/include/env_api.h +++ b/include/env_api.h @@ -95,3 +95,5 @@ extern int bgenv_get(BGENV *env, char *key, uint64_t *type, void *data, extern int bgenv_set(BGENV *env, char *key, uint64_t type, void *data, uint32_t datalen); extern uint8_t *bgenv_find_uservar(uint8_t *userdata, char *key); + +extern bool validate_envdata(BG_ENVDATA *data); diff --git a/include/uservars.h b/include/uservars.h index f2f3587..28d6502 100644 --- a/include/uservars.h +++ b/include/uservars.h @@ -14,6 +14,7 @@ #pragma once +#include #include void bgenv_map_uservar(uint8_t *udata, char **key, uint64_t *type, @@ -35,3 +36,5 @@ uint8_t *bgenv_uservar_realloc(uint8_t *udata, uint32_t new_rsize, uint8_t *p); void bgenv_del_uservar(uint8_t *udata, uint8_t *var); uint32_t bgenv_user_free(uint8_t *udata); + +bool bgenv_validate_uservars(uint8_t *udata); diff --git a/tools/bg_envtools.c b/tools/bg_envtools.c index 4d3cfa8..786d6c1 100644 --- a/tools/bg_envtools.c +++ b/tools/bg_envtools.c @@ -155,9 +155,13 @@ bool get_env(char *configfilepath, BG_ENVDATA *data) "Error closing environment file after reading.\n"); }; + if (result == false) { + return false; + } + /* enforce NULL-termination of strings */ data->kernelfile[ENV_STRING_LENGTH - 1] = 0; data->kernelparams[ENV_STRING_LENGTH - 1] = 0; - return result; + return validate_envdata(data); } -- 2.40.1