[ruijie] Replace os.system and remove subprocess with shell=True #12107

maipbui · 2022-09-19T04:15:09Z

Dependency: #12065

Why I did it

getstatusoutput is used without a static string and it uses shell=True
subprocess() - when using with shell=True is dangerous. Using subprocess function without a static string can lead to command injection.
os - not secure against maliciously constructed input and dangerous if used to evaluate dynamic content.

How I did it

use getstatusoutput without shell=True
subprocess() - use shell=False instead. use an array string. Ref: https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation
os - use with subprocess

How to verify it

Which release branch to backport (provide reason below if selected)

Description for the changelog

Ensure to add label/tag for the feature raised. example - PR#2174 where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: maipbui <maibui@microsoft.com>

lgtm-com · 2022-09-19T04:36:28Z

This pull request introduces 3 alerts when merging e6a7151 into 1effff9 - view on LGTM.com

new alerts:

3 for Unused local variable

qiluo-msft · 2022-09-19T23:43:53Z

platform/broadcom/sonic-platform-modules-ruijie/b6510-48vs8cq/sonic_platform/chassis.py

@@ -417,7 +417,7 @@ def set_status_led(self, color):
        if regval is None:
            print("Invaild color input.")
            return False
-        ret , log = subprocess.getstatusoutput(self.set_sys_led_cmd + regval)
+        ret , log = getstatusoutput_noshell(self.set_sys_led_cmd.append(regval))


append

It will change variable, so next call will be unexpected. #Closed

You initialize the member var self.set_sys_led_cmd but actually not using it.

Suggest

cmd = self.set_sys_led_cmd + [regval] getstatusoutput_noshell(cmd)

qiluo-msft · 2022-09-19T23:45:21Z

platform/broadcom/sonic-platform-modules-ruijie/common/lib/plat_hal/osutil.py

        return retcode, output


    @staticmethod
    def geti2cword_i2ctool(bus, addr, offset):
-        command_line = "i2cget -f -y %d 0x%02x 0x%02x  wp" % (bus, addr, offset)
+        command_line = ["i2cget", "-f", "-y", str(bus), "0x"+"%02x"%addr, "0x"+"%02x"%offset, "wp"]


"0x"+"%02x"

Just use one string? #Closed

qiluo-msft · 2022-09-19T23:46:26Z

platform/broadcom/sonic-platform-modules-ruijie/common/script/device_i2c.py

-        log_os_system(cmd)
+        file = "/sys/bus/i2c/devices/i2c-%d/delete_device" % bus
+        with open(file, 'w') as f:
+            f.write('0x'+'%02x'%str(bus)+'\n')


'0x'+'%02x'

Just one string? #Closed

qiluo-msft · 2022-09-19T23:47:00Z

platform/broadcom/sonic-platform-modules-ruijie/common/script/device_i2c.py

@@ -166,7 +165,9 @@ def addDev(name, bus, loc):
    cmd = "echo  %s 0x%02x > /sys/bus/i2c/devices/i2c-%d/new_device" % (name, loc, bus)


cmd

This line can be removed. #Closed

qiluo-msft · 2022-09-19T23:49:22Z

platform/broadcom/sonic-platform-modules-ruijie/common/script/ruijieutil.py

+            with open(location, 'w') as f:
+                f.write('0x'+'%02x'%value+'\n')
+        except (IOError, FileNotFoundError):
+            return False, 'cannot write to file'


'cannot write to file'

str(ex) may be better and more meaningful?
#Closed

qiluo-msft · 2022-09-19T23:55:08Z

platform/broadcom/sonic-platform-modules-ruijie/common/script/ruijieutil.py

    if ret != 0 or len(log) <= 0:
        error = "cmd find dmidecode"
        return False, error
-    cmd = log + "|grep -P -A5 \"Memory\s+Device\"|grep Size|grep -v Range"
+    cmd1 = split(log)


split(log)

The original behavior actually could be improved.

