[show] replace shell=True, replace xml by lxml, replace exit by sys.exit #2666

maipbui · 2023-02-09T21:05:32Z

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.
sys.exit is better than exit, considered good to use in production code.
Ref:
https://stackoverflow.com/questions/6501121/difference-between-exit-and-sys-exit-in-python
https://stackoverflow.com/questions/19747371/python-exit-commands-why-so-many-and-when-should-each-be-used

How I did it

subprocess() - use shell=False instead, use list of strings Ref: https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation
Replace exit() by sys.exit()

How to verify it

Pass UT
Manual test

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

Signed-off-by: maipbui <[email protected]>

qiluo-msft · 2023-02-10T20:00:01Z

show/main.py

    if route_map_name is not None:
-        cmd += ' {}'.format(route_map_name)
-    cmd += '"'
+        cmd += [str(route_map_name)]


route_map_name

It should be appended to embedded show route-map command. #Closed

Could you make sure this is tested?

fixed, thanks!

To simplify:
cmd[-1] += ' {}'.format(route_map_name)

qiluo-msft · 2023-02-10T20:00:50Z

show/main.py

    if prefix_list_name is not None:
-        cmd += ' {}'.format(prefix_list_name)
-    cmd += '"'
+        cmd += [str(prefix_list_name)]


prefix_list_name

append to embedded command #Closed

qiluo-msft · 2023-02-10T20:01:23Z

show/main.py

    if prefix_list_name is not None:
-        cmd += ' {}'.format(prefix_list_name)
-    cmd += '"'
+        cmd += [str(prefix_list_name)]


prefix_list_name

append to embedded command #Closed

qiluo-msft · 2023-02-10T20:07:15Z

show/main.py


-        run_command(cmd, display_cmd=verbose)
+        if cmd1 is None and cmd2 is None:


cmd1

You implemented a 2x2 conditions with 4 branches. This is not scalable if number of cmd increase. For example 10 cmd.

You can construct an array first, and feed the expanded array to function call. ref: https://stackoverflow.com/a/7745986/2514803 #Closed

or cmdn could be defined as array elements from the beginning.

qiluo-msft · 2023-02-10T20:11:20Z

show/main.py

+    cmd0 = ['sudo', 'docker', 'ps']
+    cmd1 = ['grep', 'bgp']
+    cmd2 = ['awk', '{print$2}']
+    cmd3 = ['cut', '-d', '-', '-f3']


'-d', '-'

originally it is one arg. #Closed

qiluo-msft · 2023-02-10T20:11:32Z

show/main.py

+    cmd1 = ['grep', 'bgp']
+    cmd2 = ['awk', '{print$2}']
+    cmd3 = ['cut', '-d', '-', '-f3']
+    cmd4 = ['cut', '-d', ':', "-f1"]


'-d', ':'

originally it is one arg. #Closed

qiluo-msft · 2023-02-10T20:16:06Z

show/plugins/barefoot.py

-        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',\
+            r'-maxdepth', '1', '-type', 'd', '-name', 'install_\*_profile', r'{}' % opts]


r'{}' % opts

What the diff if using opts directly? #Pending

r'' is to bypass escaping special characters. Using opts directly will escape special characters.

Could you be more specific?, r is applied to {} string only.

for example, r'\n' will treated the string containing 2 characters: backslash \ and letter n. But '\n' will be treated as a newline character.

fixed, raised another PR for barefoot https://github.com/sonic-net/sonic-utilities/pull/2699/files

qiluo-msft · 2023-02-10T20:18:34Z

show/plugins/barefoot.py

@@ -4,6 +4,7 @@
 import json
 import subprocess
 from sonic_py_common import device_info
+from sonic_py_common.general import getstatusoutput_noshell_pipe, check_output_pipe

 @click.group()
 def barefoot():


barefoot

This file need to reviewed by barefoot. Suggest further split into a smaller PR. #Closed

qiluo-msft · 2023-02-10T20:18:54Z

Specific command-line utility for Mellanox platform

This file need to reviewed by Mellanox. Suggest further split into a smaller PR.

In reply to: 1426293827

Refers to: show/plugins/mlnx.py:22 in 4d96b6e. [](commit_id = 4d96b6e, deletion_comment = False)

qiluo-msft · 2023-02-10T20:30:15Z

show/main.py

@@ -130,6 +133,32 @@ def run_command(command, display_cmd=False, return_cmd=False):
    if rc != 0:
        sys.exit(rc)

