From 06a976358b5946a5def73cb2532e2da666209432 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Sep 2019 13:17:28 +0200 Subject: [PATCH 1/6] shared: add nm_utils_addr_family_from_size() helper --- shared/nm-glib-aux/nm-shared-utils.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index bbbb9b64be..4abd2a004d 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -89,6 +89,16 @@ nm_utils_addr_family_to_size (int addr_family) g_return_val_if_reached (0); } +static inline int +nm_utils_addr_family_from_size (gsize len) +{ + switch (len) { + case sizeof (in_addr_t): return AF_INET; + case sizeof (struct in6_addr): return AF_INET6; + } + return AF_UNSPEC; +} + #define nm_assert_addr_family(addr_family) \ nm_assert (NM_IN_SET ((addr_family), AF_INET, AF_INET6)) From 8fbf67d13826f73be75fa301d8bd27f4dba8b52c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 3 Sep 2019 13:49:47 +0200 Subject: [PATCH 2/6] shared: add nm_utils_parse_inaddr_bin_full() to support legacy IPv4 formats as inet_aton() inet_aton() also supports IPv4 addresses in octal (with a leading '0') or where not all 4 digits of the address are present. Add nm_utils_parse_inaddr_bin_full() to optionally fallback to parse the address with inet_aton(). Note taht inet_aton() also supports all crazy formats, including ignoring trailing garbage after a whitespace. We don't want to accept that in general. Note that even in legacy format we: - accept everything that inet_pton() would accept - additionally, we also accept some forms which inet_aton() would accept, but not all. That means, the legacy format that we accept is a superset of inet_pton() and a subset of inet_aton(). Which is desirable. --- shared/nm-glib-aux/nm-shared-utils.c | 99 ++++++++++++++++++++++++++-- shared/nm-glib-aux/nm-shared-utils.h | 17 +++-- 2 files changed, 106 insertions(+), 10 deletions(-) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index be9fbdc4ae..8e1c8b5806 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -683,11 +683,80 @@ nm_utils_ip_is_site_local (int addr_family, /*****************************************************************************/ +static gboolean +_parse_legacy_addr4 (const char *text, in_addr_t *out_addr) +{ + gs_free char *s_free = NULL; + struct in_addr a1; + guint8 bin[sizeof (a1)]; + char *s; + int i; + + if (inet_aton (text, &a1) != 1) + return FALSE; + + /* OK, inet_aton() accepted the format. That's good, because we want + * to accept IPv4 addresses in octal format, like 255.255.000.000. + * That's what "legacy" means here. inet_pton() doesn't accept those. + * + * But inet_aton() also ignores trailing garbage and formats with fewer than + * 4 digits. That is just too crazy and we don't do that. Perform additional checks + * and reject some forms that inet_aton() accepted. + * + * Note that we still should (of course) accept everything that inet_pton() + * accepts. However this code never gets called if inet_pton() succeeds + * (see below, aside the assertion code). */ + + if (NM_STRCHAR_ANY (text, ch, ( !(ch >= '0' && ch <= '9') + && !NM_IN_SET (ch, '.', 'x')))) { + /* We only accepts '.', digits, and 'x' for "0x". */ + return FALSE; + } + + s = nm_memdup_maybe_a (300, text, strlen (text) + 1, &s_free); + + for (i = 0; i < G_N_ELEMENTS (bin); i++) { + char *current_token = s; + gint32 v; + + s = strchr (s, '.'); + if (s) { + s[0] = '\0'; + s++; + } + + if ((i == G_N_ELEMENTS (bin) - 1) != (s == NULL)) { + /* Exactly for the last digit, we expect to have no more following token. + * But this isn't the case. Abort. */ + return FALSE; + } + + v = _nm_utils_ascii_str_to_int64 (current_token, 0, 0, 0xFF, -1); + if (v == -1) { + /* we do accept octal and hex (even with leading "0x"). But something + * about this token is wrong. */ + return FALSE; + } + + bin[i] = v; + } + + if (memcmp (bin, &a1, sizeof (bin)) != 0) { + /* our parsing did not agree with what inet_aton() gave. Something + * is wrong. Abort. */ + return FALSE; + } + + *out_addr = a1.s_addr; + return TRUE; +} + gboolean -nm_utils_parse_inaddr_bin (int addr_family, - const char *text, - int *out_addr_family, - gpointer out_addr) +nm_utils_parse_inaddr_bin_full (int addr_family, + gboolean accept_legacy, + const char *text, + int *out_addr_family, + gpointer out_addr) { NMIPAddr addrbin; @@ -699,8 +768,26 @@ nm_utils_parse_inaddr_bin (int addr_family, } else g_return_val_if_fail (NM_IN_SET (addr_family, AF_INET, AF_INET6), FALSE); - if (inet_pton (addr_family, text, &addrbin) != 1) - return FALSE; + if (inet_pton (addr_family, text, &addrbin) != 1) { + if ( accept_legacy + && addr_family == AF_INET + && _parse_legacy_addr4 (text, &addrbin.addr4)) { + /* The address is in some legacy format which inet_aton() accepts, but not inet_pton(). + * Most likely octal digits (leading zeros). We accept the address. */ + } else + return FALSE; + } + +#if NM_MORE_ASSERTS > 10 + if (addr_family == AF_INET) { + in_addr_t a; + + /* The legacy parser should accept everything that inet_pton() accepts too. Meaning, + * it should strictly parse *more* formats. And of course, parse it the same way. */ + nm_assert (_parse_legacy_addr4 (text, &a)); + nm_assert (addrbin.addr4 == a); + } +#endif NM_SET_OUT (out_addr_family, addr_family); if (out_addr) diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index 4abd2a004d..3bd1afaea3 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -542,10 +542,19 @@ gboolean nm_utils_ip_is_site_local (int addr_family, /*****************************************************************************/ -gboolean nm_utils_parse_inaddr_bin (int addr_family, - const char *text, - int *out_addr_family, - gpointer out_addr); +gboolean nm_utils_parse_inaddr_bin_full (int addr_family, + gboolean accept_legacy, + const char *text, + int *out_addr_family, + gpointer out_addr); +static inline gboolean +nm_utils_parse_inaddr_bin (int addr_family, + const char *text, + int *out_addr_family, + gpointer out_addr) +{ + return nm_utils_parse_inaddr_bin_full (addr_family, FALSE, text, out_addr_family, out_addr); +} gboolean nm_utils_parse_inaddr (int addr_family, const char *text, From 321a323df4ad5340eff843ceae1ffee2e8930066 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 3 Oct 2019 11:22:48 +0200 Subject: [PATCH 3/6] initrd: fix use-after-free for variable "s_gateway" in nmi_dt_reader_parse() --- src/initrd/nmi-dt-reader.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/initrd/nmi-dt-reader.c b/src/initrd/nmi-dt-reader.c index d2e571d7a8..140ef12578 100644 --- a/src/initrd/nmi-dt-reader.c +++ b/src/initrd/nmi-dt-reader.c @@ -329,8 +329,6 @@ nmi_dt_reader_parse (const char *sysfs_dir) if (netmask) nm_ip_address_unref (netmask); - if (gateway) - nm_ip_address_unref (gateway); } if (!ipaddr) { @@ -377,6 +375,8 @@ nmi_dt_reader_parse (const char *sysfs_dir) if (ipaddr) nm_ip_address_unref (ipaddr); + if (gateway) + nm_ip_address_unref (gateway); if (duplex || speed || hwaddr || local_hwaddr) { s_wired = nm_setting_wired_new (); From e7cf22be3ed1e2f0a403a7809e2f791cb01af404 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 21 Sep 2019 15:20:24 +0200 Subject: [PATCH 4/6] initrd: use cleanup attribute in nmi_dt_reader_parse() --- src/initrd/nmi-dt-reader.c | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/src/initrd/nmi-dt-reader.c b/src/initrd/nmi-dt-reader.c index 140ef12578..3290689d9c 100644 --- a/src/initrd/nmi-dt-reader.c +++ b/src/initrd/nmi-dt-reader.c @@ -135,7 +135,7 @@ str_addr (const char *str, int *family) NMConnection * nmi_dt_reader_parse (const char *sysfs_dir) { - NMConnection *connection; + gs_unref_object NMConnection *connection = NULL; gs_free char *base = NULL; gs_free char *bootpath = NULL; gs_strfreev char **tokens = NULL; @@ -144,9 +144,8 @@ nmi_dt_reader_parse (const char *sysfs_dir) const char *s_ipaddr = NULL; const char *s_netmask = NULL; const char *s_gateway = NULL; - NMIPAddress *ipaddr = NULL; - NMIPAddress *netmask = NULL; - NMIPAddress *gateway = NULL; + nm_auto_unref_ip_address NMIPAddress *ipaddr = NULL; + nm_auto_unref_ip_address NMIPAddress *gateway = NULL; const char *duplex = NULL; gs_free char *hwaddr = NULL; gs_free char *local_hwaddr = NULL; @@ -279,10 +278,10 @@ nmi_dt_reader_parse (const char *sysfs_dir) connection = nm_simple_connection_new (); nm_connection_add_setting (connection, - g_object_new (NM_TYPE_SETTING_CONNECTION, - NM_SETTING_CONNECTION_TYPE, NM_SETTING_WIRED_SETTING_NAME, - NM_SETTING_CONNECTION_ID, "OpenFirmware Connection", - NULL)); + g_object_new (NM_TYPE_SETTING_CONNECTION, + NM_SETTING_CONNECTION_TYPE, NM_SETTING_WIRED_SETTING_NAME, + NM_SETTING_CONNECTION_ID, "OpenFirmware Connection", + NULL)); s_ip4 = nm_setting_ip4_config_new (); nm_connection_add_setting (connection, s_ip4); @@ -298,6 +297,8 @@ nmi_dt_reader_parse (const char *sysfs_dir) bootp = TRUE; if (!bootp) { + nm_auto_unref_ip_address NMIPAddress *netmask = NULL; + netmask = dt_get_ipaddr_property (base, "chosen", "netmask-ip", &family); gateway = dt_get_ipaddr_property (base, "chosen", "gateway-ip", &family); if (gateway) @@ -305,9 +306,9 @@ nmi_dt_reader_parse (const char *sysfs_dir) ipaddr = dt_get_ipaddr_property (base, "chosen", "client-ip", &family); if (family == AF_UNSPEC) { - g_warn_if_fail (netmask == NULL); - g_warn_if_fail (ipaddr == NULL); - g_warn_if_fail (gateway == NULL); + nm_assert (netmask == NULL); + nm_assert (gateway == NULL); + nm_assert (ipaddr == NULL); netmask = str_addr (s_netmask, &family); ipaddr = str_addr (s_ipaddr, &family); @@ -326,9 +327,6 @@ nmi_dt_reader_parse (const char *sysfs_dir) _LOGW (LOGD_CORE, "Unable to determine the network prefix"); else nm_ip_address_set_prefix (ipaddr, prefix); - - if (netmask) - nm_ip_address_unref (netmask); } if (!ipaddr) { @@ -373,11 +371,6 @@ nmi_dt_reader_parse (const char *sysfs_dir) g_object_set (s_ip, NM_SETTING_IP_CONFIG_GATEWAY, s_gateway, NULL); } - if (ipaddr) - nm_ip_address_unref (ipaddr); - if (gateway) - nm_ip_address_unref (gateway); - if (duplex || speed || hwaddr || local_hwaddr) { s_wired = nm_setting_wired_new (); nm_connection_add_setting (connection, s_wired); @@ -390,11 +383,11 @@ nmi_dt_reader_parse (const char *sysfs_dir) NULL); } - if (!nm_connection_normalize (connection, NULL, NULL, &error)) { + if (!nm_connection_normalize (connection, NULL, NULL, &error)) { _LOGW (LOGD_CORE, "Generated an invalid connection: %s", error->message); g_clear_pointer (&connection, g_object_unref); } - return connection; + return g_steal_pointer (&connection); } From 9618f1bb4b5e16f95c7d19556356b990486966b7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 21 Sep 2019 15:27:49 +0200 Subject: [PATCH 5/6] initrd: fix out-of-bounds read when detecting address family in dt_get_ipaddr_property() The @family argument is an input and output argument. Initially, the family is set to AF_UNSPEC, in which case the family gets detected based on the IP address. However, we call dt_get_ipaddr_property() multiple times to parse the netmask, the gateway and the IP address. That means, after the first successfull call, the @family is set to AF_INET or AF_INET6. Note that the previous code (in the switch block) would only check that the family is set to AF_UNSPEC, but it would not check that the @family matches the expected binary address length @len. Later, we then might call nm_ip_address_new_binary() with a family and a binary address of unexpected length. Also drop the error checking for nm_ip_address_new_binary(). nm_ip_address_new_binary() can only fail if the prefix length is larger than 32/128. The function has no way to validate the input arguments beyond that and can thus not fail (short of undefined behavior). --- src/initrd/nmi-dt-reader.c | 30 +++++++----------------------- 1 file changed, 7 insertions(+), 23 deletions(-) diff --git a/src/initrd/nmi-dt-reader.c b/src/initrd/nmi-dt-reader.c index 3290689d9c..bc574395fc 100644 --- a/src/initrd/nmi-dt-reader.c +++ b/src/initrd/nmi-dt-reader.c @@ -51,40 +51,24 @@ dt_get_ipaddr_property (const char *base, const char *prop, int *family) { - NMIPAddress *addr; gs_free char *buf = NULL; size_t len; - gs_free_error GError *error = NULL; + int f; if (!dt_get_property (base, dev, prop, &buf, &len)) return NULL; - switch (len) { - case 4: - if (*family == AF_UNSPEC) - *family = AF_INET; - break; - case 16: - if (*family == AF_UNSPEC) - *family = AF_INET6; - break; - default: - break; - } - - if (*family == AF_UNSPEC) { + f = nm_utils_addr_family_from_size (len); + if ( f == AF_UNSPEC + || ( *family != AF_UNSPEC + && *family != f)) { _LOGW (LOGD_CORE, "%s: Address %s has unrecognized length (%zd)", dev, prop, len); return NULL; } - addr = nm_ip_address_new_binary (*family, buf, 0, &error); - if (!addr) { - _LOGW (LOGD_CORE, "%s: Address %s is malformed: %s", - dev, prop, error->message); - } - - return addr; + *family = f; + return nm_ip_address_new_binary (f, buf, 0, NULL); } static char * From d68373c305fcb6ffb5a7c60a081db9b0f2832de1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 21 Sep 2019 15:39:19 +0200 Subject: [PATCH 6/6] initrd: don't use inet_aton() to parse IPv4 address inet_aton() is very accepting when parsing the address. For example, it accepts addresses with fewer octets (interpreting the last octet as a number in network byte order for multiple bytes). It also ignores any trailing garbage after the first delimiting whitespace (at least, the glibc implementation). It also accepts octets in hex and octal notation. For the initrd reader we want to be more forgiving than inet_pton() and also accept addresses like 255.000.000.000 (octal notation). For that we would want to use inet_aton(). But we should not accept all the craziness that inet_aton() otherwise accepts. Use nm_utils_parse_inaddr_bin_full() instead. This function implements our way how we want to interpret IP addresses in string representation. Under the hood, of course it also uses inet_pton() and even inet_aton(), but it is stricter than inet_aton() and only accepts certain formats. --- src/initrd/nmi-dt-reader.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/initrd/nmi-dt-reader.c b/src/initrd/nmi-dt-reader.c index bc574395fc..a8677ed9c3 100644 --- a/src/initrd/nmi-dt-reader.c +++ b/src/initrd/nmi-dt-reader.c @@ -96,24 +96,17 @@ dt_get_hwaddr_property (const char *base, static NMIPAddress * str_addr (const char *str, int *family) { - struct in_addr inp; + NMIPAddr addr_bin; - if (*family == AF_UNSPEC) - *family = guess_ip_address_family (str); - - if (*family == AF_UNSPEC) { + if (!nm_utils_parse_inaddr_bin_full (*family, + TRUE, + str, + family, + &addr_bin)) { _LOGW (LOGD_CORE, "Malformed IP address: '%s'", str); return NULL; } - - if (*family == AF_INET && inet_aton (str, &inp)) { - /* For IPv4, we need to be more tolerant than - * nm_ip_address_new(), to recognize things like - * the extra zeroes in "255.255.255.000" */ - return nm_ip_address_new_binary (*family, &inp, 0, NULL); - } - - return nm_ip_address_new (*family, str, 0, NULL); + return nm_ip_address_new_binary (*family, &addr_bin, 0, NULL); } NMConnection *