Skip to content

Commit

Permalink
[show][barefoot] replace shell=True (#2699)
Browse files Browse the repository at this point in the history
#### 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)
  • Loading branch information
maipbui authored Apr 20, 2023
1 parent 5e99edb commit 88a7daa
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 7 deletions.
17 changes: 10 additions & 7 deletions show/plugins/barefoot.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import json
import subprocess
from sonic_py_common import device_info
from sonic_py_common.general import getstatusoutput_noshell_pipe

@click.group()
def barefoot():
Expand All @@ -25,21 +26,23 @@ def profile():

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

# Exclude current and unsupported profiles
opts = ''
if chip_family == 'tofino':
opts = r'\! -name install_y\*_profile '
opts = r'\! -name install_y\*_profile'
elif chip_family == 'tofino2':
opts = r'\! -name install_x\*_profile '
opts = r'\! -name install_x\*_profile'

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

def register(cli):
version_info = device_info.get_sonic_version_info()
Expand Down
53 changes: 53 additions & 0 deletions tests/show_barefoot_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import json
import click
import pytest
from click.testing import CliRunner
import show.plugins.barefoot as show
from unittest.mock import call, patch, mock_open, MagicMock


class TestShowBarefoot(object):
def setup(self):
print('SETUP')

@patch('subprocess.run')
def test_default_profile(self, mock_run):
mock_run.return_value.returncode = 1
runner = CliRunner()
result = runner.invoke(show.profile, [])
assert result.exit_code == 0
assert result.output == 'Current profile: default\n'
mock_run.assert_called_once_with(['docker', 'exec', '-it', 'syncd', 'test', '-h', '/opt/bfn/install'])

@patch('show.plugins.barefoot.getstatusoutput_noshell_pipe')
@patch('show.plugins.barefoot.device_info.get_path_to_hwsku_dir', MagicMock(return_value='/usr/share/sonic/hwsku_dir'))
@patch('subprocess.run')
def test_nondefault_profile(self, mock_run, mock_cmd):
mock_run.return_value.returncode = 0
chip_list = [{'chip_family': 'TOFINO'}]
mock_open_args = mock_open(read_data=json.dumps({'chip_list': chip_list}))
expected_calls = [
call(
['docker', 'exec', '-it', 'syncd', 'readlink', '/opt/bfn/install'],
['sed', 's/install_\\\\\\(.\\*\\\\\\)_profile/\\\\1/']
),

call(
['docker', 'exec', '-it', 'syncd', 'find', '/opt/bfn', '-mindepth', '1',\
'-maxdepth', '1', '-type', 'd', '-name', r'install_\*_profile', r'\! -name install_y\*_profile'],
["sed", r's%/opt/bfn/install_\\\(.\*\\\)_profile%\\1%']
)
]

with patch("builtins.open", mock_open_args) as mock_open_file:
runner = CliRunner()
result = runner.invoke(show.profile)
assert result.exit_code == 0

mock_run.assert_called_once_with(['docker', 'exec', '-it', 'syncd', 'test', '-h', '/opt/bfn/install'])
mock_open_file.assert_called_once_with('/usr/share/sonic/hwsku_dir/switch-tna-sai.conf')
assert mock_cmd.call_args_list == expected_calls

def teardown(self):
print('TEARDOWN')

0 comments on commit 88a7daa

Please sign in to comment.