cmd1 = [log[0].rstrip('\n')] ``` #Closed

qiluo-msft · 2022-09-19T23:55:29Z

platform/broadcom/sonic-platform-modules-ruijie/common/script/ruijieutil.py

    if ret != 0 or len(log) <= 0:
        error = "cmd find dmidecode"
        return False, error
-    cmd = log + " -t 17 | grep  -A21 \"Memory Device\""  # 17
+    cmd1 = split(log) + ["-t", "17"]


split

The same #Closed

Signed-off-by: maipbui <maibui@microsoft.com>

lgtm-com · 2022-10-05T23:08:48Z

This pull request introduces 3 alerts when merging 7c160f9 into 1f0699f - view on LGTM.com

new alerts:

2 for Unused local variable
1 for Wrong number of arguments in a call

Signed-off-by: maipbui <maibui@microsoft.com>

lgtm-com · 2022-10-17T16:43:37Z

This pull request introduces 3 alerts when merging 9f41865 into 51eac0b - view on LGTM.com

new alerts:

2 for Unused local variable
1 for Wrong number of arguments in a call

Signed-off-by: maipbui <maibui@microsoft.com>

maipbui · 2022-10-26T15:58:03Z

@tim-rj could you help review and verify?

…ic-net#12107) Signed-off-by: maipbui <maibui@microsoft.com> Dependency: [https://github.com/sonic-net/sonic-buildimage/pull/12065](https://github.com/sonic-net/sonic-buildimage/pull/12065) #### Why I did it 1. `getstatusoutput` is used without a static string and it uses `shell=True` 2. `subprocess()` - when using with `shell=True` is dangerous. Using subprocess function without a static string can lead to command injection. 3. `os` - not secure against maliciously constructed input and dangerous if used to evaluate dynamic content. #### How I did it 1. use `getstatusoutput` without shell=True 2. `subprocess()` - use `shell=False` instead. use an array string. Ref: [https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation](https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation) 3. `os` - use with `subprocess`

) Signed-off-by: maipbui <maibui@microsoft.com> Dependency: [https://github.com/sonic-net/sonic-buildimage/pull/12065](https://github.com/sonic-net/sonic-buildimage/pull/12065) #### Why I did it 1. `getstatusoutput` is used without a static string and it uses `shell=True` 2. `subprocess()` - when using with `shell=True` is dangerous. Using subprocess function without a static string can lead to command injection. 3. `os` - not secure against maliciously constructed input and dangerous if used to evaluate dynamic content. #### How I did it 1. use `getstatusoutput` without shell=True 2. `subprocess()` - use `shell=False` instead. use an array string. Ref: [https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation](https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation) 3. `os` - use with `subprocess`

maipbui added 2 commits September 19, 2022 00:44

Replace unsafe functions in ruijie

fa8508c

Signed-off-by: maipbui <maibui@microsoft.com>

fix return text

e6a7151

Signed-off-by: maipbui <maibui@microsoft.com>

maipbui requested a review from qiluo-msft September 19, 2022 14:34

qiluo-msft reviewed Sep 19, 2022

View reviewed changes

Fix PR comments

7c160f9

Signed-off-by: maipbui <maibui@microsoft.com>

maipbui requested a review from qiluo-msft October 7, 2022 15:15

Fix PR comments

9f41865

Signed-off-by: maipbui <maibui@microsoft.com>

Fix lgtm

3c52366

Signed-off-by: maipbui <maibui@microsoft.com>

qiluo-msft approved these changes Oct 18, 2022

View reviewed changes

maipbui added the Static Code Analysis label Nov 23, 2022

maipbui marked this pull request as ready for review November 28, 2022 17:42

maipbui requested a review from lguohan as a code owner November 28, 2022 17:42

maipbui merged commit 35c4e99 into sonic-net:master Nov 28, 2022

maipbui deleted the ruijie_sec branch November 28, 2022 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ruijie] Replace os.system and remove subprocess with shell=True #12107

[ruijie] Replace os.system and remove subprocess with shell=True #12107

maipbui commented Sep 19, 2022 •

edited

Loading

lgtm-com bot commented Sep 19, 2022

qiluo-msft Sep 19, 2022 •

edited

Loading

qiluo-msft Oct 14, 2022

qiluo-msft Sep 19, 2022 •

edited

Loading

qiluo-msft Sep 19, 2022 •

edited

Loading

qiluo-msft Sep 19, 2022 •

edited

Loading

qiluo-msft Sep 19, 2022 •

edited

Loading

qiluo-msft Sep 19, 2022 •

edited

Loading

qiluo-msft Sep 19, 2022 •

edited

Loading

lgtm-com bot commented Oct 5, 2022

lgtm-com bot commented Oct 17, 2022

maipbui commented Oct 26, 2022

		@@ -166,7 +165,9 @@ def addDev(name, bus, loc):
		cmd = "echo %s 0x%02x > /sys/bus/i2c/devices/i2c-%d/new_device" % (name, loc, bus)

[ruijie] Replace os.system and remove subprocess with shell=True #12107

[ruijie] Replace os.system and remove subprocess with shell=True #12107

Conversation

maipbui commented Sep 19, 2022 • edited Loading

Why I did it

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Description for the changelog

Ensure to add label/tag for the feature raised. example - PR#2174 where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

lgtm-com bot commented Sep 19, 2022

qiluo-msft Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Oct 14, 2022

Choose a reason for hiding this comment

qiluo-msft Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

lgtm-com bot commented Oct 5, 2022

lgtm-com bot commented Oct 17, 2022

maipbui commented Oct 26, 2022

maipbui commented Sep 19, 2022 •

edited

Loading

qiluo-msft Sep 19, 2022 •

edited

Loading

qiluo-msft Sep 19, 2022 •

edited

Loading

qiluo-msft Sep 19, 2022 •

edited

Loading

qiluo-msft Sep 19, 2022 •

edited

Loading

qiluo-msft Sep 19, 2022 •

edited

Loading

qiluo-msft Sep 19, 2022 •

edited

Loading

qiluo-msft Sep 19, 2022 •

edited

Loading