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/mellanox] Mitigation for security vulnerability #11877

Merged
merged 12 commits into from
Oct 6, 2022
Merged

[device/mellanox] Mitigation for security vulnerability #11877

merged 12 commits into from
Oct 6, 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

subprocess.Popen() and subprocess.check_output() is used with shell=True, which is very dangerous for shell injection.

How I did it

Disable shell=True, enable shell=False

How to verify it

Tested on DUT, compare and verify the output between the original behavior and the new changes' behavior.
testresults.zip

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 subprocess
0b2d9ff 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Convert command string to command array
3953ca2 
Signed-off-by: maipbui <maibui@microsoft.com>
qiluo-msft
qiluo-msft approved these changes Aug 30, 2022
View reviewed changes
Copy link
Collaborator

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

LGTM. Please wait more reviewers to verify and approve.

@maipbui maipbui marked this pull request as ready for review September 1, 2022 04:35
@Yarden-Z
Copy link

I see that this change was done only for 2700, we do not want to backport this?

@maipbui
Copy link
Contributor Author

maipbui commented Sep 13, 2022

I see that this change was done only for 2700, we do not want to backport this?

@Yarden-Z, Qi added backport. can you check you can review and approve?

@Yarden-Z
Copy link

I see that this change was done only for 2700, we do not want to backport this?

@Yarden-Z, Qi added backport. can you check you can review and approve?

Seems as though it is referenced to 2700 in other platforms.
Verified and approved

@maipbui
Replace shell=True and os.system in mlnx-platform-api
d273d60 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Replace shell=True
34ab55c 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Resolve conflict
7e3d01e 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Add pytest import
98ef8c2 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Merge branch 'master' into mellanox_security
bd9cf87
@lgtm-com
Copy link

lgtm-com bot commented Sep 16, 2022

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

fixed alerts:

  • 1 for Unused import

@maipbui
Fix command
20e2cd7 
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 20e2cd7 into a1b50ca - view on LGTM.com

fixed alerts:

  • 1 for Unused import

qiluo-msft
qiluo-msft reviewed Sep 16, 2022
View reviewed changes
platform/mellanox/mlnx-platform-api/sonic_platform/component.py Outdated
fs_path = p3.communicate()[0].rstrip('\n')
p1.wait()
p2.wait()
if p1.returncode != 0 and p2.returncode != 0 and p3.returncode != 0:
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.

and

or #Closed

qiluo-msft
qiluo-msft reviewed Sep 16, 2022
View reviewed changes
platform/mellanox/mlnx-platform-api/sonic_platform/component.py Outdated
cmd1 = ['fdisk', '-l']
cmd2 = ['grep', 'ONIE boot']
cmd3 = ['awk', '{print $1}']
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 move to common library, maybe the 2 cmd function could be extended to multiple cmds. #Closed

@maipbui
Use common lib function
a16ed29 
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 1 when merging a16ed29 into 1effff9 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

fixed alerts:

  • 1 for Unused import

@maipbui
Copy link
Contributor Author

maipbui commented Sep 20, 2022

@Yarden-Z I made some changes, could you help review?

@maipbui
Copy link
Contributor Author

maipbui commented Sep 20, 2022

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Yarden-Z
Copy link

Yarden-Z commented Oct 3, 2022

@Yarden-Z I made some changes, could you help review?

Went over the new commit, did not find anything new to comment on.

@lgtm-com
Copy link

lgtm-com bot commented Oct 3, 2022

This pull request introduces 1 alert and fixes 1 when merging ba3c5de into 2f46689 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

fixed alerts:

  • 1 for Unused import

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

lgtm-com bot commented Oct 5, 2022

This pull request introduces 1 alert and fixes 1 when merging 1e62cff into 1f0699f - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

fixed alerts:

  • 1 for Unused import

@maipbui
Remove run
c35ec09 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Oct 5, 2022

This pull request introduces 1 alert and fixes 1 when merging c35ec09 into 1f0699f - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

fixed alerts:

  • 1 for Unused import

