From dce6599ec0923b721c356b46cedf81d77433b54a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ji=C5=99=C3=AD=20Klime=C5=A1?= Date: Wed, 21 May 2014 14:09:23 +0200 Subject: [PATCH 1/5] keyfile: fix reading MAC in old format (list of integers) Don't call nm_utils_hwaddr_type () with random len, because it causes ugly (NetworkManager:25325): libnm-util-CRITICAL **: file nm-utils.c: line 1989 (nm_utils_hwaddr_type): should not be reached And add a testcase. https://bugzilla.gnome.org/show_bug.cgi?id=730514 --- src/settings/plugins/keyfile/reader.c | 8 +- .../keyfile/tests/keyfiles/Makefile.am | 2 + .../tests/keyfiles/Test_MAC_IB_Old_Format | 13 ++++ .../tests/keyfiles/Test_MAC_Old_Format | 10 +++ .../plugins/keyfile/tests/test-keyfile.c | 77 ++++++++++++++++++- 5 files changed, 105 insertions(+), 5 deletions(-) create mode 100644 src/settings/plugins/keyfile/tests/keyfiles/Test_MAC_IB_Old_Format create mode 100644 src/settings/plugins/keyfile/tests/keyfiles/Test_MAC_Old_Format diff --git a/src/settings/plugins/keyfile/reader.c b/src/settings/plugins/keyfile/reader.c index 5ceb764c6d..735cbfc54b 100644 --- a/src/settings/plugins/keyfile/reader.c +++ b/src/settings/plugins/keyfile/reader.c @@ -554,17 +554,17 @@ mac_address_parser (NMSetting *setting, const char *key, GKeyFile *keyfile, cons } /* If we found enough it's probably a string-format MAC address */ - type = nm_utils_hwaddr_type (i + 1); - if (type > 0) + if (i+1 == ETH_ALEN || i+1 == INFINIBAND_ALEN) { + type = nm_utils_hwaddr_type (i + 1); array = nm_utils_hwaddr_atoba (tmp_string, type); + } } g_free (tmp_string); if (array == NULL) { /* Old format; list of ints */ tmp_list = nm_keyfile_plugin_kf_get_integer_list (keyfile, setting_name, key, &length, NULL); - type = nm_utils_hwaddr_type (length); - if (type < 0) { + if (length == ETH_ALEN || length == INFINIBAND_ALEN) { array = g_byte_array_sized_new (length); for (i = 0; i < length; i++) { int val = tmp_list[i]; diff --git a/src/settings/plugins/keyfile/tests/keyfiles/Makefile.am b/src/settings/plugins/keyfile/tests/keyfiles/Makefile.am index 2ac304e105..576164d274 100644 --- a/src/settings/plugins/keyfile/tests/keyfiles/Makefile.am +++ b/src/settings/plugins/keyfile/tests/keyfiles/Makefile.am @@ -3,6 +3,8 @@ KEYFILES = \ Test_GSM_Connection \ Test_Wireless_Connection \ Test_Wired_Connection_MAC_Case \ + Test_MAC_Old_Format \ + Test_MAC_IB_Old_Format \ Test_Wired_Connection_IP6 \ ATT_Data_Connect_BT \ ATT_Data_Connect_Plain \ diff --git a/src/settings/plugins/keyfile/tests/keyfiles/Test_MAC_IB_Old_Format b/src/settings/plugins/keyfile/tests/keyfiles/Test_MAC_IB_Old_Format new file mode 100644 index 0000000000..b2bf91550e --- /dev/null +++ b/src/settings/plugins/keyfile/tests/keyfiles/Test_MAC_IB_Old_Format @@ -0,0 +1,13 @@ +[connection] +id=Test InfiniBand Connection +uuid=5680a56d-c99f-45ad-a6dd-b44d5c398c12 +type=infiniband + +[infiniband] +mac-address=0;17;34;51;68;85;102;119;136;153;1;18;35;52;69;86;103;120;137;144; +transport-mode=datagram +mtu=1400 + +[ipv4] +method=auto + diff --git a/src/settings/plugins/keyfile/tests/keyfiles/Test_MAC_Old_Format b/src/settings/plugins/keyfile/tests/keyfiles/Test_MAC_Old_Format new file mode 100644 index 0000000000..9427b16c52 --- /dev/null +++ b/src/settings/plugins/keyfile/tests/keyfiles/Test_MAC_Old_Format @@ -0,0 +1,10 @@ +[connection] +id=Test MAC Old Format +uuid=8980a26d-c99f-4aad-a6bd-b439bc348ca4 +type=802-3-ethernet + +[802-3-ethernet] +mac-address=00:11:aa:BB:CC:55 +cloned-mac-address=00;22;170;187;204;254; +mtu=1400 + diff --git a/src/settings/plugins/keyfile/tests/test-keyfile.c b/src/settings/plugins/keyfile/tests/test-keyfile.c index f7d4122905..a300163b71 100644 --- a/src/settings/plugins/keyfile/tests/test-keyfile.c +++ b/src/settings/plugins/keyfile/tests/test-keyfile.c @@ -15,7 +15,7 @@ * with this program; if not, write to the Free Software Foundation, Inc., * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. * - * Copyright (C) 2008 - 2011 Red Hat, Inc. + * Copyright (C) 2008 - 2014 Red Hat, Inc. */ #include @@ -968,6 +968,79 @@ test_read_wired_mac_case (void) g_object_unref (connection); } +#define TEST_MAC_OLD_FORMAT_FILE TEST_KEYFILES_DIR"/Test_MAC_Old_Format" + +static void +test_read_mac_old_format (void) +{ + NMConnection *connection; + NMSettingWired *s_wired; + GError *error = NULL; + gboolean success; + const GByteArray *array; + char expected_mac[ETH_ALEN] = { 0x00, 0x11, 0xaa, 0xbb, 0xcc, 0x55 }; + char expected_cloned_mac[ETH_ALEN] = { 0x00, 0x16, 0xaa, 0xbb, 0xcc, 0xfe }; + + connection = nm_keyfile_plugin_connection_from_file (TEST_MAC_OLD_FORMAT_FILE, &error); + g_assert_no_error (error); + g_assert (connection); + + success = nm_connection_verify (connection, &error); + g_assert_no_error (error); + g_assert (success); + + s_wired = nm_connection_get_setting_wired (connection); + g_assert (s_wired); + + /* MAC address */ + array = nm_setting_wired_get_mac_address (s_wired); + g_assert (array); + g_assert_cmpint (array->len, ==, ETH_ALEN); + g_assert (memcmp (array->data, expected_mac, ETH_ALEN) == 0); + + /* Cloned MAC address */ + array = nm_setting_wired_get_cloned_mac_address (s_wired); + g_assert (array); + g_assert_cmpint (array->len, ==, ETH_ALEN); + g_assert (memcmp (array->data, expected_cloned_mac, ETH_ALEN) == 0); + + g_object_unref (connection); +} + +#define TEST_MAC_IB_OLD_FORMAT_FILE TEST_KEYFILES_DIR"/Test_MAC_IB_Old_Format" + +static void +test_read_mac_ib_old_format (void) +{ + NMConnection *connection; + NMSettingInfiniband *s_ib; + GError *error = NULL; + gboolean success; + const GByteArray *array; + guint8 expected_mac[INFINIBAND_ALEN] = { 0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, + 0x77, 0x88, 0x99, 0x01, 0x12, 0x23, 0x34, 0x45, 0x56, 0x67, 0x78, 0x89, + 0x90 }; + + connection = nm_keyfile_plugin_connection_from_file (TEST_MAC_IB_OLD_FORMAT_FILE, &error); + g_assert_no_error (error); + g_assert (connection); + + success = nm_connection_verify (connection, &error); + g_assert_no_error (error); + g_assert (success); + + s_ib = nm_connection_get_setting_infiniband (connection); + g_assert (s_ib); + + /* MAC address */ + array = nm_setting_infiniband_get_mac_address (s_ib); + g_assert (array); + g_assert_cmpint (array->len, ==, INFINIBAND_ALEN); + g_assert_cmpint (memcmp (array->data, expected_mac, sizeof (expected_mac)), ==, 0); + + g_object_unref (connection); +} + static void test_read_valid_wireless_connection (void) { @@ -3359,6 +3432,8 @@ int main (int argc, char **argv) test_write_ip6_wired_connection (); test_read_wired_mac_case (); + test_read_mac_old_format (); + test_read_mac_ib_old_format (); test_read_valid_wireless_connection (); test_write_wireless_connection (); From 3cda194b449cd14e507ec8293722487e501c8ebe Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 23 May 2014 12:50:43 +0200 Subject: [PATCH 2/5] libnm-util: make hwaddr functions more robust against invalid arguments - nm_utils_hwaddr_len() and nm_utils_hwaddr_type() no longer assert against known input types/lengths. Now they can be used to detect the hwaddr type, returning -1 on unknown. - more checking of input arguments in nm_utils_hwaddr_aton() and related. Also note, that nm_utils_hwaddr_aton_len() has @len of type gsize, so we cannot pass on the output of nm_utils_hwaddr_len() without checking for -1. Signed-off-by: Thomas Haller --- libnm-util/nm-utils.c | 56 ++++++++++++++++++++------- src/settings/plugins/keyfile/reader.c | 8 ++-- 2 files changed, 47 insertions(+), 17 deletions(-) diff --git a/libnm-util/nm-utils.c b/libnm-util/nm-utils.c index 890e17ca0e..ff8c669778 100644 --- a/libnm-util/nm-utils.c +++ b/libnm-util/nm-utils.c @@ -1946,10 +1946,7 @@ nm_utils_wifi_is_channel_valid (guint32 channel, const char *band) * * Returns the length in octets of a hardware address of type @type. * - * Note that this only accepts %ARPHRD_ETHER and %ARPHRD_INFINIBAND, - * not other types. - * - * Return value: the length + * Return value: the positive length, or -1 if the type is unknown/unsupported. */ int nm_utils_hwaddr_len (int type) @@ -1959,7 +1956,7 @@ nm_utils_hwaddr_len (int type) else if (type == ARPHRD_INFINIBAND) return INFINIBAND_ALEN; else - g_return_val_if_reached (-1); + return -1; } /** @@ -1970,6 +1967,7 @@ nm_utils_hwaddr_len (int type) * the raw address given its length. * * Return value: the type, either %ARPHRD_ETHER or %ARPHRD_INFINIBAND. + * If the length is unexpected, return -1 (unsupported type/length). * * Deprecated: This could not be extended to cover other types, since * there is not a one-to-one mapping between types and lengths. This @@ -1986,7 +1984,7 @@ nm_utils_hwaddr_type (int len) else if (len == INFINIBAND_ALEN) return ARPHRD_INFINIBAND; else - g_return_val_if_reached (-1); + return -1; } #define HEXVAL(c) ((c) <= '9' ? (c) - '0' : ((c) & 0x4F) - 'A' + 10) @@ -2009,7 +2007,13 @@ nm_utils_hwaddr_type (int len) guint8 * nm_utils_hwaddr_aton (const char *asc, int type, gpointer buffer) { - return nm_utils_hwaddr_aton_len (asc, buffer, nm_utils_hwaddr_len (type)); + int len = nm_utils_hwaddr_len (type); + + if (len <= 0) { + g_return_val_if_reached (NULL); + return NULL; + } + return nm_utils_hwaddr_aton_len (asc, buffer, len); } /** @@ -2029,9 +2033,14 @@ nm_utils_hwaddr_atoba (const char *asc, int type) GByteArray *ba; int len = nm_utils_hwaddr_len (type); + if (len <= 0) { + g_return_val_if_reached (NULL); + return NULL; + } + ba = g_byte_array_sized_new (len); - ba->len = len; - if (!nm_utils_hwaddr_aton (asc, type, ba->data)) { + g_byte_array_set_size (ba, len); + if (!nm_utils_hwaddr_aton_len (asc, ba->data, len)) { g_byte_array_unref (ba); return NULL; } @@ -2054,14 +2063,22 @@ nm_utils_hwaddr_atoba (const char *asc, int type) char * nm_utils_hwaddr_ntoa (gconstpointer addr, int type) { - return nm_utils_hwaddr_ntoa_len (addr, nm_utils_hwaddr_len (type)); + int len = nm_utils_hwaddr_len (type); + + if (len <= 0) { + g_return_val_if_reached (NULL); + return NULL; + } + + return nm_utils_hwaddr_ntoa_len (addr, len); } /** * nm_utils_hwaddr_aton_len: * @asc: the ASCII representation of a hardware address * @buffer: buffer to store the result into - * @length: the expected length in bytes of the result + * @length: the expected length in bytes of the result and + * the size of the buffer in bytes. * * Parses @asc and converts it to binary form in @buffer. * Bytes in @asc can be sepatared by colons (:), or hyphens (-), but not mixed. @@ -2078,6 +2095,13 @@ nm_utils_hwaddr_aton_len (const char *asc, gpointer buffer, gsize length) guint8 *out = (guint8 *)buffer; char delimiter = '\0'; + if (!asc) { + g_return_val_if_reached (NULL); + return NULL; + } + g_return_val_if_fail (buffer, NULL); + g_return_val_if_fail (length, NULL); + while (length && *in) { guint8 d1 = in[0], d2 = in[1]; @@ -2130,8 +2154,11 @@ char * nm_utils_hwaddr_ntoa_len (gconstpointer addr, gsize length) { const guint8 *in = addr; - GString *out = g_string_new (NULL); + GString *out; + g_return_val_if_fail (addr && length, g_strdup ("")); + + out = g_string_new (NULL); while (length--) { if (out->len) g_string_append_c (out, ':'); @@ -2156,8 +2183,11 @@ gboolean nm_utils_hwaddr_valid (const char *asc) { guint8 buf[NM_UTILS_HWADDR_LEN_MAX]; - int in_len = strlen (asc), out_len; + gsize in_len, out_len; + if (!asc && !*asc) + return FALSE; + in_len = strlen (asc); if ((in_len + 1) % 3 != 0) return FALSE; out_len = (in_len + 1) / 3; diff --git a/src/settings/plugins/keyfile/reader.c b/src/settings/plugins/keyfile/reader.c index 735cbfc54b..845f83aac9 100644 --- a/src/settings/plugins/keyfile/reader.c +++ b/src/settings/plugins/keyfile/reader.c @@ -554,17 +554,17 @@ mac_address_parser (NMSetting *setting, const char *key, GKeyFile *keyfile, cons } /* If we found enough it's probably a string-format MAC address */ - if (i+1 == ETH_ALEN || i+1 == INFINIBAND_ALEN) { - type = nm_utils_hwaddr_type (i + 1); + type = nm_utils_hwaddr_type (i + 1); + if (type >= 0) array = nm_utils_hwaddr_atoba (tmp_string, type); - } } g_free (tmp_string); if (array == NULL) { /* Old format; list of ints */ tmp_list = nm_keyfile_plugin_kf_get_integer_list (keyfile, setting_name, key, &length, NULL); - if (length == ETH_ALEN || length == INFINIBAND_ALEN) { + type = nm_utils_hwaddr_type (length); + if (type >= 0) { array = g_byte_array_sized_new (length); for (i = 0; i < length; i++) { int val = tmp_list[i]; From c4d90c56604e21b5b43539ee8e7707fbdf9c306e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 27 May 2014 15:42:08 +0200 Subject: [PATCH 3/5] cli: don't use nm_utils_hwaddr_type() to stringify HWADDR Use nm_utils_hwaddr_ntoa_len() instead of nm_utils_hwaddr_ntoa(). This makes it no longer necessary to determine the type of the MAC address based on the address length. This makes the GETTER more accepting towards other lengths. Signed-off-by: Thomas Haller --- cli/src/settings.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/src/settings.c b/cli/src/settings.c index 581db20b0a..50a96eba33 100644 --- a/cli/src/settings.c +++ b/cli/src/settings.c @@ -880,8 +880,8 @@ vpn_data_item (const char *key, const char *value, gpointer user_data) g_value_init (&val, DBUS_TYPE_G_UCHAR_ARRAY); \ g_object_get_property (G_OBJECT (setting), property_name, &val); \ array = g_value_get_boxed (&val); \ - if (array) \ - hwaddr = nm_utils_hwaddr_ntoa (array->data, nm_utils_hwaddr_type (array->len)); \ + if (array && array->len) \ + hwaddr = nm_utils_hwaddr_ntoa_len (array->data, array->len); \ g_value_unset (&val); \ return hwaddr; \ } From 415c86eb9ac88b0a8f614ea639baef9b5a39bca6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 27 May 2014 15:43:42 +0200 Subject: [PATCH 4/5] keyfile: don't check HWADDR length in mac_address_writer() When converting the MAC address to keyfile value, simply accept any given byte array and pass it to nm_utils_hwaddr_ntoa_len(). This no longer restricts the length of accepted addresses as known by nm_utils_hwaddr_type(). It is up to the caller to perform any validation of the MAC address. Signed-off-by: Thomas Haller --- src/settings/plugins/keyfile/writer.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/settings/plugins/keyfile/writer.c b/src/settings/plugins/keyfile/writer.c index e95ec87493..82bea5b8aa 100644 --- a/src/settings/plugins/keyfile/writer.c +++ b/src/settings/plugins/keyfile/writer.c @@ -414,22 +414,14 @@ mac_address_writer (GKeyFile *file, GByteArray *array; const char *setting_name = nm_setting_get_name (setting); char *mac; - int type; g_return_if_fail (G_VALUE_HOLDS (value, DBUS_TYPE_G_UCHAR_ARRAY)); array = (GByteArray *) g_value_get_boxed (value); - if (!array) + if (!array || !array->len) return; - type = nm_utils_hwaddr_type (array->len); - if (type < 0) { - nm_log_warn (LOGD_SETTINGS, "%s: invalid %s / %s MAC address length %d", - __func__, setting_name, key, array->len); - return; - } - - mac = nm_utils_hwaddr_ntoa (array->data, type); + mac = nm_utils_hwaddr_ntoa_len (array->data, array->len); nm_keyfile_plugin_kf_set_string (file, setting_name, key, mac); g_free (mac); } From d426ed28c203a6af8847a1019c6501da7dce5408 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 27 May 2014 16:04:56 +0200 Subject: [PATCH 5/5] keyfile: stricter checking for invalid HWADDR length in mac_address_parser() When reading a hardware address in keyfile plugin, check for the expected length already in mac_address_parser(). Before, we would call the deprecated function nm_utils_hwaddr_type() to see if it can be some kind of MAC address. In that case, the error was caught later during NMSetting:verify(). Signed-off-by: Thomas Haller --- src/settings/plugins/keyfile/reader.c | 54 ++++++++++++++++++--------- 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/src/settings/plugins/keyfile/reader.c b/src/settings/plugins/keyfile/reader.c index 845f83aac9..c7de6f4108 100644 --- a/src/settings/plugins/keyfile/reader.c +++ b/src/settings/plugins/keyfile/reader.c @@ -534,37 +534,43 @@ ip6_dns_parser (NMSetting *setting, const char *key, GKeyFile *keyfile, const ch } static void -mac_address_parser (NMSetting *setting, const char *key, GKeyFile *keyfile, const char *keyfile_path) +mac_address_parser (NMSetting *setting, const char *key, GKeyFile *keyfile, const char *keyfile_path, gsize enforce_length) { const char *setting_name = nm_setting_get_name (setting); char *tmp_string = NULL, *p; gint *tmp_list; GByteArray *array = NULL; gsize length; - int i, type; p = tmp_string = nm_keyfile_plugin_kf_get_string (keyfile, setting_name, key, NULL); - if (tmp_string) { + if (tmp_string && tmp_string[0]) { /* Look for enough ':' characters to signify a MAC address */ - i = 0; + guint i = 0; + while (*p) { if (*p == ':') i++; p++; } - /* If we found enough it's probably a string-format MAC address */ - type = nm_utils_hwaddr_type (i + 1); - if (type >= 0) - array = nm_utils_hwaddr_atoba (tmp_string, type); + if (enforce_length == 0 || enforce_length == i+1) { + /* If we found enough it's probably a string-format MAC address */ + array = g_byte_array_sized_new (i+1); + g_byte_array_set_size (array, i+1); + if (!nm_utils_hwaddr_aton_len (tmp_string, array->data, array->len)) { + g_byte_array_unref (array); + array = NULL; + } + } } g_free (tmp_string); if (array == NULL) { /* Old format; list of ints */ tmp_list = nm_keyfile_plugin_kf_get_integer_list (keyfile, setting_name, key, &length, NULL); - type = nm_utils_hwaddr_type (length); - if (type >= 0) { + if (length > 0 && (enforce_length == 0 || enforce_length == length)) { + gsize i; + array = g_byte_array_sized_new (length); for (i = 0; i < length; i++) { int val = tmp_list[i]; @@ -593,6 +599,18 @@ mac_address_parser (NMSetting *setting, const char *key, GKeyFile *keyfile, cons } } +static void +mac_address_parser_ETHER (NMSetting *setting, const char *key, GKeyFile *keyfile, const char *keyfile_path) +{ + mac_address_parser (setting, key, keyfile, keyfile_path, ETH_ALEN); +} + +static void +mac_address_parser_INFINIBAND (NMSetting *setting, const char *key, GKeyFile *keyfile, const char *keyfile_path) +{ + mac_address_parser (setting, key, keyfile, keyfile_path, INFINIBAND_ALEN); +} + static void read_hash_of_string (GKeyFile *file, NMSetting *setting, const char *key) { @@ -910,35 +928,35 @@ static KeyParser key_parsers[] = { { NM_SETTING_WIRED_SETTING_NAME, NM_SETTING_WIRED_MAC_ADDRESS, TRUE, - mac_address_parser }, + mac_address_parser_ETHER }, { NM_SETTING_WIRED_SETTING_NAME, NM_SETTING_WIRED_CLONED_MAC_ADDRESS, TRUE, - mac_address_parser }, + mac_address_parser_ETHER }, { NM_SETTING_WIRELESS_SETTING_NAME, NM_SETTING_WIRELESS_MAC_ADDRESS, TRUE, - mac_address_parser }, + mac_address_parser_ETHER }, { NM_SETTING_WIRELESS_SETTING_NAME, NM_SETTING_WIRELESS_CLONED_MAC_ADDRESS, TRUE, - mac_address_parser }, + mac_address_parser_ETHER }, { NM_SETTING_WIRELESS_SETTING_NAME, NM_SETTING_WIRELESS_BSSID, TRUE, - mac_address_parser }, + mac_address_parser_ETHER }, { NM_SETTING_BLUETOOTH_SETTING_NAME, NM_SETTING_BLUETOOTH_BDADDR, TRUE, - mac_address_parser }, + mac_address_parser_ETHER }, { NM_SETTING_INFINIBAND_SETTING_NAME, NM_SETTING_INFINIBAND_MAC_ADDRESS, TRUE, - mac_address_parser }, + mac_address_parser_INFINIBAND }, { NM_SETTING_WIMAX_SETTING_NAME, NM_SETTING_WIMAX_MAC_ADDRESS, TRUE, - mac_address_parser }, + mac_address_parser_ETHER }, { NM_SETTING_WIRELESS_SETTING_NAME, NM_SETTING_WIRELESS_SSID, TRUE,