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

Service validator vul #3717

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mohmamedali
Copy link

What I did

"os.system()" uses shell invocation to execute command which is dangerous. without a static string that can lead to command injection.

How I did it

pass use list of strings to subprocess() - use shell=False instead.

How to verify it

pass UT

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

FengPan-Frank
FengPan-Frank previously approved these changes Jan 16, 2025
Copy link
Contributor

@FengPan-Frank FengPan-Frank left a comment

Choose a reason for hiding this comment

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

LGTM

def _service_restart(svc_name):
rc = os.system(f"systemctl restart {svc_name}")
rc = run_command(['systemctl', 'restart', svc_name])
Copy link
Contributor

Choose a reason for hiding this comment

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

rc = run_command(['systemctl', 'restart', svc_name])

I think simply replace os.system with subprocess.call, as suggested in python doc https://docs.python.org/3/library/subprocess.html#replacing-os-system, no need to introduce new function run_command

Copy link
Author

Choose a reason for hiding this comment

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

Yes, Done!

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

linux-foundation-easycla bot commented Mar 10, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mohmamedali mohmamedali force-pushed the service_validator_vul branch from 43c0b2d to 59956dc Compare March 10, 2025 10:04
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@maipbui
Copy link
Contributor

maipbui commented Mar 11, 2025

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants