Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[device/dell] Mitigation for security vulnerability #11875

Merged
merged 10 commits into from
Jan 6, 2023
Merged

[device/dell] Mitigation for security vulnerability #11875

merged 10 commits into from
Jan 6, 2023

Conversation

maipbui
Copy link
Contributor

@maipbui maipbui commented Aug 29, 2022 •
edited
Loading

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

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

Why I did it

commands module is not protected against malicious input
getstatusoutput is detected without a static string, uses shell=True

How I did it

Eliminate the use of commands
Use subprocess.run(), commands in subprorcess.run() are totally static
Fix indentation

How to verify it

Tested on DUT
dell_log.txt

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

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205

Description for the changelog

Link to config_db schema for YANG module changes

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

@maipbui maipbui requested a review from qiluo-msft August 29, 2022 16:06
@maipbui
Replace subprocess getstatusoutput to subprocess run
24bab7a 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Fix ipmi_sdr_list
a1bd8ec 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui maipbui requested a review from qiluo-msft August 31, 2022 04:26
@lgtm-com
Copy link

lgtm-com bot commented Aug 31, 2022

This pull request introduces 6 alerts and fixes 1 when merging a1bd8ec into 1e75abc - view on LGTM.com

new alerts:

  • 4 for Variable defined multiple times
  • 2 for Syntax error

fixed alerts:

  • 1 for Variable defined multiple times

@maipbui
Fix indentation and use similar old logic
867da79 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Remove redundant replace
cd61bca 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Sep 6, 2022

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

new alerts:

  • 6 for Variable defined multiple times

fixed alerts:

  • 3 for Unused import
  • 2 for Unused local variable
  • 2 for Variable defined multiple times

@maipbui
Add awk back
017f143 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Sep 8, 2022

This pull request introduces 2 alerts and fixes 12 when merging 017f143 into 38cc35f - view on LGTM.com

new alerts:

  • 2 for Variable defined multiple times

fixed alerts:

  • 6 for Unused local variable
  • 4 for Unused import
  • 2 for Variable defined multiple times

@maipbui
Add function in sonic_py_common library
8593247 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Sep 13, 2022

This pull request introduces 1 alert and fixes 12 when merging 8593247 into 7d1b99a - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

fixed alerts:

  • 6 for Unused local variable
  • 4 for Unused import
  • 2 for Variable defined multiple times

@maipbui
Fix bug
7f54f67 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Sep 13, 2022

This pull request introduces 1 alert and fixes 12 when merging 7f54f67 into 7d1b99a - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

fixed alerts:

  • 6 for Unused local variable
  • 4 for Unused import
  • 2 for Variable defined multiple times

@maipbui
Replace unsafe functions in platform directory
f74eda9 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Sep 16, 2022

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

new alerts:

  • 1 for Variable defined multiple times

fixed alerts:

  • 6 for Unused local variable
  • 4 for Unused import
  • 2 for Variable defined multiple times

qiluo-msft
qiluo-msft reviewed Sep 16, 2022
View reviewed changes
platform/broadcom/sonic-platform-modules-dell/s5224f/scripts/platform_sensors.py
file = '/sys/module/ipmi_si/parameters/kipmid_max_busy_us'
if os.path.exists(file):
with open(file, 'w') as f:
f.write('0\n')
Copy link
Collaborator

@qiluo-msft qiluo-msft Sep 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are multiple ways to fail the block, for example, could not open file, or write failure. #Closed

qiluo-msft
qiluo-msft reviewed Sep 16, 2022
View reviewed changes
platform/broadcom/sonic-platform-modules-dell/s5224f/scripts/platform_sensors.py
file = '/sys/module/ipmi_si/parameters/kipmid_max_busy_us'
if os.path.exists(file):
with open(file, 'w') as f:
f.write('1000\n')
Copy link
Collaborator

@qiluo-msft qiluo-msft Sep 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

write

The same. #Closed

@maipbui
fix to exception
f320820 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Sep 18, 2022

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

new alerts:

  • 1 for Variable defined multiple times

fixed alerts:

  • 6 for Unused local variable
  • 4 for Unused import
  • 2 for Variable defined multiple times

qiluo-msft
qiluo-msft reviewed Sep 19, 2022
View reviewed changes
device/dell/x86_64-dellemc_z9332f_d1508-r0/plugins/psuutil.py Outdated
@@ -46,28 +42,25 @@ def isDockerEnv(self):

# Fetch a BMC register
def get_pmc_register(self, reg_name):

status = 1
Copy link
Collaborator

@qiluo-msft qiluo-msft Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

status = 1

lgtm said "This assignment to 'status' is unnecessary as it is redefined here before this value is used." #Closed

@maipbui
Fix lgtm
43934a5 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Oct 5, 2022

This pull request fixes 12 alerts when merging 43934a5 into 1f0699f - view on LGTM.com

fixed alerts:

  • 6 for Unused local variable
  • 4 for Unused import
  • 2 for Variable defined multiple times

@maipbui
Copy link
Contributor Author

maipbui commented Oct 7, 2022

@srideepDell @santhosh-kt @thaj-deen @arunlk-dell Could you help review and verify?

@maipbui maipbui marked this pull request as ready for review November 30, 2022 01:48
@maipbui maipbui requested a review from lguohan as a code owner November 30, 2022 01:48
@maipbui
Copy link
Contributor Author

maipbui commented Nov 30, 2022

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@srideepDell
Copy link
Contributor

changes looks good and also from the log.

@qiluo-msft qiluo-msft merged commit 06e1a0b into sonic-net:master Jan 6, 2023
@maipbui maipbui deleted the dell_security branch January 6, 2023 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiluo-msft qiluo-msft qiluo-msft approved these changes

@lguohan lguohan Awaiting requested review from lguohan lguohan is a code owner

Assignees
No one assigned
Labels
Static Code Analysis
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maipbui @srideepDell @qiluo-msft