From 3a66217cbb5d1f4bc4faf7cc6a3e02900b9360db Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 13 May 2020 22:48:34 +0200 Subject: [PATCH] libnm: don't compare invalid mac addresses as equal in nm_utils_hwaddr_matches() By passing as length of the MAC addresses -1 for both arguments, one could get through to compare empty strings, NULL, and addresses longer than the maximum. Such addresses are not valid, and they should never compare equal (not even to themselves). This is a change in behavior of public API, but it never made sense to claim two addresses are equal, when they are not even valid addresses. Also, avoid undefined behavior with "NULL, -1, NULL, -1" arguments, where we would call memcmp() with zero length and NULL arguments. UBSan flags that too. (cherry picked from commit 54a64edefc4222b8ef20d837c885618ea5f6e0d7) --- libnm-core/nm-utils.c | 15 ++++++++++++--- libnm-core/tests/test-general.c | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index 7391b0f00a..fed3f11056 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -4269,7 +4269,8 @@ nm_utils_hwaddr_matches (gconstpointer hwaddr1, hwaddr1 = buf1; hwaddr1_len = l; } else { - g_return_val_if_fail ((hwaddr2_len == -1 && hwaddr2) || (hwaddr2_len > 0 && hwaddr2_len <= NM_UTILS_HWADDR_LEN_MAX), FALSE); + g_return_val_if_fail ( hwaddr2_len == -1 + || (hwaddr2_len > 0 && hwaddr2_len <= NM_UTILS_HWADDR_LEN_MAX), FALSE); return FALSE; } } else { @@ -4301,9 +4302,17 @@ nm_utils_hwaddr_matches (gconstpointer hwaddr1, } } + if (G_UNLIKELY ( hwaddr1_len <= 0 + || hwaddr1_len > NM_UTILS_HWADDR_LEN_MAX)) { + /* Only valid addresses can compare equal. In particular, + * addresses that are too long or of zero bytes, never + * compare equal. */ + return FALSE; + } + if (hwaddr1_len == INFINIBAND_ALEN) { - hwaddr1 = (guint8 *)hwaddr1 + INFINIBAND_ALEN - 8; - hwaddr2 = (guint8 *)hwaddr2 + INFINIBAND_ALEN - 8; + hwaddr1 = &((guint8 *) hwaddr1)[INFINIBAND_ALEN - 8]; + hwaddr2 = &((guint8 *) hwaddr2)[INFINIBAND_ALEN - 8]; hwaddr1_len = 8; } diff --git a/libnm-core/tests/test-general.c b/libnm-core/tests/test-general.c index 138702e4ca..2b1f1e3d75 100644 --- a/libnm-core/tests/test-general.c +++ b/libnm-core/tests/test-general.c @@ -4277,7 +4277,7 @@ test_hwaddr_equal (void) g_assert (nm_utils_hwaddr_matches (null_binary, sizeof (null_binary), null_binary, sizeof (null_binary))); g_assert (nm_utils_hwaddr_matches (null_binary, sizeof (null_binary), NULL, ETH_ALEN)); - g_assert (nm_utils_hwaddr_matches (NULL, -1, NULL, -1)); + g_assert (!nm_utils_hwaddr_matches (NULL, -1, NULL, -1)); g_assert (!nm_utils_hwaddr_matches (NULL, -1, string, -1)); g_assert (!nm_utils_hwaddr_matches (string, -1, NULL, -1)); g_assert (!nm_utils_hwaddr_matches (NULL, -1, null_string, -1));