From d3a65ad8c5606842fa63b79a47a4fb6b5aec425e Mon Sep 17 00:00:00 2001 From: Kevin Day Date: Fri, 6 Aug 2021 23:48:55 -0500 Subject: [PATCH] Security: NULL dereferences discovered by GCC's -fanalyzer. The socket bind is both missing the parameter check and is passing '&' when the variable is already a pointer. The f_utf_character_to_char() is a mess. Not sure what I was trying to do, but it is clearly wrong. Redesign it to be more correct but I should revisit this for a more thorough review. There are endianess situations that also need resolved so I added an @todo. --- level_0/f_socket/c/socket.c | 5 ++- level_0/f_socket/c/socket.h | 1 + level_0/f_utf/c/utf.c | 75 ++++++++++++++++++++++----------------------- level_0/f_utf/c/utf.h | 13 +++----- 4 files changed, 47 insertions(+), 47 deletions(-) diff --git a/level_0/f_socket/c/socket.c b/level_0/f_socket/c/socket.c index af2863a..8bd42af 100644 --- a/level_0/f_socket/c/socket.c +++ b/level_0/f_socket/c/socket.c @@ -6,8 +6,11 @@ extern "C"{ #ifndef _di_f_socket_file_bind_ f_status_t f_socket_file_bind(const f_string_t path, const int id, struct sockaddr_un *address) { + #ifndef _di_level_0_parameter_checking_ + if (!address) return F_status_set_error(F_parameter); + #endif // _di_level_0_parameter_checking_ - memset(&address, 0, sizeof(struct sockaddr_un)); + memset(address, 0, sizeof(struct sockaddr_un)); address->sun_family = AF_UNIX; strncpy(address->sun_path, path, sizeof(address->sun_path) - 1); diff --git a/level_0/f_socket/c/socket.h b/level_0/f_socket/c/socket.h index a1008ac..6664939 100644 --- a/level_0/f_socket/c/socket.h +++ b/level_0/f_socket/c/socket.h @@ -53,6 +53,7 @@ extern "C"{ * F_file_found_not (with error bit) if file not found. * F_memory_not (with error bit) if out of memory. * F_name (with error bit) on path name error. + * F_parameter (with error bit) if a parameter is invalid. * F_string_too_large (with error bit) if string is too large to store in the buffer. * F_available_not_address (with error bit) if address is unavailable (is non-existent or not local). * F_failure (with error bit) for any other error. diff --git a/level_0/f_utf/c/utf.c b/level_0/f_utf/c/utf.c index 7d55a57..d9c6ef2 100644 --- a/level_0/f_utf/c/utf.c +++ b/level_0/f_utf/c/utf.c @@ -662,52 +662,51 @@ extern "C" { f_status_t f_utf_character_to_char(const f_utf_character_t utf_character, f_string_t *character, f_array_length_t *width_max) { #ifndef _di_level_0_parameter_checking_ if (!utf_character) return F_status_set_error(F_parameter); - if (!width_max && *character) return F_status_set_error(F_parameter); - if (width_max && !*character) return F_status_set_error(F_parameter); - if (width_max && *width_max > 4) return F_status_set_error(F_parameter); + if (!character) return F_status_set_error(F_parameter); + if (!width_max) return F_status_set_error(F_parameter); + if (!*width_max) return F_status_set_error(F_parameter); #endif // _di_level_0_parameter_checking_ - f_status_t status = F_none; - - uint8_t width = macro_f_utf_character_t_width_is(utf_character); - - if (!width_max) { - macro_f_string_t_clear((*character)) - macro_f_string_t_resize(status, (*character), 0, width) - if (F_status_is_error(status)) return status; - - width = 1; - *width_max = 1; - } - else if (width == 1) { - return F_status_is_error(F_utf); - } - else if (width > *width_max) { - return F_status_set_error(F_failure); - } - - *width_max = width; + if (macro_f_utf_character_t_width_is(utf_character)) { + + // @todo: endianess is compile time so a function is not needed, replace with macros. + if (f_utf_is_big_endian()) { + memcpy(*character, &utf_character, macro_f_utf_character_t_width_is(utf_character)); + } + else { + uint32_t utf = 0; + + switch (macro_f_utf_character_t_width_is(utf_character)) { + case 1: + utf = macro_f_utf_character_t_to_char_1(utf_character) << 24; + break; + case 2: + utf = (macro_f_utf_character_t_to_char_2(utf_character) << 24) | (macro_f_utf_character_t_to_char_1(utf_character) << 16); + break; + case 3: + utf = (macro_f_utf_character_t_to_char_3(utf_character) << 24) | (macro_f_utf_character_t_to_char_2(utf_character) << 16) | (macro_f_utf_character_t_to_char_1(utf_character) << 8); + break; + case 4: + utf = (macro_f_utf_character_t_to_char_4(utf_character) << 24) | (macro_f_utf_character_t_to_char_3(utf_character) << 16) | (macro_f_utf_character_t_to_char_2(utf_character) << 8) | macro_f_utf_character_t_to_char_1(utf_character); + break; + default: + return F_status_set_error(F_failure); + } - if (f_utf_is_big_endian()) { - memcpy(*character, &utf_character, width); + memcpy(*character, &utf, macro_f_utf_character_t_width_is(utf_character)); + } } else { - uint32_t utf = 0; - if (width == 1) { - utf = macro_f_utf_character_t_to_char_1(utf_character) << 24; - } - else if (width == 2) { - utf = (macro_f_utf_character_t_to_char_2(utf_character) << 24) | (macro_f_utf_character_t_to_char_1(utf_character) << 16); - } - else if (width == 3) { - utf = (macro_f_utf_character_t_to_char_3(utf_character) << 24) | (macro_f_utf_character_t_to_char_2(utf_character) << 16) | (macro_f_utf_character_t_to_char_1(utf_character) << 8); - } - else if (width == 4) { - utf = (macro_f_utf_character_t_to_char_4(utf_character) << 24) | (macro_f_utf_character_t_to_char_3(utf_character) << 16) | (macro_f_utf_character_t_to_char_2(utf_character) << 8) | macro_f_utf_character_t_to_char_1(utf_character); + // @todo: endianess is compile time so a function is not needed, replace with macros. + if (f_utf_is_big_endian()) { + memcpy(*character, &utf_character, 1); } + else { + uint32_t utf = macro_f_utf_character_t_to_char_1(utf_character) << 24; - memcpy(*character, &utf, width); + memcpy(*character, &utf, 1); + } } return F_none; diff --git a/level_0/f_utf/c/utf.h b/level_0/f_utf/c/utf.h index 059c345..5d95774 100644 --- a/level_0/f_utf/c/utf.h +++ b/level_0/f_utf/c/utf.h @@ -669,26 +669,23 @@ extern "C" { * Convert a specialized f_utf_character_t type to a uint8_t, stored as a string (character buffer). * * This will also convert ASCII characters stored in the utf_character array. + * This will not resize character. * * @param utf_character - * The UTF-8 characterr to convert from. + * The UTF-8 character to convert from. * @param character * A uint8_t representation of the UTF-8 character, stored as a string of width bytes. - * If width_max is 0, then this should not be allocated (set the pointer address to 0). + * If width_max is 0, then this should be set to 0. * @param width_max - * The number of bytes the generated character represents. - * If this is set to 0, then the character will be allocated and this will be set to the width of the utf_character. - * If this is set to some value greater than 0 (up to 4), then this represents the size of the character array (no allocations are performed). - * If this is greater than 0, and the utf_character width is larger than this size, then an error is returned. + * This is set to the max number of bytes available. + * This is then updated to represent the max bytes used if enough space is available. * * @return * F_none if conversion was successful. * * F_failure (with error bit) if width is not long enough to convert. * F_parameter (with error bit) if a parameter is invalid. - * F_memory_not (with error bit) on out of memory. * F_utf (with error bit) if character is an invalid UTF-8 character. - * F_failure (with error bit) if width is not long enough to convert. */ #ifndef _di_f_utf_character_to_char_ extern f_status_t f_utf_character_to_char(const f_utf_character_t utf_character, f_string_t *character, f_array_length_t *width_max); -- 1.8.3.1