@maipbui maipbui merged commit 648ca07 into sonic-net:master Oct 6, 2022
@maipbui maipbui deleted the mellanox_security branch October 6, 2022 21:51
stephenxs
stephenxs reviewed Oct 21, 2022
View reviewed changes
platform/mellanox/mlnx-platform-api/sonic_platform/component.py
cmd = "mount -n -r -t ext4 {} {}".format(fs_path, fs_mountpoint)
subprocess.check_call(cmd, shell=True, universal_newlines=True)
cmd = ["mount", "-n", "-r", "-t", "ext4", fs_path, fs_mountpoint]
subprocess.check_call(cmd, universal_newlines=True)
Copy link
Collaborator

@stephenxs stephenxs Oct 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It is not able to mount fs without shell=True.
Looks like it doesn't have the privilege to access /dev/sda2/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could you share the error log?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My bad. it doesn't relevant to the PR. it's because there is a \n at the end of /dev/sda2.
We will fix it on the platform API side.
Sorry for the inconvenient.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry. I realized the missing \n was still introduced by this PR. PR #12465 to fix it. can you please review it?
thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the fix!

@stephenxs stephenxs mentioned this pull request Oct 22, 2022
Merged
7 tasks
liat-grozovik pushed a commit that referenced this pull request Oct 23, 2022
@stephenxs
Remove \n from the end of fs_path in ONIEUpdater (#12465)
8c73e68 
This fixes the following error

```
admin@sonic:~$ sudo fwutil show status
mount: /mnt/onie-fs: special device /dev/sda2
 does not exist.
Error: Command '['mount', '-n', '-r', '-t', 'ext4', '/dev/sda2\n', '/mnt/onie-fs']' returned non-zero exit status 32.. Aborting...
Aborted!
admin@sonic:~$ sudo vi /usr/local/lib/python3.9/dist-packages/sonic_platform/

```
Seems like #11877 the rstrip('\n') was removed. Probably by mistake.

Signed-off-by: Stephen Sun <stephens@nvidia.com>
richardyu-ms pushed a commit to richardyu-ms/sonic-buildimage that referenced this pull request Nov 16, 2022
@yxieca
Merged PR 6880849: Manual merge github master to internal branch
2b16e56 
Related work items: sonic-net#2151, sonic-net#2194, sonic-net#2224, sonic-net#2237, sonic-net#2264, sonic-net#2281, sonic-net#2286, sonic-net#2297, sonic-net#2299, sonic-net#2305, sonic-net#2325, sonic-net#2335, sonic-net#2338, sonic-net#2341, sonic-net#2343, sonic-net#2347, sonic-net#2350, sonic-net#2355, sonic-net#2356, sonic-net#2358, sonic-net#2360, sonic-net#2363, sonic-net#2367, sonic-net#2368, sonic-net#2370, sonic-net#2374, sonic-net#2392, sonic-net#2398, sonic-net#2408, sonic-net#2414, sonic-net#2415, sonic-net#2419, sonic-net#2421, sonic-net#2422, sonic-net#2423, sonic-net#2426, sonic-net#2427, sonic-net#2430, sonic-net#2431, sonic-net#2433, sonic-net#2434, sonic-net#2436, sonic-net#2437, sonic-net#2441, sonic-net#2444, sonic-net#2445, sonic-net#2446, sonic-net#2456, sonic-net#2458, sonic-net#2460, sonic-net#2461, sonic-net#2463, sonic-net#2472, sonic-net#2475, sonic-net#11877, sonic-net#12024, sonic-net#12065, sonic-net#12097, sonic-net#12130, sonic-net#12209, sonic-net#12217, sonic-net#12244, sonic-net#12251, sonic-net#12255, sonic-net#12276, sonic-net#12284
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephenxs stephenxs stephenxs left review comments

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

@dprital dprital Awaiting requested review from dprital

@dgsudharsan dgsudharsan Awaiting requested review from dgsudharsan

@liat-grozovik liat-grozovik Awaiting requested review from liat-grozovik

@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.
4 participants
@maipbui @Yarden-Z @stephenxs @qiluo-msft