Skip to content

Commit 409ecc3

Browse files
maipbuimaipbui
authored
Remove subprocess with shell=True (sonic-net#24)
Signed-off-by: maipbui maibui@microsoft.com Why I did it subprocess is used with shell=True, which is very dangerous for shell injection. How I did it remove shell=True, use shell=False How to verify it Pass UT Manual test
1 parent 121330a commit 409ecc3

22 files changed

Lines changed: 554 additions & 467 deletions

‎scripts/caclmgrd‎

100755100644
Lines changed: 125 additions & 117 deletions
Large diffs are not rendered by default.

‎scripts/hostcfgd‎

100755100644
Lines changed: 77 additions & 63 deletions
Large diffs are not rendered by default.

‎scripts/procdockerstatsd‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ from datetime import datetime
1313

1414
from sonic_py_common import daemon_base
1515
from swsscommon import swsscommon
16+
from sonic_py_common.general import getstatusoutput_noshell_pipe
1617

1718
VERSION = '1.0'
1819

@@ -29,7 +30,7 @@ class ProcDockerStats(daemon_base.DaemonBase):
2930
self.state_db.connect("STATE_DB")
3031

3132
def run_command(self, cmd):
32-
proc = subprocess.Popen(cmd, shell=True, universal_newlines=True, stdout=subprocess.PIPE)
33+
proc = subprocess.Popen(cmd, universal_newlines=True, stdout=subprocess.PIPE)
3334
(stdout, stderr) = proc.communicate()
3435
if proc.returncode != 0:
3536
self.log_error("Error running command '{}'".format(cmd))
@@ -118,7 +119,7 @@ class ProcDockerStats(daemon_base.DaemonBase):
118119
return dockerdict
119120

120121
def update_dockerstats_command(self):
121-
cmd = "docker stats --no-stream -a"
122+
cmd = ["docker", "stats", "--no-stream", "-a"]
122123
data = self.run_command(cmd)
123124
if not data:
124125
self.log_error("'{}' returned null output".format(cmd))
@@ -135,7 +136,13 @@ class ProcDockerStats(daemon_base.DaemonBase):
135136
return True
136137

137138
def update_processstats_command(self):
138-
data = self.run_command("ps -eo uid,pid,ppid,%mem,%cpu,stime,tty,time,cmd --sort -%cpu | head -1024")
139+
cmd0 = ["ps", "-eo", "uid,pid,ppid,%mem,%cpu,stime,tty,time,cmd", "--sort", "-%cpu"]
140+
cmd1 = ["head", "-1024"]
141+
cmd = ' | '.join([' '.join(cmd0), ' '.join(cmd1)])
142+
exitcode, data = getstatusoutput_noshell_pipe(cmd0, cmd1)
143+
if any(exitcode):
144+
self.log_error("Error running command '{}'".format(cmd))
145+
data = None
139146
processdata = self.format_process_cmd_output(data)
140147
value = ""
141148
# wipe out all data before updating with new values

‎tests/caclmgrd/caclmgrd_bfd_test.py‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -34,17 +34,18 @@ def test_caclmgrd_bfd(self, test_name, test_data, fs):
3434

3535
MockConfigDb.set_config_db(test_data["config_db"])
3636

37-
with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
38-
popen_mock = mock.Mock()
39-
popen_attrs = test_data["popen_attributes"]
40-
popen_mock.configure_mock(**popen_attrs)
41-
mocked_subprocess.Popen.return_value = popen_mock
42-
mocked_subprocess.PIPE = -1
43-
44-
call_rc = test_data["call_rc"]
45-
mocked_subprocess.call.return_value = call_rc
46-
47-
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
48-
caclmgrd_daemon.allow_bfd_protocol('')
49-
mocked_subprocess.Popen.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True)
37+
with mock.patch("caclmgrd.ControlPlaneAclManager.run_commands_pipe", return_value='sonic'):
38+
with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
39+
popen_mock = mock.Mock()
40+
popen_attrs = test_data["popen_attributes"]
41+
popen_mock.configure_mock(**popen_attrs)
42+
mocked_subprocess.Popen.return_value = popen_mock
43+
mocked_subprocess.PIPE = -1
44+
45+
call_rc = test_data["call_rc"]
46+
mocked_subprocess.call.return_value = call_rc
47+
48+
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
49+
caclmgrd_daemon.allow_bfd_protocol('')
50+
mocked_subprocess.Popen.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True)
5051

‎tests/caclmgrd/caclmgrd_dhcp_test.py‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -33,21 +33,22 @@ def test_caclmgrd_dhcp(self, test_name, test_data, fs):
3333

