[device/celestica] Mitigation for command injection vulnerability by maipbui · Pull Request #11740 · sonic-net/sonic-buildimage

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


    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


    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


    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


    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


    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

Conversation

maipbui commented Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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)

Uh oh!

lgtm-com bot commented Aug 15, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qiluo-msft commented Aug 17, 2022

Uh oh!

qiluo-msft commented Aug 17, 2022

Uh oh!

lgtm-com bot commented Sep 1, 2022

Uh oh!

lgtm-com bot commented Sep 1, 2022

Uh oh!

lgtm-com bot commented Sep 1, 2022

Uh oh!

lgtm-com bot commented Sep 6, 2022

Uh oh!

Uh oh!

lgtm-com bot commented Sep 21, 2022

Uh oh!

lgtm-com bot commented Sep 21, 2022

Uh oh!

lgtm-com bot commented Oct 5, 2022

Uh oh!

lgtm-com bot commented Oct 6, 2022

Uh oh!

maipbui commented Nov 30, 2022

Uh oh!

azure-pipelines bot commented Nov 30, 2022

Uh oh!

qiluo-msft commented Nov 30, 2022

Uh oh!

Blueve commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qnos Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maipbui Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maipbui Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

qnos Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

maipbui Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

qnos Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

qnos Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

qnos Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

qnos Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

qnos left a comment

Choose a reason for hiding this comment

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