From fb1d2da97927c2415773901a2548010e78575db8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 27 Jan 2023 12:28:54 +0100 Subject: [PATCH 1/2] glib-aux: add nm_random_u64_range() helper --- src/libnm-glib-aux/nm-random-utils.c | 50 +++++++++++++++++++ src/libnm-glib-aux/nm-random-utils.h | 35 +++++++++++++ .../tests/test-shared-general.c | 50 +++++++++++++++++++ 3 files changed, 135 insertions(+) diff --git a/src/libnm-glib-aux/nm-random-utils.c b/src/libnm-glib-aux/nm-random-utils.c index 93eee7c4d..8930ed491 100644 --- a/src/libnm-glib-aux/nm-random-utils.c +++ b/src/libnm-glib-aux/nm-random-utils.c @@ -447,3 +447,53 @@ again_getrandom: return nm_utils_fd_read_loop_exact(fd, p, n, FALSE); } + +guint64 +nm_random_u64_range_full(guint64 begin, guint64 end, gboolean crypto_bytes) +{ + gboolean bad_crypto_bytes = FALSE; + guint64 remainder; + guint64 maxvalue; + guint64 x; + guint64 m; + + /* Returns a random #guint64 equally distributed in the range [@begin..@end-1]. + * + * The function always set errno. It either sets it to zero or to EAGAIN + * (if crypto_bytes were requested but not obtained). In any case, the function + * will always return a random number in the requested range (worst case, it's + * not crypto_bytes despite being requested). Check errno if you care. */ + + if (begin >= end) { + /* systemd's random_u64_range(0) is an alias for random_u64_range((uint64_t)-1). + * Not for us. It's a caller error to request an element from an empty range. */ + return nm_assert_unreachable_val(begin); + } + + m = end - begin; + + if (m == 1) { + x = 0; + goto out; + } + + remainder = G_MAXUINT64 % m; + maxvalue = G_MAXUINT64 - remainder; + + do + if (crypto_bytes) { + if (nm_random_get_crypto_bytes(&x, sizeof(x)) < 0) { + /* Cannot get good crypto numbers. We will try our best, but fail + * and set errno below. */ + crypto_bytes = FALSE; + bad_crypto_bytes = TRUE; + continue; + } + } else + nm_random_get_bytes(&x, sizeof(x)); + while (x >= maxvalue); + +out: + errno = bad_crypto_bytes ? EAGAIN : 0; + return begin + (x % m); +} diff --git a/src/libnm-glib-aux/nm-random-utils.h b/src/libnm-glib-aux/nm-random-utils.h index ab8aee1b0..729d71a4d 100644 --- a/src/libnm-glib-aux/nm-random-utils.h +++ b/src/libnm-glib-aux/nm-random-utils.h @@ -16,4 +16,39 @@ nm_random_get_bytes(void *p, size_t n) int nm_random_get_crypto_bytes(void *p, size_t n); +static inline guint32 +nm_random_u32(void) +{ + guint32 v; + + nm_random_get_bytes(&v, sizeof(v)); + return v; +} + +static inline guint64 +nm_random_u64(void) +{ + guint64 v; + + nm_random_get_bytes(&v, sizeof(v)); + return v; +} + +static inline bool +nm_random_bool(void) +{ + guint8 ch; + + nm_random_get_bytes(&ch, sizeof(ch)); + return ch % 2u; +} + +guint64 nm_random_u64_range_full(guint64 begin, guint64 end, gboolean crypto_bytes); + +static inline guint64 +nm_random_u64_range(guint64 end) +{ + return nm_random_u64_range_full(0, end, FALSE); +} + #endif /* __NM_RANDOM_UTILS_H__ */ diff --git a/src/libnm-glib-aux/tests/test-shared-general.c b/src/libnm-glib-aux/tests/test-shared-general.c index 7503dc9b9..3eaca5474 100644 --- a/src/libnm-glib-aux/tests/test-shared-general.c +++ b/src/libnm-glib-aux/tests/test-shared-general.c @@ -137,6 +137,55 @@ test_nmhash(void) /*****************************************************************************/ +static void +test_nm_random(void) +{ + int i_run; + + for (i_run = 0; i_run < 1000; i_run++) { + guint64 begin; + guint64 end; + guint64 m; + guint64 x; + + m = nmtst_get_rand_uint64(); + m = m >> (nmtst_get_rand_uint32() % 64); + + if (m == 0) + continue; + + switch (nmtst_get_rand_uint32() % 4) { + case 0: + begin = 0; + break; + case 1: + begin = nmtst_get_rand_uint64() % 1000; + break; + case 2: + begin = ((G_MAXUINT64 - m) - 500) + (nmtst_get_rand_uint64() % 1000); + break; + default: + begin = nmtst_get_rand_uint64() % (G_MAXUINT64 - m); + break; + } + + end = (begin + m) - 10 + (nmtst_get_rand_uint64() % 5); + + if (begin >= end) + continue; + + if (begin == 0 && nmtst_get_rand_bool()) + x = nm_random_u64_range(end); + else + x = nm_random_u64_range_full(begin, end, nmtst_get_rand_bool()); + + g_assert_cmpuint(x, >=, begin); + g_assert_cmpuint(x, <, end); + } +} + +/*****************************************************************************/ + static const char * _make_strv_foo(void) { @@ -2417,6 +2466,7 @@ main(int argc, char **argv) g_test_add_func("/general/test_inet_utils", test_inet_utils); g_test_add_func("/general/test_garray", test_garray); g_test_add_func("/general/test_nm_prioq", test_nm_prioq); + g_test_add_func("/general/test_nm_random", test_nm_random); return g_test_run(); } From 6e96d7173168e2231cf576bc9f2aeb1f13529bca Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 27 Jan 2023 12:52:20 +0100 Subject: [PATCH 2/2] all: use nm_random_*() instead of g_random_*() g_random_*() is based on GRand, which is not a CSPRNG. Instead, rely on kernel to give us good random numbers, which is what nm_random_*() does. Note that nm_random_*() calls getrandom() (or reads /dev/urandom), which most likely is slower than GRand. It doesn't matter for our uses though. It is cumbersome to review all uses of g_rand_*() whether their usage of a non-cryptographically secure generator is appropriate. Instead, just always use an appropriate function, thereby avoiding this question. Even glib documentation refers to reading "/dev/urandom" as alternative. Which is what nm_random_*() does. These days, it seems unnecessary to not use the best random generator available, unless it's not fast enough or you need a stable/seedable stream of random numbers. In particular in nmcli, we used g_random_int_range() to generate passwords. That is not appropriate. Sure, it's *only* for the hotspot, but still. --- src/core/ndisc/nm-ndisc.c | 19 ++++++++++--------- src/core/nm-core-utils.c | 4 +--- src/libnm-client-aux-extern/nm-libnm-aux.c | 4 +++- src/nmcli/devices.c | 13 ++++++++----- 4 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/core/ndisc/nm-ndisc.c b/src/core/ndisc/nm-ndisc.c index 39a4df482..20dd21212 100644 --- a/src/core/ndisc/nm-ndisc.c +++ b/src/core/ndisc/nm-ndisc.c @@ -10,6 +10,7 @@ #include #include +#include "libnm-glib-aux/nm-random-utils.h" #include "libnm-platform/nm-platform-utils.h" #include "libnm-platform/nm-platform.h" #include "libnm-platform/nmp-netns.h" @@ -858,7 +859,7 @@ solicit_retransmit_time_jitter(gint32 solicit_retransmit_time_msec) ten_percent = NM_MAX(1, solicit_retransmit_time_msec / 10); return solicit_retransmit_time_msec - ten_percent - + ((gint32) (g_random_int() % (2u * ((guint32) ten_percent)))); + + ((gint32) (nm_random_u32() % (2u * ((guint32) ten_percent)))); } static gboolean @@ -936,7 +937,7 @@ solicit_timer_start(NMNDisc *ndisc) * a suitable delay in 2021. Wait only up to 250 msec instead. */ delay_msec = - g_random_int() % ((guint32) (NM_NDISC_RFC4861_MAX_RTR_SOLICITATION_DELAY * 1000 / 4)); + nm_random_u32() % ((guint32) (NM_NDISC_RFC4861_MAX_RTR_SOLICITATION_DELAY * 1000 / 4)); _LOGD("solicit: schedule sending first solicitation (of %d) in %.3f seconds", priv->config.router_solicitations, @@ -974,8 +975,9 @@ announce_router(NMNDisc *ndisc) /* Schedule next initial announcement retransmit. */ priv->send_ra_id = - g_timeout_add_seconds(g_random_int_range(NM_NDISC_ROUTER_ADVERT_DELAY, - NM_NDISC_ROUTER_ADVERT_INITIAL_INTERVAL), + g_timeout_add_seconds(nm_random_u64_range_full(NM_NDISC_ROUTER_ADVERT_DELAY, + NM_NDISC_ROUTER_ADVERT_INITIAL_INTERVAL, + FALSE), (GSourceFunc) announce_router, ndisc); } else { @@ -1009,10 +1011,9 @@ announce_router_initial(NMNDisc *ndisc) /* Schedule the initial send rather early. Clamp the delay by minimal * delay and not the initial advert internal so that we start fast. */ if (G_LIKELY(!priv->send_ra_id)) { - priv->send_ra_id = - g_timeout_add_seconds(g_random_int_range(0, NM_NDISC_ROUTER_ADVERT_DELAY), - (GSourceFunc) announce_router, - ndisc); + priv->send_ra_id = g_timeout_add_seconds(nm_random_u64_range(NM_NDISC_ROUTER_ADVERT_DELAY), + (GSourceFunc) announce_router, + ndisc); } } @@ -1028,7 +1029,7 @@ announce_router_solicited(NMNDisc *ndisc) nm_clear_g_source(&priv->send_ra_id); if (!priv->send_ra_id) { - priv->send_ra_id = g_timeout_add(g_random_int_range(0, NM_NDISC_ROUTER_ADVERT_DELAY_MS), + priv->send_ra_id = g_timeout_add(nm_random_u64_range(NM_NDISC_ROUTER_ADVERT_DELAY_MS), (GSourceFunc) announce_router, ndisc); } diff --git a/src/core/nm-core-utils.c b/src/core/nm-core-utils.c index 9448ba7b0..53afaa547 100644 --- a/src/core/nm-core-utils.c +++ b/src/core/nm-core-utils.c @@ -3673,9 +3673,7 @@ _hw_addr_eth_complete(struct ether_addr *addr, nm_assert((ouis == NULL) ^ (ouis_len != 0)); if (ouis) { - /* g_random_int() is good enough here. It uses a static GRand instance - * that is seeded from /dev/urandom. */ - oui = ouis[g_random_int() % ouis_len]; + oui = ouis[nm_random_u64_range(ouis_len)]; g_free(ouis); } else { if (!nm_utils_hwaddr_aton(current_mac_address, &oui, ETH_ALEN)) diff --git a/src/libnm-client-aux-extern/nm-libnm-aux.c b/src/libnm-client-aux-extern/nm-libnm-aux.c index 82a75de69..5855bc299 100644 --- a/src/libnm-client-aux-extern/nm-libnm-aux.c +++ b/src/libnm-client-aux-extern/nm-libnm-aux.c @@ -4,6 +4,8 @@ #include "nm-libnm-aux.h" +#include "libnm-glib-aux/nm-random-utils.h" + /*****************************************************************************/ NMClient * @@ -101,7 +103,7 @@ nmc_client_new_waitsync(GCancellable *cancellable, * code no longer uses that, we hardly test those code paths. But they should * work just the same. Randomly use instead the sync initialization in a debug * build... */ - if ((g_random_int() % 2) == 0) { + if (nm_random_bool()) { gboolean success; va_start(ap, first_property_name); diff --git a/src/nmcli/devices.c b/src/nmcli/devices.c index f71a7d036..4a0c3ab8a 100644 --- a/src/nmcli/devices.c +++ b/src/nmcli/devices.c @@ -17,6 +17,7 @@ #include #include "libnm-glib-aux/nm-secret-utils.h" +#include "libnm-glib-aux/nm-random-utils.h" #include "common.h" #include "connections.h" #include "libnmc-base/nm-client-utils.h" @@ -4098,10 +4099,11 @@ generate_wpa_key(char *key, size_t len) /* generate a 8-chars ASCII WPA key */ for (i = 0; i < WPA_PASSKEY_SIZE; i++) { int c; - c = g_random_int_range(33, 126); - /* too many non alphanumeric characters are hard to remember for humans */ - while (!g_ascii_isalnum(c)) - c = g_random_int_range(33, 126); + + do { + c = nm_random_u64_range_full(33, 126, TRUE); + /* too many non alphanumeric characters are hard to remember for humans */ + } while (g_ascii_isalnum(c)); key[i] = (char) c; } @@ -4120,7 +4122,8 @@ generate_wep_key(char *key, size_t len) /* generate a 10-digit hex WEP key */ for (i = 0; i < 10; i++) { int digit; - digit = g_random_int_range(0, 16); + + digit = nm_random_u64_range_full(0, 16, TRUE); key[i] = hexdigits[digit]; } key[10] = '\0';