Skip to content

Conversation

@ColeMurray
Copy link
Contributor

This PR addresses a security vulnerability in the llama-dev changelog generation tool by replacing the unsafe shell=True parameter in subprocess calls with proper command parsing using shlex.split().

  • Modified _run_command() function in llama-dev/llama_dev/release/changelog.py to parse commands into argument lists using shlex.split()
  • Removed shell=True parameter from subprocess.run() calls, preventing potential command injection attacks
  • Added unit test to verify commands are executed without shell and properly use argument lists

Risk Level: Low - While this fixes a genuine shell injection vulnerability, the exploitability is limited because:

  1. The llama-dev tool is a development utility not exposed to external inputs
  2. Commands passed to _run_command() are typically hardcoded Git commands within the codebase
  3. No user-controlled input flows directly into these command strings

This is a proactive security hardening measure following best practices for subprocess execution, even though the practical exploit risk in this context is minimal.

Version Bump?

Did I bump the version in the pyproject.toml file of the package I am updating? (Except for the llama-index-core package)

  • Yes
  • [ x] No

Type of Change

Please delete options that are not relevant.

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

How Has This Been Tested?

Your pull-request will likely not be merged unless it is covered by some form of impactful unit testing.

  • I added new unit tests to cover this change
  • [x ] I believe this change is already covered by existing unit tests

Suggested Checklist:

  • [x ] I have performed a self-review of my own code
  • [ x] I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added Google Colab support for the newly added notebooks.
  • [ x] My changes generate no new warnings
  • [ x] I have added tests that prove my fix is effective or that my feature works
  • [ x] New and existing unit tests pass locally with my changes
  • I ran uv run make format; uv run make lint to appease the lint gods

This PR addresses a security vulnerability in the `llama-dev` changelog generation tool by replacing the unsafe `shell=True` parameter in subprocess calls with proper command parsing using `shlex.split()`.

- Modified `_run_command()` function in `llama-dev/llama_dev/release/changelog.py` to parse commands into argument lists using `shlex.split()`
- Removed `shell=True` parameter from `subprocess.run()` calls, preventing potential command injection attacks
- Added unit test to verify commands are executed without shell and properly use argument lists

**Risk Level: Low** - While this fixes a genuine shell injection vulnerability, the exploitability is limited because:
1. The `llama-dev` tool is a development utility not exposed to external inputs
2. Commands passed to `_run_command()` are typically hardcoded Git commands within the codebase
3. No user-controlled input flows directly into these command strings

This is a proactive security hardening measure following best practices for subprocess execution, even though the practical exploit risk in this context is minimal.
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Sep 26, 2025
@ColeMurray ColeMurray changed the title Fix shell injection vulnerability in llama-dev changelog command Fix shell usage in llama-dev changelog command Sep 26, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Sep 29, 2025
@logan-markewich logan-markewich merged commit 3edc42e into run-llama:main Sep 29, 2025
12 checks passed
@ColeMurray ColeMurray deleted the fix/changelog-shell-false branch September 29, 2025 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants