Skip to content

Commit 88a7daa

Browse files
authored
[show][barefoot] replace shell=True (sonic-net#2699)
#### 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)
1 parent 5e99edb commit 88a7daa

File tree

2 files changed

+63
-7
lines changed

2 files changed

+63
-7
lines changed

show/plugins/barefoot.py

+10-7
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import json
55
import subprocess
66
from sonic_py_common import device_info
7+
from sonic_py_common.general import getstatusoutput_noshell_pipe
78

89
@click.group()
910
def barefoot():
@@ -25,21 +26,23 @@ def profile():
2526

2627
# Print current profile
2728
click.echo('Current profile: ', nl=False)
28-
subprocess.run('docker exec -it syncd readlink /opt/bfn/install | sed '
29-
r's/install_\\\(.\*\\\)_profile/\\1/', check=True, shell=True)
29+
cmd0 = ['docker', 'exec', '-it', 'syncd', 'readlink', '/opt/bfn/install']
30+
cmd1 = ['sed', r's/install_\\\(.\*\\\)_profile/\\1/']
31+
getstatusoutput_noshell_pipe(cmd0, cmd1)
3032

3133
# Exclude current and unsupported profiles
3234
opts = ''
3335
if chip_family == 'tofino':
34-
opts = r'\! -name install_y\*_profile '
36+
opts = r'\! -name install_y\*_profile'
3537
elif chip_family == 'tofino2':
36-
opts = r'\! -name install_x\*_profile '
38+
opts = r'\! -name install_x\*_profile'
3739

3840
# Print profile list
3941
click.echo('Available profile(s):')
40-
subprocess.run('docker exec -it syncd find /opt/bfn -mindepth 1 '
41-
r'-maxdepth 1 -type d -name install_\*_profile ' + opts + '| sed '
42-
r's%/opt/bfn/install_\\\(.\*\\\)_profile%\\1%', shell=True)
42+
cmd0 = ['docker', 'exec', '-it', 'syncd', 'find', '/opt/bfn', '-mindepth', '1',\
43+
'-maxdepth', '1', '-type', 'd', '-name', r'install_\*_profile', opts]
44+
cmd1 = ["sed", r's%/opt/bfn/install_\\\(.\*\\\)_profile%\\1%']
45+
getstatusoutput_noshell_pipe(cmd0, cmd1)
4346

4447
def register(cli):
4548
version_info = device_info.get_sonic_version_info()

tests/show_barefoot_test.py

+53
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
import json
2+
import click
3+
import pytest
4+
from click.testing import CliRunner
5+
import show.plugins.barefoot as show
6+
from unittest.mock import call, patch, mock_open, MagicMock
7+
8+
9+
class TestShowBarefoot(object):
10+
def setup(self):
11+
print('SETUP')
12+
13+
@patch('subprocess.run')
14+
def test_default_profile(self, mock_run):
15+
mock_run.return_value.returncode = 1
16+
runner = CliRunner()
17+
result = runner.invoke(show.profile, [])
18+
assert result.exit_code == 0
19+
assert result.output == 'Current profile: default\n'
20+
mock_run.assert_called_once_with(['docker', 'exec', '-it', 'syncd', 'test', '-h', '/opt/bfn/install'])
21+
22+
@patch('show.plugins.barefoot.getstatusoutput_noshell_pipe')
23+
@patch('show.plugins.barefoot.device_info.get_path_to_hwsku_dir', MagicMock(return_value='/usr/share/sonic/hwsku_dir'))
24+
@patch('subprocess.run')
25+
def test_nondefault_profile(self, mock_run, mock_cmd):
26+
mock_run.return_value.returncode = 0
27+
chip_list = [{'chip_family': 'TOFINO'}]
28+
mock_open_args = mock_open(read_data=json.dumps({'chip_list': chip_list}))
29+
expected_calls = [
30+
call(
31+
['docker', 'exec', '-it', 'syncd', 'readlink', '/opt/bfn/install'],
32+
['sed', 's/install_\\\\\\(.\\*\\\\\\)_profile/\\\\1/']
33+
),
34+
35+
call(
36+
['docker', 'exec', '-it', 'syncd', 'find', '/opt/bfn', '-mindepth', '1',\
37+
'-maxdepth', '1', '-type', 'd', '-name', r'install_\*_profile', r'\! -name install_y\*_profile'],
38+
["sed", r's%/opt/bfn/install_\\\(.\*\\\)_profile%\\1%']
39+
)
40+
]
41+
42+
with patch("builtins.open", mock_open_args) as mock_open_file:
43+
runner = CliRunner()
44+
result = runner.invoke(show.profile)
45+
assert result.exit_code == 0
46+
47+
mock_run.assert_called_once_with(['docker', 'exec', '-it', 'syncd', 'test', '-h', '/opt/bfn/install'])
48+
mock_open_file.assert_called_once_with('/usr/share/sonic/hwsku_dir/switch-tna-sai.conf')
49+
assert mock_cmd.call_args_list == expected_calls
50+
51+
def teardown(self):
52+
print('TEARDOWN')
53+

0 commit comments

Comments
 (0)