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/juniper] Mitigation for security vulnerability #11838

Merged
merged 19 commits into from
Nov 22, 2022
Merged

[device/juniper] Mitigation for security vulnerability #11838

merged 19 commits into from
Nov 22, 2022

Conversation

maipbui
Copy link
Contributor

@maipbui maipbui commented Aug 24, 2022 •
edited
Loading

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

Dependency: #12065

Why I did it

commands module is not secure
command injection in getstatusoutput being used without a static string

How I did it

Eliminate commands module, use subprocess module only
Convert Python 2 to Python 3

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
Replace subprocess getstatusoutput by run
04d7d16 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui maipbui requested a review from qiluo-msft August 24, 2022 18:02
@maipbui
Fix var name
fba2445 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Add capture_output and minimize code change
e783a42 
Signed-off-by: maipbui <maibui@microsoft.com>
@qiluo-msft
Copy link
Collaborator

@ciju-juniper Could you help verify and review?

@lgtm-com
Copy link

lgtm-com bot commented Aug 24, 2022

This pull request introduces 1 alert when merging e783a42 into adffbd4 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

maipbui added 3 commits August 24, 2022 22:20
@maipbui
Remove unused variable
25af1be 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Add more args
954c0da 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Remove center directory
83d60d5 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Remove os.system
f380aae 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Aug 31, 2022

This pull request introduces 3 alerts when merging f380aae into 6e878a3 - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Unused import

@maipbui
Remove shutil and use subprocess and with
2fa4ded 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Use with
109303e 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Simplify code
d40d086 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Sep 1, 2022

This pull request introduces 1 alert when merging d40d086 into 88191b0 - view on LGTM.com

new alerts:

  • 1 for Unused import

@maipbui
Fix lgtm
b44a588 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui maipbui marked this pull request as ready for review September 2, 2022 13:34
@maipbui
Output text processing
a000b35 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Copy link
Contributor Author

maipbui commented Sep 12, 2022

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@maipbui
Copy link
Contributor Author

maipbui commented Sep 13, 2022

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@maipbui
Replace unsafe funcs in platform
9acf59c 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui maipbui requested a review from lguohan as a code owner September 19, 2022 15:26
@lgtm-com
Copy link

lgtm-com bot commented Sep 19, 2022

This pull request introduces 6 alerts when merging 9acf59c into c243af0 - view on LGTM.com

new alerts:

  • 4 for Unused import
  • 1 for First parameter of a method is not named 'self'
  • 1 for Wrong number of arguments in a call

@maipbui
Fix lgtm
e667990 
Signed-off-by: maipbui <maibui@microsoft.com>
qiluo-msft
qiluo-msft reviewed Sep 19, 2022
View reviewed changes
platform/broadcom/sonic-platform-modules-juniper/qfx5210/utils/juniper_qfx5210_util.py Outdated
@@ -474,7 +473,7 @@ def set_device(args):
if int(args[1])>4:
show_set_help()
return
#print ALL_DEVICE['led']
#print( ALL_DEVICE['led']
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.

(

This change does not make sense. Please revert. #Closed

qiluo-msft
qiluo-msft reviewed Sep 19, 2022
View reviewed changes
platform/broadcom/sonic-platform-modules-juniper/qfx5210/utils/juniper_qfx5210_util.py Outdated
@@ -506,7 +505,7 @@ def set_device(args):
show_set_help()
return

#print ALL_DEVICE[args[0]]
#print( ALL_DEVICE[args[0]]
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.

(

Please revert. #Closed

qiluo-msft
qiluo-msft reviewed Sep 19, 2022
View reviewed changes
platform/broadcom/sonic-platform-modules-juniper/qfx5210/utils/juniper_qfx5210_util.py
print(i.upper()+": ")
print("============================================")

for j in sorted(ALL_DEVICE[i].keys(), key=get_value):
print " "+j+":",
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.

,

Original print without newline, the new code changes behavior. #Closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Strictly speaking, you are using a blank as end, you should use empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think blank space should be used, ref: https://stackoverflow.com/a/18908914/19880750

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree.

@lgtm-com
Copy link

lgtm-com bot commented Oct 5, 2022

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

new alerts:

  • 1 for Comparison using is when operands support `__eq__`

@maipbui
Fix lgtm
59ce323 
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Copy link
Contributor Author

maipbui commented Oct 18, 2022

@ciju-juniper Could you help review and verify?

@maipbui
Fix code to be consistent with original behavior
fb8b24f 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Nov 21, 2022

This pull request introduces 2 alerts when merging fb8b24f into 7b4032e - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Syntax error

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.

@maipbui
fix lgtm
2b45169 
Signed-off-by: maipbui <maibui@microsoft.com>
@lgtm-com
Copy link

lgtm-com bot commented Nov 21, 2022

This pull request introduces 1 alert when merging 2b45169 into 77b1be7 - view on LGTM.com

new alerts:

  • 1 for Syntax error

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.

@maipbui maipbui merged commit 2f6b34a into sonic-net:master Nov 22, 2022
@maipbui maipbui deleted the juniper_security branch November 22, 2022 15:46
StormLiangMS pushed a commit to StormLiangMS/sonic-buildimage that referenced this pull request Dec 8, 2022
@maipbui @StormLiangMS
[device/juniper] Mitigation for security vulnerability (sonic-net#11838)
8f52ee0 
Signed-off-by: maipbui maibui@microsoft.com
Dependency: [https://github.com/sonic-net/sonic-buildimage/pull/12065](https://github.com/sonic-net/sonic-buildimage/pull/12065)
#### Why I did it
`commands` module is not secure
command injection in `getstatusoutput` being used without a static string
#### How I did it
Eliminate `commands` module, use `subprocess` module only
Convert Python 2 to Python 3
StormLiangMS pushed a commit that referenced this pull request Dec 10, 2022
@maipbui @StormLiangMS
[device/juniper] Mitigation for security vulnerability (#11838)
4963c1c 
Signed-off-by: maipbui maibui@microsoft.com
Dependency: [https://github.com/sonic-net/sonic-buildimage/pull/12065](https://github.com/sonic-net/sonic-buildimage/pull/12065)
#### Why I did it
`commands` module is not secure
command injection in `getstatusoutput` being used without a static string
#### How I did it
Eliminate `commands` module, use `subprocess` module only
Convert Python 2 to Python 3
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.
2 participants
@maipbui @qiluo-msft