From f221864d6ed6ffc10b93ea15e3d9c10136f9d459 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Mon, 14 Oct 2024 18:56:55 +0300 Subject: [PATCH 1/3] glib: Don't require GLIB_DOMAIN to be a NUL-terminated string The length might be passed explicitly in the field instead, and the string might not have a NUL-terminator as happens for example when passed from the Rust bindings. This might lead to out of bounds reads. Thanks to Sebastian Wiesner for noticing this. --- glib/gmessages.c | 74 +++++++++++++++++++++++++++++++++++++++++------- meson.build | 1 + 2 files changed, 65 insertions(+), 10 deletions(-) diff --git a/glib/gmessages.c b/glib/gmessages.c index 8f0fd5bba..9db137b76 100644 --- a/glib/gmessages.c +++ b/glib/gmessages.c @@ -1518,7 +1518,8 @@ done_query: * `"GLIB_DOMAIN"`) must be passed as nul-terminated UTF-8 strings until GLib * version 2.74.1 because the default log handler did not consider the length of * the `GLogField`. Starting with GLib 2.74.1 this is fixed and - * non-nul-terminated UTF-8 strings can be passed with their correct length. + * non-nul-terminated UTF-8 strings can be passed with their correct length, + * with the exception of `"GLIB_DOMAIN"` which was only fixed with GLib 2.82.3. * * The @log_domain will be converted into a `GLIB_DOMAIN` field. @log_level will * be converted into a @@ -2627,26 +2628,70 @@ log_is_old_api (const GLogField *fields, g_strcmp0 (fields[0].value, "1") == 0); } +#ifndef HAVE_MEMMEM +// memmem() is a GNU extension so if it's not available we'll need +// our own implementation here. Thanks C. +static void * +my_memmem (const void *haystack, + size_t haystacklen, + const void *needle, + size_t needlelen) +{ + const guint8 *cur, *end; + + if (needlelen > haystacklen) + return NULL; + if (needlelen == 0) + return (void *) haystack; + + cur = haystack; + end = cur + haystacklen - needlelen; + + for (; cur <= end; cur++) + { + if (memcmp (cur, needle, needlelen) == 0) + return (void *) cur; + } + + return NULL; +} +#else +#define my_memmem memmem +#endif + +static void * +memmem_with_end_pointer (const void *haystack, + const void *haystack_end, + const void *needle, + size_t needle_len) +{ + return my_memmem (haystack, (const char *) haystack_end - (const char *) haystack, needle, needle_len); +} + static gboolean domain_found (const gchar *domains, - const char *log_domain) + const char *log_domain, + gsize log_domain_length) { - guint len; - const gchar *found; + const gchar *found = domains; + gsize domains_length = strlen (domains); + const gchar *domains_end = domains + domains_length; - len = strlen (log_domain); - - for (found = strstr (domains, log_domain); found; - found = strstr (found + 1, log_domain)) + for (found = memmem_with_end_pointer (domains, domains_end, log_domain, log_domain_length); found; + found = memmem_with_end_pointer (found + 1, domains_end, log_domain, log_domain_length)) { if ((found == domains || found[-1] == ' ') - && (found[len] == 0 || found[len] == ' ')) + && (found[log_domain_length] == 0 || found[log_domain_length] == ' ')) return TRUE; } return FALSE; } +#ifdef my_memmem +#undef my_memmem +#endif + static struct { GRWLock lock; gchar *domains; @@ -2697,6 +2742,7 @@ should_drop_message (GLogLevelFlags log_level, !g_log_get_debug_enabled ()) { gsize i; + gsize log_domain_length; g_rw_lock_reader_lock (&g_log_global.lock); @@ -2720,13 +2766,21 @@ should_drop_message (GLogLevelFlags log_level, if (g_strcmp0 (fields[i].key, "GLIB_DOMAIN") == 0) { log_domain = fields[i].value; + if (fields[i].length < 0) + log_domain_length = strlen (fields[i].value); + else + log_domain_length = fields[i].length; break; } } } + else + { + log_domain_length = strlen (log_domain); + } if (strcmp (g_log_global.domains, "all") != 0 && - (log_domain == NULL || !domain_found (g_log_global.domains, log_domain))) + (log_domain == NULL || !domain_found (g_log_global.domains, log_domain, log_domain_length))) { g_rw_lock_reader_unlock (&g_log_global.lock); return TRUE; diff --git a/meson.build b/meson.build index cb70f9b24..69e4b036e 100644 --- a/meson.build +++ b/meson.build @@ -715,6 +715,7 @@ functions = [ 'lstat', 'mbrtowc', 'memalign', + 'memmem', 'mmap', 'newlocale', 'pipe2', From 97198535078f6fbcc0ac8ce80617a220d0632c78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Mon, 14 Oct 2024 18:59:04 +0300 Subject: [PATCH 2/3] glib: Make sure GLIB_OLD_LOG_API is a NUL-terminated string Every usage in GLib ensures this but theoretically external code might pass something else. As this is only meant to be used internally from GLib, don't support the other case but at least avoid potential out of bound reads. --- glib/gmessages.c | 1 + 1 file changed, 1 insertion(+) diff --git a/glib/gmessages.c b/glib/gmessages.c index 9db137b76..fba55b619 100644 --- a/glib/gmessages.c +++ b/glib/gmessages.c @@ -2625,6 +2625,7 @@ log_is_old_api (const GLogField *fields, { return (n_fields >= 1 && g_strcmp0 (fields[0].key, "GLIB_OLD_LOG_API") == 0 && + fields[0].length < 0 && g_strcmp0 (fields[0].value, "1") == 0); } From 99bf0c966a182d80cfb9df14aeae4fc15f2c868d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Tue, 22 Oct 2024 18:48:56 +0300 Subject: [PATCH 3/3] glib: Add test for handling of non-NUL terminated strings in default log handler --- glib/tests/logging.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/glib/tests/logging.c b/glib/tests/logging.c index 858cad8ce..5961967b9 100644 --- a/glib/tests/logging.c +++ b/glib/tests/logging.c @@ -435,6 +435,32 @@ test_default_handler_0x400 (void) exit (0); } +static void +test_default_handler_structured_logging_non_nul_terminated_strings (void) +{ + g_log_writer_default_set_use_stderr (FALSE); + g_log_set_default_handler (g_log_default_handler, NULL); + g_setenv ("G_MESSAGES_DEBUG", "foo", TRUE); + + const gchar domain_1[] = {'f', 'o', 'o' }; + const gchar domain_2[] = { 'b', 'a', 'r' }; + const gchar message_1[] = { 'b', 'a', 'z' }; + const gchar message_2[] = { 'b', 'l', 'a' }; + const GLogField fields[] = { + { "GLIB_DOMAIN", domain_1, sizeof (domain_1) }, + { "MESSAGE", message_1, sizeof (message_1) }, + }; + const GLogField other_fields[] = { + { "GLIB_DOMAIN", domain_2, sizeof (domain_2) }, + { "MESSAGE", message_2, sizeof (message_2) }, + }; + + g_log_structured_array (G_LOG_LEVEL_DEBUG, fields, G_N_ELEMENTS (fields)); + g_log_structured_array (G_LOG_LEVEL_DEBUG, other_fields, G_N_ELEMENTS (other_fields)); + + exit (0); +} + static void test_default_handler (void) { @@ -535,6 +561,12 @@ test_default_handler (void) g_test_trap_subprocess ("/logging/default-handler/subprocess/would-drop-robustness", 0, G_TEST_SUBPROCESS_DEFAULT); g_test_trap_assert_passed (); + g_test_trap_subprocess ("/logging/default-handler/subprocess/structured-logging-non-null-terminated-strings", 0, + G_TEST_SUBPROCESS_DEFAULT); + g_test_trap_assert_passed (); + g_test_trap_assert_stdout_unmatched ("*bar*"); + g_test_trap_assert_stdout_unmatched ("*bla*"); + g_test_trap_assert_stdout ("*foo-DEBUG*baz*"); } static void @@ -1074,6 +1106,7 @@ main (int argc, char *argv[]) g_test_add_func ("/logging/default-handler/subprocess/would-drop-env4", test_default_handler_would_drop_env4); g_test_add_func ("/logging/default-handler/subprocess/would-drop-env5", test_default_handler_would_drop_env5); g_test_add_func ("/logging/default-handler/subprocess/would-drop-robustness", test_default_handler_would_drop_robustness); + g_test_add_func ("/logging/default-handler/subprocess/structured-logging-non-null-terminated-strings", test_default_handler_structured_logging_non_nul_terminated_strings); g_test_add_func ("/logging/warnings", test_warnings); g_test_add_func ("/logging/fatal-log-mask", test_fatal_log_mask); g_test_add_func ("/logging/set-handler", test_set_handler);