3434
MockConfigDb.set_config_db(test_data["config_db"])
3535

36-
with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
37-
popen_mock = mock.Mock()
38-
popen_attrs = test_data["popen_attributes"]
39-
popen_mock.configure_mock(**popen_attrs)
40-
mocked_subprocess.Popen.return_value = popen_mock
36+
with mock.patch("caclmgrd.ControlPlaneAclManager.run_commands_pipe", return_value='sonic'):
37+
with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
38+
popen_mock = mock.Mock()
39+
popen_attrs = test_data["popen_attributes"]
40+
popen_mock.configure_mock(**popen_attrs)
41+
mocked_subprocess.Popen.return_value = popen_mock
4142

42-
call_rc = test_data["call_rc"]
43-
mocked_subprocess.call.return_value = call_rc
43+
call_rc = test_data["call_rc"]
44+
mocked_subprocess.call.return_value = call_rc
4445

45-
mark = test_data["mark"]
46+
mark = test_data["mark"]
4647

47-
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
48-
mux_update = test_data["mux_update"]
48+
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
49+
mux_update = test_data["mux_update"]
4950

50-
for key,data in mux_update:
51-
caclmgrd_daemon.update_dhcp_acl(key, '', data, mark)
51+
for key,data in mux_update:
52+
caclmgrd_daemon.update_dhcp_acl(key, '', data, mark)
5253

53-
mocked_subprocess.call.assert_has_calls(test_data["expected_subprocess_calls"], any_order=False)
54+
mocked_subprocess.call.assert_has_calls(test_data["expected_subprocess_calls"], any_order=False)

‎tests/caclmgrd/caclmgrd_external_client_acl_test.py‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,10 @@ def test_caclmgrd_external_client_acl(self, test_name, test_data, fs):
4141
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
4242

4343
iptables_rules_ret, _ = caclmgrd_daemon.get_acl_rules_and_translate_to_iptables_commands('')
44+
test_data['return'] = [tuple(i) for i in test_data['return']]
45+
iptables_rules_ret = [tuple(i) for i in iptables_rules_ret]
4446
self.assertEqual(set(test_data["return"]).issubset(set(iptables_rules_ret)), True)
45-
caclmgrd_daemon.iptables_cmd_ns_prefix['asic0'] = 'ip netns exec asic0'
47+
caclmgrd_daemon.iptables_cmd_ns_prefix['asic0'] = ['ip', 'netns', 'exec', 'asic0']
4648
caclmgrd_daemon.namespace_docker_mgmt_ip['asic0'] = '1.1.1.1'
4749
caclmgrd_daemon.namespace_mgmt_ip = '2.2.2.2'
4850
caclmgrd_daemon.namespace_docker_mgmt_ipv6['asic0'] = 'fd::01'

‎tests/caclmgrd/caclmgrd_feature_test.py‎

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -34,17 +34,18 @@ def test_feature_present(self, test_name, test_data, fs):
3434

3535
MockConfigDb.set_config_db(test_data["config_db"])
3636

37-
with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
38-
popen_mock = mock.Mock()
39-
popen_attrs = test_data["popen_attributes"]
40-
popen_mock.configure_mock(**popen_attrs)
41-
mocked_subprocess.Popen.return_value = popen_mock
42-
mocked_subprocess.PIPE = -1
43-
44-
call_rc = test_data["call_rc"]
45-
mocked_subprocess.call.return_value = call_rc
46-
47-
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
48-
caclmgrd_daemon.update_feature_present()
49-
self.assertTrue("bgp" in caclmgrd_daemon.feature_present)
50-
self.assertEqual(caclmgrd_daemon.feature_present["bgp"], True)
37+
with mock.patch("caclmgrd.ControlPlaneAclManager.run_commands_pipe", return_value='sonic'):
38+
with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
39+
popen_mock = mock.Mock()
40+
popen_attrs = test_data["popen_attributes"]
41+
popen_mock.configure_mock(**popen_attrs)
42+
mocked_subprocess.Popen.return_value = popen_mock
43+
mocked_subprocess.PIPE = -1
44+
45+
call_rc = test_data["call_rc"]
46+
mocked_subprocess.call.return_value = call_rc
47+
48+
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
49+
caclmgrd_daemon.update_feature_present()
50+
self.assertTrue("bgp" in caclmgrd_daemon.feature_present)
51+
self.assertEqual(caclmgrd_daemon.feature_present["bgp"], True)

‎tests/caclmgrd/caclmgrd_soc_rules_test.py‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -36,19 +36,20 @@ def test_caclmgrd_soc(self, test_name, test_data, fs):
3636

