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.

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

[nokia] Replace os.system and remove subprocess with shell=True #12100

Merged
merged 6 commits into from
Dec 1, 2022

Conversation

maipbui
Copy link
Contributor

@maipbui maipbui commented Sep 17, 2022

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

Dependency: #12065

Why I did it

subprocess.Popen() and subprocess.run() is used with shell=True, which is very dangerous for shell injection.
os - not secure against maliciously constructed input and dangerous if used to evaluate dynamic content
getstatusoutput is dangerous because it contains shell=True in the implementation

How I did it

Replace os by subprocess, use with shell=False
Remove unused functions

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

Ensure to add label/tag for the feature raised. example - PR#2174 where, Generic Config and Update feature has been labelled as GCU.

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>
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui maipbui requested a review from qiluo-msft September 17, 2022 21:56
Signed-off-by: maipbui <maibui@microsoft.com>
Signed-off-by: maipbui <maibui@microsoft.com>
os.system("echo 24c02 0x53 > /sys/class/i2c-adapter/i2c-0/new_device")
file = "/sys/class/i2c-adapter/i2c-0/new_device"
with open(file, 'w') as f:
f.write('24c02 0x53')
Copy link
Collaborator

@qiluo-msft qiluo-msft Sep 18, 2022

Choose a reason for hiding this comment

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

0x53

Did you miss \n? #Closed

@@ -331,7 +327,7 @@ def set_status_led(self, color):

# Write sys led
if smbus_present == 0: # called from host (e.g. 'show system-health')
cmdstatus, value = cmd.getstatusoutput('sudo i2cset -y 0 0x41 0x7 %d' % value)
cmdstatus, value = getstatusoutput_noshell(['sudo', 'i2cset', '-y', '0', '0x41', '0x7', value])
Copy link
Collaborator

@qiluo-msft qiluo-msft Sep 18, 2022

Choose a reason for hiding this comment

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

value

str(value). #Closed

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

maipbui commented Oct 7, 2022

@carl-nokia @dflynn-Nokia Could you help review and verify?

@maipbui
Copy link
Contributor Author

maipbui commented Oct 11, 2022

@mlok-nokia 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
Copy link
Collaborator

@Blueve is connecting Nokia reviewers.

@Blueve
Copy link
Contributor

Blueve commented Dec 1, 2022

@mlok-nokia

@mlok-nokia
Copy link
Contributor

I am ok with the change. Thanks

@maipbui maipbui merged commit 2b3e884 into sonic-net:master Dec 1, 2022
@maipbui maipbui deleted the nokia_sec branch December 1, 2022 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants