Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

CI: Add Security Check Using Bandit in CI #3312

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

CI: Add Security Check Using Bandit in CI #3312

wants to merge 3 commits into from

Conversation

ChengyuZhu6
Copy link

@ChengyuZhu6 ChengyuZhu6 commented Sep 12, 2024 •
edited
Loading

Description

Currently, the project does not have a security linter integrated into its CI pipeline. This poses potential risks as security vulnerabilities in Python code can go undetected. I found many high security issues using Bandit

error log: https://github.com/user-attachments/files/16975951/security-issues.log

Resolve two high security issues:

  • [B602:subprocess_popen_with_shell_equals_true] subprocess call with shell=True identified
  • [B605:start_process_with_a_shell] Starting a process with a shell, possible injection detected

Fixes #3311

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Feature/Issue validation/testing

> pip install bandit
> bandit -r . --severity-level high -s B501 # Skip the B501 rule related to SSL certificate validation checks
Working... ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:03
Run started:2024-09-12 08:45:39.950729

Test results:
	No issues identified.

Code scanned:
	Total lines of code: 34920
	Total lines skipped (#nosec): 0
	Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

Run metrics:
	Total issues (by severity):
		Undefined: 0
		Low: 1075
		Medium: 222
		High: 0
	Total issues (by confidence):
		Undefined: 0
		Low: 200
		Medium: 20
		High: 1077
Files skipped (0):

@mreso mreso self-requested a review September 12, 2024 19:10
mreso
mreso requested changes Sep 12, 2024
View reviewed changes
Copy link
Collaborator

@mreso mreso left a comment

Choose a reason for hiding this comment

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

Generally LGTM, thank you for this contribution! Just blocking this to work out potential quirks, run some tests and coordinate with release cycle

ts_scripts/install_dependencies.py Outdated Show resolved Hide resolved
ts_scripts/regression_utils.py
print("Could not generate gRPC client stubs")
sys.exit(1)

cmd = "python -m pytest -v ./ --ignore=sanity"
pytest_cmd = ["python", "-m", "pytest", "-v", "./", "--ignore=sanity"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This command is not running in REPO_ROOT/test/pytest/ which might break some tests. Not 100% sure if all of of them are resilient to that.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but I haven’t changed the execution logic. So, if it worked before, it should be fine now.

ts_scripts/sanity_utils.py Outdated Show resolved Hide resolved
ts_scripts/sanity_utils.py Show resolved Hide resolved
ts_scripts/sanity_utils.py Outdated Show resolved Hide resolved
@ChengyuZhu6 ChengyuZhu6 force-pushed the secure-check branch 3 times, most recently from c1d296d to 47a194a Compare September 13, 2024 01:21
@ChengyuZhu6
Copy link
Author

Generally LGTM, thank you for this contribution! Just blocking this to work out potential quirks, run some tests and coordinate with release cycle

Thanks for your review @mreso. I have addressed all your comments. PTAL thanks.

@ChengyuZhu6 ChengyuZhu6 requested a review from mreso September 13, 2024 07:12
@ChengyuZhu6
Copy link
Author

Just rebased to pick up the latest changes.

@ChengyuZhu6
Copy link
Author

kindly ping @mreso

@ChengyuZhu6
CI: Add Bandit security check to lint workflow
15f0300 
- Integrate Bandit to scan for security issues in the codebase.
- Configure Bandit to fail the workflow if any high-severity issues are found.

e.g.:

```bash
>> Issue: [B605:start_process_with_a_shell] Starting a process with a shell, possible injection detected, security issue.
   Severity: High   Confidence: High
   CWE: CWE-78 (https://cwe.mitre.org/data/definitions/78.html)
   More Info: https://bandit.readthedocs.io/en/1.7.9/plugins/b605_start_process_with_a_shell.html
   Location: ./binaries/build.py:52:30
51	        if not args.dry_run:
52	            build_exit_code = os.system(cur_wheel_cmd)
53	            # If any one of the steps fail, exit with error
```

Fixes: pytorch#3311

Signed-off-by: ChengyuZhu6 <chengyu.zhu@intel.com>
@ChengyuZhu6
security-fix: resolve security issue with subprocess call using shell…
e7a26d4 
…=True

- Issue: Fixed [B602:subprocess_popen_with_shell_equals_true] identified by Bandit,
  which flagged the use of `subprocess.Popen` with `shell=True` as a high-severity security risk (CWE-78: OS Command Injection).
- Ensures that the command is executed more securely without exposing it to shell injection vulnerabilities.

Signed-off-by: ChengyuZhu6 <chengyu.zhu@intel.com>
@ChengyuZhu6
security-fix: resolve security issue with starting a process with a s…
a7e7d6e 
…hell

- Issue: Fixed [B605:start_process_with_a_shell] identified by Bandit,
  which flagged starting a process with a shell as a high-severity security risk (CWE-78: OS Command Injection).
- Replaced os.system call with a safer alternative to prevent shell injection vulnerabilities.

Signed-off-by: ChengyuZhu6 <chengyu.zhu@intel.com>
@ChengyuZhu6
Copy link
Author

Just rebased to pick up the latest changes.

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@mreso mreso Awaiting requested review from mreso

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CI: missing security check for security issues in the codebase
2 participants
@ChengyuZhu6 @mreso