From 73e58adc6b80c7a83d92ec9cf95ffe98db87d367 Mon Sep 17 00:00:00 2001 From: frostb1te Date: Fri, 8 Nov 2024 05:00:24 -0600 Subject: [PATCH] src/gpasswd.c: is_valid_user_list(): Fix invalid free(3) This fix addresses an issue in is_valid_user_list() where the free operation was attempted on an address not allocated with malloc(). By duplicating the pointer with xstrdup(users) into dup, and using dup as the original pointer, we ensure that only the valid pointer is freed, avoiding an invalid free operation. This bug was introduced when changing some code that used strchrnul(3) to use strsep(3) instead. strsep(3) advances the pointer, unlike the previous code. This unconditionally leads to a bug: - Passing NULL to free(3), if the last field in the colon-separated-value list is non-empty. This results in a memory leak. - Passing a pointer to the null byte ('\0') that terminates the string, if the last element of the colon-separated-value list is empty. The most obvious reproducer of such a bogus free(3) call is: free(strdup("foo:") + 4); This results in Undefined Behavior, and could result in allocator data corruption. Fixes: 16cb66486554 (2024-07-01, "lib/, src/: Use strsep(3) instead of its pattern") Suggested-by: Reported-by: Reviewed-by: Serge Hallyn Reviewed-by: Alejandro Colomar Cc: Iker Pedrosa Cc: Christian Brauner --- src/gpasswd.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/gpasswd.c b/src/gpasswd.c index b6b3d84f..4fdd2812 100644 --- a/src/gpasswd.c +++ b/src/gpasswd.c @@ -174,7 +174,9 @@ static void catch_signals (int killed) static bool is_valid_user_list (const char *users) { bool is_valid = true; - /*@owned@*/char *tmpusers = xstrdup (users); + char *dup, *tmpusers; + + tmpusers = dup = xstrdup(users); while (NULL != tmpusers && '\0' != *tmpusers) { const char *u; @@ -193,7 +195,7 @@ static bool is_valid_user_list (const char *users) } } - free (tmpusers); + free(dup); return is_valid; }