3737
MockConfigDb.set_config_db(test_data["config_db"])
3838

39-
with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
40-
popen_mock = mock.Mock()
41-
popen_attrs = test_data["popen_attributes"]
42-
popen_mock.configure_mock(**popen_attrs)
43-
mocked_subprocess.Popen.return_value = popen_mock
44-
mocked_subprocess.PIPE = -1
45-
46-
call_rc = test_data["call_rc"]
47-
mocked_subprocess.call.return_value = call_rc
48-
49-
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
50-
caclmgrd_daemon.update_control_plane_nat_acls('', {})
51-
mocked_subprocess.Popen.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True)
39+
with mock.patch("caclmgrd.ControlPlaneAclManager.run_commands_pipe", return_value='sonic'):
40+
with mock.patch("caclmgrd.subprocess") as mocked_subprocess:
41+
popen_mock = mock.Mock()
42+
popen_attrs = test_data["popen_attributes"]
43+
popen_mock.configure_mock(**popen_attrs)
44+
mocked_subprocess.Popen.return_value = popen_mock
45+
mocked_subprocess.PIPE = -1
46+
47+
call_rc = test_data["call_rc"]
48+
mocked_subprocess.call.return_value = call_rc
49+
50+
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
51+
caclmgrd_daemon.update_control_plane_nat_acls('', {})
52+
mocked_subprocess.Popen.assert_has_calls(test_data["expected_subprocess_calls"], any_order=True)
5253

5354
def test_get_ip_from_interface_table(self):
5455
if not os.path.exists(DBCONFIG_PATH):

‎tests/caclmgrd/caclmgrd_test.py‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
import os
2+
import sys
3+
import swsscommon
4+
from unittest.mock import call
5+
from unittest import TestCase, mock
6+
from tests.common.mock_configdb import MockConfigDb
7+
from sonic_py_common.general import load_module_from_source
8+
9+
class TestCaclmgrd(TestCase):
10+
def setUp(self):
11+
swsscommon.swsscommon.ConfigDBConnector = MockConfigDb
12+
test_path = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
13+
modules_path = os.path.dirname(test_path)
14+
scripts_path = os.path.join(modules_path, "scripts")
15+
sys.path.insert(0, modules_path)
16+
caclmgrd_path = os.path.join(scripts_path, 'caclmgrd')
17+
self.caclmgrd = load_module_from_source('caclmgrd', caclmgrd_path)
18+
19+
def test_run_commands_pipe(self):
20+
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
21+
output = caclmgrd_daemon.run_commands_pipe(['echo', 'caclmgrd'], ['awk', '{print $1}'])
22+
assert output == 'caclmgrd'
23+
24+
output = caclmgrd_daemon.run_commands_pipe([sys.executable, "-c", "import sys; sys.exit(6)"], [sys.executable, "-c", "import sys; sys.exit(8)"])
25+
assert output == ''
26+
27+
def test_get_chain_list(self):
28+
expected_calls = [call(['iptables', '-L', '-v', '-n'], ['grep', 'Chain'], ['awk', '{print $2}'])]
29+
caclmgrd_daemon = self.caclmgrd.ControlPlaneAclManager("caclmgrd")
30+
with mock.patch("caclmgrd.ControlPlaneAclManager.run_commands_pipe") as mock_run_commands_pipe:
31+
caclmgrd_daemon.get_chain_list([], [''])
32+
mock_run_commands_pipe.assert_has_calls(expected_calls)
33+

‎tests/caclmgrd/test_bfd_vectors.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,8 @@
2323
},
2424
},
2525
"expected_subprocess_calls": [
26-
call("iptables -I INPUT 2 -p udp -m multiport --dports 3784,4784 -j ACCEPT", shell=True, universal_newlines=True, stdout=subprocess.PIPE),
27-
call("ip6tables -I INPUT 2 -p udp -m multiport --dports 3784,4784 -j ACCEPT", shell=True, universal_newlines=True, stdout=subprocess.PIPE)
26+
call(['iptables', '-I', 'INPUT', '2', '-p', 'udp', '-m', 'multiport', '--dports', '3784,4784', '-j', 'ACCEPT'], universal_newlines=True, stdout=subprocess.PIPE),
27+
call(['ip6tables', '-I', 'INPUT', '2', '-p', 'udp', '-m', 'multiport', '--dports', '3784,4784', '-j', 'ACCEPT'], universal_newlines=True, stdout=subprocess.PIPE)
2828
],
2929
"popen_attributes": {
3030
'communicate.return_value': ('output', 'error'),

0 commit comments

Comments
 (0)