[show][mlnx] replace shell=True, replace xml (#2700)

Signed-off-by: maipbui <[email protected]>
#### 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
Add UT
Manual test
```
admin@***:~$ show platform mlnx issu
ISSU is enabled
admin@***:~$ show platform mlnx sniffer
sdk sniffer is disabled
```

Author: maipbui

show/plugins/mlnx.py

@@ -26,7 +26,8 @@
     import sys
     import subprocess
     import click
-    import xml.etree.ElementTree as ET
+    from shlex import join
+    from lxml import etree as ET
     from sonic_py_common import device_info
 except ImportError as e:
     raise ImportError("%s - required module not found" % str(e))
@@ -46,9 +47,9 @@ def run_command(command, display_cmd=False, ignore_error=False, print_to_console
     """Run bash command and print output to stdout
     """
     if display_cmd == True:
-        click.echo(click.style("Running command: ", fg='cyan') + click.style(command, fg='green'))
+        click.echo(click.style("Running command: ", fg='cyan') + click.style(join(command), fg='green'))
 
-    proc = subprocess.Popen(command, shell=True, text=True, stdout=subprocess.PIPE)
+    proc = subprocess.Popen(command, text=True, stdout=subprocess.PIPE)
     (out, err) = proc.communicate()
 
     if len(out) > 0 and print_to_console:
@@ -70,17 +71,17 @@ def mlnx():
 # get current status of sniffer from conf file
 def sniffer_status_get(env_variable_name):
     enabled = False
-    command = "docker exec {} bash -c 'touch {}'".format(CONTAINER_NAME, SNIFFER_CONF_FILE)
+    command = ["docker", "exec", CONTAINER_NAME, "bash", "-c", 'touch {}'.format(SNIFFER_CONF_FILE)]
     run_command(command)
-    command = 'docker cp {} {}'.format(SNIFFER_CONF_FILE_IN_CONTAINER, TMP_SNIFFER_CONF_FILE)
+    command = ['docker', 'cp', SNIFFER_CONF_FILE_IN_CONTAINER, TMP_SNIFFER_CONF_FILE]
     run_command(command)
     conf_file = open(TMP_SNIFFER_CONF_FILE, 'r')
     for env_variable_string in conf_file:
         if env_variable_string.find(env_variable_name) >= 0:
             enabled = True
             break
     conf_file.close()
-    command = 'rm -rf {}'.format(TMP_SNIFFER_CONF_FILE)
+    command = ['rm', '-rf', TMP_SNIFFER_CONF_FILE]
     run_command(command)
     return enabled
 
@@ -97,10 +98,8 @@ def is_issu_status_enabled():
     # Get the SAI XML path from sai.profile
     sai_profile_path = '/{}/sai.profile'.format(HWSKU_PATH)
 
-    DOCKER_CAT_COMMAND = 'docker exec {container_name} cat {path}'
-
-    command = DOCKER_CAT_COMMAND.format(container_name=CONTAINER_NAME, path=sai_profile_path)
-    sai_profile_content, _ = run_command(command, print_to_console=False)
+    DOCKER_CAT_COMMAND = ['docker', 'exec', CONTAINER_NAME, 'cat', sai_profile_path]
+    sai_profile_content, _ = run_command(DOCKER_CAT_COMMAND, print_to_console=False)
 
     sai_profile_kvs = {}
 
@@ -117,8 +116,8 @@ def is_issu_status_enabled():
         sys.exit(1)
 
     # Get ISSU from SAI XML
-    command = DOCKER_CAT_COMMAND.format(container_name=CONTAINER_NAME, path=sai_xml_path)
-    sai_xml_content, _ = run_command(command, print_to_console=False)
+    DOCKER_CAT_COMMAND = ['docker', 'exec', CONTAINER_NAME, 'cat', sai_xml_path]
+    sai_xml_content, _ = run_command(DOCKER_CAT_COMMAND, print_to_console=False)
 
     try:
         root = ET.fromstring(sai_xml_content)

tests/show_mlnx_test.py

@@ -0,0 +1,63 @@
+import sys
+import click
+import pytest
+import show.plugins.mlnx as show
+from unittest.mock import call, patch, mock_open, MagicMock
+
+
+class TestShowMlnx(object):
+    def setup(self):
+        print('SETUP')
+
+    @patch('click.style')
+    def test_run_command(self, mock_click):
+        cmd0 = ['echo', 'test']
+        out, err = show.run_command(cmd0, display_cmd=True)
+
+        assert mock_click.call_args_list == [call('Running command: ', fg='cyan'), call(' '.join(cmd0), fg='green')]
+        assert out == 'test\n'
+
+        cmd1 = [sys.executable, "-c", "import sys; sys.exit(6)"]
+        with pytest.raises(SystemExit) as e:
+            show.run_command(cmd1)
+        assert e.value.code == 6
+
+    @patch('builtins.open', mock_open(read_data=show.ENV_VARIABLE_SX_SNIFFER))
+    @patch('show.plugins.mlnx.run_command')
+    def test_sniffer_status_get_enable(self, mock_runcmd):
+        expected_calls = [
+            call(["docker", "exec", show.CONTAINER_NAME, "bash", "-c", 'touch {}'.format(show.SNIFFER_CONF_FILE)]),
+            call(['docker', 'cp', show.SNIFFER_CONF_FILE_IN_CONTAINER, show.TMP_SNIFFER_CONF_FILE]),
+            call(['rm', '-rf', show.TMP_SNIFFER_CONF_FILE])
+        ]
+
+        output = show.sniffer_status_get(show.ENV_VARIABLE_SX_SNIFFER)
+        assert mock_runcmd.call_args_list == expected_calls
+        assert output
+
+    @patch('builtins.open', mock_open(read_data='not_enable'))
+    @patch('show.plugins.mlnx.run_command')
+    def test_sniffer_status_get_disable(self, mock_runcmd):
+        expected_calls = [
+            call(["docker", "exec", show.CONTAINER_NAME, "bash", "-c", 'touch {}'.format(show.SNIFFER_CONF_FILE)]),
+            call(['docker', 'cp', show.SNIFFER_CONF_FILE_IN_CONTAINER, show.TMP_SNIFFER_CONF_FILE]),
+            call(['rm', '-rf', show.TMP_SNIFFER_CONF_FILE])
+        ]
+
+        output = show.sniffer_status_get(show.ENV_VARIABLE_SX_SNIFFER)
+        assert mock_runcmd.call_args_list == expected_calls
+        assert not output
+
+    @patch('show.plugins.mlnx.run_command')
+    def test_is_issu_status_enabled_systemexit(self, mock_runcmd):
+        mock_runcmd.return_value = ('key0=value0\n', '')
+        expected_calls = ['docker', 'exec', show.CONTAINER_NAME, 'cat', r'/{}/sai.profile'.format(show.HWSKU_PATH)]
+
+        with pytest.raises(SystemExit) as e:
+            show.is_issu_status_enabled()
+        assert e.value.code == 1
+        mock_runcmd.assert_called_with(expected_calls, print_to_console=False)
+
+    def teardown(self):
+        print('TEARDOWN')
+