+def run_command_pipe(*args, display_cmd=False, return_cmd=False):


run_command_pipe

Treat run_command as a special case, call the other with only one command in args, so most of the code are reused. #Closed

qiluo-msft · 2023-02-10T20:31:31Z

clicommon.run_command(cmd + options, display_cmd=verbose)

Use string array instead of string for run_command.

In reply to: 1426311506

Refers to: show/platform.py:111 in 4d96b6e. [](commit_id = 4d96b6e, deletion_comment = False)

Signed-off-by: maipbui <[email protected]>

maipbui · 2023-02-15T23:40:43Z

clicommon.run_command(cmd + options, display_cmd=verbose)
Use string array instead of string for run_command.

Refers to: show/platform.py:111 in 4d96b6e. [](commit_id = 4d96b6e, deletion_comment = False)

this will be covered in another PR. I'll draft another PR.

Signed-off-by: maipbui <[email protected]>

qiluo-msft · 2023-02-28T23:12:56Z

show/plugins/barefoot.py

-        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',\
+            r'-maxdepth', '1', '-type', 'd', '-name', 'install_\*_profile', r'{}' % opts]


r

This r is not needed. #Pending

Signed-off-by: maipbui <[email protected]>

maipbui · 2023-03-03T04:18:16Z

/azp run Azure.sonic-utilities

azure-pipelines · 2023-03-03T04:18:26Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Mai Bui <[email protected]>

qiluo-msft · 2023-05-25T21:01:07Z

show/main.py

 @click.option('-f', '--follow', is_flag=True)
 @click.option('--verbose', is_flag=True, help="Enable verbose output")
 def logging(process, lines, follow, verbose):
    """Show system log"""
+    if process and not re.match(r'^[a-zA-Z0-9\s]+$', process):
+        sys.exit('Process contains only number, alphabet, and whitespace.')


exit

What is the reason to add this check? #Closed

Offline discussed, this check is not accurate. Let's revert back, and please use another future PR to focus on this fix.

Signed-off-by: Mai Bui <[email protected]>

qiluo-msft · 2023-05-31T18:17:45Z

class TestShowPlatform(object):

Thanks for cleaning up the codebase! #Closed

Refers to: tests/show_test.py:498 in a20435a. [](commit_id = a20435a, deletion_comment = True)

Update sonic-utilities submodule pointer to include the following: * 5c9b2177 Fix issue: out of range sflow polling interval is accepted and stored in config_db ([sonic-net#2847](sonic-net/sonic-utilities#2847)) * 72ca4848 Add CLI configuration options for teamd retry count feature ([sonic-net#2642](sonic-net/sonic-utilities#2642)) * 359dfc0c [Clock] Implement clock CLI ([sonic-net#2793](sonic-net/sonic-utilities#2793)) * b316fc27 Add transceiver status CLI to show output from TRANSCEIVER_STATUS table ([sonic-net#2772](sonic-net/sonic-utilities#2772)) * dc59dbd2 Replace pickle by json ([sonic-net#2849](sonic-net/sonic-utilities#2849)) * a66f41c4 [show] replace shell=True, replace xml by lxml, replace exit by sys.exit ([sonic-net#2666](sonic-net/sonic-utilities#2666)) * 57500572 [utilities_common] replace shell=True ([sonic-net#2718](sonic-net/sonic-utilities#2718)) * 6e0ee3e7 [CRM][DASH] Extend CRM utility to support DASH resources. ([sonic-net#2800](sonic-net/sonic-utilities#2800)) * b2c29b0b [config] Generate sysinfo in single asic ([sonic-net#2856](sonic-net/sonic-utilities#2856)) Signed-off-by: dprital <[email protected]>

…nic-utilities submodule on master (#15193) Dependency: sonic-net/sonic-utilities#2718 Why I did it This PR sonic-net/sonic-utilities#2718 reduce shell=True usage in utilities_common.cli.run_command() function. Work item tracking Microsoft ADO (number only): 15022050 How I did it Replace strings commands using utilities_common.cli.run_command() function to list of strings due to circular dependency, advance sonic-utilities submodule 72ca4848 (HEAD -> master, upstream/master, upstream/HEAD) Add CLI configuration options for teamd retry count feature (sonic-net/sonic-utilities#2642) 359dfc0c [Clock] Implement clock CLI (sonic-net/sonic-utilities#2793) b316fc27 Add transceiver status CLI to show output from TRANSCEIVER_STATUS table (sonic-net/sonic-utilities#2772) dc59dbd2 Replace pickle by json (sonic-net/sonic-utilities#2849) a66f41c4 [show] replace shell=True, replace xml by lxml, replace exit by sys.exit (sonic-net/sonic-utilities#2666) 57500572 [utilities_common] replace shell=True (sonic-net/sonic-utilities#2718) 6e0ee3e7 [CRM][DASH] Extend CRM utility to support DASH resources. (sonic-net/sonic-utilities#2800) b2c29b0b [config] Generate sysinfo in single asic (sonic-net/sonic-utilities#2856)

…xit (sonic-net#2666) #### What I did `subprocess()` - when using with `shell=True` is dangerous. Using subprocess function without a static string can lead to command injection. `sys.exit` is better than `exit`, considered good to use in production code. Ref: https://stackoverflow.com/questions/6501121/difference-between-exit-and-sys-exit-in-python https://stackoverflow.com/questions/19747371/python-exit-commands-why-so-many-and-when-should-each-be-used #### 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) Replace `exit()` by `sys.exit()` #### How to verify it Pass UT Manual test Signed-off-by: Mai Bui <[email protected]>

…nic-utilities submodule on master (sonic-net#15193) Dependency: sonic-net/sonic-utilities#2718 Why I did it This PR sonic-net/sonic-utilities#2718 reduce shell=True usage in utilities_common.cli.run_command() function. Work item tracking Microsoft ADO (number only): 15022050 How I did it Replace strings commands using utilities_common.cli.run_command() function to list of strings due to circular dependency, advance sonic-utilities submodule 72ca4848 (HEAD -> master, upstream/master, upstream/HEAD) Add CLI configuration options for teamd retry count feature (sonic-net/sonic-utilities#2642) 359dfc0c [Clock] Implement clock CLI (sonic-net/sonic-utilities#2793) b316fc27 Add transceiver status CLI to show output from TRANSCEIVER_STATUS table (sonic-net/sonic-utilities#2772) dc59dbd2 Replace pickle by json (sonic-net/sonic-utilities#2849) a66f41c4 [show] replace shell=True, replace xml by lxml, replace exit by sys.exit (sonic-net/sonic-utilities#2666) 57500572 [utilities_common] replace shell=True (sonic-net/sonic-utilities#2718) 6e0ee3e7 [CRM][DASH] Extend CRM utility to support DASH resources. (sonic-net/sonic-utilities#2800) b2c29b0b [config] Generate sysinfo in single asic (sonic-net/sonic-utilities#2856)

replace shell=True

4d96b6e

Signed-off-by: maipbui <[email protected]>

qiluo-msft reviewed Feb 10, 2023

View reviewed changes

Address PR comments

6b0d24b

Signed-off-by: maipbui <[email protected]>

maipbui added 2 commits February 27, 2023 21:55

Address PR comments

b0b5bb6

Signed-off-by: maipbui <[email protected]>

remove comments

5fcc659

Signed-off-by: maipbui <[email protected]>

sonic-net deleted a comment from azure-pipelines bot Feb 27, 2023

qiluo-msft reviewed Feb 28, 2023

View reviewed changes

modify string to list of str in bgp quagga

09cba0d

Signed-off-by: maipbui <[email protected]>

qiluo-msft previously approved these changes Mar 7, 2023

View reviewed changes

maipbui added 2 commits April 26, 2023 18:49

resolve conflict

df8429d

Signed-off-by: Mai Bui <[email protected]>

fix UT

c4ec419

Signed-off-by: Mai Bui <[email protected]>

maipbui dismissed qiluo-msft’s stale review via c4ec419 April 26, 2023 20:24

maipbui added the static-code-analysis label May 12, 2023

maipbui added 3 commits May 24, 2023 20:58

fix run_command and UT

bc8659d

Signed-off-by: Mai Bui <[email protected]>

add UT

f3ee836

Signed-off-by: Mai Bui <[email protected]>

add UT

8afda93

Signed-off-by: Mai Bui <[email protected]>

qiluo-msft reviewed May 25, 2023

View reviewed changes

maipbui added 2 commits May 26, 2023 02:58

remove validation

2c29446

Signed-off-by: Mai Bui <[email protected]>

fix UT

a20435a

Signed-off-by: Mai Bui <[email protected]>

qiluo-msft previously approved these changes May 31, 2023

View reviewed changes

maipbui added 3 commits May 31, 2023 13:18

remove UT

b7f5c04

Signed-off-by: Mai Bui <[email protected]>

resolve conflict

531ab6a

Signed-off-by: Mai Bui <[email protected]>

add UT

7bbdc97

Signed-off-by: Mai Bui <[email protected]>

maipbui dismissed qiluo-msft’s stale review via 7bbdc97 May 31, 2023 13:27

qiluo-msft approved these changes May 31, 2023

View reviewed changes

maipbui merged commit a66f41c into sonic-net:master May 31, 2023

maipbui deleted the show_pysec branch May 31, 2023 18:20

maipbui mentioned this pull request Jun 4, 2023

modify commands using utilities_common.cli.run_command and advance sonic-utilities submodule on master sonic-net/sonic-buildimage#15193

Merged

10 tasks

dprital mentioned this pull request Jun 5, 2023

[submodule] Advance sonic-utilities pointer sonic-net/sonic-buildimage#15336

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[show] replace shell=True, replace xml by lxml, replace exit by sys.exit #2666

[show] replace shell=True, replace xml by lxml, replace exit by sys.exit #2666

maipbui commented Feb 9, 2023 •

edited

Loading

qiluo-msft Feb 10, 2023 •

edited

Loading

qiluo-msft Feb 10, 2023

maipbui Feb 15, 2023

qiluo-msft Feb 15, 2023

qiluo-msft Feb 10, 2023 •

edited

Loading

qiluo-msft Feb 15, 2023

qiluo-msft Feb 10, 2023 •

edited

Loading

qiluo-msft Feb 15, 2023

qiluo-msft Feb 10, 2023 •

edited

Loading

qiluo-msft Feb 10, 2023

qiluo-msft Feb 10, 2023 •

edited

Loading

qiluo-msft Feb 10, 2023 •

edited

Loading

qiluo-msft Feb 10, 2023 •

edited

Loading

maipbui Feb 15, 2023

qiluo-msft Feb 15, 2023

maipbui Feb 16, 2023

maipbui Feb 28, 2023

qiluo-msft Feb 10, 2023 •

edited

Loading

maipbui Feb 15, 2023

qiluo-msft commented Feb 10, 2023 •

edited

Loading

Specific command-line utility for Mellanox platform

qiluo-msft Feb 10, 2023 •

edited

Loading

qiluo-msft commented Feb 10, 2023 •

edited

Loading

maipbui commented Feb 15, 2023

qiluo-msft Feb 28, 2023 •

edited

Loading

maipbui commented Mar 3, 2023

azure-pipelines bot commented Mar 3, 2023

qiluo-msft May 25, 2023 •

edited

Loading

qiluo-msft May 25, 2023

qiluo-msft commented May 31, 2023 •

edited

Loading


		run_command(cmd, display_cmd=verbose)
		if cmd1 is None and cmd2 is None:

[show] replace shell=True, replace xml by lxml, replace exit by sys.exit #2666

[show] replace shell=True, replace xml by lxml, replace exit by sys.exit #2666

Conversation

maipbui commented Feb 9, 2023 • edited Loading

What I did

How I did it

How to verify it

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

qiluo-msft Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft commented Feb 10, 2023 • edited Loading

Specific command-line utility for Mellanox platform

qiluo-msft Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft commented Feb 10, 2023 • edited Loading

maipbui commented Feb 15, 2023

qiluo-msft Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

maipbui commented Mar 3, 2023

azure-pipelines bot commented Mar 3, 2023

qiluo-msft May 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft commented May 31, 2023 • edited Loading

maipbui commented Feb 9, 2023 •

edited

Loading

qiluo-msft Feb 10, 2023 •

edited

Loading

qiluo-msft Feb 10, 2023 •

edited

Loading

qiluo-msft Feb 10, 2023 •

edited

Loading

qiluo-msft Feb 10, 2023 •

edited

Loading

qiluo-msft Feb 10, 2023 •

edited

Loading

qiluo-msft Feb 10, 2023 •

edited

Loading

qiluo-msft Feb 10, 2023 •

edited

Loading

qiluo-msft Feb 10, 2023 •

edited

Loading

qiluo-msft commented Feb 10, 2023 •

edited

Loading

qiluo-msft Feb 10, 2023 •

edited

Loading

qiluo-msft commented Feb 10, 2023 •

edited

Loading

qiluo-msft Feb 28, 2023 •

edited

Loading

qiluo-msft May 25, 2023 •

edited

Loading

qiluo-msft commented May 31, 2023 •

edited

Loading