From 3266203bf1579207f1206b6220a5943e0b7905bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Fri, 25 Apr 2025 09:19:55 +0200 Subject: [PATCH 1/2] dns: ensure that no wrong separators are used for DNS search domains If wrong separators are used in they keyfile, like commas, the whole line is considered as a single domain string, like "a.org,b.org". Obviously this is invalid. Ideally we should validate that the string is a valid domain, but this gets quite complex if we want to support unicode characters, which are valid for many top domains. For now, validate at least that no wrong separators have been used. Fixes https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/1740 --- src/libnm-core-impl/nm-setting-ip-config.c | 22 ++++++++++++ src/libnm-core-impl/tests/test-setting.c | 39 ++++++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/src/libnm-core-impl/nm-setting-ip-config.c b/src/libnm-core-impl/nm-setting-ip-config.c index 6c77d954b6..20aca478d8 100644 --- a/src/libnm-core-impl/nm-setting-ip-config.c +++ b/src/libnm-core-impl/nm-setting-ip-config.c @@ -5671,6 +5671,28 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) } } + /* Validate DNS search domains */ + if (nm_strvarray_get_strv_notempty(priv->dns_search.arr, NULL)) { + for (i = 0; i < priv->dns_search.arr->len; i++) { + const char *dns_search = nm_strvarray_get_idx(priv->dns_search.arr, i); + + /* TODO: currently we only check that no wrong list separators have + * been used by mistake. Proper domain name validation would be better. */ + if (strpbrk(dns_search, ",; ")) { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("DNS search domain '%s' is invalid"), + dns_search); + g_prefix_error(error, + "%s.%s: ", + nm_setting_get_name(setting), + NM_SETTING_IP_CONFIG_DNS_SEARCH); + return FALSE; + } + } + } + /* Validate addresses */ for (i = 0; i < priv->addresses->len; i++) { NMIPAddress *addr = (NMIPAddress *) priv->addresses->pdata[i]; diff --git a/src/libnm-core-impl/tests/test-setting.c b/src/libnm-core-impl/tests/test-setting.c index 2064162a9f..ee0fa1586c 100644 --- a/src/libnm-core-impl/tests/test-setting.c +++ b/src/libnm-core-impl/tests/test-setting.c @@ -5460,6 +5460,44 @@ test_settings_dns(void) } } +static void +_assert_dns_searches(gboolean valid, ...) +{ + NMConnection *con; + NMSettingIPConfig *ip4, *ip6; + const char *dns_search; + va_list args; + + con = nmtst_create_minimal_connection("test-dns-search", + NULL, + NM_SETTING_WIRED_SETTING_NAME, + NULL); + nmtst_connection_normalize(con); + ip4 = nm_connection_get_setting_ip4_config(con); + ip6 = nm_connection_get_setting_ip6_config(con); + + va_start(args, valid); + while ((dns_search = va_arg(args, const char *))) { + nm_setting_ip_config_add_dns_search(ip4, dns_search); + nm_setting_ip_config_add_dns_search(ip6, dns_search); + } + va_end(args); + + g_assert(valid == nm_setting_verify((NMSetting *) ip4, con, NULL)); + g_assert(valid == nm_setting_verify((NMSetting *) ip6, con, NULL)); +} + +static void +test_settings_dns_search_domains(void) +{ + _assert_dns_searches(TRUE, "example.com", NULL); + _assert_dns_searches(TRUE, "sub.example.com", NULL); + _assert_dns_searches(TRUE, "example.com", "sub.example.com", NULL); + _assert_dns_searches(FALSE, "example.com,sub.example.com", NULL); + _assert_dns_searches(FALSE, "example.com;sub.example.com", NULL); + _assert_dns_searches(FALSE, "example.com sub.example.com", NULL); +} + /*****************************************************************************/ static void @@ -5570,6 +5608,7 @@ main(int argc, char **argv) g_test_add_func("/libnm/settings/6lowpan/1", test_6lowpan_1); g_test_add_func("/libnm/settings/dns", test_settings_dns); + g_test_add_func("/libnm/settings/dns_search_domain", test_settings_dns_search_domains); g_test_add_func("/libnm/settings/sriov/vf", test_sriov_vf); g_test_add_func("/libnm/settings/sriov/vf-dup", test_sriov_vf_dup); From b0b72dd2f1f0f9324b44486e3528913de05aa030 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Mon, 5 May 2025 13:56:04 +0200 Subject: [PATCH 2/2] dns: don't break existing configs with wrong separators in dns-search The previous commit will raise an error if wrong list separators are being used in an nmconnection file for dns-search to avoid that they are all considered a single string. However, existing users might have wrong values of dns-search that currently are not preventing the connection of being activated. To avoid that a NetworkManager update breaks existing configs, potentially even cutting connectivity with remote machines, accept wrong separators in keyfiles but emitting a warning. Fixes: 919156552ede ('dns: ensure that no wrong separators are used for DNS search domains') --- src/libnm-core-impl/nm-keyfile.c | 42 ++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/libnm-core-impl/nm-keyfile.c b/src/libnm-core-impl/nm-keyfile.c index bc5272bcd9..70758f0f85 100644 --- a/src/libnm-core-impl/nm-keyfile.c +++ b/src/libnm-core-impl/nm-keyfile.c @@ -1186,6 +1186,46 @@ ip_dns_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) g_object_set(setting, key, list, NULL); } +static void +ip_dns_search_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) +{ + gs_strfreev char **list = NULL; + gsize length; + + nm_assert(NM_IS_SETTING_IP4_CONFIG(setting) || NM_IS_SETTING_IP6_CONFIG(setting)); + + list = nm_keyfile_plugin_kf_get_string_list(info->keyfile, + nm_setting_get_name(setting), + key, + &length, + NULL); + nm_assert(length == NM_PTRARRAY_LEN(list)); + if (length == 0) + return; + + if (length == 1 && strpbrk(list[0], ", ")) { + /* By mistake, we accepted invalid characters like ',' in DNS search domains. + * Now we do some validation that would cause the connection to be rejected by + * the daemon. Let's continue accepting ',' and ' ' as separators but emit a + * warning */ + char **list2; + + read_handle_warn(info, + key, + key, + NM_KEYFILE_WARN_SEVERITY_WARN, + _("normalizing invalid separator ',' or ' ' in DNS search value '%s', " + "only ';' will be valid separators in keyfiles in the future"), + list[0]); + + list2 = g_strsplit_set(list[0], ", ", -1); + g_strfreev(list); + list = list2; + } + + g_object_set(setting, key, list, NULL); +} + static void ip6_addr_gen_mode_parser(KeyfileReaderInfo *info, NMSetting *setting, const char *key) { @@ -3084,6 +3124,7 @@ static const ParseInfoSetting *const parse_infos[_NM_META_SETTING_TYPE_NUM] = { .parser = ip_dns_parser, .writer = dns_writer, ), PARSE_INFO_PROPERTY(NM_SETTING_IP_CONFIG_DNS_OPTIONS, .always_write = TRUE, ), + PARSE_INFO_PROPERTY(NM_SETTING_IP_CONFIG_DNS_SEARCH, .parser = ip_dns_search_parser, ), PARSE_INFO_PROPERTY(NM_SETTING_IP_CONFIG_GATEWAY, .parser = gateway_parser, ), PARSE_INFO_PROPERTY(NM_SETTING_IP_CONFIG_ROUTES, .parser_no_check_key = TRUE, @@ -3112,6 +3153,7 @@ static const ParseInfoSetting *const parse_infos[_NM_META_SETTING_TYPE_NUM] = { .parser = ip_dns_parser, .writer = dns_writer, ), PARSE_INFO_PROPERTY(NM_SETTING_IP_CONFIG_DNS_OPTIONS, .always_write = TRUE, ), + PARSE_INFO_PROPERTY(NM_SETTING_IP_CONFIG_DNS_SEARCH, .parser = ip_dns_search_parser, ), PARSE_INFO_PROPERTY(NM_SETTING_IP_CONFIG_GATEWAY, .parser = gateway_parser, ), PARSE_INFO_PROPERTY(NM_SETTING_IP_CONFIG_ROUTES, .parser_no_check_key = TRUE,