[device/celestica] Mitigation for command injection vulnerability #11740

maipbui · 2022-08-15T17:20:44Z

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

Dependency: PR (#12065) needs to merge first.

Why I did it

eval() - not secure against maliciously constructed input, can be dangerous if used to evaluate dynamic content. This may be a code injection vulnerability.
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.
is operator - string comparison should not be used with reference equality.
globals() - extremely dangerous because it may allow an attacker to execute arbitrary code on the system

How I did it

eval() - use literal_eval()
subprocess() - use shell=False instead. use an array string. Ref: https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation
os - use with subprocess
is - replace by == operator for value equality
globals() - avoid the use of globals()

How to verify it

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

Description for the changelog

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-08-15T22:15:28Z

This pull request introduces 1 alert and fixes 6 when merging c24eec3 into a66941a - view on LGTM.com

new alerts:

1 for Syntax error

fixed alerts:

3 for Variable defined multiple times
2 for Unused import
1 for Wrong name for an argument in a class instantiation

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

device/celestica/x86_64-cel_e1031-r0/sonic_platform/common.py

device/celestica/x86_64-cel_e1031-r0/sonic_platform/component.py

device/celestica/x86_64-cel_silverstone-r0/sonic_platform/psu.py

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

device/celestica/x86_64-cel_e1031-r0/sonic_platform/common.py

qiluo-msft · 2022-08-17T22:55:27Z

def get_output(self, index, config, default):

Seems this function is not used. Please remove.
@mudsut4ke Could you help review?

Refers to: device/celestica/x86_64-cel_e1031-r0/sonic_platform/common.py:226 in ca56944. [](commit_id = ca56944, deletion_comment = False)

qiluo-msft · 2022-08-17T22:55:32Z

def set_output(self, index, input, config):

Seems this function is not used. Please remove.
@mudsut4ke Could you help review?

Refers to: device/celestica/x86_64-cel_e1031-r0/sonic_platform/common.py:281 in ca56944. [](commit_id = ca56944, deletion_comment = False)

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

lgtm-com · 2022-09-01T19:44:44Z

This pull request introduces 7 alerts and fixes 6 when merging dada9ae into fdd9130 - view on LGTM.com

new alerts:

3 for Unused local variable
2 for Unused import
1 for Unreachable code
1 for Wrong number of arguments in a call

fixed alerts:

5 for Except block handles 'BaseException'
1 for Unused import

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

lgtm-com · 2022-09-01T20:43:23Z

This pull request introduces 5 alerts and fixes 6 when merging 91781fe into fdd9130 - view on LGTM.com

new alerts:

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

fixed alerts:

5 for Except block handles 'BaseException'
1 for Unused import

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

lgtm-com · 2022-09-01T21:20:17Z

This pull request introduces 1 alert and fixes 6 when merging 6647f81 into fdd9130 - view on LGTM.com

new alerts:

1 for Wrong number of arguments in a call

fixed alerts:

5 for Except block handles 'BaseException'
1 for Unused import

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

lgtm-com · 2022-09-06T16:36:37Z

This pull request introduces 1 alert and fixes 12 when merging a7b8055 into 1b5d07f - view on LGTM.com

new alerts:

1 for Wrong number of arguments in a call

fixed alerts:

6 for Except block handles 'BaseException'
6 for Unused import

device/celestica/x86_64-cel_silverstone-r0/sonic_platform/helper.py

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

lgtm-com · 2022-09-21T20:58:09Z

This pull request introduces 1 alert and fixes 13 when merging 65b4300 into fd6a1b0 - view on LGTM.com

new alerts:

1 for Wrong number of arguments in a call

fixed alerts:

8 for Unused import
5 for Except block handles 'BaseException'

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

lgtm-com · 2022-09-21T21:23:31Z

This pull request introduces 1 alert and fixes 13 when merging 0ce54ef into fd6a1b0 - view on LGTM.com

new alerts:

1 for Wrong number of arguments in a call

fixed alerts:

8 for Unused import
5 for Except block handles 'BaseException'

…ecurity

lgtm-com · 2022-10-05T22:30:04Z

This pull request fixes 13 alerts when merging a971633 into 1f0699f - view on LGTM.com

fixed alerts:

8 for Unused import
5 for Except block handles 'BaseException'

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

lgtm-com · 2022-10-06T16:45:03Z

This pull request fixes 13 alerts when merging 92147d7 into 1073a47 - view on LGTM.com

fixed alerts:

8 for Unused import
5 for Except block handles 'BaseException'

maipbui · 2022-11-30T01:49:17Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2022-11-30T01:49:29Z

Azure Pipelines successfully started running 1 pipeline(s).

qiluo-msft · 2022-11-30T08:23:23Z

@Blueve is connecting with Celestica reviewers.

Blueve · 2022-12-06T03:13:45Z

Celestica team acked to review, ETA: By Dec 11

qnos · 2022-12-08T01:44:29Z

device/celestica/x86_64-cel_e1031-r0/sonic_platform/common.py

@@ -195,7 +185,7 @@ def write_txt_file(self, file_path, value):
        return True

    def is_host(self):
-        return os.system(self.HOST_CHK_CMD) == 0
+        return subprocess.call(self.HOST_CHK_CMD) == 0


If use subprocess.call, need to suppress stdout/stderr and handle docker command not found exception.
The following example codes work in both host and docker mode.

def is_host(): try: subprocess.call(HOST_CHK_CMD, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) except FileNotFoundError: return False return True

I think os.system is equivalent with subprocess.call without specifying stdout and stderr to DEVNULL. Ref: https://docs.python.org/3/library/subprocess.html#replacing-os-system

I got what you meant, I will change per your suggestion

I think os.system is equivalent with subprocess.call without specifying stdout and stderr to DEVNULL. Ref: https://docs.python.org/3/library/subprocess.html#replacing-os-system

Yes, no essential differences between the two functions, but HOST_CHK_CMD had been changed from "docker > /dev/null 2>&1" to ["docker"], so there is no way to suppress the 'docker' command output any more in host mode. And handling the FileNotFoundError exception is also necessary for docker mode.

Fixed. Could you review and approve? Thank you!

qnos · 2022-12-08T01:48:12Z

device/celestica/x86_64-cel_seastone-r0/sonic_platform/helper.py

@@ -15,7 +15,7 @@ def __init__(self):
        (self.platform, self.hwsku) = device_info.get_platform_and_hwsku()

    def is_host(self):
-        return os.system(HOST_CHK_CMD) == 0
+        return subprocess.call(HOST_CHK_CMD) == 0


Please consider changing the is_host function implementation as suggested above.

qnos · 2022-12-08T01:49:26Z

device/celestica/x86_64-cel_silverstone-r0/sonic_platform/helper.py

@@ -13,7 +14,7 @@ def __init__(self):
        pass

    def is_host(self):
-        return os.system(HOST_CHK_CMD) == 0
+        return subprocess.call(HOST_CHK_CMD) == 0


Please consider changing the is_host function implementation as suggested above.

qnos · 2022-12-08T01:50:04Z

device/celestica/x86_64-cel_silverstone-r0/sonic_platform/sfp.py

@@ -270,7 +269,7 @@ def __convert_string_to_num(self, value_str):
            return 'N/A'

    def __is_host(self):
-        return os.system(self.HOST_CHK_CMD) == 0
+        return subprocess.call(self.HOST_CHK_CMD) == 0


Please consider changing the is_host function implementation as suggested above.

qnos · 2022-12-08T01:50:29Z

platform/broadcom/sonic-platform-modules-cel/services/platform_api/sonic_platform/common.py

@@ -166,7 +167,7 @@ def write_txt_file(self, file_path, value):
        return True

    def is_host(self):
-        return os.system(self.HOST_CHK_CMD) == 0
+        return subprocess.call(self.HOST_CHK_CMD) == 0


Please consider changing the is_host function implementation as suggested above.

qnos

Except the is_host() function implementation, agree with all other changes.

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

lgtm-com · 2022-12-08T05:37:53Z

This pull request fixes 13 alerts when merging 34991a5 into d993444 - view on LGTM.com

fixed alerts:

8 for Unused import
5 for Except block handles 'BaseException'

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

Improve command injection in subprocess and eval

956da45

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

maipbui requested a review from qiluo-msft August 15, 2022 17:43

maipbui added 2 commits August 15, 2022 18:29

Use literal_evals instead of eval

8811f7a

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

Add sanitize command input

652921f

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

maipbui changed the title ~~[device/celestica][e1031] Mitigation for command injection vulnerability~~ [device/celestica] Mitigation for command injection vulnerability Aug 15, 2022

Remove globals()

c24eec3

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

Fix syntax error

9ca1d96

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

qiluo-msft reviewed Aug 15, 2022

View reviewed changes

device/celestica/x86_64-cel_e1031-r0/sonic_platform/common.py Outdated Show resolved Hide resolved

qiluo-msft reviewed Aug 15, 2022

View reviewed changes

device/celestica/x86_64-cel_e1031-r0/sonic_platform/common.py Outdated Show resolved Hide resolved

qiluo-msft reviewed Aug 15, 2022

View reviewed changes

device/celestica/x86_64-cel_e1031-r0/sonic_platform/component.py Outdated Show resolved Hide resolved

qiluo-msft reviewed Aug 15, 2022

View reviewed changes

device/celestica/x86_64-cel_e1031-r0/sonic_platform/component.py Outdated Show resolved Hide resolved

qiluo-msft reviewed Aug 16, 2022

View reviewed changes

device/celestica/x86_64-cel_silverstone-r0/sonic_platform/psu.py Outdated Show resolved Hide resolved

Fix command in subprocess

ca56944

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

qiluo-msft reviewed Aug 17, 2022

View reviewed changes

device/celestica/x86_64-cel_e1031-r0/sonic_platform/common.py Outdated Show resolved Hide resolved

maipbui added 4 commits August 17, 2022 23:16

Change data structure and fix static input in subprocess

ba61fd4

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

Remove unnecessary parameters

cebc440

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

Fix static subprocess

0a5d46a

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

Remove os.system and subprocess shell=True

dada9ae

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

Fix lgtm

91781fe

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

Fix lgtm

6647f81

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

maipbui added 2 commits September 6, 2022 16:04

Remove unused funcs and fix subprocess cmd

35aedce

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

Remove brackets

a7b8055

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

qiluo-msft reviewed Sep 7, 2022

View reviewed changes

device/celestica/x86_64-cel_silverstone-r0/sonic_platform/helper.py Outdated Show resolved Hide resolved

fix bug

65b4300

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

Add universal_newliness for py3

0ce54ef

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

Merge remote-tracking branch 'upstream/master' into celestica_e1031_s…

a971633

…ecurity

maipbui added 2 commits October 6, 2022 15:43

Revert solution

bba06ff

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

Revert deleted line

92147d7

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

qiluo-msft approved these changes Oct 14, 2022

View reviewed changes

maipbui added the Static Code Analysis label Nov 23, 2022

maipbui marked this pull request as ready for review November 30, 2022 01:48

maipbui requested a review from lguohan as a code owner November 30, 2022 01:48

qnos reviewed Dec 8, 2022

View reviewed changes

qnos suggested changes Dec 8, 2022

View reviewed changes

maipbui added 2 commits December 8, 2022 02:12

Address PR comments

22bad5e

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

Address PR comments

34991a5

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

qnos approved these changes Dec 9, 2022

View reviewed changes

maipbui merged commit 51a1eb1 into sonic-net:master Dec 9, 2022

maipbui deleted the celestica_e1031_security branch December 9, 2022 15:30

qnos mentioned this pull request Jun 7, 2023

[DX010] fix possible cpld race access issue #15371

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[device/celestica] Mitigation for command injection vulnerability #11740

[device/celestica] Mitigation for command injection vulnerability #11740

maipbui commented Aug 15, 2022 •

edited

Loading

lgtm-com bot commented Aug 15, 2022

qiluo-msft commented Aug 17, 2022

qiluo-msft commented Aug 17, 2022

lgtm-com bot commented Sep 1, 2022

lgtm-com bot commented Sep 1, 2022

lgtm-com bot commented Sep 1, 2022

lgtm-com bot commented Sep 6, 2022

lgtm-com bot commented Sep 21, 2022

lgtm-com bot commented Sep 21, 2022

lgtm-com bot commented Oct 5, 2022

lgtm-com bot commented Oct 6, 2022

maipbui commented Nov 30, 2022

azure-pipelines bot commented Nov 30, 2022

qiluo-msft commented Nov 30, 2022

Blueve commented Dec 6, 2022 •

edited

Loading

qnos Dec 8, 2022 •

edited

Loading

maipbui Dec 8, 2022 •

edited

Loading

maipbui Dec 8, 2022

qnos Dec 8, 2022

maipbui Dec 8, 2022

qnos Dec 8, 2022

qnos Dec 8, 2022

qnos Dec 8, 2022

qnos Dec 8, 2022

qnos left a comment

lgtm-com bot commented Dec 8, 2022

[device/celestica] Mitigation for command injection vulnerability #11740

[device/celestica] Mitigation for command injection vulnerability #11740

Conversation

maipbui commented Aug 15, 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

Link to config_db schema for YANG module changes

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

lgtm-com bot commented Aug 15, 2022

qiluo-msft commented Aug 17, 2022

qiluo-msft commented Aug 17, 2022

lgtm-com bot commented Sep 1, 2022

lgtm-com bot commented Sep 1, 2022

lgtm-com bot commented Sep 1, 2022

lgtm-com bot commented Sep 6, 2022

lgtm-com bot commented Sep 21, 2022

lgtm-com bot commented Sep 21, 2022

lgtm-com bot commented Oct 5, 2022

lgtm-com bot commented Oct 6, 2022

maipbui commented Nov 30, 2022

azure-pipelines bot commented Nov 30, 2022

qiluo-msft commented Nov 30, 2022

Blueve commented Dec 6, 2022 • edited Loading

qnos Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

maipbui Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

maipbui Dec 8, 2022

Choose a reason for hiding this comment

qnos Dec 8, 2022

Choose a reason for hiding this comment

maipbui Dec 8, 2022

Choose a reason for hiding this comment

qnos Dec 8, 2022

Choose a reason for hiding this comment

qnos Dec 8, 2022

Choose a reason for hiding this comment

qnos Dec 8, 2022

Choose a reason for hiding this comment

qnos Dec 8, 2022

Choose a reason for hiding this comment

qnos left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Dec 8, 2022

maipbui commented Aug 15, 2022 •

edited

Loading

Blueve commented Dec 6, 2022 •

edited

Loading

qnos Dec 8, 2022 •

edited

Loading

maipbui Dec 8, 2022 •

edited

Loading