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

[consutil] replace shell=True #2725

Merged
merged 2 commits into from
Jun 7, 2023
Merged

[consutil] replace shell=True #2725

merged 2 commits into from
Jun 7, 2023

Conversation

maipbui
Copy link
Contributor

@maipbui maipbui commented Mar 7, 2023 •
edited
Loading

What I did

subprocess() - when using with shell=True is dangerous. Using subprocess function without a static string can lead to command injection.

How I did it

subprocess() - use shell=False instead, use list of strings Ref: https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation

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)

@maipbui
replace shell=True
c508119 
Signed-off-by: maipbui <[email protected]>
qiluo-msft
qiluo-msft reviewed Mar 7, 2023
View reviewed changes
consutil/lib.py Outdated
@@ -276,7 +277,7 @@ def init_device_prefix():
@staticmethod
def list_console_ttys():
"""Lists all console tty devices"""
cmd = "ls " + SysInfoProvider.DEVICE_PREFIX + "*"
cmd = ["ls", SysInfoProvider.DEVICE_PREFIX, "*"]
Copy link
Contributor

@qiluo-msft qiluo-msft Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

"*"

This is part of 2nd item, not another item. #Closed

qiluo-msft
qiluo-msft reviewed Mar 7, 2023
View reviewed changes
consutil/lib.py Outdated
@@ -326,15 +329,23 @@ def _parse_processes_info(output):

@staticmethod
def run_command(cmd, abort=True):
Copy link
Contributor

@qiluo-msft qiluo-msft Mar 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

run_command

Is it possible to implement this function by calling run_command_pipe? If yes, we can reuse some code. #Closed

@qiluo-msft qiluo-msft requested a review from Blueve March 7, 2023 21:43
@maipbui
address PR comments
33645ef 
Signed-off-by: maipbui <[email protected]>
@Blueve
Copy link
Contributor

Blueve commented Mar 21, 2023

@maipbui can you help fill the PR description templete? The change is looks good to me.

Blueve
Blueve requested changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@Blueve Blueve left a comment

Choose a reason for hiding this comment

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

Please help update the PR description form

@maipbui
Copy link
Contributor Author

maipbui commented Mar 31, 2023

Please help update the PR description form

Sure, I updated PR description, could you check?

