From 45bf7b6ffc29b0df6bd43b74e376398c48e084d3 Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Sat, 20 Jun 2026 13:04:07 +0200 Subject: ifconfig: T9008: refactor vyos.ifconfig to use cmdl() for safer subprocess execution Replace all cmd() calls in the vyos.ifconfig library with cmdl(), passing arguments as lists instead of interpolated strings. This eliminates a class of command-injection risks that arise when building shell commands with f-strings or other string formatting. --- python/vyos/ifconfig/bridge.py | 21 +++++++-------------- python/vyos/ifconfig/control.py | 14 ++++++++++++++ python/vyos/ifconfig/interface.py | 19 ++++++++----------- python/vyos/ifconfig/vxlan.py | 10 +++++----- python/vyos/ifconfig/wireguard.py | 4 ++-- 5 files changed, 36 insertions(+), 32 deletions(-) (limited to 'python') diff --git a/python/vyos/ifconfig/bridge.py b/python/vyos/ifconfig/bridge.py index f1cc7c0e0..b608f1e05 100644 --- a/python/vyos/ifconfig/bridge.py +++ b/python/vyos/ifconfig/bridge.py @@ -193,8 +193,7 @@ class BridgeIf(Interface): # VLAN of bridge parent interface is always 1 # VLAN 1 is the default VLAN for all unlabeled packets - cmd = f'bridge vlan add dev {self.ifname} vid 1 pvid untagged self' - self._cmd(cmd) + self._cmdl(['bridge', 'vlan', 'add', 'dev', self.ifname, 'vid', '1', 'pvid', 'untagged', 'self']) def set_multicast_querier(self, enable): """ @@ -329,17 +328,14 @@ class BridgeIf(Interface): if 'enable_vlan' in config: for vlan in config.get('vif_remove', {}): # Remove old VLANs from the bridge - cmd = f'bridge vlan del dev {self.ifname} vid {vlan} self' - self._cmd(cmd) + self._cmdl(['bridge', 'vlan', 'del', 'dev', self.ifname, 'vid', str(vlan), 'self']) for vlan in config.get('vif', {}): - cmd = f'bridge vlan add dev {self.ifname} vid {vlan} self' - self._cmd(cmd) + self._cmdl(['bridge', 'vlan', 'add', 'dev', self.ifname, 'vid', str(vlan), 'self']) # VLAN of bridge parent interface is always 1. VLAN 1 is the default # VLAN for all unlabeled packets - cmd = f'bridge vlan add dev {self.ifname} vid 1 pvid untagged self' - self._cmd(cmd) + self._cmdl(['bridge', 'vlan', 'add', 'dev', self.ifname, 'vid', '1', 'pvid', 'untagged', 'self']) tmp = dict_search('member.interface', config) if tmp: @@ -409,16 +405,13 @@ class BridgeIf(Interface): # Remove redundant VLANs from the system for vlan in list_diff(cur_vlan_ids, add_vlan): - cmd = f'bridge vlan del dev {interface} vid {vlan} master' - self._cmd(cmd) + self._cmdl(['bridge', 'vlan', 'del', 'dev', interface, 'vid', str(vlan), 'master']) for vlan in allowed_vlan_ids: - cmd = f'bridge vlan add dev {interface} vid {vlan} master' - self._cmd(cmd) + self._cmdl(['bridge', 'vlan', 'add', 'dev', interface, 'vid', str(vlan), 'master']) # Setting native VLAN to system if native_vlan_id: - cmd = f'bridge vlan add dev {interface} vid {native_vlan_id} pvid untagged master' - self._cmd(cmd) + self._cmdl(['bridge', 'vlan', 'add', 'dev', interface, 'vid', str(native_vlan_id), 'pvid', 'untagged', 'master']) super().update(config) diff --git a/python/vyos/ifconfig/control.py b/python/vyos/ifconfig/control.py index 1ef42f981..8d307ac4a 100644 --- a/python/vyos/ifconfig/control.py +++ b/python/vyos/ifconfig/control.py @@ -65,6 +65,20 @@ class Control(Section): command = f'ip netns exec {self.config["netns"]} {command}' return cmd(command, self.debug, env=env) + def _cmdl(self, command, env=None): + from vyos.utils.process import cmdl + if not isinstance(command, list): + raise TypeError(f'_cmdl() requires a list, got {type(command).__name__}') + netns = self.config.get('netns') + vrf = self.config.get('vrf') + # 'ip link set dev netns ' moves the interface into the + # namespace and must execute from the default netns — skip wrapping. + if (netns and command[:4] == ['ip', 'link', 'set', 'dev'] + and 'netns' in command + and command[command.index('netns') + 1] == netns): + netns = None + return cmdl(command, self.debug, env=env, netns=netns, vrf=vrf) + def _get_command(self, config, name): """ Using the defined names, set data write to sysfs. diff --git a/python/vyos/ifconfig/interface.py b/python/vyos/ifconfig/interface.py index 9d6b38fa9..4846ddfa7 100644 --- a/python/vyos/ifconfig/interface.py +++ b/python/vyos/ifconfig/interface.py @@ -372,10 +372,9 @@ class Interface(Control): if self.exists(f'{self.ifname}', netns=netns): return - cmd = f'ip link add dev {self.ifname}' - if type: cmd += f' type {type}' - if 'netns' in self.config: cmd = f'ip netns exec {netns} {cmd}' - self._cmd(cmd) + cmd = ['ip', 'link', 'add', 'dev', self.ifname] + if type: cmd += ['type', type] + self._cmdl(cmd) def remove(self, skip_delete=False): """ @@ -602,7 +601,7 @@ class Interface(Control): >>> from vyos.ifconfig import Interface >>> Interface('dum0').set_netns('foo') """ - self._cmd(f'ip link set dev {self.ifname} netns {netns}') + self._cmdl(['ip', 'link', 'set', 'dev', self.ifname, 'netns', netns]) return True def get_vrf(self): @@ -1310,11 +1309,10 @@ class Interface(Control): elif addr == 'dhcpv6': self.set_dhcpv6(True, vrf_changed=vrf_changed) elif not is_intf_addr_assigned(self.ifname, addr, netns=netns): - netns_cmd = f'ip netns exec {netns}' if netns else '' - tmp = f'{netns_cmd} ip addr add {addr} dev {self.ifname}' + tmp = ['ip', 'addr', 'add', addr, 'dev', self.ifname] # Add broadcast address for IPv4 - if is_ipv4(addr): tmp += ' brd +' - self._cmd(tmp) + if is_ipv4(addr): tmp += ['brd', '+'] + self._cmdl(tmp) else: return False @@ -1359,8 +1357,7 @@ class Interface(Control): elif addr == 'dhcpv6': self.set_dhcpv6(False) elif is_intf_addr_assigned(self.ifname, addr, netns=netns): - netns_cmd = f'ip netns exec {netns}' if netns else '' - self._cmd(f'{netns_cmd} ip addr del {addr} dev {self.ifname}') + self._cmdl(['ip', 'addr', 'del', addr, 'dev', self.ifname]) else: return False diff --git a/python/vyos/ifconfig/vxlan.py b/python/vyos/ifconfig/vxlan.py index a72cb6201..996a5c8f8 100644 --- a/python/vyos/ifconfig/vxlan.py +++ b/python/vyos/ifconfig/vxlan.py @@ -158,8 +158,8 @@ class VXLANIf(Interface): if cur_vni_filter != None: vni = vlan_config['vni'] if vni in cur_vni_filter: - self._cmd(f'bridge vni delete dev {self.ifname} vni {vni}') - self._cmd(f'bridge vlan del dev {self.ifname} vid {vlan}') + self._cmdl(['bridge', 'vni', 'delete', 'dev', self.ifname, 'vni', str(vni)]) + self._cmdl(['bridge', 'vlan', 'del', 'dev', self.ifname, 'vid', str(vlan)]) # Determine current OS Kernel vlan_tunnel setting - only adjust when needed tmp = get_interface_config(self.ifname) @@ -182,12 +182,12 @@ class VXLANIf(Interface): vni = vlan_config['vni'] # The following commands must be run one after another, # they cannot be combined with linux 6.1 and iproute2 6.1 - self._cmd(f'bridge vlan add dev {self.ifname} vid {vlan}') - self._cmd(f'bridge vlan add dev {self.ifname} vid {vlan} tunnel_info id {vni}') + self._cmdl(['bridge', 'vlan', 'add', 'dev', self.ifname, 'vid', str(vlan)]) + self._cmdl(['bridge', 'vlan', 'add', 'dev', self.ifname, 'vid', str(vlan), 'tunnel_info', 'id', str(vni)]) # If VNI filtering is enabled, install matching VNI filter if dict_search('parameters.vni_filter', self.config) != None: - self._cmd(f'bridge vni add dev {self.ifname} vni {vni}') + self._cmdl(['bridge', 'vni', 'add', 'dev', self.ifname, 'vni', str(vni)]) def update(self, config): """ General helper function which works on a dictionary retrieved by diff --git a/python/vyos/ifconfig/wireguard.py b/python/vyos/ifconfig/wireguard.py index fb576bac0..94de0bb6b 100644 --- a/python/vyos/ifconfig/wireguard.py +++ b/python/vyos/ifconfig/wireguard.py @@ -36,7 +36,7 @@ class WireGuardOperational(Operational): output = {} # Dump wireguard connection data - _f = self._cmd('wg show all dump') + _f = self._cmdl(['wg', 'show', 'all', 'dump']) for line in _f.split('\n'): if not line: # Skip empty lines and last line @@ -166,7 +166,7 @@ class WireGuardOperational(Operational): output = {} # Dump wireguard last handshake - tmp = self._cmd(f'wg show {self.ifname} latest-handshakes') + tmp = self._cmdl(['wg', 'show', self.ifname, 'latest-handshakes']) # Output: # PUBLIC-KEY= 1732812147 for line in tmp.split('\n'): -- cgit v1.2.3