From ba21cfee23af7136b357a60c9815e2ceedca6502 Mon Sep 17 00:00:00 2001 From: Danilo Egea Gondolfo Date: Wed, 22 May 2024 15:44:16 +0100 Subject: [PATCH 1/4] libnetplan: use more restrictive file permissions A new util.c:_netplan_g_string_free_to_file_with_permissions() was added and accepts the owner, group and file mode as arguments. When these properties can't be set, when the generator is called by a non-root user for example, it will not hard-fail. This function is called by unit tests where we can't set the owner to a privileged account for example. When generating backend files, use more restrictive permissions: networkd related files will be owned by root:systemd-network and have mode 0640. service unit files will be owned by root:root and have mode 0640. udevd files will be owned by root:root with mode 0640. wpa_supplicant and Network Manager files will continue with the existing permissions. Autopkgtests will check if the permissions are set as expected when calling the generator. This fix addresses CVE-2022-4968 --- src/networkd.c | 40 ++++------------- src/networkd.h | 2 + src/nm.c | 4 +- src/openvswitch.c | 2 +- src/sriov.c | 4 +- src/util-internal.h | 3 ++ src/util.c | 46 +++++++++++++++++++ tests/generator/test_auth.py | 2 +- tests/generator/test_wifis.py | 2 +- tests/integration/base.py | 85 +++++++++++++++++++++++++++++++++++ 10 files changed, 152 insertions(+), 38 deletions(-) diff --git a/src/networkd.c b/src/networkd.c index babde5d07..3ea753132 100644 --- a/src/networkd.c +++ b/src/networkd.c @@ -302,7 +302,6 @@ STATIC void write_link_file(const NetplanNetDefinition* def, const char* rootdir, const char* path) { GString* s = NULL; - mode_t orig_umask; /* Don't write .link files for virtual devices; they use .netdev instead. * Don't write .link files for MODEM devices, as they aren't supported by networkd. @@ -374,9 +373,7 @@ write_link_file(const NetplanNetDefinition* def, const char* rootdir, const char g_string_append_printf(s, "LargeReceiveOffload=%s\n", (def->large_receive_offload ? "true" : "false")); - orig_umask = umask(022); - _netplan_g_string_free_to_file(s, rootdir, path, ".link"); - umask(orig_umask); + _netplan_g_string_free_to_file_with_permissions(s, rootdir, path, ".link", "root", "root", 0640); } STATIC gboolean @@ -394,7 +391,7 @@ write_regdom(const NetplanNetDefinition* def, const char* rootdir, GError** erro g_string_append(s, "\n[Service]\nType=oneshot\n"); g_string_append_printf(s, "ExecStart="SBINDIR"/iw reg set %s\n", def->regulatory_domain); - _netplan_g_string_free_to_file(s, rootdir, path, NULL); + _netplan_g_string_free_to_file_with_permissions(s, rootdir, path, NULL, "root", "root", 0640); _netplan_safe_mkdir_p_dir(link); if (symlink(path, link) < 0 && errno != EEXIST) { // LCOV_EXCL_START @@ -586,7 +583,6 @@ STATIC void write_netdev_file(const NetplanNetDefinition* def, const char* rootdir, const char* path) { GString* s = NULL; - mode_t orig_umask; g_assert(def->type >= NETPLAN_DEF_TYPE_VIRTUAL); @@ -685,11 +681,7 @@ write_netdev_file(const NetplanNetDefinition* def, const char* rootdir, const ch default: g_assert_not_reached(); // LCOV_EXCL_LINE } - /* these do not contain secrets and need to be readable by - * systemd-networkd - LP: #1736965 */ - orig_umask = umask(022); - _netplan_g_string_free_to_file(s, rootdir, path, ".netdev"); - umask(orig_umask); + _netplan_g_string_free_to_file_with_permissions(s, rootdir, path, ".netdev", "root", NETWORKD_GROUP, 0640); } STATIC void @@ -833,7 +825,6 @@ _netplan_netdef_write_network_file( g_autoptr(GString) network = NULL; g_autoptr(GString) link = NULL; GString* s = NULL; - mode_t orig_umask; SET_OPT_OUT_PTR(has_been_written, FALSE); @@ -1099,11 +1090,7 @@ _netplan_netdef_write_network_file( if (network->len > 0) g_string_append_printf(s, "\n[Network]\n%s", network->str); - /* these do not contain secrets and need to be readable by - * systemd-networkd - LP: #1736965 */ - orig_umask = umask(022); - _netplan_g_string_free_to_file(s, rootdir, path, ".network"); - umask(orig_umask); + _netplan_g_string_free_to_file_with_permissions(s, rootdir, path, ".network", "root", NETWORKD_GROUP, 0640); } SET_OPT_OUT_PTR(has_been_written, TRUE); @@ -1115,7 +1102,6 @@ write_rules_file(const NetplanNetDefinition* def, const char* rootdir) { GString* s = NULL; g_autofree char* path = g_strjoin(NULL, "run/udev/rules.d/99-netplan-", def->id, ".rules", NULL); - mode_t orig_umask; /* do we need to write a .rules file? * It's only required for reliably setting the name of a physical device @@ -1149,9 +1135,7 @@ write_rules_file(const NetplanNetDefinition* def, const char* rootdir) g_string_append_printf(s, "NAME=\"%s\"\n", def->set_name); - orig_umask = umask(022); - _netplan_g_string_free_to_file(s, rootdir, path, NULL); - umask(orig_umask); + _netplan_g_string_free_to_file_with_permissions(s, rootdir, path, NULL, "root", "root", 0640); } STATIC gboolean @@ -1300,7 +1284,6 @@ STATIC void write_wpa_unit(const NetplanNetDefinition* def, const char* rootdir) { g_autofree gchar *stdouth = NULL; - mode_t orig_umask; stdouth = systemd_escape(def->id); @@ -1319,9 +1302,7 @@ write_wpa_unit(const NetplanNetDefinition* def, const char* rootdir) } else { g_string_append(s, " -Dnl80211,wext\n"); } - orig_umask = umask(022); - _netplan_g_string_free_to_file(s, rootdir, path, NULL); - umask(orig_umask); + _netplan_g_string_free_to_file_with_permissions(s, rootdir, path, NULL, "root", "root", 0640); } STATIC gboolean @@ -1330,7 +1311,6 @@ write_wpa_conf(const NetplanNetDefinition* def, const char* rootdir, GError** er GHashTableIter iter; GString* s = g_string_new("ctrl_interface=/run/wpa_supplicant\n\n"); g_autofree char* path = g_strjoin(NULL, "run/netplan/wpa-", def->id, ".conf", NULL); - mode_t orig_umask; g_debug("%s: Creating wpa_supplicant configuration file %s", def->id, path); if (def->type == NETPLAN_DEF_TYPE_WIFI) { @@ -1423,9 +1403,7 @@ write_wpa_conf(const NetplanNetDefinition* def, const char* rootdir, GError** er } /* use tight permissions as this contains secrets */ - orig_umask = umask(077); - _netplan_g_string_free_to_file(s, rootdir, path, NULL); - umask(orig_umask); + _netplan_g_string_free_to_file_with_permissions(s, rootdir, path, NULL, "root", "root", 0600); return TRUE; } @@ -1569,7 +1547,7 @@ _netplan_networkd_write_wait_online(const NetplanState* np_state, const char* ro GString* content = g_string_new("[Unit]\n" "ConditionPathIsSymbolicLink=/run/systemd/generator/network-online.target.wants/systemd-networkd-wait-online.service\n"); if (g_hash_table_size(non_optional_interfaces) == 0) { - _netplan_g_string_free_to_file(content, rootdir, override, NULL); + _netplan_g_string_free_to_file_with_permissions(content, rootdir, override, NULL, "root", "root", 0640); g_hash_table_destroy(non_optional_interfaces); return FALSE; } @@ -1593,7 +1571,7 @@ _netplan_networkd_write_wait_online(const NetplanState* np_state, const char* ro } g_string_append(content, "\n"); - _netplan_g_string_free_to_file(content, rootdir, override, NULL); + _netplan_g_string_free_to_file_with_permissions(content, rootdir, override, NULL, "root", "root", 0640); g_hash_table_destroy(non_optional_interfaces); return TRUE; } diff --git a/src/networkd.h b/src/networkd.h index 8332b7b2a..9f805859e 100644 --- a/src/networkd.h +++ b/src/networkd.h @@ -20,6 +20,8 @@ #include "netplan.h" #include +#define NETWORKD_GROUP "systemd-network" + NETPLAN_INTERNAL gboolean _netplan_netdef_write_networkd( const NetplanState* np_state, diff --git a/src/nm.c b/src/nm.c index c5135aa39..81be997c9 100644 --- a/src/nm.c +++ b/src/nm.c @@ -1152,13 +1152,13 @@ netplan_state_finish_nm_write( /* write generated NetworkManager drop-in config */ if (nm_conf->len > 0) - _netplan_g_string_free_to_file(nm_conf, rootdir, "run/NetworkManager/conf.d/netplan.conf", NULL); + _netplan_g_string_free_to_file_with_permissions(nm_conf, rootdir, "run/NetworkManager/conf.d/netplan.conf", NULL, "root", "root", 0640); else g_string_free(nm_conf, TRUE); /* write generated udev rules */ if (udev_rules->len > 0) - _netplan_g_string_free_to_file(udev_rules, rootdir, "run/udev/rules.d/90-netplan.rules", NULL); + _netplan_g_string_free_to_file_with_permissions(udev_rules, rootdir, "run/udev/rules.d/90-netplan.rules", NULL, "root", "root", 0640); else g_string_free(udev_rules, TRUE); diff --git a/src/openvswitch.c b/src/openvswitch.c index 6eb06883a..2ab77e7f0 100644 --- a/src/openvswitch.c +++ b/src/openvswitch.c @@ -66,7 +66,7 @@ write_ovs_systemd_unit(const char* id, const GString* cmds, const char* rootdir, g_string_append(s, "StartLimitBurst=0\n"); g_string_append(s, cmds->str); - _netplan_g_string_free_to_file(s, rootdir, path, NULL); + _netplan_g_string_free_to_file_with_permissions(s, rootdir, path, NULL, "root", "root", 0640); _netplan_safe_mkdir_p_dir(link); if (symlink(path, link) < 0 && errno != EEXIST) { diff --git a/src/sriov.c b/src/sriov.c index 53c0b8682..63ef7363f 100644 --- a/src/sriov.c +++ b/src/sriov.c @@ -54,7 +54,7 @@ write_sriov_rebind_systemd_unit(GHashTable* pfs, const char* rootdir, GError** e g_string_truncate(interfaces, interfaces->len-1); /* cut trailing whitespace */ g_string_append_printf(s, "ExecStart=" SBINDIR "/netplan rebind --debug %s\n", interfaces->str); - _netplan_g_string_free_to_file(s, rootdir, path, NULL); + _netplan_g_string_free_to_file_with_permissions(s, rootdir, path, NULL, "root", "root", 0640); g_string_free(interfaces, TRUE); _netplan_safe_mkdir_p_dir(link); @@ -90,7 +90,7 @@ write_sriov_apply_systemd_unit(GHashTable* pfs, const char* rootdir, GError** er g_string_append(s, "\n[Service]\nType=oneshot\n"); g_string_append_printf(s, "ExecStart=" SBINDIR "/netplan apply --sriov-only\n"); - _netplan_g_string_free_to_file(s, rootdir, path, NULL); + _netplan_g_string_free_to_file_with_permissions(s, rootdir, path, NULL, "root", "root", 0640); _netplan_safe_mkdir_p_dir(link); if (symlink(path, link) < 0 && errno != EEXIST) { diff --git a/src/util-internal.h b/src/util-internal.h index 0fa0e00ab..8278e85b1 100644 --- a/src/util-internal.h +++ b/src/util-internal.h @@ -40,6 +40,9 @@ _netplan_safe_mkdir_p_dir(const char* file_path); NETPLAN_INTERNAL void _netplan_g_string_free_to_file(GString* s, const char* rootdir, const char* path, const char* suffix); +void +_netplan_g_string_free_to_file_with_permissions(GString* s, const char* rootdir, const char* path, const char* suffix, const char* owner, const char* group, mode_t mode); + NETPLAN_INTERNAL void _netplan_unlink_glob(const char* rootdir, const char* _glob); diff --git a/src/util.c b/src/util.c index cabc29331..af3e16688 100644 --- a/src/util.c +++ b/src/util.c @@ -23,6 +23,9 @@ #include #include #include +#include +#include +#include #include #include @@ -87,6 +90,49 @@ void _netplan_g_string_free_to_file(GString* s, const char* rootdir, const char* } } +void _netplan_g_string_free_to_file_with_permissions(GString* s, const char* rootdir, const char* path, const char* suffix, const char* owner, const char* group, mode_t mode) +{ + g_autofree char* full_path = NULL; + g_autofree char* path_suffix = NULL; + g_autofree char* contents = g_string_free(s, FALSE); + GError* error = NULL; + struct passwd* pw = NULL; + struct group* gr = NULL; + int ret = 0; + + path_suffix = g_strjoin(NULL, path, suffix, NULL); + full_path = g_build_path(G_DIR_SEPARATOR_S, rootdir ?: G_DIR_SEPARATOR_S, path_suffix, NULL); + _netplan_safe_mkdir_p_dir(full_path); + if (!g_file_set_contents_full(full_path, contents, -1, G_FILE_SET_CONTENTS_CONSISTENT | G_FILE_SET_CONTENTS_ONLY_EXISTING, mode, &error)) { + /* the mkdir() just succeeded, there is no sensible + * method to test this without root privileges, bind mounts, and + * simulating ENOSPC */ + // LCOV_EXCL_START + g_fprintf(stderr, "ERROR: cannot create file %s: %s\n", path, error->message); + exit(1); + // LCOV_EXCL_STOP + } + + /* Here we take the owner and group names and look up for their IDs in the passwd and group files. + * It's OK to fail to set the owners and mode as this code will be called from unit tests. + * The autopkgtests will check if the owner/group and mode are correctly set. + */ + pw = getpwnam(owner); + if (!pw) { + g_debug("Failed to determine the UID of user %s: %s", owner, strerror(errno)); // LCOV_EXCL_LINE + } + gr = getgrnam(group); + if (!gr) { + g_debug("Failed to determine the GID of group %s: %s", group, strerror(errno)); // LCOV_EXCL_LINE + } + if (pw && gr) { + ret = chown(full_path, pw->pw_uid, gr->gr_gid); + if (ret != 0) { + g_debug("Failed to set owner and group for file %s: %s", full_path, strerror(errno)); + } + } +} + /** * Remove all files matching given glob. */ diff --git a/tests/generator/test_auth.py b/tests/generator/test_auth.py index de23adbcc..d3d886c87 100644 --- a/tests/generator/test_auth.py +++ b/tests/generator/test_auth.py @@ -226,7 +226,7 @@ def test_auth_wired(self): with open(os.path.join(self.workdir.name, 'run/systemd/system/netplan-wpa-eth0.service')) as f: self.assertEqual(f.read(), SD_WPA % {'iface': 'eth0', 'drivers': 'wired'}) - self.assertEqual(stat.S_IMODE(os.fstat(f.fileno()).st_mode), 0o644) + self.assertEqual(stat.S_IMODE(os.fstat(f.fileno()).st_mode), 0o640) self.assertTrue(os.path.islink(os.path.join( self.workdir.name, 'run/systemd/system/systemd-networkd.service.wants/netplan-wpa-eth0.service'))) diff --git a/tests/generator/test_wifis.py b/tests/generator/test_wifis.py index 4b151d892..0962a8817 100644 --- a/tests/generator/test_wifis.py +++ b/tests/generator/test_wifis.py @@ -140,7 +140,7 @@ def test_wifi(self): self.workdir.name, 'run/systemd/system/netplan-wpa-wl0.service'))) with open(os.path.join(self.workdir.name, 'run/systemd/system/netplan-wpa-wl0.service')) as f: self.assertEqual(f.read(), SD_WPA % {'iface': 'wl0', 'drivers': 'nl80211,wext'}) - self.assertEqual(stat.S_IMODE(os.fstat(f.fileno()).st_mode), 0o644) + self.assertEqual(stat.S_IMODE(os.fstat(f.fileno()).st_mode), 0o640) self.assertTrue(os.path.islink(os.path.join( self.workdir.name, 'run/systemd/system/systemd-networkd.service.wants/netplan-wpa-wl0.service'))) diff --git a/tests/integration/base.py b/tests/integration/base.py index 83f16f369..3c64ad663 100644 --- a/tests/integration/base.py +++ b/tests/integration/base.py @@ -32,6 +32,8 @@ import gi import glob import json +import pwd +import grp # make sure we point to libnetplan properly. os.environ.update({'LD_LIBRARY_PATH': '.:{}'.format(os.environ.get('LD_LIBRARY_PATH'))}) @@ -375,6 +377,89 @@ def generate_and_settle(self, wait_interfaces=None, state_dir=None): if state: self.wait_output(['ip', 'addr', 'show', iface], state, 30) + # Assert file permissions + self.assert_file_permissions() + + def assert_file_permissions(self): + """ Check if the generated files have the expected permissions """ + + nd_expected_mode = 0o100640 + nd_expected_owner = 'root' + nd_expected_group = 'systemd-network' + + sd_expected_mode = 0o100640 + sd_expected_owner = 'root' + sd_expected_group = 'root' + + udev_expected_mode = 0o100640 + udev_expected_owner = 'root' + udev_expected_group = 'root' + + nm_expected_mode = 0o100600 + nm_expected_owner = 'root' + nm_expected_group = 'root' + + wpa_expected_mode = 0o100600 + wpa_expected_owner = 'root' + wpa_expected_group = 'root' + + # Check systemd-networkd files + base_path = '/run/systemd/network' + files = glob.glob(f'{base_path}/*.network') + glob.glob(f'{base_path}/*.netdev') + for file in files: + res = os.stat(file) + user = pwd.getpwuid(res.st_uid) + group = grp.getgrgid(res.st_gid) + self.assertEqual(res.st_mode, nd_expected_mode, f'file {file}') + self.assertEqual(user.pw_name, nd_expected_owner, f'file {file}') + self.assertEqual(group.gr_name, nd_expected_group, f'file {file}') + + # Check Network Manager files + base_path = '/run/NetworkManager/system-connections' + files = glob.glob(f'{base_path}/*.nmconnection') + for file in files: + res = os.stat(file) + user = pwd.getpwuid(res.st_uid) + group = grp.getgrgid(res.st_gid) + self.assertEqual(res.st_mode, nm_expected_mode, f'file {file}') + self.assertEqual(user.pw_name, nm_expected_owner, f'file {file}') + self.assertEqual(group.gr_name, nm_expected_group, f'file {file}') + + # Check wpa_supplicant configuration files + base_path = '/run/netplan' + files = glob.glob(f'{base_path}/wpa-*.conf') + for file in files: + res = os.stat(file) + user = pwd.getpwuid(res.st_uid) + group = grp.getgrgid(res.st_gid) + self.assertEqual(res.st_mode, wpa_expected_mode, f'file {file}') + self.assertEqual(user.pw_name, wpa_expected_owner, f'file {file}') + self.assertEqual(group.gr_name, wpa_expected_group, f'file {file}') + + # Check systemd service unit files + base_path = '/run/systemd/system/' + files = glob.glob(f'{base_path}/netplan-*.service') + files += glob.glob(f'{base_path}/systemd-networkd-wait-online.service.d/*.conf') + for file in files: + res = os.stat(file) + user = pwd.getpwuid(res.st_uid) + group = grp.getgrgid(res.st_gid) + self.assertEqual(res.st_mode, sd_expected_mode, f'file {file}') + self.assertEqual(user.pw_name, sd_expected_owner, f'file {file}') + self.assertEqual(group.gr_name, sd_expected_group, f'file {file}') + + # Check systemd-udevd files + udev_path = '/run/udev/rules.d' + link_path = '/run/systemd/network' + files = glob.glob(f'{udev_path}/*-netplan*.rules') + glob.glob(f'{link_path}/*.link') + for file in files: + res = os.stat(file) + user = pwd.getpwuid(res.st_uid) + group = grp.getgrgid(res.st_gid) + self.assertEqual(res.st_mode, udev_expected_mode, f'file {file}') + self.assertEqual(user.pw_name, udev_expected_owner, f'file {file}') + self.assertEqual(group.gr_name, udev_expected_group, f'file {file}') + def state(self, iface, state): '''Tell generate_and_settle() to wait for a specific state''' return iface + '/' + state From ee1703fdac5cdabb03db1dcf4c0e99da406342f2 Mon Sep 17 00:00:00 2001 From: Danilo Egea Gondolfo Date: Wed, 29 May 2024 14:50:55 +0100 Subject: [PATCH 2/4] libnetplan: escape control characters Control characters are escaped in the parser using glib's g_strescape. Quotes and backslashes were added to the list of exception. In places where double quotes are not escaped, such as netdef IDs as it is allowed as interface names, they are escaped as needed when generating back end configuration. To support escaping in wpa_supplicant configuration, the syntax for setting the SSID was changed to 'ssid=P"string here"'. With that, escaping is support in a printf-style. --- src/networkd.c | 32 ++++++++---- src/nm.c | 21 +++++--- src/parse.c | 94 ++++++++++++++++++++++------------ src/util-internal.h | 3 ++ src/util.c | 11 ++++ tests/generator/test_auth.py | 20 ++++---- tests/generator/test_common.py | 42 +++++++++++++-- tests/generator/test_wifis.py | 78 +++++++++++++++++++++------- 8 files changed, 217 insertions(+), 84 deletions(-) diff --git a/src/networkd.c b/src/networkd.c index 3ea753132..72606a587 100644 --- a/src/networkd.c +++ b/src/networkd.c @@ -1125,7 +1125,8 @@ write_rules_file(const NetplanNetDefinition* def, const char* rootdir) g_string_append(s, "SUBSYSTEM==\"net\", ACTION==\"add\", "); if (def->match.driver) { - g_string_append_printf(s,"DRIVERS==\"%s\", ", def->match.driver); + g_autofree char* driver = _netplan_scrub_string(def->match.driver); + g_string_append_printf(s,"DRIVERS==\"%s\", ", driver); } else { g_string_append(s, "DRIVERS==\"?*\", "); } @@ -1133,7 +1134,8 @@ write_rules_file(const NetplanNetDefinition* def, const char* rootdir) if (def->match.mac) g_string_append_printf(s, "ATTR{address}==\"%s\", ", def->match.mac); - g_string_append_printf(s, "NAME=\"%s\"\n", def->set_name); + g_autofree char* set_name = _netplan_scrub_string(def->set_name); + g_string_append_printf(s, "NAME=\"%s\"\n", set_name); _netplan_g_string_free_to_file_with_permissions(s, rootdir, path, NULL, "root", "root", 0640); } @@ -1221,10 +1223,12 @@ append_wpa_auth_conf(GString* s, const NetplanAuthenticationSettings* auth, cons } if (auth->identity) { - g_string_append_printf(s, " identity=\"%s\"\n", auth->identity); + g_autofree char* identity = _netplan_scrub_string(auth->identity); + g_string_append_printf(s, " identity=\"%s\"\n", identity); } if (auth->anonymous_identity) { - g_string_append_printf(s, " anonymous_identity=\"%s\"\n", auth->anonymous_identity); + g_autofree char* anonymous_identity = _netplan_scrub_string(auth->anonymous_identity); + g_string_append_printf(s, " anonymous_identity=\"%s\"\n", anonymous_identity); } char* psk = NULL; @@ -1262,19 +1266,23 @@ append_wpa_auth_conf(GString* s, const NetplanAuthenticationSettings* auth, cons } } if (auth->ca_certificate) { - g_string_append_printf(s, " ca_cert=\"%s\"\n", auth->ca_certificate); + g_autofree char* ca_certificate = _netplan_scrub_string(auth->ca_certificate); + g_string_append_printf(s, " ca_cert=\"%s\"\n", ca_certificate); } if (auth->client_certificate) { - g_string_append_printf(s, " client_cert=\"%s\"\n", auth->client_certificate); + g_autofree char* client_certificate = _netplan_scrub_string(auth->client_certificate); + g_string_append_printf(s, " client_cert=\"%s\"\n", client_certificate); } if (auth->client_key) { - g_string_append_printf(s, " private_key=\"%s\"\n", auth->client_key); + g_autofree char* client_key = _netplan_scrub_string(auth->client_key); + g_string_append_printf(s, " private_key=\"%s\"\n", client_key); } if (auth->client_key_password) { g_string_append_printf(s, " private_key_passwd=\"%s\"\n", auth->client_key_password); } if (auth->phase2_auth) { - g_string_append_printf(s, " phase2=\"auth=%s\"\n", auth->phase2_auth); + g_autofree char* phase2_auth = _netplan_scrub_string(auth->phase2_auth); + g_string_append_printf(s, " phase2=\"auth=%s\"\n", phase2_auth); } return TRUE; } @@ -1327,14 +1335,16 @@ write_wpa_conf(const NetplanNetDefinition* def, const char* rootdir, GError** er } /* available as of wpa_supplicant version 0.6.7 */ if (def->regulatory_domain) { - g_string_append_printf(s, "country=%s\n", def->regulatory_domain); + g_autofree char* regdom = _netplan_scrub_string(def->regulatory_domain); + g_string_append_printf(s, "country=%s\n", regdom); } NetplanWifiAccessPoint* ap; g_hash_table_iter_init(&iter, def->access_points); while (g_hash_table_iter_next(&iter, NULL, (gpointer) &ap)) { gchar* freq_config_str = ap->mode == NETPLAN_WIFI_MODE_ADHOC ? "frequency" : "freq_list"; + g_autofree char* ssid = _netplan_scrub_string(ap->ssid); - g_string_append_printf(s, "network={\n ssid=\"%s\"\n", ap->ssid); + g_string_append_printf(s, "network={\n ssid=P\"%s\"\n", ssid); if (ap->bssid) { g_string_append_printf(s, " bssid=%s\n", ap->bssid); } @@ -1381,7 +1391,7 @@ write_wpa_conf(const NetplanNetDefinition* def, const char* rootdir, GError** er /* wifi auth trumps netdef auth */ if (ap->has_auth) { - if (!append_wpa_auth_conf(s, &ap->auth, ap->ssid, error)) { + if (!append_wpa_auth_conf(s, &ap->auth, ssid, error)) { g_string_free(s, TRUE); return FALSE; } diff --git a/src/nm.c b/src/nm.c index 81be997c9..95f4a867d 100644 --- a/src/nm.c +++ b/src/nm.c @@ -1084,28 +1084,30 @@ netplan_state_finish_nm_write( GString *tmp = NULL; guint unmanaged = nd->backend == NETPLAN_BACKEND_NM ? 0 : 1; + g_autofree char* netdef_id = _netplan_scrub_string(nd->id); /* Special case: manage or ignore any device of given type on empty "match: {}" stanza */ if (nd->has_match && !nd->match.driver && !nd->match.mac && !nd->match.original_name) { nm_type = type_str(nd); g_assert(nm_type); g_string_append_printf(nm_conf, "[device-netplan.%s.%s]\nmatch-device=type:%s\n" "managed=%d\n\n", netplan_def_type_name(nd->type), - nd->id, nm_type, !unmanaged); + netdef_id, nm_type, !unmanaged); } /* Normal case: manage or ignore devices by specific udev rules */ else { const gchar *prefix = "SUBSYSTEM==\"net\", ACTION==\"add|change|move\","; const gchar *suffix = nd->backend == NETPLAN_BACKEND_NM ? " ENV{NM_UNMANAGED}=\"0\"\n" : " ENV{NM_UNMANAGED}=\"1\"\n"; g_string_append_printf(udev_rules, "# netplan: network.%s.%s (on NetworkManager %s)\n", - netplan_def_type_name(nd->type), nd->id, + netplan_def_type_name(nd->type), netdef_id, unmanaged ? "deny-list" : "allow-list"); /* Match by explicit interface name, if possible */ if (nd->set_name) { // simple case: explicit new interface name - g_string_append_printf(udev_rules, "%s ENV{ID_NET_NAME}==\"%s\",%s", prefix, nd->set_name, suffix); + g_autofree char* set_name = _netplan_scrub_string(nd->set_name); + g_string_append_printf(udev_rules, "%s ENV{ID_NET_NAME}==\"%s\",%s", prefix, set_name, suffix); } else if (!nd->has_match) { // simple case: explicit netplan ID is interface name - g_string_append_printf(udev_rules, "%s ENV{ID_NET_NAME}==\"%s\",%s", prefix, nd->id, suffix); + g_string_append_printf(udev_rules, "%s ENV{ID_NET_NAME}==\"%s\",%s", prefix, netdef_id, suffix); } /* Also, match by explicit (new) MAC, if available */ if (nd->set_mac && _is_valid_macaddress(nd->set_mac)) { @@ -1121,9 +1123,10 @@ netplan_state_finish_nm_write( // match on original name glob // TODO: maybe support matching on multiple name globs in the future (like drivers) g_string_append(udev_rules, prefix); - if (nd->match.original_name) - g_string_append_printf(udev_rules, " ENV{ID_NET_NAME}==\"%s\",", nd->match.original_name); - + if (nd->match.original_name) { + g_autofree char* original_name = _netplan_scrub_string(nd->match.original_name); + g_string_append_printf(udev_rules, " ENV{ID_NET_NAME}==\"%s\",", original_name); + } // match on (explicit) MAC address. Yes this would be unique on its own, but we // keep it within the "full match" to make the logic more comprehensible. if (nd->match.mac) { @@ -1141,7 +1144,9 @@ netplan_state_finish_nm_write( g_strfreev(split); } else drivers = g_strdup(nd->match.driver); - g_string_append_printf(udev_rules, " ENV{ID_NET_DRIVER}==\"%s\",", drivers); + + g_autofree char* escaped_drivers = _netplan_scrub_string(drivers); + g_string_append_printf(udev_rules, " ENV{ID_NET_DRIVER}==\"%s\",", escaped_drivers); g_free(drivers); } g_string_append(udev_rules, suffix); diff --git a/src/parse.c b/src/parse.c index 589ce0ec3..1b9aa792a 100644 --- a/src/parse.c +++ b/src/parse.c @@ -53,6 +53,15 @@ dst = g_strdup(src); \ } } +/* + * We use g_strescape to escape control characters from the input. + * Besides control characters, g_strescape will also escape double quotes and backslashes. + * Quotes are escaped at configuration generation time as needed, as they might be part of passwords for example. + * Escaping backslashes in the parser affects "netplan set" as it will always escape \'s from + * the input and update YAMLs with all the \'s escaped again. +*/ +static char* STRESCAPE_EXCEPTIONS = "\"\\"; + STATIC gboolean insert_kv_into_hash(void *key, void *value, void *hash); @@ -359,7 +368,7 @@ handle_generic_str(NetplanParser* npp, yaml_node_t* node, void* entryptr, const guint offset = GPOINTER_TO_UINT(data); char** dest = (char**) ((void*) entryptr + offset); g_free(*dest); - *dest = g_strdup(scalar(node)); + *dest = g_strescape(scalar(node), STRESCAPE_EXCEPTIONS); mark_data_as_dirty(npp, dest); return TRUE; } @@ -479,22 +488,25 @@ handle_generic_map(NetplanParser *npp, yaml_node_t* node, const char* key_prefix assert_type(npp, key, YAML_SCALAR_NODE); assert_type(npp, value, YAML_SCALAR_NODE); + g_autofree char* escaped_key = g_strescape(scalar(key), STRESCAPE_EXCEPTIONS); + g_autofree char* escaped_value = g_strescape(scalar(value), STRESCAPE_EXCEPTIONS); + if (key_prefix && npp->null_fields) { g_autofree char* full_key = NULL; - full_key = g_strdup_printf("%s\t%s", key_prefix, key->data.scalar.value); + full_key = g_strdup_printf("%s\t%s", key_prefix, escaped_key); if (g_hash_table_contains(npp->null_fields, full_key)) continue; } char* stored_value = NULL; - if (g_hash_table_lookup_extended(*map, scalar(key), NULL, (void**)&stored_value)) { + if (g_hash_table_lookup_extended(*map, escaped_key, NULL, (void**)&stored_value)) { /* We can safely skip this if it is the exact key/value match * (probably caused by multi-pass processing) */ - if (g_strcmp0(stored_value, scalar(value)) == 0) + if (g_strcmp0(stored_value, escaped_value) == 0) continue; - return yaml_error(npp, node, error, "duplicate map entry '%s'", scalar(key)); + return yaml_error(npp, node, error, "duplicate map entry '%s'", escaped_key); } else - g_hash_table_insert(*map, g_strdup(scalar(key)), g_strdup(scalar(value))); + g_hash_table_insert(*map, g_strdup(escaped_key), g_strdup(escaped_value)); } mark_data_as_dirty(npp, map); @@ -517,20 +529,26 @@ handle_generic_datalist(NetplanParser *npp, yaml_node_t* node, const char* key_p for (yaml_node_pair_t* entry = node->data.mapping.pairs.start; entry < node->data.mapping.pairs.top; entry++) { yaml_node_t* key, *value; g_autofree char* full_key = NULL; + g_autofree char* escaped_key = NULL; + g_autofree char* escaped_value = NULL; key = yaml_document_get_node(&npp->doc, entry->key); value = yaml_document_get_node(&npp->doc, entry->value); assert_type(npp, key, YAML_SCALAR_NODE); assert_type(npp, value, YAML_SCALAR_NODE); + + escaped_key = g_strescape(scalar(key), STRESCAPE_EXCEPTIONS); + escaped_value = g_strescape(scalar(value), STRESCAPE_EXCEPTIONS); + if (npp->null_fields && key_prefix) { - full_key = g_strdup_printf("%s\t%s", key_prefix, scalar(key)); + full_key = g_strdup_printf("%s\t%s", key_prefix, escaped_key); if (g_hash_table_contains(npp->null_fields, full_key)) continue; } - g_datalist_id_set_data_full(list, g_quark_from_string(scalar(key)), - g_strdup(scalar(value)), g_free); + g_datalist_id_set_data_full(list, g_quark_from_string(escaped_key), + g_strdup(escaped_value), g_free); } mark_data_as_dirty(npp, list); @@ -931,13 +949,14 @@ handle_match_driver(NetplanParser* npp, yaml_node_t* node, __unused const char* for (yaml_node_item_t *iter = node->data.sequence.items.start; iter < node->data.sequence.items.top; iter++) { elem = yaml_document_get_node(&npp->doc, *iter); assert_type(npp, elem, YAML_SCALAR_NODE); - if (g_strrstr(scalar(elem), " ")) + g_autofree char* escaped_elem = g_strescape(scalar(elem), STRESCAPE_EXCEPTIONS); + if (g_strrstr(escaped_elem, " ")) return yaml_error(npp, node, error, "A 'driver' glob cannot contain whitespace"); if (!sequence) - sequence = g_string_new(scalar(elem)); + sequence = g_string_new(escaped_elem); else - g_string_append_printf(sequence, "\t%s", scalar(elem)); /* tab separated */ + g_string_append_printf(sequence, "\t%s", escaped_elem); /* tab separated */ } if (!sequence) @@ -969,7 +988,7 @@ handle_auth_str(NetplanParser* npp, yaml_node_t* node, const void* data, __unuse guint offset = GPOINTER_TO_UINT(data); char** dest = (char**) ((void*) npp->current.auth + offset); g_free(*dest); - *dest = g_strdup(scalar(node)); + *dest = g_strescape(scalar(node), STRESCAPE_EXCEPTIONS); mark_data_as_dirty(npp, dest); return TRUE; } @@ -1129,7 +1148,7 @@ handle_access_point_password(NetplanParser* npp, yaml_node_t* node, __unused con access_point->auth.pmf_mode = NETPLAN_AUTH_PMF_MODE_OPTIONAL; g_free(access_point->auth.psk); - access_point->auth.psk = g_strdup(scalar(node)); + access_point->auth.psk = g_strescape(scalar(node), STRESCAPE_EXCEPTIONS); return TRUE; } @@ -1521,6 +1540,7 @@ handle_wifi_access_points(NetplanParser* npp, yaml_node_t* node, const char* key for (yaml_node_pair_t* entry = node->data.mapping.pairs.start; entry < node->data.mapping.pairs.top; entry++) { NetplanWifiAccessPoint *access_point = NULL; g_autofree char* full_key = NULL; + g_autofree char* escaped_key = NULL; yaml_node_t* key, *value; const gchar* ssid; @@ -1529,13 +1549,15 @@ handle_wifi_access_points(NetplanParser* npp, yaml_node_t* node, const char* key value = yaml_document_get_node(&npp->doc, entry->value); assert_type(npp, value, YAML_MAPPING_NODE); + escaped_key = g_strescape(scalar(key), STRESCAPE_EXCEPTIONS); + if (key_prefix && npp->null_fields) { - full_key = g_strdup_printf("%s\t%s", key_prefix, key->data.scalar.value); + full_key = g_strdup_printf("%s\t%s", key_prefix, escaped_key); if (g_hash_table_contains(npp->null_fields, full_key)) continue; } - ssid = scalar(key); + ssid = escaped_key; /* * Delete the access-point if it already exists in the netdef and let the new @@ -1693,15 +1715,16 @@ handle_nameservers_search(NetplanParser* npp, yaml_node_t* node, __unused const for (yaml_node_item_t *i = node->data.sequence.items.start; i < node->data.sequence.items.top; i++) { yaml_node_t *entry = yaml_document_get_node(&npp->doc, *i); assert_type(npp, entry, YAML_SCALAR_NODE); + g_autofree char* escaped_entry = g_strescape(scalar(entry), STRESCAPE_EXCEPTIONS); if (!npp->current.netdef->search_domains) npp->current.netdef->search_domains = g_array_new(FALSE, FALSE, sizeof(char*)); - if (!is_string_in_array(npp->current.netdef->search_domains, scalar(entry))) { - char* s = g_strdup(scalar(entry)); + if (!is_string_in_array(npp->current.netdef->search_domains, escaped_entry)) { + char* s = g_strdup(escaped_entry); g_array_append_val(npp->current.netdef->search_domains, s); } else { - g_debug("%s: Search domain '%s' has already been added", npp->current.netdef->id, scalar(entry)); + g_debug("%s: Search domain '%s' has already been added", npp->current.netdef->id, escaped_entry); } } mark_data_as_dirty(npp, &npp->current.netdef->search_domains); @@ -2812,19 +2835,19 @@ handle_ovs_bridge_controller_addresses(NetplanParser* npp, yaml_node_t* node, __ /* Format: [p]unix:file */ if (is_unix && vec[1] != NULL && vec[2] == NULL) { - char* s = g_strdup(scalar(entry)); + char* s = g_strescape(scalar(entry), STRESCAPE_EXCEPTIONS); g_array_append_val(npp->current.netdef->ovs_settings.controller.addresses, s); g_strfreev(vec); continue; /* Format tcp:host[:port] or ssl:host[:port] */ } else if (is_host && validate_ovs_target(TRUE, vec[1])) { - char* s = g_strdup(scalar(entry)); + char* s = g_strescape(scalar(entry), STRESCAPE_EXCEPTIONS); g_array_append_val(npp->current.netdef->ovs_settings.controller.addresses, s); g_strfreev(vec); continue; /* Format ptcp:[port][:host] or pssl:[port][:host] */ } else if (is_port && validate_ovs_target(FALSE, vec[1])) { - char* s = g_strdup(scalar(entry)); + char* s = g_strescape(scalar(entry), STRESCAPE_EXCEPTIONS); g_array_append_val(npp->current.netdef->ovs_settings.controller.addresses, s); g_strfreev(vec); continue; @@ -3146,6 +3169,8 @@ handle_network_ovs_settings_global_ports(NetplanParser* npp, yaml_node_t* node, NetplanNetDefinition *component2 = NULL; for (yaml_node_item_t *iter = node->data.sequence.items.start; iter < node->data.sequence.items.top; iter++) { + g_autofree char* escaped_port = NULL; + g_autofree char* escaped_peer = NULL; pair = yaml_document_get_node(&npp->doc, *iter); assert_type(npp, pair, YAML_SEQUENCE_NODE); @@ -3160,14 +3185,17 @@ handle_network_ovs_settings_global_ports(NetplanParser* npp, yaml_node_t* node, peer = yaml_document_get_node(&npp->doc, *(item+1)); assert_type(npp, peer, YAML_SCALAR_NODE); - if (!g_strcmp0(scalar(port), scalar(peer))) + escaped_port = g_strescape(scalar(port), STRESCAPE_EXCEPTIONS); + escaped_peer = g_strescape(scalar(peer), STRESCAPE_EXCEPTIONS); + + if (!g_strcmp0(escaped_port, escaped_peer)) return yaml_error(npp, peer, error, "Open vSwitch patch ports must be of different name"); /* Create port 1 netdef */ - component1 = npp->parsed_defs ? g_hash_table_lookup(npp->parsed_defs, scalar(port)) : NULL; + component1 = npp->parsed_defs ? g_hash_table_lookup(npp->parsed_defs, escaped_port) : NULL; if (!component1) { - component1 = netplan_netdef_new(npp, scalar(port), NETPLAN_DEF_TYPE_PORT, NETPLAN_BACKEND_OVS); - if (g_hash_table_remove(npp->missing_id, scalar(port))) + component1 = netplan_netdef_new(npp, escaped_port, NETPLAN_DEF_TYPE_PORT, NETPLAN_BACKEND_OVS); + if (g_hash_table_remove(npp->missing_id, escaped_port)) npp->missing_ids_found++; } @@ -3178,15 +3206,15 @@ handle_network_ovs_settings_global_ports(NetplanParser* npp, yaml_node_t* node, component1->filepath = g_strdup(npp->current.filepath); } - if (component1->peer && g_strcmp0(component1->peer, scalar(peer))) + if (component1->peer && g_strcmp0(component1->peer, escaped_peer)) return yaml_error(npp, port, error, "Open vSwitch port '%s' is already assigned to peer '%s'", component1->id, component1->peer); /* Create port 2 (peer) netdef */ - component2 = npp->parsed_defs ? g_hash_table_lookup(npp->parsed_defs, scalar(peer)) : NULL; + component2 = npp->parsed_defs ? g_hash_table_lookup(npp->parsed_defs, escaped_peer) : NULL; if (!component2) { - component2 = netplan_netdef_new(npp, scalar(peer), NETPLAN_DEF_TYPE_PORT, NETPLAN_BACKEND_OVS); - if (g_hash_table_remove(npp->missing_id, scalar(peer))) + component2 = netplan_netdef_new(npp, escaped_peer, NETPLAN_DEF_TYPE_PORT, NETPLAN_BACKEND_OVS); + if (g_hash_table_remove(npp->missing_id, escaped_peer)) npp->missing_ids_found++; } @@ -3197,16 +3225,16 @@ handle_network_ovs_settings_global_ports(NetplanParser* npp, yaml_node_t* node, component2->filepath = g_strdup(npp->current.filepath); } - if (component2->peer && g_strcmp0(component2->peer, scalar(port))) + if (component2->peer && g_strcmp0(component2->peer, escaped_port)) return yaml_error(npp, peer, error, "Open vSwitch port '%s' is already assigned to peer '%s'", component2->id, component2->peer); if (!component1->peer) { - component1->peer = g_strdup(scalar(peer)); + component1->peer = g_strdup(escaped_peer); component1->peer_link = component2; } if (!component2->peer) { - component2->peer = g_strdup(scalar(port)); + component2->peer = g_strdup(escaped_port); component2->peer_link = component1; } } diff --git a/src/util-internal.h b/src/util-internal.h index 8278e85b1..2c5595823 100644 --- a/src/util-internal.h +++ b/src/util-internal.h @@ -214,3 +214,6 @@ _netplan_netdef_get_gateway4(const NetplanNetDefinition* netdef, char* out_buffe */ NETPLAN_INTERNAL ssize_t _netplan_netdef_get_gateway6(const NetplanNetDefinition* netdef, char* out_buffer, size_t out_buffer_size); + +gchar* +_netplan_scrub_string(const char* content); diff --git a/src/util.c b/src/util.c index af3e16688..f41923436 100644 --- a/src/util.c +++ b/src/util.c @@ -1277,3 +1277,14 @@ _netplan_parser_find_bond_for_primary_member(const NetplanParser* npp, const cha return netdef; } + +gchar* +_netplan_scrub_string(const char* content) +{ + GString* s = g_string_new(content); + + // Escape double quotes + g_string_replace(s, "\"", "\\\"", 0); + + return g_string_free(s, FALSE); +} diff --git a/tests/generator/test_auth.py b/tests/generator/test_auth.py index d3d886c87..bf0108ab3 100644 --- a/tests/generator/test_auth.py +++ b/tests/generator/test_auth.py @@ -92,21 +92,21 @@ def test_auth_wifi_detailed(self): self.assertIn('ctrl_interface=/run/wpa_supplicant', new_config) self.assertIn(''' network={ - ssid="peer2peer" + ssid=P"peer2peer" mode=1 key_mgmt=NONE } ''', new_config) self.assertIn(''' network={ - ssid="Luke's Home" + ssid=P"Luke's Home" key_mgmt=WPA-PSK psk="4lsos3kr1t" } ''', new_config) self.assertIn(''' network={ - ssid="BobsHome" + ssid=P"BobsHome" key_mgmt=WPA-PSK WPA-PSK-SHA256 SAE ieee80211w=1 psk=e03ce667c87bc81ca968d9120ca37f89eb09aec3c55b80386e5d772efd6b926e @@ -114,14 +114,14 @@ def test_auth_wifi_detailed(self): ''', new_config) self.assertIn(''' network={ - ssid="BillsHome" + ssid=P"BillsHome" key_mgmt=WPA-PSK psk=db3b0acf5653aeaddd5fe034fb9f07175b2864f847b005aaa2f09182d9411b04 } ''', new_config) self.assertIn(''' network={ - ssid="workplace2" + ssid=P"workplace2" key_mgmt=WPA-EAP eap=PEAP identity="joe@internal.example.com" @@ -131,7 +131,7 @@ def test_auth_wifi_detailed(self): ''', new_config) self.assertIn(''' network={ - ssid="workplace" + ssid=P"workplace" key_mgmt=WPA-EAP eap=TTLS identity="joe@internal.example.com" @@ -141,7 +141,7 @@ def test_auth_wifi_detailed(self): ''', new_config) self.assertIn(''' network={ - ssid="workplacehashed" + ssid=P"workplacehashed" key_mgmt=WPA-EAP eap=TTLS identity="joe@internal.example.com" @@ -151,7 +151,7 @@ def test_auth_wifi_detailed(self): ''', new_config) self.assertIn(''' network={ - ssid="customernet" + ssid=P"customernet" key_mgmt=WPA-EAP eap=TLS identity="cert-joe@cust.example.com" @@ -164,13 +164,13 @@ def test_auth_wifi_detailed(self): ''', new_config) self.assertIn(''' network={ - ssid="opennet" + ssid=P"opennet" key_mgmt=NONE } ''', new_config) self.assertIn(''' network={ - ssid="Joe's Home" + ssid=P"Joe's Home" key_mgmt=WPA-PSK WPA-PSK-SHA256 SAE ieee80211w=1 psk="s0s3kr1t" diff --git a/tests/generator/test_common.py b/tests/generator/test_common.py index d6552f3de..71216c1c7 100644 --- a/tests/generator/test_common.py +++ b/tests/generator/test_common.py @@ -19,7 +19,7 @@ import os import textwrap -from .base import TestBase, ND_DHCP4, ND_DHCP6, ND_DHCPYES, ND_EMPTY, NM_MANAGED, NM_UNMANAGED +from .base import UDEV_NO_MAC_RULE, TestBase, ND_DHCP4, ND_DHCP6, ND_DHCPYES, ND_EMPTY, NM_MANAGED, NM_UNMANAGED class TestNetworkd(TestBase): @@ -810,6 +810,18 @@ def test_ip6_addr_gen_token(self): UseMTU=true '''}) + def test_nd_udev_rules_escaped(self): + self.generate('''network: + version: 2 + renderer: NetworkManager + ethernets: + def1: + match: + driver: "abc\\"xyz\\n0\\n\\n1" + set-name: "eth\\"\\n\\nxyz\\n0"''', skip_generated_yaml_validation=True) + + self.assert_networkd_udev({'def1.rules': (UDEV_NO_MAC_RULE % ('abc\\"xyz\\n0\\n\\n1', 'eth\\"\\n\\nxyz\\n0'))}) + class TestNetworkManager(TestBase): @@ -1315,6 +1327,28 @@ def test_nameserver(self): method=ignore '''}) + def test_nm_udev_rules_escaped(self): + self.generate('''network: + version: 2 + renderer: networkd + ethernets: + eth0: + match: + name: "eth\\"0" + dhcp4: true''') + self.assert_nm_udev(NM_UNMANAGED % 'eth\\"0') + + self.generate('''network: + version: 2 + renderer: networkd + ethernets: + eth0: + match: + name: "eth0" + set-name: "eth\\n0" + dhcp4: true''', skip_generated_yaml_validation=True) + self.assert_nm_udev(NM_UNMANAGED % 'eth\\n0' + NM_UNMANAGED % 'eth0') + class TestForwardDeclaration(TestBase): @@ -1777,13 +1811,13 @@ def test_wifi_access_points_merging(self): self.assert_wpa_supplicant("wlan0", """ctrl_interface=/run/wpa_supplicant network={ - ssid="mynewwifi" + ssid=P"mynewwifi" key_mgmt=WPA-PSK WPA-PSK-SHA256 SAE ieee80211w=1 psk="aaaaaaaa" } network={ - ssid="mywifi" + ssid=P"mywifi" key_mgmt=WPA-PSK WPA-PSK-SHA256 SAE ieee80211w=1 psk="aaaaaaaa" @@ -1814,7 +1848,7 @@ def test_wifi_access_points_overwriting(self): self.assert_wpa_supplicant("wlan0", """ctrl_interface=/run/wpa_supplicant network={ - ssid="mywifi" + ssid=P"mywifi" key_mgmt=WPA-PSK WPA-PSK-SHA256 SAE ieee80211w=1 psk="bbbbbbbb" diff --git a/tests/generator/test_wifis.py b/tests/generator/test_wifis.py index 0962a8817..27e91e76e 100644 --- a/tests/generator/test_wifis.py +++ b/tests/generator/test_wifis.py @@ -65,7 +65,7 @@ def test_wifi(self): with open(os.path.join(self.workdir.name, 'run/netplan/wpa-wl0.conf')) as f: new_config = f.read() - network = 'ssid="{}"\n freq_list='.format('band-no-channel2') + network = 'ssid=P"{}"\n freq_list='.format('band-no-channel2') freqs_5GHz = [5610, 5310, 5620, 5320, 5630, 5640, 5340, 5035, 5040, 5045, 5055, 5060, 5660, 5680, 5670, 5080, 5690, 5700, 5710, 5720, 5825, 5745, 5755, 5805, 5765, 5160, 5775, 5170, 5480, 5180, 5795, 5190, 5500, 5200, 5510, 5210, 5520, 5220, 5530, 5230, 5540, 5240, 5550, 5250, 5560, 5260, 5570, 5270, 5580, 5280, 5590, @@ -76,7 +76,7 @@ def test_wifi(self): for freq in freqs_5GHz: self.assertRegex(new_config, '{}[ 0-9]*{}[ 0-9]*\n'.format(network, freq)) - network = 'ssid="{}"\n freq_list='.format('band-no-channel') + network = 'ssid=P"{}"\n freq_list='.format('band-no-channel') freqs_24GHz = [2412, 2417, 2422, 2427, 2432, 2442, 2447, 2437, 2452, 2457, 2462, 2467, 2472, 2484] freqs = new_config.split(network) freqs = freqs[1].split('\n')[0] @@ -86,20 +86,20 @@ def test_wifi(self): self.assertIn(''' network={ - ssid="channel-no-band" + ssid=P"channel-no-band" key_mgmt=NONE } ''', new_config) self.assertIn(''' network={ - ssid="peer2peer" + ssid=P"peer2peer" mode=1 key_mgmt=NONE } ''', new_config) self.assertIn(''' network={ - ssid="hidden-y" + ssid=P"hidden-y" scan_ssid=1 key_mgmt=WPA-PSK WPA-PSK-SHA256 SAE ieee80211w=1 @@ -108,7 +108,7 @@ def test_wifi(self): ''', new_config) self.assertIn(''' network={ - ssid="hidden-n" + ssid=P"hidden-n" key_mgmt=WPA-PSK WPA-PSK-SHA256 SAE ieee80211w=1 psk="5ecur1ty" @@ -116,7 +116,7 @@ def test_wifi(self): ''', new_config) self.assertIn(''' network={ - ssid="workplace" + ssid=P"workplace" bssid=de:ad:be:ef:ca:fe freq_list=5500 key_mgmt=WPA-PSK WPA-PSK-SHA256 SAE @@ -126,7 +126,7 @@ def test_wifi(self): ''', new_config) self.assertIn(''' network={ - ssid="Joe's Home" + ssid=P"Joe's Home" bssid=00:11:22:33:44:55 freq_list=2462 key_mgmt=WPA-PSK WPA-PSK-SHA256 SAE @@ -232,7 +232,7 @@ def test_wifi_wowlan(self): self.assertIn(''' wowlan_triggers=any disconnect magic_pkt gtk_rekey_failure eap_identity_req four_way_handshake rfkill_release network={ - ssid="homenet" + ssid=P"homenet" key_mgmt=NONE } ''', new_config) @@ -265,7 +265,7 @@ def test_wifi_wowlan_default(self): new_config = f.read() self.assertIn(''' network={ - ssid="homenet" + ssid=P"homenet" key_mgmt=NONE } ''', new_config) @@ -289,7 +289,7 @@ def test_wifi_wpa3_personal(self): self.assert_wpa_supplicant("wl0", """ctrl_interface=/run/wpa_supplicant network={ - ssid="homenet" + ssid=P"homenet" key_mgmt=SAE ieee80211w=2 psk="********" @@ -316,7 +316,7 @@ def test_wifi_wpa3_enterprise_eap_sha256(self): self.assert_wpa_supplicant("wl0", """ctrl_interface=/run/wpa_supplicant network={ - ssid="homenet" + ssid=P"homenet" key_mgmt=WPA-EAP WPA-EAP-SHA256 eap=TLS ieee80211w=1 @@ -349,7 +349,7 @@ def test_wifi_wpa3_enterprise_eap_suite_b_192(self): self.assert_wpa_supplicant("wl0", """ctrl_interface=/run/wpa_supplicant network={ - ssid="homenet" + ssid=P"homenet" key_mgmt=WPA-EAP-SUITE-B-192 eap=TLS ieee80211w=2 @@ -378,7 +378,7 @@ def test_wifi_ieee8021x_eap_leap(self): self.assert_wpa_supplicant("wl0", """ctrl_interface=/run/wpa_supplicant network={ - ssid="homenet" + ssid=P"homenet" key_mgmt=IEEE8021X eap=LEAP identity="some-id" @@ -402,7 +402,7 @@ def test_wifi_ieee8021x_eap_pwd(self): self.assert_wpa_supplicant("wl0", """ctrl_interface=/run/wpa_supplicant network={ - ssid="homenet" + ssid=P"homenet" key_mgmt=IEEE8021X eap=PWD identity="some-id" @@ -427,7 +427,7 @@ def test_wifi_ieee8021x_eap_and_psk(self): self.assert_wpa_supplicant("wl0", """ctrl_interface=/run/wpa_supplicant network={ - ssid="homenet" + ssid=P"homenet" key_mgmt=WPA-EAP eap=LEAP ieee80211w=1 @@ -437,6 +437,48 @@ def test_wifi_ieee8021x_eap_and_psk(self): } """) + def test_escaping_special_characters(self): + self.generate('''network: + version: 2 + wifis: + wl0: + regulatory-domain: "abc\\n\\n321\\n\\"123" + access-points: + "abc\\n\\n123\\"x\\ry\\bz": + password: "abc\\n\\n\\n\\"123" + auth: + key-management: eap + method: leap + anonymous-identity: "abc\\n\\n321\\n\\"123" + identity: "abc\\n\\n321\\n\\"123" + password: "abc\\n\\n\\n\\"123" + ca-certificate: "abc\\n\\n321\\n\\"123" + client-certificate: "abc\\n\\n321\\n\\"123" + client-key: "abc\\n\\n321\\n\\"123" + client-key-password: "abc\\n\\n321\\n\\"123" + phase2-auth: "abc\\n\\n321\\n\\"123" + ''', skip_generated_yaml_validation=True) + + self.assert_wpa_supplicant("wl0", """ctrl_interface=/run/wpa_supplicant + +country=abc\\n\\n321\\n\\"123 +network={ + ssid=P"abc\\n\\n123\\\"x\\ry\\bz" + key_mgmt=WPA-EAP + eap=LEAP + ieee80211w=1 + identity="abc\\n\\n321\\n\\\"123" + anonymous_identity="abc\\n\\n321\\n\\\"123" + psk="abc\\n\\n\\n\"123" + password="abc\\n\\n\\n\"123" + ca_cert="abc\\n\\n321\\n\\\"123" + client_cert="abc\\n\\n321\\n\\\"123" + private_key="abc\\n\\n321\\n\\\"123" + private_key_passwd="abc\\n\\n321\\n\"123" + phase2="auth=abc\\n\\n321\\n\\\"123" +} +""") + class TestNetworkManager(TestBase): @@ -719,7 +761,7 @@ def test_wifi_adhoc_wpa_24ghz(self): self.assert_wpa_supplicant("wl0", """ctrl_interface=/run/wpa_supplicant network={ - ssid="homenet" + ssid=P"homenet" frequency=2442 mode=1 key_mgmt=WPA-PSK WPA-PSK-SHA256 SAE @@ -743,7 +785,7 @@ def test_wifi_adhoc_wpa_5ghz(self): self.assert_wpa_supplicant("wl0", """ctrl_interface=/run/wpa_supplicant network={ - ssid="homenet" + ssid=P"homenet" frequency=5035 mode=1 key_mgmt=WPA-PSK WPA-PSK-SHA256 SAE From 85f1a6ce0672fbf32cfb8d3a05ae9f98e37793e9 Mon Sep 17 00:00:00 2001 From: Danilo Egea Gondolfo Date: Thu, 23 May 2024 15:54:43 +0100 Subject: [PATCH 3/4] backends: escape file paths Escape strings used to build paths with g_uri_escape_string(). systemd_escape() could also be used but it has the downside of calling an external program and, by default, it escapes dashes (which are present in files generated from Network Manager for example). --- src/networkd.c | 13 ++++--- src/nm.c | 5 +-- src/openvswitch.c | 19 +++++----- src/util.c | 7 ++-- tests/generator/test_common.py | 66 ++++++++++++++++++++++++++++++++++ tests/generator/test_ovs.py | 24 +++++++++++++ 6 files changed, 115 insertions(+), 19 deletions(-) diff --git a/src/networkd.c b/src/networkd.c index 72606a587..a1ccb88bf 100644 --- a/src/networkd.c +++ b/src/networkd.c @@ -1101,7 +1101,8 @@ STATIC void write_rules_file(const NetplanNetDefinition* def, const char* rootdir) { GString* s = NULL; - g_autofree char* path = g_strjoin(NULL, "run/udev/rules.d/99-netplan-", def->id, ".rules", NULL); + g_autofree char* escaped_netdef_id = g_uri_escape_string(def->id, NULL, TRUE); + g_autofree char* path = g_strjoin(NULL, "run/udev/rules.d/99-netplan-", escaped_netdef_id, ".rules", NULL); /* do we need to write a .rules file? * It's only required for reliably setting the name of a physical device @@ -1318,7 +1319,8 @@ write_wpa_conf(const NetplanNetDefinition* def, const char* rootdir, GError** er { GHashTableIter iter; GString* s = g_string_new("ctrl_interface=/run/wpa_supplicant\n\n"); - g_autofree char* path = g_strjoin(NULL, "run/netplan/wpa-", def->id, ".conf", NULL); + g_autofree char* escaped_netdef_id = g_uri_escape_string(def->id, NULL, TRUE); + g_autofree char* path = g_strjoin(NULL, "run/netplan/wpa-", escaped_netdef_id, ".conf", NULL); g_debug("%s: Creating wpa_supplicant configuration file %s", def->id, path); if (def->type == NETPLAN_DEF_TYPE_WIFI) { @@ -1434,7 +1436,8 @@ _netplan_netdef_write_networkd( GError** error) { /* TODO: make use of netplan_netdef_get_output_filename() */ - g_autofree char* path_base = g_strjoin(NULL, "run/systemd/network/10-netplan-", def->id, NULL); + g_autofree char* escaped_netdef_id = g_uri_escape_string(def->id, NULL, TRUE); + g_autofree char* path_base = g_strjoin(NULL, "run/systemd/network/10-netplan-", escaped_netdef_id, NULL); SET_OPT_OUT_PTR(has_been_written, FALSE); /* We want this for all backends when renaming, as *.link and *.rules files are @@ -1456,8 +1459,8 @@ _netplan_netdef_write_networkd( } if (def->type == NETPLAN_DEF_TYPE_WIFI || def->has_auth) { - g_autofree char* link = g_strjoin(NULL, rootdir ?: "", "/run/systemd/system/systemd-networkd.service.wants/netplan-wpa-", def->id, ".service", NULL); - g_autofree char* slink = g_strjoin(NULL, "/run/systemd/system/netplan-wpa-", def->id, ".service", NULL); + g_autofree char* link = g_strjoin(NULL, rootdir ?: "", "/run/systemd/system/systemd-networkd.service.wants/netplan-wpa-", escaped_netdef_id, ".service", NULL); + g_autofree char* slink = g_strjoin(NULL, "/run/systemd/system/netplan-wpa-", escaped_netdef_id, ".service", NULL); if (def->type == NETPLAN_DEF_TYPE_WIFI && def->has_match) { g_set_error(error, NETPLAN_BACKEND_ERROR, NETPLAN_ERROR_UNSUPPORTED, "ERROR: %s: networkd backend does not support wifi with match:, only by interface name\n", def->id); return FALSE; diff --git a/src/nm.c b/src/nm.c index 95f4a867d..c82b87b39 100644 --- a/src/nm.c +++ b/src/nm.c @@ -932,10 +932,11 @@ write_nm_conf_access_point(const NetplanNetDefinition* def, const char* rootdir, g_datalist_foreach((GData**)&def->backend_settings.passthrough, write_fallback_key_value, kf); } + g_autofree char* escaped_netdef_id = g_uri_escape_string(def->id, NULL, TRUE); if (ap) { g_autofree char* escaped_ssid = g_uri_escape_string(ap->ssid, NULL, TRUE); /* TODO: make use of netplan_netdef_get_output_filename() */ - conf_path = g_strjoin(NULL, "run/NetworkManager/system-connections/netplan-", def->id, "-", escaped_ssid, ".nmconnection", NULL); + conf_path = g_strjoin(NULL, "run/NetworkManager/system-connections/netplan-", escaped_netdef_id, "-", escaped_ssid, ".nmconnection", NULL); g_key_file_set_string(kf, "wifi", "ssid", ap->ssid); if (ap->mode < NETPLAN_WIFI_MODE_OTHER) @@ -970,7 +971,7 @@ write_nm_conf_access_point(const NetplanNetDefinition* def, const char* rootdir, } } else { /* TODO: make use of netplan_netdef_get_output_filename() */ - conf_path = g_strjoin(NULL, "run/NetworkManager/system-connections/netplan-", def->id, ".nmconnection", NULL); + conf_path = g_strjoin(NULL, "run/NetworkManager/system-connections/netplan-", escaped_netdef_id, ".nmconnection", NULL); if (def->has_auth) { write_dot1x_auth_parameters(&def->auth, kf); } diff --git a/src/openvswitch.c b/src/openvswitch.c index 2ab77e7f0..0a81a5962 100644 --- a/src/openvswitch.c +++ b/src/openvswitch.c @@ -31,9 +31,9 @@ STATIC gboolean write_ovs_systemd_unit(const char* id, const GString* cmds, const char* rootdir, gboolean physical, gboolean cleanup, const char* dependency, GError** error) { - g_autofree gchar* id_escaped = NULL; - g_autofree char* link = g_strjoin(NULL, rootdir ?: "", "/run/systemd/system/systemd-networkd.service.wants/netplan-ovs-", id, ".service", NULL); - g_autofree char* path = g_strjoin(NULL, "/run/systemd/system/netplan-ovs-", id, ".service", NULL); + g_autofree char* escaped_netdef_id = g_uri_escape_string(id, NULL, TRUE); + g_autofree char* link = g_strjoin(NULL, rootdir ?: "", "/run/systemd/system/systemd-networkd.service.wants/netplan-ovs-", escaped_netdef_id, ".service", NULL); + g_autofree char* path = g_strjoin(NULL, "/run/systemd/system/netplan-ovs-", escaped_netdef_id, ".service", NULL); GString* s = g_string_new("[Unit]\n"); g_string_append_printf(s, "Description=OpenVSwitch configuration for %s\n", id); @@ -42,9 +42,8 @@ write_ovs_systemd_unit(const char* id, const GString* cmds, const char* rootdir, g_string_append_printf(s, "Wants=ovsdb-server.service\n"); g_string_append_printf(s, "After=ovsdb-server.service\n"); if (physical) { - id_escaped = systemd_escape((char*) id); - g_string_append_printf(s, "Requires=sys-subsystem-net-devices-%s.device\n", id_escaped); - g_string_append_printf(s, "After=sys-subsystem-net-devices-%s.device\n", id_escaped); + g_string_append_printf(s, "Requires=sys-subsystem-net-devices-%s.device\n", escaped_netdef_id); + g_string_append_printf(s, "After=sys-subsystem-net-devices-%s.device\n", escaped_netdef_id); } if (!cleanup) { g_string_append_printf(s, "After=netplan-ovs-cleanup.service\n"); @@ -54,8 +53,9 @@ write_ovs_systemd_unit(const char* id, const GString* cmds, const char* rootdir, } g_string_append(s, "Before=network.target\nWants=network.target\n"); if (dependency) { - g_string_append_printf(s, "Requires=netplan-ovs-%s.service\n", dependency); - g_string_append_printf(s, "After=netplan-ovs-%s.service\n", dependency); + g_autofree char* escaped_dependency = g_uri_escape_string(dependency, NULL, TRUE); + g_string_append_printf(s, "Requires=netplan-ovs-%s.service\n", escaped_dependency); + g_string_append_printf(s, "After=netplan-ovs-%s.service\n", escaped_dependency); } g_string_append(s, "\n[Service]\nType=oneshot\nTimeoutStartSec=10s\n"); @@ -325,6 +325,7 @@ _netplan_netdef_write_ovs(const NetplanState* np_state, const NetplanNetDefiniti gchar* dependency = NULL; const char* type = netplan_type_to_table_name(def->type); g_autofree char* base_config_path = NULL; + g_autofree char* escaped_netdef_id = g_uri_escape_string(def->id, NULL, TRUE); char* value = NULL; const NetplanOVSSettings* settings = &np_state->ovs_settings; @@ -415,7 +416,7 @@ _netplan_netdef_write_ovs(const NetplanState* np_state, const NetplanNetDefiniti /* Try writing out a base config */ /* TODO: make use of netplan_netdef_get_output_filename() */ - base_config_path = g_strjoin(NULL, "run/systemd/network/10-netplan-", def->id, NULL); + base_config_path = g_strjoin(NULL, "run/systemd/network/10-netplan-", escaped_netdef_id, NULL); if (!_netplan_netdef_write_network_file(np_state, def, rootdir, base_config_path, has_been_written, error)) return FALSE; } else { diff --git a/src/util.c b/src/util.c index f41923436..8bbaf133b 100644 --- a/src/util.c +++ b/src/util.c @@ -663,17 +663,18 @@ ssize_t netplan_netdef_get_output_filename(const NetplanNetDefinition* netdef, const char* ssid, char* out_buffer, size_t out_buf_size) { g_autofree gchar* conf_path = NULL; + g_autofree char* escaped_netdef_id = g_uri_escape_string(netdef->id, NULL, TRUE); if (netdef->backend == NETPLAN_BACKEND_NM) { if (ssid) { g_autofree char* escaped_ssid = g_uri_escape_string(ssid, NULL, TRUE); - conf_path = g_strjoin(NULL, "/run/NetworkManager/system-connections/netplan-", netdef->id, "-", escaped_ssid, ".nmconnection", NULL); + conf_path = g_strjoin(NULL, "/run/NetworkManager/system-connections/netplan-", escaped_netdef_id, "-", escaped_ssid, ".nmconnection", NULL); } else { - conf_path = g_strjoin(NULL, "/run/NetworkManager/system-connections/netplan-", netdef->id, ".nmconnection", NULL); + conf_path = g_strjoin(NULL, "/run/NetworkManager/system-connections/netplan-", escaped_netdef_id, ".nmconnection", NULL); } } else if (netdef->backend == NETPLAN_BACKEND_NETWORKD || netdef->backend == NETPLAN_BACKEND_OVS) { - conf_path = g_strjoin(NULL, "/run/systemd/network/10-netplan-", netdef->id, ".network", NULL); + conf_path = g_strjoin(NULL, "/run/systemd/network/10-netplan-", escaped_netdef_id, ".network", NULL); } if (conf_path) diff --git a/tests/generator/test_common.py b/tests/generator/test_common.py index 71216c1c7..fd892517f 100644 --- a/tests/generator/test_common.py +++ b/tests/generator/test_common.py @@ -822,6 +822,47 @@ def test_nd_udev_rules_escaped(self): self.assert_networkd_udev({'def1.rules': (UDEV_NO_MAC_RULE % ('abc\\"xyz\\n0\\n\\n1', 'eth\\"\\n\\nxyz\\n0'))}) + def test_nd_file_paths_escaped(self): + self.generate('''network: + version: 2 + ethernets: + "abc/../../xyz0": + match: + driver: "drv" + set-name: "eth123"''') + + self.assert_networkd_udev({'abc%2F..%2F..%2Fxyz0.rules': (UDEV_NO_MAC_RULE % ('drv', 'eth123'))}) + self.assert_networkd({'abc%2F..%2F..%2Fxyz0.network': '''[Match]\nDriver=drv +Name=eth123 + +[Network] +LinkLocalAddressing=ipv6 +''', + 'abc%2F..%2F..%2Fxyz0.link': '''[Match]\nDriver=drv\n +[Link] +Name=eth123 +WakeOnLan=off +'''}) + + self.generate('''network: + version: 2 + wifis: + "abc/../../xyz0": + dhcp4: true + access-points: + "mywifi": + password: "aaaaaaaa"''') + + self.assert_wpa_supplicant("abc%2F..%2F..%2Fxyz0", """ctrl_interface=/run/wpa_supplicant + +network={ + ssid=P"mywifi" + key_mgmt=WPA-PSK WPA-PSK-SHA256 SAE + ieee80211w=1 + psk="aaaaaaaa" +} +""") + class TestNetworkManager(TestBase): @@ -1349,6 +1390,31 @@ def test_nm_udev_rules_escaped(self): dhcp4: true''', skip_generated_yaml_validation=True) self.assert_nm_udev(NM_UNMANAGED % 'eth\\n0' + NM_UNMANAGED % 'eth0') + def test_nm_file_paths_escaped(self): + self.generate('''network: + version: 2 + renderer: NetworkManager + ethernets: + "abc/../../xyz0": + match: + driver: "drv" + set-name: "eth123"''') + + self.assert_nm({'abc%2F..%2F..%2Fxyz0': '''[connection] +id=netplan-abc/../../xyz0 +type=ethernet +interface-name=eth123 + +[ethernet] +wake-on-lan=0 + +[ipv4] +method=link-local + +[ipv6] +method=ignore +'''}) + class TestForwardDeclaration(TestBase): diff --git a/tests/generator/test_ovs.py b/tests/generator/test_ovs.py index a6ebbcb84..71167053e 100644 --- a/tests/generator/test_ovs.py +++ b/tests/generator/test_ovs.py @@ -1110,3 +1110,27 @@ def test_both_ports_with_same_name(self): - [portname, portname] ''', expect_fail=True) self.assertIn('Open vSwitch patch ports must be of different name', err) + + def test_file_paths_escaped(self): + self.generate('''network: + version: 2 + bridges: + "abc/../../123": + openvswitch: {} + vlans: + "abc/../../123.100": + id: 100 + link: "abc/../../123" +''') + self.assert_ovs({'abc%2F..%2F..%2F123.service': OVS_BR_EMPTY % {'iface': 'abc/../../123'}, + 'abc%2F..%2F..%2F123.100.service': OVS_VIRTUAL % {'iface': 'abc/../../123.100', 'extra': + '''Requires=netplan-ovs-abc%2F..%2F..%2F123.service +After=netplan-ovs-abc%2F..%2F..%2F123.service + +[Service] +Type=oneshot +TimeoutStartSec=10s +ExecStart=/usr/bin/ovs-vsctl --may-exist add-br abc/../../123.100 abc/../../123 100 +ExecStart=/usr/bin/ovs-vsctl set Interface abc/../../123.100 external-ids:netplan=true +'''}, + 'cleanup.service': OVS_CLEANUP % {'iface': 'cleanup'}}) From 36010fad4f63356635d437933b5fbab7ea8883a4 Mon Sep 17 00:00:00 2001 From: Danilo Egea Gondolfo Date: Fri, 24 May 2024 14:11:12 +0100 Subject: [PATCH 4/4] backends: escape semicolons in service units Semicolons separated from other words by a combination of spaces and/or tabs will be escaped. --- src/networkd.c | 10 ++++++ src/openvswitch.c | 3 ++ src/sriov.c | 6 ++++ src/util-internal.h | 3 ++ src/util.c | 34 +++++++++++++++++++ tests/ctests/test_netplan_misc.c | 45 +++++++++++++++++++++++++ tests/generator/test_args.py | 54 +++++++++++++++++++++++++++++ tests/generator/test_ovs.py | 58 ++++++++++++++++++++++++++++++++ tests/test_sriov.py | 40 ++++++++++++++++++++++ 9 files changed, 253 insertions(+) diff --git a/src/networkd.c b/src/networkd.c index a1ccb88bf..34d14a629 100644 --- a/src/networkd.c +++ b/src/networkd.c @@ -391,6 +391,9 @@ write_regdom(const NetplanNetDefinition* def, const char* rootdir, GError** erro g_string_append(s, "\n[Service]\nType=oneshot\n"); g_string_append_printf(s, "ExecStart="SBINDIR"/iw reg set %s\n", def->regulatory_domain); + g_autofree char* new_s = _netplan_scrub_systemd_unit_contents(s->str); + g_string_free(s, TRUE); + s = g_string_new(new_s); _netplan_g_string_free_to_file_with_permissions(s, rootdir, path, NULL, "root", "root", 0640); _netplan_safe_mkdir_p_dir(link); if (symlink(path, link) < 0 && errno != EEXIST) { @@ -1311,6 +1314,10 @@ write_wpa_unit(const NetplanNetDefinition* def, const char* rootdir) } else { g_string_append(s, " -Dnl80211,wext\n"); } + + g_autofree char* new_s = _netplan_scrub_systemd_unit_contents(s->str); + g_string_free(s, TRUE); + s = g_string_new(new_s); _netplan_g_string_free_to_file_with_permissions(s, rootdir, path, NULL, "root", "root", 0640); } @@ -1584,6 +1591,9 @@ _netplan_networkd_write_wait_online(const NetplanState* np_state, const char* ro } g_string_append(content, "\n"); + g_autofree char* new_content = _netplan_scrub_systemd_unit_contents(content->str); + g_string_free(content, TRUE); + content = g_string_new(new_content); _netplan_g_string_free_to_file_with_permissions(content, rootdir, override, NULL, "root", "root", 0640); g_hash_table_destroy(non_optional_interfaces); return TRUE; diff --git a/src/openvswitch.c b/src/openvswitch.c index 0a81a5962..472323039 100644 --- a/src/openvswitch.c +++ b/src/openvswitch.c @@ -66,6 +66,9 @@ write_ovs_systemd_unit(const char* id, const GString* cmds, const char* rootdir, g_string_append(s, "StartLimitBurst=0\n"); g_string_append(s, cmds->str); + g_autofree char* new_s = _netplan_scrub_systemd_unit_contents(s->str); + g_string_free(s, TRUE); + s = g_string_new(new_s); _netplan_g_string_free_to_file_with_permissions(s, rootdir, path, NULL, "root", "root", 0640); _netplan_safe_mkdir_p_dir(link); diff --git a/src/sriov.c b/src/sriov.c index 63ef7363f..ff1acb649 100644 --- a/src/sriov.c +++ b/src/sriov.c @@ -54,6 +54,9 @@ write_sriov_rebind_systemd_unit(GHashTable* pfs, const char* rootdir, GError** e g_string_truncate(interfaces, interfaces->len-1); /* cut trailing whitespace */ g_string_append_printf(s, "ExecStart=" SBINDIR "/netplan rebind --debug %s\n", interfaces->str); + g_autofree char* new_s = _netplan_scrub_systemd_unit_contents(s->str); + g_string_free(s, TRUE); + s = g_string_new(new_s); _netplan_g_string_free_to_file_with_permissions(s, rootdir, path, NULL, "root", "root", 0640); g_string_free(interfaces, TRUE); @@ -90,6 +93,9 @@ write_sriov_apply_systemd_unit(GHashTable* pfs, const char* rootdir, GError** er g_string_append(s, "\n[Service]\nType=oneshot\n"); g_string_append_printf(s, "ExecStart=" SBINDIR "/netplan apply --sriov-only\n"); + g_autofree char* new_s = _netplan_scrub_systemd_unit_contents(s->str); + g_string_free(s, TRUE); + s = g_string_new(new_s); _netplan_g_string_free_to_file_with_permissions(s, rootdir, path, NULL, "root", "root", 0640); _netplan_safe_mkdir_p_dir(link); diff --git a/src/util-internal.h b/src/util-internal.h index 2c5595823..3695aeec3 100644 --- a/src/util-internal.h +++ b/src/util-internal.h @@ -217,3 +217,6 @@ _netplan_netdef_get_gateway6(const NetplanNetDefinition* netdef, char* out_buffe gchar* _netplan_scrub_string(const char* content); + +gchar* +_netplan_scrub_systemd_unit_contents(const char* content); diff --git a/src/util.c b/src/util.c index 8bbaf133b..086403d1b 100644 --- a/src/util.c +++ b/src/util.c @@ -1289,3 +1289,37 @@ _netplan_scrub_string(const char* content) return g_string_free(s, FALSE); } + +STATIC gboolean +_is_space_or_tab(char c) +{ + return c == ' ' || c == '\t'; +} + +char* +_netplan_scrub_systemd_unit_contents(const char* content) +{ + size_t content_len = strlen(content); + // Assume a few replacements will happen to reduce reallocation + GString* s = g_string_sized_new(content_len + 8); + + // Append the first character of "content" to the result string + g_string_append_len(s, content, 1); + + // Walk from the second element to the one before the last looking for isolated semicolons + // A semicolon is isolated if it's surrounded by either tabs or spaces + const char* p = content + 1; + while (p < (content + content_len - 1)) { + if (*p == ';' && _is_space_or_tab(*(p - 1)) && _is_space_or_tab(*(p + 1))) { + g_string_append_len(s, "\\;", 2); + } else { + g_string_append_len(s, p, 1); + } + p++; + } + + // Append the last character of "content" to the result string + g_string_append_len(s, p, 1); + + return g_string_free(s, FALSE); +} diff --git a/tests/ctests/test_netplan_misc.c b/tests/ctests/test_netplan_misc.c index 960766237..06676da39 100644 --- a/tests/ctests/test_netplan_misc.c +++ b/tests/ctests/test_netplan_misc.c @@ -502,6 +502,50 @@ test_normalize_ip_address(__unused void** state) assert_string_equal(normalize_ip_address("0.0.0.0/0", AF_INET), "0.0.0.0/0"); } +void +test_scrub_systemd_unit_content(__unused void** state) +{ + char* str = ";abc;"; + char* res = _netplan_scrub_systemd_unit_contents(str); + assert_string_equal(res, ";abc;"); + g_free(res); + + str = ";;;;"; + res = _netplan_scrub_systemd_unit_contents(str); + assert_string_equal(res, ";;;;"); + g_free(res); + + str = " ;;;; "; + res = _netplan_scrub_systemd_unit_contents(str); + assert_string_equal(res, " ;;;; "); + g_free(res); + + str = "; ; ; ; ;"; + res = _netplan_scrub_systemd_unit_contents(str); + assert_string_equal(res, "; \\; \\; \\; ;"); + g_free(res); + + str = " ; ; ; ; ; "; + res = _netplan_scrub_systemd_unit_contents(str); + assert_string_equal(res, " \\; \\; \\; \\; \\; "); + g_free(res); + + str = "a ; ; ; ; ; b"; + res = _netplan_scrub_systemd_unit_contents(str); + assert_string_equal(res, "a \\; \\; \\; \\; \\; b"); + g_free(res); + + str = "\t;\t; ;\t; \t ;\t "; + res = _netplan_scrub_systemd_unit_contents(str); + assert_string_equal(res, "\t\\;\t\\; \\;\t\\; \t \\;\t "); + g_free(res); + + str = "\t;\t;\t;\t;\t;\t"; + res = _netplan_scrub_systemd_unit_contents(str); + assert_string_equal(res, "\t\\;\t\\;\t\\;\t\\;\t\\;\t"); + g_free(res); +} + int setup(__unused void** state) { @@ -539,6 +583,7 @@ main() cmocka_unit_test(test_normalize_ip_address), cmocka_unit_test(test_util_get_link_local_true), cmocka_unit_test(test_util_get_link_local_false), + cmocka_unit_test(test_scrub_systemd_unit_content), }; return cmocka_run_group_tests(tests, setup, tear_down); diff --git a/tests/generator/test_args.py b/tests/generator/test_args.py index a824a8a05..6909194c1 100644 --- a/tests/generator/test_args.py +++ b/tests/generator/test_args.py @@ -262,3 +262,57 @@ def test_systemd_generator_badcall(self): except subprocess.CalledProcessError as e: self.assertEqual(e.returncode, 1) self.assertIn(b'can not be called directly', e.output) + + def test_systemd_generator_escaping(self): + conf = os.path.join(self.confdir, 'a.yaml') + os.makedirs(os.path.dirname(conf)) + with open(conf, 'w') as f: + f.write('''network: + version: 2 + ethernets: + lo: + match: + name: lo + set-name: "a ; b\\t; c\\t; d \\n 123 ; echo " + addresses: ["127.0.0.1/8", "::1/128"]''') + os.chmod(conf, mode=0o600) + outdir = os.path.join(self.workdir.name, 'out') + os.mkdir(outdir) + + generator = os.path.join(self.workdir.name, 'systemd', 'system-generators', 'netplan') + os.makedirs(os.path.dirname(generator)) + os.symlink(exe_generate, generator) + + subprocess.check_call([generator, '--root-dir', self.workdir.name, outdir, outdir, outdir]) + n = os.path.join(self.workdir.name, 'run', 'systemd', 'network', '10-netplan-lo.network') + self.assertTrue(os.path.exists(n)) + os.unlink(n) + + # should auto-enable networkd and -wait-online + service_dir = os.path.join(self.workdir.name, 'run', 'systemd', 'system') + self.assertTrue(os.path.islink(os.path.join( + outdir, 'multi-user.target.wants', 'systemd-networkd.service'))) + self.assertTrue(os.path.islink(os.path.join( + outdir, 'network-online.target.wants', 'systemd-networkd-wait-online.service'))) + override = os.path.join(service_dir, 'systemd-networkd-wait-online.service.d', '10-netplan.conf') + self.assertTrue(os.path.isfile(override)) + with open(override, 'r') as f: + # eth99 does not exist on the system, so will not be listed + self.assertEqual(f.read(), '''[Unit] +ConditionPathIsSymbolicLink=/run/systemd/generator/network-online.target.wants/systemd-networkd-wait-online.service + +[Service] +ExecStart= +ExecStart=/lib/systemd/systemd-networkd-wait-online -i a \\; b\\t; c\\t; d \\n 123 \\; echo :degraded\n''') + + # should be a no-op the second time while the stamp exists + out = subprocess.check_output([generator, '--root-dir', self.workdir.name, outdir, outdir, outdir], + stderr=subprocess.STDOUT) + self.assertFalse(os.path.exists(n)) + self.assertIn(b'netplan generate already ran', out) + + # after removing the stamp it generates again, and not trip over the + # existing enablement symlink + os.unlink(os.path.join(outdir, 'netplan.stamp')) + subprocess.check_output([generator, '--root-dir', self.workdir.name, outdir, outdir, outdir]) + self.assertTrue(os.path.exists(n)) diff --git a/tests/generator/test_ovs.py b/tests/generator/test_ovs.py index 71167053e..4393cea38 100644 --- a/tests/generator/test_ovs.py +++ b/tests/generator/test_ovs.py @@ -1134,3 +1134,61 @@ def test_file_paths_escaped(self): ExecStart=/usr/bin/ovs-vsctl set Interface abc/../../123.100 external-ids:netplan=true '''}, 'cleanup.service': OVS_CLEANUP % {'iface': 'cleanup'}}) + + def test_control_characters_and_semicolons_escaping(self): + self.generate('''network: + version: 2 + bridges: # bridges first, to trigger multi-pass processing + ovs0: + interfaces: [eth0, eth1] + openvswitch: {} + ethernets: + eth0: + openvswitch: + external-ids: + "a\\n1\\ra": " ; a ; 1 ;a; ;b\\t;\\t3 ;\\ta\\t; 1" + other-config: + "a\\n1\\ra": " ; a ; 1 ;a; ;b\\t;\\t3 ;\\ta\\t; 1" + dhcp6: true + eth1: + dhcp4: true + openvswitch: + other-config: + disable-in-band: false\n''', skip_generated_yaml_validation=True) + self.assert_ovs({'ovs0.service': OVS_VIRTUAL % {'iface': 'ovs0', 'extra': ''' +[Service] +Type=oneshot +TimeoutStartSec=10s +ExecStart=/usr/bin/ovs-vsctl --may-exist add-br ovs0 +ExecStart=/usr/bin/ovs-vsctl --may-exist add-port ovs0 eth1 +ExecStart=/usr/bin/ovs-vsctl --may-exist add-port ovs0 eth0 +''' + OVS_BR_DEFAULT % {'iface': 'ovs0'}}, + 'eth0.service': OVS_PHYSICAL % {'iface': 'eth0', 'extra': '''\ +Requires=netplan-ovs-ovs0.service +After=netplan-ovs-ovs0.service + +[Service] +Type=oneshot +TimeoutStartSec=10s +ExecStart=/usr/bin/ovs-vsctl set Interface eth0 external-ids:a\\n1\\ra= \\; a \\; 1 ;a; ;b\\t;\\t3 ;\\ta\\t; 1 +ExecStart=/usr/bin/ovs-vsctl set Interface eth0 external-ids:netplan/external-ids/a\\n1\\ra=,;,a,;,1,;a;,;b\\t;\\t3,;\\ta\\t;,1 +ExecStart=/usr/bin/ovs-vsctl set Interface eth0 other-config:a\\n1\\ra= \\; a \\; 1 ;a; ;b\\t;\\t3 ;\\ta\\t; 1 +ExecStart=/usr/bin/ovs-vsctl set Interface eth0 external-ids:netplan/other-config/a\\n1\\ra=,;,a,;,1,;a;,;b\\t;\\t3,;\\ta\\t;,1 +'''}, + 'eth1.service': OVS_PHYSICAL % {'iface': 'eth1', 'extra': '''\ +Requires=netplan-ovs-ovs0.service +After=netplan-ovs-ovs0.service + +[Service] +Type=oneshot +TimeoutStartSec=10s +ExecStart=/usr/bin/ovs-vsctl set Interface eth1 other-config:disable-in-band=false +ExecStart=/usr/bin/ovs-vsctl set Interface eth1 external-ids:netplan/other-config/disable-in-band=false +'''}, + 'cleanup.service': OVS_CLEANUP % {'iface': 'cleanup'}}) + # Confirm that the networkd config is still sane + self.assert_networkd({'ovs0.network': ND_EMPTY % ('ovs0', 'ipv6'), + 'eth0.network': (ND_DHCP6 % 'eth0') + .replace('LinkLocalAddressing=ipv6', 'LinkLocalAddressing=no\nBridge=ovs0'), + 'eth1.network': (ND_DHCP4 % 'eth1') + .replace('LinkLocalAddressing=ipv6', 'LinkLocalAddressing=no\nBridge=ovs0')}) diff --git a/tests/test_sriov.py b/tests/test_sriov.py index 27fb30180..8b625f3e7 100644 --- a/tests/test_sriov.py +++ b/tests/test_sriov.py @@ -1469,6 +1469,46 @@ def test_rebind_service_generation(self): After=sys-subsystem-net-devices-engreen.device After=sys-subsystem-net-devices-enblue.device +[Service] +Type=oneshot +ExecStart=/usr/sbin/netplan apply --sriov-only +'''}) + + def test_escaping_semicolons_from_unit_file(self): + ''' Check if semicolons and line breaks are properly escaped in the generated + systemd service unit. + ''' + self.generate('''network: + version: 2 + ethernets: + engreen: + embedded-switch-mode: switchdev + delay-virtual-functions-rebind: true + enblue: + match: {driver: dummy_driver} + set-name: ";en ; a\\t;\\tb ;\\tc\\t; d; \\n;\\nabc" + embedded-switch-mode: legacy + delay-virtual-functions-rebind: true + virtual-function-count: 4 + sriov_vf0: + link: engreen''', skip_generated_yaml_validation=True) + self.assert_sriov({'rebind.service': '''[Unit] +Description=(Re-)bind SR-IOV Virtual Functions to their driver +After=network.target +After=netplan-sriov-apply.service +After=sys-subsystem-net-devices-engreen.device +After=sys-subsystem-net-devices-;en \\; a\\t;\\tb ;\\tc\\t; d; \\n;\\nabc.device + +[Service] +Type=oneshot +ExecStart=/usr/sbin/netplan rebind --debug engreen ;en \\; a\\t;\\tb ;\\tc\\t; d; \\n;\\nabc +''', 'apply.service': '''[Unit] +Description=Apply SR-IOV configuration +DefaultDependencies=no +Before=network-pre.target +After=sys-subsystem-net-devices-engreen.device +After=sys-subsystem-net-devices-;en \\; a\\t;\\tb ;\\tc\\t; d; \\n;\\nabc.device + [Service] Type=oneshot ExecStart=/usr/sbin/netplan apply --sriov-only