From 54a64edefc4222b8ef20d837c885618ea5f6e0d7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 13 May 2020 22:48:34 +0200 Subject: 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. --- 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 f9bccea930..59798c1d9b 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 52f05dc306..a6fb700715 100644 --- a/libnm-core/tests/test-general.c +++ b/libnm-core/tests/test-general.c @@ -4278,7 +4278,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)); -- cgit v1.2.1