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/marvell] Mitigation for security vulnerability #11876

Merged
merged 16 commits into from
Nov 30, 2022
Merged

[device/marvell] Mitigation for security vulnerability #11876

merged 16 commits into from
Nov 30, 2022

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

os and commands modules are not secure against maliciously constructed input
getstatusoutput is detected without a static string, uses shell=True

How I did it

Eliminate the use of os and commands
Use subprocess instead

How to verify it

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
Mitigate security vulnerabilities in os and commands
9a05f34 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Aug 29, 2022

This pull request introduces 6 alerts and fixes 4 when merging 9a05f34 into 3bf1abb - view on LGTM.com

new alerts:

  • 5 for Unused import
  • 1 for Syntax error

fixed alerts:

  • 3 for Unused local variable
  • 1 for Unreachable code

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

lgtm-com bot commented Aug 29, 2022

This pull request introduces 2 alerts and fixes 5 when merging a455cae into 3bf1abb - view on LGTM.com

new alerts:

  • 2 for Syntax error

fixed alerts:

  • 3 for Unused local variable
  • 1 for Redundant comparison
  • 1 for Unreachable code

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

lgtm-com bot commented Aug 29, 2022

This pull request introduces 1 alert and fixes 1 when merging 3702046 into 3bf1abb - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

fixed alerts:

  • 1 for Unused local variable

@maipbui
Convert cmd string to cmd array
23f0174 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Add args
db35a2a 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Aug 30, 2022

This pull request introduces 1 alert and fixes 1 when merging db35a2a into 092e039 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

fixed alerts:

  • 1 for Unused local variable

@maipbui
Write str to file not using subprocess
0296296 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Rewrite str to file
394afe0 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Open file using with
076bac8 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Sep 1, 2022

This pull request introduces 1 alert and fixes 1 when merging 076bac8 into 88191b0 - view on LGTM.com

new alerts:

  • 1 for Syntax error

fixed alerts:

  • 1 for Redundant comparison

@maipbui
Fix lgtm
9766191 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Use with
4f8874e 
Signed-off-by: maipbui <maibui@microsoft.com>
@qiluo-msft
Copy link
Collaborator

@Sabareesh-Kumar-Anandan @antony-rheneus @shilimkarvg Could you help verify and review this PR?

@maipbui maipbui marked this pull request as ready for review September 2, 2022 21:28
@maipbui
Add output processing
ccc7e52 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Sep 8, 2022

This pull request fixes 1 alert when merging ccc7e52 into 38cc35f - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@maipbui
Use functions from sonic-py-common
072d8e9 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Sep 13, 2022

This pull request fixes 1 alert when merging 072d8e9 into 7d1b99a - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@maipbui
Replace os.system and shell=True
dbd999a 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
replace getstatusoutput
e2aef69 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Sep 16, 2022

This pull request fixes 1 alert when merging e2aef69 into a1b50ca - view on LGTM.com

fixed alerts:

  • 1 for Unused import

qiluo-msft
qiluo-msft reviewed Sep 16, 2022
View reviewed changes
platform/marvell-armhf/sonic-platform-nokia/7215/sonic_platform/component.py Outdated
cmd1 = ['grep', '--null-data', 'U-Boot', '/dev/mtd0ro']
cmd2 = ['head', '-1']
cmd3 = ['cut', '-d', ' ', '-f2-4']
with subprocess.Popen(cmd1, universal_newlines=True, stdout=subprocess.PIPE) as p1:
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.

with subprocess.Popen(cmd1, universal_newlines=True, stdout=subprocess.PIPE) as p1:

Possible to reuse 3 cmds function? #WontFix

@qiluo-msft
Copy link
Collaborator

qiluo-msft commented Sep 16, 2022 •
edited
Loading 

import os

Nokia's folder should be in a separate PR.

In reply to: 1249925323

In reply to: 1249925323

Refers to: platform/marvell-armhf/sonic-platform-nokia/7215/sonic_platform/chassis.py:9 in e2aef69. [](commit_id = e2aef69, deletion_comment = False)

@lgtm-com
Copy link

lgtm-com bot commented Sep 17, 2022

This pull request fixes 1 alert when merging fd912a9 into 1effff9 - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@maipbui
change run to call
11cf6d9 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Oct 5, 2022

This pull request fixes 1 alert when merging 11cf6d9 into 1f0699f - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@maipbui
Copy link
Contributor Author

maipbui commented Oct 7, 2022

@antony-rheneus Could you help review and verify?

@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).

@qiluo-msft qiluo-msft merged commit 0bd3be3 into sonic-net:master Nov 30, 2022
@maipbui maipbui deleted the marvell_security branch November 30, 2022 15:41
StormLiangMS pushed a commit to StormLiangMS/sonic-buildimage that referenced this pull request Dec 8, 2022
@maipbui @StormLiangMS
[device/marvell] Mitigation for security vulnerability (sonic-net#11876)
4c8162c 
#### Why I did it
`os` and `commands` modules are not secure against maliciously constructed input
`getstatusoutput` is detected without a static string, uses `shell=True`
#### How I did it
Eliminate the use of `os` and `commands`
Use `subprocess` instead
StormLiangMS pushed a commit that referenced this pull request Dec 10, 2022
@maipbui @StormLiangMS
[device/marvell] Mitigation for security vulnerability (#11876)
c3c37f4 
#### Why I did it
`os` and `commands` modules are not secure against maliciously constructed input
`getstatusoutput` is detected without a static string, uses `shell=True`
#### How I did it
Eliminate the use of `os` and `commands`
Use `subprocess` instead
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

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

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