[utilities_common] replace shell=True #2718

maipbui · 2023-03-06T23:09:10Z

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

remove shell=True in utilities_common.cli.run_command() function.
subprocess() - use shell=False instead, use list of strings Ref: https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation

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-03-20T23:01:57Z

config/main.py

-        clicommon.run_command('hostname -F /etc/hostname', display_cmd=True)
-        clicommon.run_command(r'sed -i "/\s{}$/d" /etc/hosts'.format(current_hostname), display_cmd=True)
-        clicommon.run_command('echo "127.0.0.1 {}" >> /etc/hosts'.format(hostname), display_cmd=True)
+        with open('etc/hostname', 'w') as f:


etc/hostname

It should be absolute path. #Closed

qiluo-msft · 2023-03-20T23:02:09Z

config/main.py

+            f.write(str(hostname) + '\n')
+        clicommon.run_command(['hostname', '-F', '/etc/hostname'], display_cmd=True)
+        clicommon.run_command(['sed', '-i', "/\s{}$/d".format(current_hostname), '/etc/hosts'], display_cmd=True)
+        with open('etc/hosts', 'a') as f:


etc/hosts

The same #Closed

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

qiluo-msft · 2023-05-02T20:27:06Z

config/main.py

@@ -1305,9 +1303,9 @@ def load(filename, yes):
            return

        if namespace is None:
-            command = "{} -j {} --write-to-db".format(SONIC_CFGGEN_PATH, file)


--write-to-db

Why remove "--write-to-db" ? #Closed

qiluo-msft · 2023-05-02T20:29:24Z

config/main.py

            # Ignore error for IPv6 configuration command due to it not allows config the same IP twice
            clicommon.run_command(command, display_cmd=True, ignore_error=True)
-        command = "ip{} route add default via {} dev eth0 table default".format(" -6" if mgmt_conf.version == 6 else "", gw_addr)
+        command = ['ip{}'.format(" -6" if mgmt_conf.version == 6 else ""), 'route', 'add', 'default', 'via', str(gw_addr), 'dev', 'eth0', 'table', 'default']


" -6"

Should be the 2nd arg #Closed

qiluo-msft · 2023-05-02T20:29:56Z

config/main.py

        clicommon.run_command(command, display_cmd=True, ignore_error=True)
-        command = "ip{} rule add from {} table default".format(" -6" if mgmt_conf.version == 6 else "", str(mgmt_conf.ip))
+        command = ['ip{}'.format(" -6" if mgmt_conf.version == 6 else ""), 'rule', 'add', 'from', str(mgmt_conf.ip), 'table', 'default']


" -6"

The same. #Closed

qiluo-msft · 2023-05-02T20:34:16Z

config/main.py

-        clicommon.run_command(command, display_cmd=True, ignore_error=True)
+        pid_cmd = ['cat', '/var/run/dhclient.eth0.pid']
+        _, pid = getstatusoutput_noshell(pid_cmd)
+        cmd0 = ['[',  '-f', '/var/run/dhclient.eth0.pid', ']']


'-f'

-f is a bash/sh feature, I guess you need to run_command with shell=True to have this feature. Could you check? If yes, you may consider using python to check, like os.path.isfile(path). #Closed

qiluo-msft · 2023-05-02T20:46:41Z

show/platform.py

-    cmd = "sudo ssdutil -d " + device
-    options = " -v" if verbose else ""
-    options += " -e" if vendor else ""
+    cmd = ['sudo', 'ssdutil', '-d', device]


device

Is it always a str? #Closed

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

qiluo-msft · 2023-05-24T21:52:40Z

config/main.py



 def _get_sonic_services():
-    out, _ = clicommon.run_command("systemctl list-dependencies --plain sonic.target | sed '1d'", return_cmd=True)
+    cmd = "systemctl list-dependencies --plain sonic.target | sed '1d'"
+    out, _ = clicommon.run_command(cmd, return_cmd=True, shell=True)


cmd

you may use one command systemctl list-dependencies --plain sonic.target.
And then parse the output in python instead of sed. #Closed

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

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)

#### 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 remove shell=True in utilities_common.cli.run_command() function. `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 Manual test Signed-off-by: Mai Bui <[email protected]>

Resolves sonic-net/sonic-buildimage#16542 #### What I did Update str -> list[str] commands which were missed in #2718 #### How I did it #### How to verify it Pass UT. Manual test, issue resolved, tested in internal.79435802-3bbd91c86e version Signed-off-by: Mai Bui <[email protected]>

Resolves sonic-net/sonic-buildimage#16542 Update str -> list[str] commands which were missed in sonic-net#2718 Pass UT. Manual test, issue resolved, tested in internal.79435802-3bbd91c86e version Signed-off-by: Mai Bui <[email protected]>

