Skip to content
This repository was archived by the owner on Aug 7, 2025. It is now read-only.

CI: Add Security Check Using Bandit in CI - #3312

Open
ChengyuZhu6 wants to merge 3 commits into
pytorch:masterfrom
ChengyuZhu6:secure-check
Open

ChengyuZhu6 wants to merge 3 commits into
pytorch:masterfrom
ChengyuZhu6:secure-check

Conversation

@ChengyuZhu6

@ChengyuZhu6 ChengyuZhu6 commented Sep 12, 2024 •

Copy link
Copy Markdown

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 left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread ts_scripts/install_dependencies.py Resolved
sys.exit(1)

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

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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.

Comment thread ts_scripts/sanity_utils.py Resolved
Comment thread ts_scripts/sanity_utils.py Resolved
Comment thread ts_scripts/sanity_utils.py 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
Copy Markdown
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
Copy Markdown
Author

Just rebased to pick up the latest changes.

@ChengyuZhu6

Copy link
Copy Markdown
Author

kindly ping @mreso

ChengyuZhu6 added 3 commits October 7, 2024 14:03
- 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
Copy Markdown
Author

Just rebased to pick up the latest changes.

This branch has not been deployed

No deployments
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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