@maipbui maipbui requested a review from Blueve June 6, 2023 05:12
@maipbui maipbui merged commit 56b6ac2 into sonic-net:master Jun 7, 2023
@maipbui maipbui deleted the consutil_pysec branch June 7, 2023 13:49
pdhruv-marvell pushed a commit to pdhruv-marvell/sonic-utilities that referenced this pull request Aug 23, 2023
@maipbui @pdhruv-marvell
[consutil] replace shell=True (sonic-net#2725)
6354cc2 
#### What I did
`subprocess()` - when using with `shell=True` is dangerous. Using subprocess function without a static string can lead to command injection.
#### How I did it
`subprocess()` - use `shell=False` instead, use list of strings Ref: [https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation](https://semgrep.dev/docs/cheat-sheets/python-command-injection/#mitigation)
#### How to verify it
Pass UT
Signed-off-by: maipbui <[email protected]>
@lizhijianrd lizhijianrd mentioned this pull request Jun 7, 2024
Merged
lizhijianrd added a commit that referenced this pull request Jun 9, 2024
@lizhijianrd
[consutil] Fix consule CLI and enhance unittest (#3360)
f96a132 
**What I did?**
1. Bugfix for console CLI (This is introduced by [consutil] replace shell=True #2725, * cannot be treated as wildcard correctly).
```
admin@sonic:~$ show line
ls: cannot access '/dev/C0-*': No such file or directory
```
2. Enhance UT to avoid regression mentioned in 1.
3. Fix incorrect statement in UT.
4. Fix critical Flake8 error.

**How to verify it**
1. Verified on Nokia-7215 MC0 device.
2. Verified by UT

Sign-Off By: Zhijian Li <[email protected]>
mssonicbld pushed a commit to mssonicbld/sonic-utilities that referenced this pull request Jun 11, 2024
@lizhijianrd @mssonicbld
[consutil] Fix consule CLI and enhance unittest (sonic-net#3360)
64e61cb 
**What I did?**
1. Bugfix for console CLI (This is introduced by [consutil] replace shell=True sonic-net#2725, * cannot be treated as wildcard correctly).
```
admin@sonic:~$ show line
ls: cannot access '/dev/C0-*': No such file or directory
```
2. Enhance UT to avoid regression mentioned in 1.
3. Fix incorrect statement in UT.
4. Fix critical Flake8 error.

**How to verify it**
1. Verified on Nokia-7215 MC0 device.
2. Verified by UT

Sign-Off By: Zhijian Li <[email protected]>
@mssonicbld mssonicbld mentioned this pull request Jun 11, 2024
Merged
mssonicbld pushed a commit that referenced this pull request Jun 11, 2024
@lizhijianrd @mssonicbld
[consutil] Fix consule CLI and enhance unittest (#3360)
79d8f5f 
**What I did?**
1. Bugfix for console CLI (This is introduced by [consutil] replace shell=True #2725, * cannot be treated as wildcard correctly).
```
admin@sonic:~$ show line
ls: cannot access '/dev/C0-*': No such file or directory
```
2. Enhance UT to avoid regression mentioned in 1.
3. Fix incorrect statement in UT.
4. Fix critical Flake8 error.

**How to verify it**
1. Verified on Nokia-7215 MC0 device.
2. Verified by UT

Sign-Off By: Zhijian Li <[email protected]>
mssonicbld pushed a commit to mssonicbld/sonic-utilities that referenced this pull request Jun 11, 2024
@lizhijianrd @mssonicbld
[consutil] Fix consule CLI and enhance unittest (sonic-net#3360)
62d3073 
**What I did?**
1. Bugfix for console CLI (This is introduced by [consutil] replace shell=True sonic-net#2725, * cannot be treated as wildcard correctly).
```
admin@sonic:~$ show line
ls: cannot access '/dev/C0-*': No such file or directory
```
2. Enhance UT to avoid regression mentioned in 1.
3. Fix incorrect statement in UT.
4. Fix critical Flake8 error.

**How to verify it**
1. Verified on Nokia-7215 MC0 device.
2. Verified by UT

Sign-Off By: Zhijian Li <[email protected]>
@mssonicbld mssonicbld mentioned this pull request Jun 11, 2024
Merged
mssonicbld pushed a commit that referenced this pull request Jun 12, 2024
@lizhijianrd @mssonicbld
[consutil] Fix consule CLI and enhance unittest (#3360)
ea9497a 
**What I did?**
1. Bugfix for console CLI (This is introduced by [consutil] replace shell=True #2725, * cannot be treated as wildcard correctly).
```
admin@sonic:~$ show line
ls: cannot access '/dev/C0-*': No such file or directory
```
2. Enhance UT to avoid regression mentioned in 1.
3. Fix incorrect statement in UT.
4. Fix critical Flake8 error.

**How to verify it**
1. Verified on Nokia-7215 MC0 device.
2. Verified by UT

Sign-Off By: Zhijian Li <[email protected]>
arfeigin pushed a commit to arfeigin/sonic-utilities that referenced this pull request Jun 16, 2024
@lizhijianrd @arfeigin
[consutil] Fix consule CLI and enhance unittest (sonic-net#3360)
49fc4cd 
**What I did?**
1. Bugfix for console CLI (This is introduced by [consutil] replace shell=True sonic-net#2725, * cannot be treated as wildcard correctly).
```
admin@sonic:~$ show line
ls: cannot access '/dev/C0-*': No such file or directory
```
2. Enhance UT to avoid regression mentioned in 1.
3. Fix incorrect statement in UT.
4. Fix critical Flake8 error.

**How to verify it**
1. Verified on Nokia-7215 MC0 device.
2. Verified by UT

Sign-Off By: Zhijian Li <[email protected]>
mssonicbld pushed a commit to mssonicbld/sonic-utilities that referenced this pull request Jun 20, 2024
@lizhijianrd @mssonicbld
[consutil] Fix consule CLI and enhance unittest (sonic-net#3360)
0f59671 
**What I did?**
1. Bugfix for console CLI (This is introduced by [consutil] replace shell=True sonic-net#2725, * cannot be treated as wildcard correctly).
```
admin@sonic:~$ show line
ls: cannot access '/dev/C0-*': No such file or directory
```
2. Enhance UT to avoid regression mentioned in 1.
3. Fix incorrect statement in UT.
4. Fix critical Flake8 error.

**How to verify it**
1. Verified on Nokia-7215 MC0 device.
2. Verified by UT

Sign-Off By: Zhijian Li <[email protected]>
@mssonicbld mssonicbld mentioned this pull request Jun 20, 2024
Merged
mssonicbld pushed a commit that referenced this pull request Jul 12, 2024
@lizhijianrd @mssonicbld
[consutil] Fix consule CLI and enhance unittest (#3360)
18c17ae 
**What I did?**
1. Bugfix for console CLI (This is introduced by [consutil] replace shell=True #2725, * cannot be treated as wildcard correctly).
```
admin@sonic:~$ show line
ls: cannot access '/dev/C0-*': No such file or directory
```
2. Enhance UT to avoid regression mentioned in 1.
3. Fix incorrect statement in UT.
4. Fix critical Flake8 error.

**How to verify it**
1. Verified on Nokia-7215 MC0 device.
2. Verified by UT

Sign-Off By: Zhijian Li <[email protected]>
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

@Blueve Blueve Blueve approved these changes

Assignees
No one assigned
Labels
static-code-analysis
Projects
None yet
Milestone
No milestone
Development

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