Why I did Use static str to run_command would encounter error because change in this PR: #2718. How I did it Change command from static str to list. How to verify it UT passed. Build whl and install in testbed. Signed-off-by: Yaqiang Zhu <[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)

Why I did Use static str to run_command would encounter error because change in this PR: #2718. How I did it Change command from static str to list. How to verify it UT passed. Build whl and install in testbed. Signed-off-by: Yaqiang Zhu <[email protected]>

Resolves sonic-net/sonic-buildimage#16542 #### What I did Update str -> list[str] commands which were missed in sonic-net#2718 #### How I did it #### How to verify it Pass UT. Manual test, issue resolved, tested in internal.79435802-3bbd91c86e version Signed-off-by: Mai Bui <[email protected]>

Why I did Use static str to run_command would encounter error because change in this PR: sonic-net#2718. How I did it Change command from static str to list. How to verify it UT passed. Build whl and install in testbed. Signed-off-by: Yaqiang Zhu <[email protected]>

maipbui added 2 commits March 6, 2023 23:07

replace shell=True

50360f2

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

fix UT

7db89ca

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

qiluo-msft reviewed Mar 20, 2023

View reviewed changes

maipbui added 6 commits April 6, 2023 18:42

address comments

3e22b86

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

resolve conflict

3c76466

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

resolve conflict

855d870

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

add verbose

d425b07

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

add namespace

ffb350f

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

add UT

b6c643c

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

maipbui marked this pull request as draft May 2, 2023 20:16

qiluo-msft reviewed May 2, 2023

View reviewed changes

maipbui added 5 commits May 11, 2023 02:41

fix run_command

f1c85bb

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

resolve conflict

bdc63e4

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

fix UT

82c7797

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

fix run_command

37dd0b1

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

address comments

084805e

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

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

maipbui added 5 commits May 17, 2023 21:26

Merge remote-tracking branch 'upstream/master' into util_cli_pysec

db6ef58

add show/fabric.py

cf57640

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

fix

515a49b

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

fix

68585b3

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

fix

fab39cb

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

maipbui mentioned this pull request May 23, 2023

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

Merged

10 tasks

qiluo-msft reviewed May 24, 2023

View reviewed changes

remove shell=True

f6c49a9

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

maipbui marked this pull request as ready for review May 25, 2023 19:17

qiluo-msft previously approved these changes May 25, 2023

View reviewed changes

address comments

63a7c16

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

maipbui dismissed qiluo-msft’s stale review via 63a7c16 May 26, 2023 03:44

fix

c30480e

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

qiluo-msft approved these changes May 31, 2023

View reviewed changes

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

maipbui deleted the util_cli_pysec branch May 31, 2023 13:00

dprital mentioned this pull request Jun 5, 2023

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

Closed

10 tasks

dgsudharsan mentioned this pull request Sep 13, 2023

[Vlan] config vlan del command results in traceback with supervisorctl status ndppd command failing in swss sonic-net/sonic-buildimage#16542

Closed

maipbui mentioned this pull request Sep 14, 2023

Fix del vlan command #2982

Merged

This was referenced Sep 20, 2023

draft #2988

Closed

[vlan][dhcp_relay] Fix error while delete vlan #2987

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[utilities_common] replace shell=True #2718

[utilities_common] replace shell=True #2718

maipbui commented Mar 6, 2023 •

edited

Loading

qiluo-msft Mar 20, 2023 •

edited

Loading

qiluo-msft Mar 20, 2023 •

edited

Loading

qiluo-msft May 2, 2023 •

edited

Loading

qiluo-msft May 2, 2023 •

edited

Loading

qiluo-msft May 2, 2023 •

edited

Loading

qiluo-msft May 2, 2023 •

edited

Loading

qiluo-msft May 2, 2023 •

edited

Loading

qiluo-msft May 24, 2023 •

edited

Loading

[utilities_common] replace shell=True #2718

[utilities_common] replace shell=True #2718

Conversation

maipbui commented Mar 6, 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 Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft May 2, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft May 2, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft May 2, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft May 2, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft May 2, 2023 • edited Loading

Choose a reason for hiding this comment

qiluo-msft May 24, 2023 • edited Loading

Choose a reason for hiding this comment

maipbui commented Mar 6, 2023 •

edited

Loading

qiluo-msft Mar 20, 2023 •

edited

Loading

qiluo-msft Mar 20, 2023 •

edited

Loading

qiluo-msft May 2, 2023 •

edited

Loading

qiluo-msft May 2, 2023 •

edited

Loading

qiluo-msft May 2, 2023 •

edited

Loading

qiluo-msft May 2, 2023 •

edited

Loading

qiluo-msft May 2, 2023 •

edited

Loading

qiluo-msft May 24, 2023 •

edited

Loading