Skip to content

Commit 5e99edb

Browse files
authored
[sonic_package_manager] replace shell=True (sonic-net#2726)
#### What I did `subprocess()` - when using with `shell=True` is dangerous. Using subprocess function without a static string can lead to command injection. #### How I did it `subprocess()` - use `shell=False` instead, use list of strings Ref: [https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation](https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation) #### How to verify it Pass UT
1 parent b547bb4 commit 5e99edb

File tree

2 files changed

+33
-10
lines changed

2 files changed

+33
-10
lines changed

sonic_package_manager/service_creator/creator.py

+10-9
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,11 @@
22

33
import contextlib
44
import os
5+
import sys
56
import stat
67
import subprocess
78
from collections import defaultdict
8-
from typing import Dict, Type
9+
from typing import Dict, Type, List
910

1011
import jinja2 as jinja2
1112
from config.config_mgmt import ConfigMgmt
@@ -96,22 +97,22 @@ def remove_if_exists(path):
9697
os.remove(path)
9798
log.info(f'removed {path}')
9899

100+
def is_list_of_strings(command):
101+
return isinstance(command, list) and all(isinstance(item, str) for item in command)
99102

100-
def run_command(command: str):
103+
def run_command(command: List[str]):
101104
""" Run arbitrary bash command.
102105
Args:
103-
command: String command to execute as bash script
106+
command: List of Strings command to execute as bash script
104107
Raises:
105108
ServiceCreatorError: Raised when the command return code
106109
is not 0.
107110
"""
108-
111+
if not is_list_of_strings(command):
112+
sys.exit("Input command should be a list of strings")
109113
log.debug(f'running command: {command}')
110114

111-
proc = subprocess.Popen(command,
112-
shell=True,
113-
executable='/bin/bash',
114-
stdout=subprocess.PIPE)
115+
proc = subprocess.Popen(command, stdout=subprocess.PIPE)
115116
(_, _) = proc.communicate()
116117
if proc.returncode != 0:
117118
raise ServiceCreatorError(f'Failed to execute "{command}"')
@@ -647,4 +648,4 @@ def _post_operation_hook(self):
647648
""" Common operations executed after service is created/removed. """
648649

649650
if not in_chroot():
650-
run_command('systemctl daemon-reload')
651+
run_command(['systemctl', 'daemon-reload'])

tests/sonic_package_manager/test_service_creator.py

+23-1
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
#!/usr/bin/env python
22

33
import os
4+
import sys
45
import copy
5-
from unittest.mock import Mock, MagicMock, call
6+
from unittest.mock import Mock, MagicMock, call, patch
67

78
import pytest
89

@@ -62,6 +63,22 @@ def manifest():
6263
]
6364
})
6465

66+
def test_is_list_of_strings():
67+
output = is_list_of_strings(['a', 'b', 'c'])
68+
assert output is True
69+
70+
output = is_list_of_strings('abc')
71+
assert output is False
72+
73+
output = is_list_of_strings(['a', 'b', 1])
74+
assert output is False
75+
76+
def test_run_command():
77+
with pytest.raises(SystemExit) as e:
78+
run_command('echo 1')
79+
80+
with pytest.raises(ServiceCreatorError) as e:
81+
run_command([sys.executable, "-c", "import sys; sys.exit(6)"])
6582

6683
@pytest.fixture()
6784
def service_creator(mock_feature_registry,
@@ -203,6 +220,11 @@ def test_service_creator_autocli(sonic_fs, manifest, mock_cli_gen,
203220
any_order=True
204221
)
205222

223+
def test_service_creator_post_operation_hook(sonic_fs, manifest, mock_sonic_db, mock_config_mgmt, service_creator):
224+
with patch('sonic_package_manager.service_creator.creator.run_command') as run_command:
225+
with patch('sonic_package_manager.service_creator.creator.in_chroot', MagicMock(return_value=False)):
226+
service_creator._post_operation_hook()
227+
run_command.assert_called_with(['systemctl', 'daemon-reload'])
206228

207229
def test_feature_registration(mock_sonic_db, manifest):
208230
mock_connector = Mock()

0 commit comments

Comments
 (0)