fix: sanitize subprocess call in utilities.py#1739
Open
orbisai0security wants to merge 1 commit intohacksider:mainfrom
Open
fix: sanitize subprocess call in utilities.py#1739orbisai0security wants to merge 1 commit intohacksider:mainfrom
orbisai0security wants to merge 1 commit intohacksider:mainfrom
Conversation
Automated security fix generated by Orbis Security AI
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideSanitizes subprocess usage in utilities.py by explicitly disabling shell invocation in ffmpeg helper functions to mitigate command injection risk. Sequence diagram for sanitized run_ffmpeg subprocess executionsequenceDiagram
actor Caller
participant utilities_run_ffmpeg
participant subprocess_module
Caller->>utilities_run_ffmpeg: run_ffmpeg(args)
utilities_run_ffmpeg->>utilities_run_ffmpeg: build commands list
utilities_run_ffmpeg->>subprocess_module: check_output(commands, stderr=STDOUT, shell=False)
subprocess_module-->>utilities_run_ffmpeg: command output
utilities_run_ffmpeg-->>Caller: True on success
Caller->>utilities_run_ffmpeg: run_ffmpeg(args) with invalid command
utilities_run_ffmpeg->>subprocess_module: check_output(commands, stderr=STDOUT, shell=False)
subprocess_module-->>utilities_run_ffmpeg: CalledProcessError
utilities_run_ffmpeg->>utilities_run_ffmpeg: decode error.output
utilities_run_ffmpeg-->>Caller: False on failure
Flow diagram for detect_fps using safe subprocess callflowchart TD
A[start detect_fps target_path] --> B[build ffprobe command list]
B --> C[call subprocess.check_output command shell=False]
C --> D[decode output]
D --> E[split output by /]
E --> F[map numerator and denominator to int]
F --> G[compute fps as numerator divided by denominator]
G --> H[return fps]
C -->|subprocess.CalledProcessError or ValueError| I[exception raised to caller]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 security issues, 1 other issue, and left some high level feedback:
Security issues:
- Detected subprocess function 'check_output' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'check_output' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- Passing a list to
subprocess.check_outputalready impliesshell=False, so the addedshell=Falsearguments are redundant; consider instead ensuring all callers pass argument lists (not strings) so the security property is guaranteed at the API boundary. detect_fpscurrently assumes the subprocess call succeeds and returns a valid fraction; consider mirroring the error handling pattern fromrun_ffmpeg(catchCalledProcessErrorand handle malformed output) to avoid uncaught exceptions and clearer failure behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Passing a list to `subprocess.check_output` already implies `shell=False`, so the added `shell=False` arguments are redundant; consider instead ensuring all callers pass argument lists (not strings) so the security property is guaranteed at the API boundary.
- `detect_fps` currently assumes the subprocess call succeeds and returns a valid fraction; consider mirroring the error handling pattern from `run_ffmpeg` (catch `CalledProcessError` and handle malformed output) to avoid uncaught exceptions and clearer failure behavior.
## Individual Comments
### Comment 1
<location path="modules/utilities.py" line_range="55" />
<code_context>
target_path,
]
- output = subprocess.check_output(command).decode().strip().split("/")
+ output = subprocess.check_output(command, shell=False).decode().strip().split("/")
try:
numerator, denominator = map(int, output)
</code_context>
<issue_to_address>
**suggestion:** Use `text=True`/`encoding` instead of manual `.decode()` for readability and encoding control.
You can call `subprocess.check_output(command, shell=False, text=True)` (or `encoding="utf-8"`) and drop the `.decode()`. This makes the encoding explicit, improves readability, and avoids platform-specific decoding issues.
```suggestion
output = subprocess.check_output(command, shell=False, encoding="utf-8").strip().split("/")
```
</issue_to_address>
### Comment 2
<location path="modules/utilities.py" line_range="31" />
<code_context>
subprocess.check_output(commands, stderr=subprocess.STDOUT, shell=False)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'check_output' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 3
<location path="modules/utilities.py" line_range="55" />
<code_context>
output = subprocess.check_output(command, shell=False).decode().strip().split("/")
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'check_output' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| target_path, | ||
| ] | ||
| output = subprocess.check_output(command).decode().strip().split("/") | ||
| output = subprocess.check_output(command, shell=False).decode().strip().split("/") |
Contributor
There was a problem hiding this comment.
suggestion: Use text=True/encoding instead of manual .decode() for readability and encoding control.
You can call subprocess.check_output(command, shell=False, text=True) (or encoding="utf-8") and drop the .decode(). This makes the encoding explicit, improves readability, and avoids platform-specific decoding issues.
Suggested change
| output = subprocess.check_output(command, shell=False).decode().strip().split("/") | |
| output = subprocess.check_output(command, shell=False, encoding="utf-8").strip().split("/") |
| commands.extend(args) | ||
| try: | ||
| subprocess.check_output(commands, stderr=subprocess.STDOUT) | ||
| subprocess.check_output(commands, stderr=subprocess.STDOUT, shell=False) |
Contributor
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'check_output' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
| target_path, | ||
| ] | ||
| output = subprocess.check_output(command).decode().strip().split("/") | ||
| output = subprocess.check_output(command, shell=False).decode().strip().split("/") |
Contributor
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'check_output' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix critical severity security issue in
modules/utilities.py.Vulnerability
V-001modules/utilities.py:31Description: The utilities.py module executes subprocess commands without proper input validation or sanitization. The subprocess.check_output() call at line 31 accepts a 'commands' parameter that may contain user-controlled input from command-line arguments or file paths. Without shell=False explicitly set and without input validation, this creates a command injection vulnerability where attackers can inject shell metacharacters to execute arbitrary commands.
Changes
modules/utilities.pyVerification
Automated security fix by OrbisAI Security
Summary by Sourcery
Harden subprocess usage in utilities to prevent command injection vulnerabilities by explicitly disabling shell invocation.
Bug Fixes: