Skip to content

feat: security ci - #11

Closed
ShivamBhasin2002 wants to merge 1 commit into
masterfrom
ft-security-ci
Closed

ShivamBhasin2002 wants to merge 1 commit into
masterfrom
ft-security-ci

Conversation

@ShivamBhasin2002

Copy link
Copy Markdown

Summary

Adds a signature-driven supply-chain malware scanner that runs on every PR.

  • Walks every commit in the PR (not just final state) via git log + git diff-tree
  • Two bundled signatures:
    • PolinRider RAT (HIGH) — known IOCs: rmcej, numeric constants, obfuscated global[]= assignments; covers JS configs + font binaries
    • Generic Config Injection (MEDIUM) — catches child_process, eval, new Function, env-var exfil, base64+eval chains, outbound HTTP, SSH key refs, AWS keys, high-entropy obfuscation
  • Adding a new attack = drop one YAML file, zero other changes needed
  • Pure-Python strings fallback so no hard dependency on binutils

File structure

.github/
  workflows/security-scan.yml          ← thin orchestrator
  security/
    scanner.py                          ← Python engine
    signatures/
      pollinrider.yml                   ← PolinRider RAT IOCs (HIGH)
      generic-config-injection.yml      ← broad suspicious patterns (MEDIUM)

Automated rollout — scanner source lives in headout/zapdos#822

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@vulcanho vulcanho Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📋 Summary

This PR introduces a signature-driven supply-chain malware scanner (.github/security/scanner.py) with two initial signatures (PolinRider RAT, Generic Config Injection) and a GitHub Actions workflow to run it on every PR. The scanner design is generally solid: two-phase scanning (per-commit and working-tree final state), deduplication, configurable severity thresholds, and a pure-Python fallback for binary extraction. However, there are two interlocking logic bugs in how the workflow interprets scanner results — one of which is a security bypass where a scanner crash silently allows a PR to merge — plus unpinned action versions that expose the security-critical workflow to a supply-chain attack, a hardcoded multiline output delimiter, and a broken FAIL_SEVERITY override mechanism.

🔀 Architecture

flowchart TD
    PR([Pull Request opened/synced]) --> WF[security-scan.yml]
    WF --> CO[Checkout full history]
    CO --> PY[Setup Python 3.11]
    PY --> SIG[Load signatures/*.yml]
    SIG --> SCAN[scanner.py]
    SCAN --> P1[Phase 1: walk every commit\ngit log + git diff-tree + git show]
    SCAN --> P2[Phase 2: final working-tree state\ngit diff BASE..HEAD → read files]
    P1 --> DEDUP{Dedup by\nsig+file+pattern}
    P2 --> DEDUP
    DEDUP --> OUT[Write GITHUB_OUTPUT\nfound / report / summary]
    OUT --> COMMENT{found == true?}
    COMMENT -- yes --> POST[Post PR comment]
    POST --> BLOCK
    COMMENT -- no --> SKIP([Workflow passes])
    OUT --> BLOCK{found == true?\n⚠ BUG: should be\noutcome != success}
    BLOCK -- yes --> FAIL([exit 1 — merge blocked])
    BLOCK -- no --> SKIP
    style BLOCK fill:#f88,stroke:#c00
    style POST fill:#ffd,stroke:#aa0
Loading

# continue-on-error lets subsequent steps read outputs even when the
# scanner exits 1 (blocking findings). The "Block merge" step below
# is what actually fails the workflow.
continue-on-error: true

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scanner crash silently allows PR to merge — block condition checks output value instead of step outcome

The 'Block merge' step checks steps.scan.outputs.found == 'true', which is only ever written when the scanner completes successfully and finds something. Any scanner crash, config error (exit 2), or unhandled exception leaves this output unset — the condition is false, the block step never runs, and the workflow exits green.

The correct sentinel is steps.scan.outcome, not the output value:

  • outcome == 'failure' when scanner exits non-zero (blocking findings, config error, crash) → block
  • outcome == 'success' when scanner exits 0 (clean PR or only sub-threshold findings) → allow
  • outcome == 'skipped' if an earlier step failed and the scan step never ran → also block

Change the 'Block merge' condition to steps.scan.outcome != 'success' so every non-clean outcome blocks the merge. Keep found == 'true' only on the 'Post PR comment' step (which is correct as-is — comments should only be posted when there are findings to show).

Suggested change
continue-on-error: true
- name: Block merge (findings detected)
if: steps.scan.outcome != 'success'
run: |
echo ""
echo "┌──────────────────────────────────────────────────┐"
echo "│ 🚨 SECURITY ALERT — MERGE BLOCKED │"
echo "│ │"
echo "│ Supply-chain malware indicators found. │"
echo "│ See the PR comment and workflow logs for details. │"
echo "└──────────────────────────────────────────────────┘"
exit 1
Prompt for agents
In .github/workflows/security-scan.yml at line 117, change the 'Block merge' step condition from `if: steps.scan.outputs.found == 'true'` to `if: steps.scan.outcome != 'success'`. The current condition only blocks when the scanner explicitly writes found=true, meaning any scanner crash or config error (which never writes outputs) silently allows the PR to merge. Using `outcome != 'success'` means failure, crash, and skipped all block the merge, while only a clean exit-0 allows it through. Keep the 'Post PR comment' step condition unchanged at `found == 'true'`.

Comment on lines +461 to +468
blocking = [f for f in findings if _severity_meets_threshold(f["severity"], FAIL_SEVERITY)]
report = _build_report_table(findings)
summary = (
f"{len(findings)} finding(s); "
f"{len(blocking)} at or above {FAIL_SEVERITY} severity."
)

_write_output("found", "true")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

found=true is written for sub-threshold findings, causing the workflow to block PRs the scanner decided not to block

The found output is set to "true" for any finding, including those below FAIL_SEVERITY (line 468 is reached before the if blocking: branch on line 472). The scanner then exits 0 to signal "non-blocking," but the workflow's 'Block merge' step uses found == 'true' and ignores the exit code — so it blocks the PR anyway.

If you apply the steps.scan.outcome != 'success' fix to the workflow (the other finding), this specific Python-side issue becomes moot, because the block decision moves to the exit code. However, if you want the scanner to be usable standalone or with a different CI wrapper, emit a dedicated blocking output instead of overloading found:

_write_output("found", "true")
_write_output("blocking", "true" if blocking else "false")

Then in the workflow use steps.scan.outputs.blocking == 'true' for the block step and steps.scan.outputs.found == 'true' for the comment step.

Suggested change
blocking = [f for f in findings if _severity_meets_threshold(f["severity"], FAIL_SEVERITY)]
report = _build_report_table(findings)
summary = (
f"{len(findings)} finding(s); "
f"{len(blocking)} at or above {FAIL_SEVERITY} severity."
)
_write_output("found", "true")
_write_output("found", "true")
_write_output("report", report)
_write_output("summary", summary)
_write_output("blocking", "true" if blocking else "false")
Prompt for agents
In .github/security/scanner.py around line 468-470, after `_write_output("found", "true")`, add a new line: `_write_output("blocking", "true" if blocking else "false")`. This separates 'has any findings' from 'should block the PR'. Then in .github/workflows/security-scan.yml line 117, change the 'Block merge' condition from `steps.scan.outputs.found == 'true'` to `steps.scan.outputs.blocking == 'true'` (or use `steps.scan.outcome != 'success'` as described in the companion fix). This ensures the workflow respects the scanner's FAIL_SEVERITY threshold rather than blocking on every finding regardless of severity.


steps:
- name: Checkout with full history
uses: actions/checkout@v4

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub Actions used by the security scanner are not pinned to immutable SHAs — vulnerable to the same supply-chain attacks they are meant to detect

Using floating version tags (@v4, @v5, @v7) means a compromised update to any of these actions silently runs attacker code inside this security-critical workflow. Pin each action to its full SHA digest so the resolved code cannot change without a deliberate PR update:

uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683  # v4.2.2
uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b  # v5.3.0
uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea  # v7.0.1

Verify the current SHAs against the official repos before committing. Also applies to: .github/workflows/security-scan.yml:31 (setup-python@v5), .github/workflows/security-scan.yml:64 (github-script@v7).

Prompt for agents
In .github/workflows/security-scan.yml, replace the three floating action version tags with pinned SHA digests. Look up the current SHA for each action at their respective GitHub repos:
- actions/checkout tag v4 → pin to full SHA
- actions/setup-python tag v5 → pin to full SHA
- actions/github-script tag v7 → pin to full SHA
Add an inline comment with the human-readable version next to each pinned SHA so it's clear which version is pinned. Example format: `uses: actions/checkout@<SHA>  # v4.x.x`

Comment on lines +404 to +406
if "\n" in value:
delimiter = "__SCAN_EOF__"
fh.write(f"{key}<<{delimiter}\n{value}\n{delimiter}\n")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoded multiline output delimiter __SCAN_EOF__ can be injected via a crafted file path

GitHub Actions' multiline output format requires the delimiter to not appear in the value. Using a fixed string like __SCAN_EOF__ means any file whose path or pattern description contains that exact substring would break the output. Use a random UUID-based delimiter instead, which is the approach GitHub Actions itself uses internally:

import uuid
delimiter = f"ghadelimiter_{uuid.uuid4().hex}"

The probability of a collision with file content is effectively zero.

Suggested change
if "\n" in value:
delimiter = "__SCAN_EOF__"
fh.write(f"{key}<<{delimiter}\n{value}\n{delimiter}\n")
def _write_output(key: str, value: str) -> None:
if not GITHUB_OUTPUT:
return
with open(GITHUB_OUTPUT, "a", encoding="utf-8") as fh:
if "\n" in value:
import uuid
delimiter = f"ghadelimiter_{uuid.uuid4().hex}"
fh.write(f"{key}<<{delimiter}\n{value}\n{delimiter}\n")
else:
fh.write(f"{key}={value}\n")
Prompt for agents
In .github/security/scanner.py in the `_write_output` function (around line 405), replace the hardcoded `delimiter = "__SCAN_EOF__"` with a random per-call delimiter: `import uuid` at the top of the file (or inside the function), then `delimiter = f"ghadelimiter_{uuid.uuid4().hex}"`. This prevents a crafted file path or pattern description in the scan report from prematurely terminating the multiline GitHub Actions output value.

Comment on lines +14 to +17
# Minimum severity that fails the build.
# Override at the repo level via Settings → Secrets and variables → Actions.
# Valid values: CRITICAL | HIGH | MEDIUM | LOW | INFO
FAIL_SEVERITY: MEDIUM

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FAIL_SEVERITY cannot be overridden via repository variables as the comment claims — env context is used instead of vars

The comment says to override FAIL_SEVERITY via repository variables (Settings → Secrets and variables → Actions), but the step reads ${{ env.FAIL_SEVERITY }} which resolves only the workflow-level env: block (always MEDIUM). Repository variables use the vars context. Use ${{ vars.FAIL_SEVERITY || env.FAIL_SEVERITY }} to prefer a repo variable when set and fall back to the workflow default otherwise:

Prompt for agents
In .github/workflows/security-scan.yml at line 52, change `FAIL_SEVERITY: ${{ env.FAIL_SEVERITY }}` to `FAIL_SEVERITY: ${{ vars.FAIL_SEVERITY || env.FAIL_SEVERITY }}`. This makes the value prefer a repository-level Actions variable named FAIL_SEVERITY (set under Settings → Variables → Actions) and fall back to the workflow-level default of MEDIUM. Also update the comment on line 15 to say 'Settings → Variables → Actions' (not 'Secrets and variables') so operators know the correct location.

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.

1 participant