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

CI: Add Security Check Using Bandit in CI #3312

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

Conversation

ChengyuZhu6
Copy link

@ChengyuZhu6 ChengyuZhu6 commented Sep 12, 2024

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
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
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
Copy link
Author

Just rebased to pick up the latest changes.

@ChengyuZhu6
Copy link
Author

kindly ping @mreso

- 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>
…=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>
…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.

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.

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