From 58020cc42e03e883115be14eb37bb4112e584442 Mon Sep 17 00:00:00 2001 From: GTC6244 <95836911+GTC6244@users.noreply.github.com> Date: Tue, 30 Jun 2026 22:08:21 -0400 Subject: [PATCH 1/4] ci: add gated per-PR security review (Trail of Bits differential-review) Add .github/workflows/security-review.yml that runs a security-focused differential review on every PR and fails the check (the gate) on any CRITICAL/HIGH finding at MEDIUM+ confidence, computed deterministically from a verdict JSON via jq. Uses pull_request (not pull_request_target) so fork PRs run read-only. Vendor the Trail of Bits differential-review skill + adversarial-modeler agent into .claude/ (pinned, attributed CC BY-SA 4.0) and un-ignore those paths while keeping local Claude state ignored. Co-Authored-By: Claude Opus 4.8 --- .claude/agents/adversarial-modeler.md | 173 ++++++++ .../skills/differential-review/ATTRIBUTION.md | 17 + .claude/skills/differential-review/SKILL.md | 228 +++++++++++ .../skills/differential-review/adversarial.md | 203 ++++++++++ .../differential-review/agents/openai.yaml | 4 + .../assets/trail-of-bits-mark.svg | 1 + .../skills/differential-review/methodology.md | 234 +++++++++++ .../skills/differential-review/patterns.md | 300 ++++++++++++++ .../skills/differential-review/reporting.md | 369 ++++++++++++++++++ .github/workflows/security-review.yml | 174 +++++++++ .gitignore | 6 +- 11 files changed, 1708 insertions(+), 1 deletion(-) create mode 100644 .claude/agents/adversarial-modeler.md create mode 100644 .claude/skills/differential-review/ATTRIBUTION.md create mode 100644 .claude/skills/differential-review/SKILL.md create mode 100644 .claude/skills/differential-review/adversarial.md create mode 100644 .claude/skills/differential-review/agents/openai.yaml create mode 100644 .claude/skills/differential-review/assets/trail-of-bits-mark.svg create mode 100644 .claude/skills/differential-review/methodology.md create mode 100644 .claude/skills/differential-review/patterns.md create mode 100644 .claude/skills/differential-review/reporting.md create mode 100644 .github/workflows/security-review.yml diff --git a/.claude/agents/adversarial-modeler.md b/.claude/agents/adversarial-modeler.md new file mode 100644 index 00000000..b7c1dbac --- /dev/null +++ b/.claude/agents/adversarial-modeler.md @@ -0,0 +1,173 @@ +--- +name: adversarial-modeler +description: "Models attacker perspectives and builds exploit scenarios for HIGH RISK code changes. Use when differential review identifies high-risk changes that need adversarial threat modeling and concrete attack vector analysis." +tools: Read, Grep, Glob, Bash +--- + +# Adversarial Modeler + +You are an adversarial threat modeler specializing in security-focused +analysis of high-risk code changes. Your role is to think like an attacker: +identify concrete exploit paths, rate exploitability, and produce +vulnerability reports with measurable impact. + +## Key Principle + +**Concrete impact only — never "could cause issues."** Every finding must +include specific, measurable harm: exact data exposed, privileges escalated, +funds at risk, or invariants broken. Vague warnings are not findings. + +## When to Activate + +Run adversarial modeling when differential review classifies a change as +HIGH RISK. High-risk triggers include: + +- Authentication or authorization changes +- Cryptographic code modifications +- External call additions or modifications +- Value transfer logic changes +- Validation removal or weakening +- Access control modifier changes + +## 5-Step Methodology + +Follow these steps in order for each high-risk change. + +### Step 1: Define the Attacker Model + +Establish WHO is attacking, WHAT access they have, and WHERE they interact +with the system. + +**Attacker types to consider:** +- Unauthenticated external user +- Authenticated regular user +- Malicious administrator +- Compromised upstream service or contract +- Front-runner / MEV bot (for blockchain contexts) + +**Determine attacker capabilities:** +- What interfaces are accessible (HTTP endpoints, contract functions, RPCs)? +- What privileges does the attacker hold? +- What system state can the attacker observe or influence? + +### Step 2: Identify Concrete Attack Vectors + +For each potential vulnerability in the diff: + +``` +ENTRY POINT: [Exact function/endpoint attacker can access] + +ATTACK SEQUENCE: +1. [Specific API call/transaction with parameters] +2. [How this reaches the vulnerable code] +3. [What happens in the vulnerable code] +4. [Impact achieved] + +PROOF OF ACCESSIBILITY: +- Show the function is public/external +- Demonstrate attacker has required permissions +- Prove attack path exists through actual interfaces +``` + +Use `Grep` and `Read` to trace call chains from public interfaces to the +changed code. Verify that the attack path is reachable — do not assume. + +### Step 3: Rate Exploitability + +Assign a realistic exploitability rating with justification: + +| Rating | Criteria | +|--------|----------| +| EASY | Single call/request, public interface, no special state | +| MEDIUM | Multiple steps, specific timing, elevated but obtainable privileges | +| HARD | Admin access needed, rare conditions, significant resources | + +### Step 4: Build Complete Exploit Scenario + +Construct a step-by-step exploit with concrete values: + +``` +ATTACKER STARTING POSITION: +[What the attacker has at the beginning] + +STEP-BY-STEP EXPLOITATION: +Step 1: [Concrete action through accessible interface] + - Command: [Exact call/request] + - Parameters: [Specific values] + - Expected result: [What happens] + +Step 2: [Next action] + - Command: [Exact call/request] + - Why this works: [Reference to code change with file:line] + - System state change: [What changed] + +CONCRETE IMPACT: +[Specific, measurable impact] +- Exact data/funds/privileges affected +- Quantified scope (number of users, dollar amount, etc.) +``` + +### Step 5: Cross-Reference with Baseline + +Check each finding against the codebase baseline: + +- Does this violate a system-wide invariant? +- Does this break a trust boundary? +- Does this bypass a validation pattern used elsewhere? +- Is this a regression of a previous fix? (Check git blame/log) + +Use `Bash` with `git log` and `git blame` to verify historical context. + +## Vulnerability Report Template + +Generate one report per finding: + +```markdown +## [SEVERITY] Vulnerability Title + +**Attacker Model:** +- WHO: [Specific attacker type] +- ACCESS: [Exact privileges] +- INTERFACE: [Specific entry point] + +**Attack Vector:** +[Step-by-step exploit through accessible interfaces] + +**Exploitability:** EASY / MEDIUM / HARD +**Justification:** [Why this rating] + +**Concrete Impact:** +[Specific, measurable harm — not theoretical] + +**Proof of Concept:** +[Exact code/commands to reproduce] + +**Root Cause:** +[Reference specific code change at file:line] + +**Blast Radius:** [N callers affected] +**Baseline Violation:** [Which invariant/pattern broken] +``` + +## Working with the Codebase + +- Use `{baseDir}/skills/differential-review/adversarial.md` for the full adversarial methodology with examples +- Use `{baseDir}/skills/differential-review/patterns.md` for common vulnerability pattern reference +- Use `{baseDir}/skills/differential-review/methodology.md` for the broader review workflow context + +## When NOT to Use + +- **LOW or MEDIUM risk changes** -- only activate for HIGH RISK classifications +- **Greenfield code without a baseline** -- adversarial modeling requires existing + invariants and trust boundaries to cross-reference against +- **Documentation, test, or formatting changes** -- no attack surface to model +- **When the user explicitly requests quick triage only** -- use the + Quick Reference in SKILL.md instead + +## Anti-Patterns to Avoid + +- **Generic findings** without specific attack paths ("input validation could be bypassed") +- **Theoretical vulnerabilities** without proof of reachability +- **Missing attacker model** — every finding must specify WHO exploits it +- **Assuming access** — verify that the attacker can actually reach the vulnerable code +- **Severity inflation** — rate exploitability honestly based on real conditions diff --git a/.claude/skills/differential-review/ATTRIBUTION.md b/.claude/skills/differential-review/ATTRIBUTION.md new file mode 100644 index 00000000..fe3c75cd --- /dev/null +++ b/.claude/skills/differential-review/ATTRIBUTION.md @@ -0,0 +1,17 @@ +# Attribution + +The `differential-review` skill (and the bundled `adversarial-modeler` agent at +`.claude/agents/adversarial-modeler.md`) is vendored from the Trail of Bits +Claude Code skills marketplace: + +- Source: https://github.com/trailofbits/skills (`plugins/differential-review`) +- Author: Omar Inuwa, Trail of Bits (opensource@trailofbits.com) +- Version vendored: 1.1.1 +- License: Creative Commons Attribution-ShareAlike 4.0 International (CC BY-SA 4.0) + — https://creativecommons.org/licenses/by-sa/4.0/ + +It is vendored (rather than fetched at CI time) so the exact reviewed content is +pinned in-repo, reviewable in PRs, and carries no CI-time supply-chain dependency. + +To update: re-copy from the upstream marketplace and bump the version above in the +same PR so the change is reviewable. diff --git a/.claude/skills/differential-review/SKILL.md b/.claude/skills/differential-review/SKILL.md new file mode 100644 index 00000000..3755d477 --- /dev/null +++ b/.claude/skills/differential-review/SKILL.md @@ -0,0 +1,228 @@ +--- +name: differential-review +description: > + Performs security-focused differential review of code changes (PRs, commits, diffs). + Adapts analysis depth to codebase size, uses git history for context, calculates + blast radius, checks test coverage, and generates comprehensive markdown reports. + Automatically detects and prevents security regressions. +allowed-tools: Read Write Grep Glob Bash +--- + +# Differential Security Review + +Security-focused code review for PRs, commits, and diffs. + +## Core Principles + +1. **Risk-First**: Focus on auth, crypto, value transfer, external calls +2. **Evidence-Based**: Every finding backed by git history, line numbers, attack scenarios +3. **Adaptive**: Scale to codebase size (SMALL/MEDIUM/LARGE) +4. **Honest**: Explicitly state coverage limits and confidence level +5. **Output-Driven**: Always generate comprehensive markdown report file + +--- + +## Rationalizations (Do Not Skip) + +| Rationalization | Why It's Wrong | Required Action | +|-----------------|----------------|-----------------| +| "Small PR, quick review" | Heartbleed was 2 lines | Classify by RISK, not size | +| "I know this codebase" | Familiarity breeds blind spots | Build explicit baseline context | +| "Git history takes too long" | History reveals regressions | Never skip Phase 1 | +| "Blast radius is obvious" | You'll miss transitive callers | Calculate quantitatively | +| "No tests = not my problem" | Missing tests = elevated risk rating | Flag in report, elevate severity | +| "Just a refactor, no security impact" | Refactors break invariants | Analyze as HIGH until proven LOW | +| "I'll explain verbally" | No artifact = findings lost | Always write report | + +--- + +## Quick Reference + +### Codebase Size Strategy + +| Codebase Size | Strategy | Approach | +|---------------|----------|----------| +| SMALL (<20 files) | DEEP | Read all deps, full git blame | +| MEDIUM (20-200) | FOCUSED | 1-hop deps, priority files | +| LARGE (200+) | SURGICAL | Critical paths only | + +### Risk Level Triggers + +| Risk Level | Triggers | +|------------|----------| +| HIGH | Auth, crypto, external calls, value transfer, validation removal | +| MEDIUM | Business logic, state changes, new public APIs | +| LOW | Comments, tests, UI, logging | + +--- + +## Workflow Overview + +``` +Pre-Analysis → Phase 0: Triage → Phase 1: Code Analysis → Phase 2: Test Coverage + ↓ ↓ ↓ ↓ +Phase 3: Blast Radius → Phase 4: Deep Context → Phase 5: Adversarial → Phase 6: Report +``` + +--- + +## Decision Tree + +**Starting a review?** + +``` +├─ Need detailed phase-by-phase methodology? +│ └─ Read: methodology.md +│ (Pre-Analysis + Phases 0-4: triage, code analysis, test coverage, blast radius) +│ +├─ Analyzing HIGH RISK change? +│ ├─ Read: adversarial.md +│ │ (Phase 5: Attacker modeling, exploit scenarios, exploitability rating) +│ └─ Or delegate to: adversarial-modeler agent +│ (Autonomous attacker modeling with concrete exploit scenarios) +│ +├─ Writing the final report? +│ └─ Read: reporting.md +│ (Phase 6: Report structure, templates, formatting guidelines) +│ +├─ Looking for specific vulnerability patterns? +│ └─ Read: patterns.md +│ (Regressions, reentrancy, access control, overflow, etc.) +│ +└─ Quick triage only? + └─ Use Quick Reference above, skip detailed docs +``` + +--- + +## Agents + +**`adversarial-modeler`** — Models attacker perspectives and builds exploit +scenarios for HIGH RISK code changes. Follows the 5-step adversarial +methodology (attacker model, attack vectors, exploitability rating, exploit +scenario, baseline cross-reference) and produces structured vulnerability +reports. Delegate to this agent when Phase 5 analysis is needed on high-risk +changes. + +--- + +## Quality Checklist + +Before delivering: + +- [ ] All changed files analyzed +- [ ] Git blame on removed security code +- [ ] Blast radius calculated for HIGH risk +- [ ] Attack scenarios are concrete (not generic) +- [ ] Findings reference specific line numbers + commits +- [ ] Report file generated +- [ ] User notified with summary + +--- + +## Integration + +**audit-context-building skill:** +- Pre-Analysis: Build baseline context +- Phase 4: Deep context on HIGH RISK changes + +**issue-writer skill:** +- Transform findings into formal audit reports +- Command: `issue-writer --input DIFFERENTIAL_REVIEW_REPORT.md --format audit-report` + +--- + +## Example Usage + +### Quick Triage (Small PR) +``` +Input: 5 file PR, 2 HIGH RISK files +Strategy: Use Quick Reference +1. Classify risk level per file (2 HIGH, 3 LOW) +2. Focus on 2 HIGH files only +3. Git blame removed code +4. Generate minimal report +Time: ~30 minutes +``` + +### Standard Review (Medium Codebase) +``` +Input: 80 files, 12 HIGH RISK changes +Strategy: FOCUSED (see methodology.md) +1. Full workflow on HIGH RISK files +2. Surface scan on MEDIUM +3. Skip LOW risk files +4. Complete report with all sections +Time: ~3-4 hours +``` + +### Deep Audit (Large, Critical Change) +``` +Input: 450 files, auth system rewrite +Strategy: SURGICAL + audit-context-building +1. Baseline context with audit-context-building +2. Deep analysis on auth changes only +3. Blast radius analysis +4. Adversarial modeling +5. Comprehensive report +Time: ~6-8 hours +``` + +--- + +## When NOT to Use This Skill + +- **Greenfield code** (no baseline to compare) +- **Documentation-only changes** (no security impact) +- **Formatting/linting** (cosmetic changes) +- **User explicitly requests quick summary only** (they accept risk) + +For these cases, use standard code review instead. + +--- + +## Red Flags (Stop and Investigate) + +**Immediate escalation triggers:** +- Removed code from "security", "CVE", or "fix" commits +- Access control modifiers removed (onlyOwner, internal → external) +- Validation removed without replacement +- External calls added without checks +- High blast radius (50+ callers) + HIGH risk change + +These patterns require adversarial analysis even in quick triage. + +--- + +## Tips for Best Results + +**Do:** +- Start with git blame for removed code +- Calculate blast radius early to prioritize +- Generate concrete attack scenarios +- Reference specific line numbers and commits +- Be honest about coverage limitations +- Always generate the output file + +**Don't:** +- Skip git history analysis +- Make generic findings without evidence +- Claim full analysis when time-limited +- Forget to check test coverage +- Miss high blast radius changes +- Output report only to chat (file required) + +--- + +## Supporting Documentation + +- **[methodology.md](methodology.md)** - Detailed phase-by-phase workflow (Phases 0-4) +- **[adversarial.md](adversarial.md)** - Attacker modeling and exploit scenarios (Phase 5) +- **[reporting.md](reporting.md)** - Report structure and formatting (Phase 6) +- **[patterns.md](patterns.md)** - Common vulnerability patterns reference + +--- + +**For first-time users:** Start with [methodology.md](methodology.md) to understand the complete workflow. + +**For experienced users:** Use this page's Quick Reference and Decision Tree to navigate directly to needed content. diff --git a/.claude/skills/differential-review/adversarial.md b/.claude/skills/differential-review/adversarial.md new file mode 100644 index 00000000..0176d374 --- /dev/null +++ b/.claude/skills/differential-review/adversarial.md @@ -0,0 +1,203 @@ +# Adversarial Vulnerability Analysis (Phase 5) + +Structured methodology for finding vulnerabilities through attacker modeling. + +**When to use:** After completing deep context analysis (Phase 4), apply this to all HIGH RISK changes. + +--- + +## 1. Define Specific Attacker Model + +**WHO is the attacker?** +- Unauthenticated external user +- Authenticated regular user +- Malicious administrator +- Compromised contract/service +- Front-runner/MEV bot + +**WHAT access/privileges do they have?** +- Public API access only +- Authenticated user role +- Specific permissions/tokens +- Contract call capabilities + +**WHERE do they interact with the system?** +- Specific HTTP endpoints +- Smart contract functions +- RPC interfaces +- External APIs + +--- + +## 2. Identify Concrete Attack Vectors + +``` +ENTRY POINT: [Exact function/endpoint attacker can access] + +ATTACK SEQUENCE: +1. [Specific API call/transaction with parameters] +2. [How this reaches the vulnerable code] +3. [What happens in the vulnerable code] +4. [Impact achieved] + +PROOF OF ACCESSIBILITY: +- Show the function is public/external +- Demonstrate attacker has required permissions +- Prove attack path exists through actual interfaces +``` + +--- + +## 3. Rate Realistic Exploitability + +**EASY:** Exploitable via public APIs with no special privileges +- Single transaction/call +- Common user access level +- No complex conditions required + +**MEDIUM:** Requires specific conditions or elevated privileges +- Multiple steps or timing requirements +- Elevated but obtainable privileges +- Specific system state needed + +**HARD:** Requires privileged access or rare conditions +- Admin/owner privileges needed +- Rare edge case conditions +- Significant resources required + +--- + +## 4. Build Complete Exploit Scenario + +``` +ATTACKER STARTING POSITION: +[What the attacker has at the beginning] + +STEP-BY-STEP EXPLOITATION: +Step 1: [Concrete action through accessible interface] + - Command: [Exact call/request] + - Parameters: [Specific values] + - Expected result: [What happens] + +Step 2: [Next action] + - Command: [Exact call/request] + - Why this works: [Reference to code change] + - System state change: [What changed] + +Step 3: [Final impact] + - Result: [Concrete harm achieved] + - Evidence: [How to verify impact] + +CONCRETE IMPACT: +[Specific, measurable impact - not "could cause issues"] +- Exact amount of funds drained +- Specific privileges escalated +- Particular data exposed +``` + +--- + +## 5. Cross-Reference with Baseline Context + +From baseline analysis (see [methodology.md](methodology.md#pre-analysis-baseline-context-building)), check: +- Does this violate a system-wide invariant? +- Does this break a trust boundary? +- Does this bypass a validation pattern? +- Is this a regression of a previous fix? + +--- + +## Vulnerability Report Template + +Generate this for each finding: + +```markdown +## [SEVERITY] Vulnerability Title + +**Attacker Model:** +- WHO: [Specific attacker type] +- ACCESS: [Exact privileges] +- INTERFACE: [Specific entry point] + +**Attack Vector:** +[Step-by-step exploit through accessible interfaces] + +**Exploitability:** EASY/MEDIUM/HARD +**Justification:** [Why this rating] + +**Concrete Impact:** +[Specific, measurable harm - not theoretical] + +**Proof of Concept:** +```code +// Exact code to reproduce +``` + +**Root Cause:** +[Reference specific code change at file.sol:L123] + +**Blast Radius:** [N callers affected] +**Baseline Violation:** [Which invariant/pattern broken] +``` + +--- + +## Example: Complete Adversarial Analysis + +**Change:** Removed `require(amount > 0)` check from `withdraw()` function + +### 1. Attacker Model +- **WHO:** Unauthenticated external user +- **ACCESS:** Can call public contract functions +- **INTERFACE:** `withdraw(uint256 amount)` at 0x1234... + +### 2. Attack Vector +**ENTRY POINT:** `withdraw(0)` + +**ATTACK SEQUENCE:** +1. Call `withdraw(0)` from attacker address +2. Code bypasses amount check (removed) +3. Withdraw event emitted with 0 amount +4. Accounting updated incorrectly + +**PROOF:** Function is `external`, no auth required + +### 3. Exploitability +**RATING:** EASY +- Single transaction +- Public function +- No special state required + +### 4. Exploit Scenario +**ATTACKER POSITION:** Has user account with 0 balance + +**EXPLOITATION:** +```solidity +Step 1: attacker.withdraw(0) + - Passes removed validation + - Emits Withdraw(user, 0) + - Updates withdrawnAmount[user] += 0 + +Step 2: Off-chain indexer sees Withdraw event + - Credits attacker for 0 withdrawal + - But accounting thinks withdrawal happened + +Step 3: Accounting mismatch exploited + - Total supply decremented + - User balance not changed + - System invariants broken +``` + +**IMPACT:** +- Protocol accounting corrupted +- Can be used to manipulate LP calculations +- Estimated $50K impact on pool prices + +### 5. Baseline Violation +- Violates invariant: "All withdrawals must transfer non-zero value" +- Breaks validation pattern: Amount checks present in all other value transfers +- Regression: Check added in commit abc123 "Fix zero-amount exploit" + +--- + +**Next:** Document all findings in final report (see [reporting.md](reporting.md)) diff --git a/.claude/skills/differential-review/agents/openai.yaml b/.claude/skills/differential-review/agents/openai.yaml new file mode 100644 index 00000000..1d437b6d --- /dev/null +++ b/.claude/skills/differential-review/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + icon_small: "assets/trail-of-bits-mark.svg" + icon_large: "assets/trail-of-bits-mark.svg" + brand_color: "#D83A34" diff --git a/.claude/skills/differential-review/assets/trail-of-bits-mark.svg b/.claude/skills/differential-review/assets/trail-of-bits-mark.svg new file mode 100644 index 00000000..7cd6e7ca --- /dev/null +++ b/.claude/skills/differential-review/assets/trail-of-bits-mark.svg @@ -0,0 +1 @@ + diff --git a/.claude/skills/differential-review/methodology.md b/.claude/skills/differential-review/methodology.md new file mode 100644 index 00000000..71d9dc0b --- /dev/null +++ b/.claude/skills/differential-review/methodology.md @@ -0,0 +1,234 @@ +# Differential Review Methodology + +Detailed phase-by-phase workflow for security-focused code review. + +## Pre-Analysis: Baseline Context Building + +**FIRST ACTION - Build complete baseline understanding:** + +If `audit-context-building` skill is available: + +```bash +# Checkout baseline commit +git checkout + +# Invoke audit-context-building skill on baseline codebase +# Scope = entire relevant project (e.g., packages/contracts/contracts/ for Solidity, src/ for Rust, etc.) +audit-context-building --scope [entire project or main contract directory] --focus invariants,trust-boundaries,validation-patterns,call-graphs,state-flows + +# Examples: +# For Solidity: audit-context-building --scope packages/contracts/contracts +# For Rust: audit-context-building --scope src +# For full repo: audit-context-building --scope . +``` + +**Capture from baseline analysis:** +- System-wide invariants (what must ALWAYS be true across all code) +- Trust boundaries and privilege levels (who can do what) +- Validation patterns (what gets checked where - defense-in-depth) +- Complete call graphs for critical functions (who calls what) +- State flow diagrams (how state changes) +- External dependencies and trust assumptions + +**Why this matters:** +- Understand what the code was SUPPOSED to do before changes +- Identify implicit security assumptions in baseline +- Detect when changes violate baseline invariants +- Know which patterns are system-wide vs local +- Catch when changes break defense-in-depth + +**Store baseline context for reference during differential analysis.** + +After baseline analysis, checkout back to head commit to analyze changes. + +--- + +## Phase 0: Intake & Triage + +**Extract changes:** +```bash +# For commit range +git diff .. --stat +git log .. --oneline + +# For PR +gh pr view --json files,additions,deletions + +# Get all changed files +git diff .. --name-only +``` + +**Assess codebase size:** +```bash +find . -name "*.sol" -o -name "*.rs" -o -name "*.go" -o -name "*.ts" | wc -l +``` + +**Classify complexity:** +- **SMALL**: <20 files → Deep analysis (read all deps) +- **MEDIUM**: 20-200 files → Focused analysis (1-hop deps) +- **LARGE**: 200+ files → Surgical (critical paths only) + +**Risk score each file:** +- **HIGH**: Auth, crypto, external calls, value transfer, validation removal +- **MEDIUM**: Business logic, state changes, new public APIs +- **LOW**: Comments, tests, UI, logging + +--- + +## Phase 1: Changed Code Analysis + +For each changed file: + +1. **Read both versions** (baseline and changed) + +2. **Analyze each diff region:** + ``` + BEFORE: [exact code] + AFTER: [exact code] + CHANGE: [behavioral impact] + SECURITY: [implications] + ``` + +3. **Git blame removed code:** + ```bash + # When was it added? Why? + git log -S "removed_code" --all --oneline + git blame -- file.sol | grep "pattern" + ``` + + **Red flags:** + - Removed code from "fix", "security", "CVE" commits → CRITICAL + - Recently added (<1 month) then removed → HIGH + +4. **Check for regressions (re-added code):** + ```bash + git log -S "added_code" --all -p + ``` + + Pattern: Code added → removed for security → re-added now = REGRESSION + +5. **Micro-adversarial analysis** for each change: + - What attack did removed code prevent? + - What new surface does new code expose? + - Can modified logic be bypassed? + - Are checks weaker? Edge cases covered? + +6. **Generate concrete attack scenarios:** + ``` + SCENARIO: [attack goal] + PRECONDITIONS: [required state] + STEPS: + 1. [specific action] + 2. [expected outcome] + 3. [exploitation] + WHY IT WORKS: [reference code change] + IMPACT: [severity + scope] + ``` + +--- + +## Phase 2: Test Coverage Analysis + +**Identify coverage gaps:** +```bash +# Production code changes (exclude tests) +git diff --name-only | grep -v "test" + +# Test changes +git diff --name-only | grep "test" + +# For each changed function, search for tests +grep -r "test.*functionName" test/ --include="*.sol" --include="*.js" +``` + +**Risk elevation rules:** +- NEW function + NO tests → Elevate risk MEDIUM→HIGH +- MODIFIED validation + UNCHANGED tests → HIGH RISK +- Complex logic (>20 lines) + NO tests → HIGH RISK + +--- + +## Phase 3: Blast Radius Analysis + +**Calculate impact:** +```bash +# Count callers for each modified function +grep -r "functionName(" --include="*.sol" . | wc -l +``` + +**Classify blast radius:** +- 1-5 calls: LOW +- 6-20 calls: MEDIUM +- 21-50 calls: HIGH +- 50+ calls: CRITICAL + +**Priority matrix:** + +| Change Risk | Blast Radius | Priority | Analysis Depth | +|-------------|--------------|----------|----------------| +| HIGH | CRITICAL | P0 | Deep + all deps | +| HIGH | HIGH/MEDIUM | P1 | Deep | +| HIGH | LOW | P2 | Standard | +| MEDIUM | CRITICAL/HIGH | P1 | Standard + callers | + +--- + +## Phase 4: Deep Context Analysis + +**If `audit-context-building` skill is available**, invoke it to help answer all the questions below for each HIGH RISK changed function: + +```bash +# Run audit-context-building on the changed function and its dependencies +audit-context-building --scope [file containing changed function] --focus flow-analysis,call-graphs,invariants,root-cause +``` + +**The audit-context-building skill will help you answer:** + +1. **Map complete function flow:** + - Entry conditions (preconditions, requires, modifiers) + - State reads (which variables accessed) + - State writes (which variables modified) + - External calls (to contracts, APIs, system) + - Return values and side effects + +2. **Trace internal calls:** + - List all functions called + - Recursively map their flows + - Build complete call graph + +3. **Trace external calls:** + - Identify trust boundaries crossed + - List assumptions about external behavior + - Check for reentrancy risks + +4. **Identify invariants:** + - What must ALWAYS be true? + - What must NEVER happen? + - Are invariants maintained after changes? + +5. **Five Whys root cause:** + - WHY was this code changed? + - WHY did the original code exist? + - WHY might this break? + - WHY is this approach chosen? + - WHY could this fail in production? + +**If `audit-context-building` skill is NOT available**, manually perform the line-by-line analysis above using Read, Grep, and code tracing. + +**Cross-cutting pattern detection:** +```bash +# Find repeated validation patterns +grep -r "require.*amount > 0" --include="*.sol" . +grep -r "onlyOwner" --include="*.sol" . + +# Check if any removed in diff +git diff | grep "^-.*require.*amount > 0" +``` + +**Flag if removal breaks defense-in-depth.** + +--- + +**Next steps:** +- For HIGH RISK changes, proceed to [adversarial.md](adversarial.md) +- For report generation, see [reporting.md](reporting.md) diff --git a/.claude/skills/differential-review/patterns.md b/.claude/skills/differential-review/patterns.md new file mode 100644 index 00000000..71078a72 --- /dev/null +++ b/.claude/skills/differential-review/patterns.md @@ -0,0 +1,300 @@ +# Common Vulnerability Patterns + +Quick reference for detecting common security issues in code changes. + +**Specialized Pattern Resources:** +For specific contexts, reference these additional pattern databases: + +**Domain-Specific:** +- `domain-specific-audits/defi-bridges/resources/` - 127 bridge-specific findings +- `domain-specific-audits/tick-math/resources/` - 81 tick math findings +- `domain-specific-audits/merkle-trees/resources/` - 67 merkle tree findings +- [Check `domain-specific-audits/skills/` for additional domains] + +**Solidity-Specific:** +- `not-so-smart-contracts` - Automated Solidity vulnerability detectors +- `token-integration-analyzer` - Token integration safety patterns +- `building-secure-contracts/development-guidelines` - Solidity best practices + +These complement the generic patterns below. + +--- + +## Security Regressions + +**Pattern:** Previously removed code is re-added + +**Detection:** +```bash +# Code previously removed for security +git log -S "pattern" --all --grep="security\|fix\|CVE" +``` + +**Red flags:** +- Commit message contains "security", "fix", "CVE", "vulnerability" +- Code removed <6 months ago +- No explanation in current PR for re-addition + +**Example:** +```solidity +// Removed in commit abc123 "Fix reentrancy CVE-2024-1234" +// Re-added in current PR +function emergencyWithdraw() { + // REGRESSION: Reentrancy vulnerability re-introduced +} +``` + +--- + +## Double Decrease/Increase Bugs + +**Pattern:** Same accounting operation twice for same event + +**Detection:** Look for two state updates in related functions for same logical action + +**Example:** +```solidity +// Request exit +function requestExit() { + balance[user] -= amount; // First decrease +} + +// Process exit +function processExit() { + balance[user] -= amount; // Second decrease - BUG! +} +``` + +**Impact:** User balance decremented twice, protocol loses funds + +--- + +## Missing Validation + +**Pattern:** Removed `require`/`assert`/`check` without replacement + +**Detection:** +```bash +git diff | grep "^-.*require" +git diff | grep "^-.*assert" +git diff | grep "^-.*revert" +``` + +**Questions to ask:** +- Was validation moved elsewhere? +- Is it redundant (defensive programming)? +- Does removal expose vulnerability? + +**Example:** +```diff +function withdraw(uint256 amount) { +- require(amount > 0, "Zero amount"); +- require(amount <= balance[msg.sender], "Insufficient"); + balance[msg.sender] -= amount; +} +``` + +**Risk:** Zero-amount withdrawals, underflow attacks now possible + +--- + +## Underflow/Overflow + +**Pattern:** Arithmetic without SafeMath or checks + +**Detection:** +- Look for `+`, `-`, `*`, `/` in Solidity <0.8.0 +- Check if SafeMath removed +- Look for unchecked blocks in Solidity >=0.8.0 + +**Example:** +```solidity +// Solidity 0.7 without SafeMath +balance[user] -= amount; // Can underflow if amount > balance + +// Solidity 0.8+ with unchecked +unchecked { + balance[user] -= amount; // Deliberately bypasses overflow check +} +``` + +**Risk:** Integer wrap-around leads to incorrect balances + +--- + +## Reentrancy + +**Pattern:** External call before state update + +**Detection:** Look for CEI (Checks-Effects-Interactions) pattern violations + +**Example:** +```solidity +// VULNERABLE: External call before state update +function withdraw() { + uint amount = balances[msg.sender]; + (bool success,) = msg.sender.call{value: amount}(""); // External call FIRST + require(success); + balances[msg.sender] = 0; // State update AFTER +} + +// SAFE: State update before external call +function withdraw() { + uint amount = balances[msg.sender]; + balances[msg.sender] = 0; // State update FIRST + (bool success,) = msg.sender.call{value: amount}(""); // External call AFTER + require(success); +} +``` + +**Impact:** Attacker can recursively call withdraw() before balance is zeroed + +--- + +## Access Control Bypass + +**Pattern:** Removed or relaxed permission checks + +**Detection:** +```bash +git diff | grep "^-.*onlyOwner" +git diff | grep "^-.*onlyAdmin" +git diff | grep "^-.*require.*msg.sender" +``` + +**Questions:** +- Who can now call this function? +- What's the new trust model? +- Was check moved to caller? + +**Example:** +```diff +- function setConfig(uint value) external onlyOwner { ++ function setConfig(uint value) external { + config = value; + } +``` + +**Risk:** Any user can now modify critical configuration + +--- + +## Race Conditions / Front-Running + +**Pattern:** State-dependent logic without protection + +**Detection:** Look for two-step processes without commit-reveal or timelocks + +**Example:** +```solidity +// Step 1: Approve +function approve(address spender, uint amount) { + allowance[msg.sender][spender] = amount; +} + +// Step 2: User can front-run between approval changes +// Attacker sees tx changing approval from 100 to 50 +// Front-runs to spend 100, then spends 50 after = 150 total +``` + +**Risk:** MEV/front-running exploits state transitions + +--- + +## Timestamp Manipulation + +**Pattern:** Security logic depending on `block.timestamp` + +**Detection:** +```bash +grep -r "block.timestamp" --include="*.sol" +grep -r "now\b" --include="*.sol" # Solidity <0.7 +``` + +**Example:** +```solidity +// VULNERABLE +require(block.timestamp > deadline, "Too early"); +// Miner can manipulate timestamp by ~15 seconds + +// SAFER +require(block.number > deadlineBlock, "Too early"); +// Block numbers are harder to manipulate +``` + +**Risk:** Miners can manipulate timestamps within tolerance + +--- + +## Unchecked Return Values + +**Pattern:** External call without checking success + +**Detection:** +```bash +git diff | grep "\.call\|\.send\|\.transfer" +``` + +**Example:** +```solidity +// VULNERABLE +token.transfer(user, amount); // Ignores return value + +// SAFE +require(token.transfer(user, amount), "Transfer failed"); +// Or use SafeERC20 wrapper +``` + +**Risk:** Silent failures lead to inconsistent state + +--- + +## Denial of Service + +**Pattern:** Unbounded loops, external call reverts blocking execution + +**Detection:** +- Arrays that grow without limit +- Loops over user-controlled array +- Critical function depends on external call success + +**Example:** +```solidity +// DOS: Attacker adds many users, making loop too expensive +function distributeRewards() { + for (uint i = 0; i < users.length; i++) { + users[i].transfer(reward); // Runs out of gas + } +} +``` + +**Risk:** Function becomes unusable due to gas limits + +--- + +## Quick Detection Commands + +**Find removed security checks:** +```bash +git diff | grep "^-" | grep -E "require|assert|revert" +``` + +**Find new external calls:** +```bash +git diff | grep "^+" | grep -E "\.call|\.delegatecall|\.staticcall" +``` + +**Find changed access modifiers:** +```bash +git diff | grep -E "onlyOwner|onlyAdmin|internal|private|public|external" +``` + +**Find arithmetic changes:** +```bash +git diff | grep -E "\+|\-|\*|/" +``` + +--- + +**For detailed analysis workflow, see [methodology.md](methodology.md)** +**For building exploit scenarios, see [adversarial.md](adversarial.md)** diff --git a/.claude/skills/differential-review/reporting.md b/.claude/skills/differential-review/reporting.md new file mode 100644 index 00000000..cb5e37b4 --- /dev/null +++ b/.claude/skills/differential-review/reporting.md @@ -0,0 +1,369 @@ +# Report Generation (Phase 6) + +Comprehensive markdown report structure and formatting guidelines. + +--- + +## Report Structure + +Generate markdown report with these mandatory sections: + +### 1. Executive Summary + +- Severity distribution table +- Risk assessment (CRITICAL/HIGH/MEDIUM/LOW) +- Final recommendation (APPROVE/REJECT/CONDITIONAL) +- Key metrics (test gaps, blast radius, red flags) + +**Template:** +```markdown +# Executive Summary + +| Severity | Count | +|----------|-------| +| 🔴 CRITICAL | X | +| 🟠 HIGH | Y | +| 🟡 MEDIUM | Z | +| 🟢 LOW | W | + +**Overall Risk:** CRITICAL/HIGH/MEDIUM/LOW +**Recommendation:** APPROVE/REJECT/CONDITIONAL + +**Key Metrics:** +- Files analyzed: X/Y (Z%) +- Test coverage gaps: N functions +- High blast radius changes: M functions +- Security regressions detected: P +``` + +--- + +### 2. What Changed + +- Commit timeline with visual +- File summary table +- Lines changed stats + +**Template:** +```markdown +## What Changed + +**Commit Range:** `base..head` +**Commits:** X +**Timeline:** YYYY-MM-DD to YYYY-MM-DD + +| File | +Lines | -Lines | Risk | Blast Radius | +|------|--------|--------|------|--------------| +| file1.sol | +50 | -20 | HIGH | CRITICAL | +| file2.sol | +10 | -5 | MEDIUM | LOW | + +**Total:** +N, -M lines across K files +``` + +--- + +### 3. Critical Findings + +For each HIGH/CRITICAL issue: + +```markdown +### [SEVERITY] Title + +**File**: path/to/file.ext:lineNumber +**Commit**: hash +**Blast Radius**: N callers (HIGH/MEDIUM/LOW) +**Test Coverage**: YES/NO/PARTIAL + +**Description**: [clear explanation] + +**Historical Context**: +- Git blame: Added in commit X (date) +- Message: "[original commit message]" +- [Why this code existed] + +**Attack Scenario**: +[Concrete exploitation steps from adversarial.md] + +**Proof of Concept**: +```code demonstrating issue``` + +**Recommendation**: +[Specific fix with code] +``` + +**Example:** +```markdown +### 🔴 CRITICAL: Authorization Bypass in Withdraw + +**File**: TokenVault.sol:156 +**Commit**: abc123def +**Blast Radius**: 23 callers (HIGH) +**Test Coverage**: NO + +**Description**: +Removed `require(msg.sender == owner)` check allows any user to withdraw funds. + +**Historical Context**: +- Git blame: Added 2024-06-15 (commit def456) +- Message: "Add owner check per audit finding #45" +- Code existed to prevent unauthorized withdrawals + +**Attack Scenario**: +1. Attacker calls `withdraw(1000 ether)` +2. No authorization check (removed) +3. 1000 ETH transferred to attacker +4. Protocol funds drained + +**Proof of Concept**: +```solidity +// As any address +vault.withdraw(vault.balance()); +// Success - funds stolen +``` + +**Recommendation**: +```solidity +function withdraw(uint256 amount) external { ++ require(msg.sender == owner, "Unauthorized"); + // ... rest of function +} +``` +``` + +--- + +### 4. Test Coverage Analysis + +- Coverage statistics +- Untested changes list +- Risk assessment + +**Template:** +```markdown +## Test Coverage Analysis + +**Coverage:** X% of changed code + +**Untested Changes:** +| Function | Risk | Impact | +|----------|------|--------| +| functionA() | HIGH | No validation tests | +| functionB() | MEDIUM | Logic untested | + +**Risk Assessment:** +N HIGH-risk functions without tests → Recommend blocking merge +``` + +--- + +### 5. Blast Radius Analysis + +- High-impact functions table +- Dependency graph +- Impact quantification + +**Template:** +```markdown +## Blast Radius Analysis + +**High-Impact Changes:** +| Function | Callers | Risk | Priority | +|----------|---------|------|----------| +| transfer() | 89 | HIGH | P0 | +| validate() | 45 | MEDIUM | P1 | +``` + +--- + +### 6. Historical Context + +- Security-related removals +- Regression risks +- Commit message red flags + +**Template:** +```markdown +## Historical Context + +**Security-Related Removals:** +- Line 45: `require` removed (added 2024-03 for CVE-2024-1234) +- Line 78: Validation removed (added 2023-12 "security hardening") + +**Regression Risks:** +- Code pattern removed in commit X, re-added in commit Y +``` + +--- + +### 7. Recommendations + +- Immediate actions (blocking) +- Before production (tracking) +- Technical debt (future) + +**Template:** +```markdown +## Recommendations + +### Immediate (Blocking) +- [ ] Fix CRITICAL issue in TokenVault.sol:156 +- [ ] Add tests for withdraw() function + +### Before Production +- [ ] Security audit of auth changes +- [ ] Load test blast radius functions + +### Technical Debt +- [ ] Refactor validation pattern consistency +``` + +--- + +### 8. Analysis Methodology + +- Strategy used (DEEP/FOCUSED/SURGICAL) +- Files analyzed +- Coverage estimate +- Techniques applied +- Limitations +- Confidence level + +**Template:** +```markdown +## Analysis Methodology + +**Strategy:** FOCUSED (80 files, medium codebase) + +**Analysis Scope:** +- Files reviewed: 45/80 (56%) +- HIGH RISK: 100% coverage +- MEDIUM RISK: 60% coverage +- LOW RISK: Excluded + +**Techniques:** +- Git blame on all removals +- Blast radius calculation +- Test coverage analysis +- Adversarial modeling for HIGH RISK + +**Limitations:** +- Did not analyze external dependencies +- Limited to 1-hop caller analysis + +**Confidence:** HIGH for analyzed scope, MEDIUM overall +``` + +--- + +### 9. Appendices + +- Commit reference table +- Key definitions +- Contact info + +--- + +## Formatting Guidelines + +**Tables:** Use markdown tables for structured data + +**Code blocks:** Always include syntax highlighting +```solidity +// Solidity code +``` +```rust +// Rust code +``` + +**Status indicators:** +- ✅ Complete +- ⚠️ Warning +- ❌ Failed/Blocked + +**Severity:** +- 🔴 CRITICAL +- 🟠 HIGH +- 🟡 MEDIUM +- 🟢 LOW + +**Before/After comparisons:** +```markdown +**BEFORE:** +```code +old code +``` + +**AFTER:** +```code +new code +``` +``` + +**Line number references:** Always include +- Format: `file.sol:L123` +- Link to commit: `file.sol:L123 (commit abc123)` + +--- + +## File Naming and Location + +**Priority order for output:** +1. Current working directory (if project repo) +2. User's Desktop +3. `~/.claude/skills/differential-review/output/` + +**Filename format:** +``` +_DIFFERENTIAL_REVIEW_.md + +Example: VeChain_Stargate_DIFFERENTIAL_REVIEW_2025-12-26.md +``` + +--- + +## User Notification Template + +After generating report: + +```markdown +Report generated successfully! + +📄 File: [filename] +📁 Location: [path] +📏 Size: XX KB +⏱️ Review Time: ~X hours + +Summary: +- X findings (Y critical, Z high) +- Final recommendation: APPROVE/REJECT/CONDITIONAL +- Confidence: HIGH/MEDIUM/LOW + +Next steps: +- Review findings in detail +- Address CRITICAL/HIGH issues before merge +- Consider chaining with issue-writer for stakeholder report +``` + +--- + +## Integration with issue-writer + +After generating differential review, transform into audit report: + +```bash +issue-writer --input DIFFERENTIAL_REVIEW_REPORT.md --format audit-report +``` + +This creates polished documentation for non-technical stakeholders. + +--- + +## Error Handling + +If file write fails: +1. Try Desktop location +2. Try temp directory +3. As last resort, output full report to chat +4. Notify user to save manually + +**Always prioritize persistent artifact generation over ephemeral chat output.** diff --git a/.github/workflows/security-review.yml b/.github/workflows/security-review.yml new file mode 100644 index 00000000..6589ace8 --- /dev/null +++ b/.github/workflows/security-review.yml @@ -0,0 +1,174 @@ +name: Security Review (gated) + +# Runs a Trail of Bits "differential-review" security analysis on every PR and +# GATES the merge: the job fails (red required check) when the review surfaces a +# CRITICAL or HIGH severity finding at MEDIUM+ confidence. +# +# Make this a required status check in branch protection (Settings -> Branches) +# so a failing review blocks merge. +# +# Security note: this uses `pull_request` (NOT `pull_request_target`). Fork PRs +# therefore run with a read-only GITHUB_TOKEN and cannot exfiltrate secrets, at +# the cost of the inline PR comment being skipped on forks (the gate + job +# summary still work). Internal PRs get write access and post the review comment. + +on: + pull_request: + types: [opened, synchronize, reopened, ready_for_review] + +concurrency: + group: security-review-${{ github.event.pull_request.number }} + cancel-in-progress: true + +permissions: + contents: read + pull-requests: write + id-token: write + actions: read + +jobs: + security-review: + # Skip draft PRs to save spend; they re-trigger on `ready_for_review`. + if: github.event.pull_request.draft == false + runs-on: ubuntu-latest + timeout-minutes: 30 + env: + # Gate policy (edit to taste). Findings matching BOTH lists block merge. + GATE_SEVERITIES: "CRITICAL HIGH" + GATE_MIN_CONFIDENCES: "HIGH MEDIUM" + VERDICT_FILE: ".security-review/verdict.json" + steps: + - name: Checkout PR head (full history for git blame / regression detection) + uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + fetch-depth: 0 + + - name: Fetch base ref + run: git fetch --no-tags origin "${{ github.event.pull_request.base.sha }}" + + - name: Run differential security review + id: review + uses: anthropics/claude-code-action@v1 + with: + claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + additional_permissions: | + actions: read + # Broad read/write + Bash/Task so the skill can git-blame, grep the + # codebase, delegate to the adversarial-modeler agent, and comment. + claude_args: '--allowedTools "Read,Write,Edit,Grep,Glob,Bash,Task"' + prompt: | + You are running as a GATED, automated security reviewer on pull request + #${{ github.event.pull_request.number }} in ${{ github.repository }}. + + Base SHA: ${{ github.event.pull_request.base.sha }} + Head SHA: ${{ github.event.pull_request.head.sha }} + The changes under review are: `git diff ${{ github.event.pull_request.base.sha }}...HEAD` + + TASK: + 1. Use the `differential-review` skill (vendored at + `.claude/skills/differential-review/`) to perform a security-focused + differential review of ONLY the changes in the diff range above. + Follow its methodology: triage by risk, git-blame removed security + code to catch regressions, calculate blast radius, and run the + adversarial-modeler agent on any HIGH RISK change. + 2. Write the human-readable report to `.security-review/report.md`. + 3. Write a machine-readable verdict to `${{ env.VERDICT_FILE }}` EXACTLY + matching this JSON schema (this file is REQUIRED — the CI gate fails + closed if it is missing or unparseable): + + { + "schema_version": 1, + "head_sha": "", + "base_sha": "", + "summary": "<=400 char plain-English summary of risk posture", + "findings": [ + { + "id": "F1", + "title": "short title", + "severity": "CRITICAL|HIGH|MEDIUM|LOW|INFO", + "confidence": "HIGH|MEDIUM|LOW", + "file": "path/to/file", + "line": 123, + "description": "what is wrong and why it is exploitable", + "recommendation": "concrete fix" + } + ], + "counts": {"CRITICAL":0,"HIGH":0,"MEDIUM":0,"LOW":0,"INFO":0} + } + + SEVERITY/CONFIDENCE RULES: + - Only assign HIGH/MEDIUM confidence when you have concrete evidence + (line numbers + an attack scenario). Speculative concerns are LOW. + - Doc-only / formatting / comment changes: emit zero findings. + - Do NOT include a "gate" field; the gate is computed by CI from + `findings` so it cannot be misjudged in prose. + 4. If you have write access, post a concise PR comment summarizing the + verdict (counts + top findings). If commenting fails (e.g. fork PR), + continue — do not error out. + + Output a one-line final message: "verdict written: N findings". + + - name: Evaluate gate + if: always() + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_NUMBER: ${{ github.event.pull_request.number }} + run: | + set -euo pipefail + VERDICT="$VERDICT_FILE" + + if [ ! -f "$VERDICT" ]; then + echo "::error title=Security review incomplete::No verdict file at $VERDICT. Failing closed." + { + echo "## 🔒 Security Review — FAILED (incomplete)" + echo "" + echo "The reviewer did not produce \`$VERDICT\`. Re-run the job; if it" + echo "keeps failing, inspect the **Run differential security review** step logs." + } >> "$GITHUB_STEP_SUMMARY" + exit 1 + fi + + if ! jq -e . "$VERDICT" >/dev/null 2>&1; then + echo "::error title=Security review invalid::$VERDICT is not valid JSON. Failing closed." + exit 1 + fi + + echo "Gate policy: severity in [$GATE_SEVERITIES] AND confidence in [$GATE_MIN_CONFIDENCES] blocks merge." + + # Split the space-separated env lists inside jq so the logic does not + # depend on shell word-splitting behavior. + blocking=$(jq --arg sev "$GATE_SEVERITIES" --arg conf "$GATE_MIN_CONFIDENCES" ' + ($sev | split(" ")) as $S | ($conf | split(" ")) as $C | + [.findings[]? | select((.severity as $s | $S | index($s)) and (.confidence as $c | $C | index($c)))]' \ + "$VERDICT") + blocking_count=$(echo "$blocking" | jq 'length') + total=$(jq '.findings | length' "$VERDICT") + summary=$(jq -r '.summary // "(no summary)"' "$VERDICT") + + { + echo "## 🔒 Security Review" + echo "" + echo "$summary" + echo "" + echo "**Findings:** $total total · **$blocking_count blocking** (severity ∈ {$GATE_SEVERITIES}, confidence ∈ {$GATE_MIN_CONFIDENCES})" + echo "" + if [ "$total" -gt 0 ]; then + echo "| Severity | Confidence | File:Line | Title |" + echo "|---|---|---|---|" + jq -r '.findings[]? | "| \(.severity) | \(.confidence) | \(.file // "?"):\(.line // "?") | \(.title) |"' "$VERDICT" + echo "" + fi + echo "
Full report" + echo "" + echo '```' + [ -f .security-review/report.md ] && cat .security-review/report.md || echo "(report.md not produced)" + echo '```' + echo "
" + } >> "$GITHUB_STEP_SUMMARY" + + if [ "$blocking_count" -gt 0 ]; then + echo "::error title=Security gate failed::$blocking_count blocking finding(s). See job summary / PR comment." + exit 1 + fi + echo "Security gate passed: no blocking findings." diff --git a/.gitignore b/.gitignore index 1c694dbf..493dc94d 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,10 @@ .vscode .cursor -.claude +# Ignore local Claude state, but track the vendored security-review skill + agent +# used by .github/workflows/security-review.yml +.claude/* +!.claude/skills/ +!.claude/agents/ .agents .env docker-compose.deploy.yml From 0a217bb931aa684af930d08efe8d7cb4bdb225ba Mon Sep 17 00:00:00 2001 From: GTC6244 <95836911+GTC6244@users.noreply.github.com> Date: Thu, 2 Jul 2026 15:02:06 -0400 Subject: [PATCH 2/4] ci(security-review): don't fail closed when claude-code-action self-skips MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit anthropics/claude-code-action performs "workflow validation": it refuses to run unless this workflow file is byte-identical to the copy on the default branch. That makes the review unable to run on the very PR that introduces or edits it, so the fail-closed gate turned that PR red and unmergeable (chicken-and-egg). Detect that exact condition — workflow file absent from or differing from the default branch — and pass with a notice instead, while still failing closed for genuine post-merge runs. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/security-review.yml | 31 +++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/.github/workflows/security-review.yml b/.github/workflows/security-review.yml index 6589ace8..817f4fca 100644 --- a/.github/workflows/security-review.yml +++ b/.github/workflows/security-review.yml @@ -114,10 +114,41 @@ jobs: env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} PR_NUMBER: ${{ github.event.pull_request.number }} + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} + WORKFLOW_FILE: .github/workflows/security-review.yml run: | set -euo pipefail VERDICT="$VERDICT_FILE" + # anthropics/claude-code-action refuses to run ("workflow validation") + # unless this workflow file is byte-identical to the copy on the default + # branch — so the review CANNOT run on the PR that introduces or edits + # this file (GitHub says as much: it "will begin working once you merge"). + # Failing closed there is a false positive that would make such PRs + # unmergeable, so we detect that condition exactly as GitHub does and + # pass with a notice. This adds no bypass: PRs touching CI workflows are + # human-reviewed, and fork PRs cannot alter privileged workflows. + git fetch --no-tags --depth=1 origin "$DEFAULT_BRANCH" 2>/dev/null || true + review_skipped_reason="" + if ! git cat-file -e "FETCH_HEAD:$WORKFLOW_FILE" 2>/dev/null; then + review_skipped_reason="is not yet present on the default branch ($DEFAULT_BRANCH)" + elif ! diff -q <(git show "FETCH_HEAD:$WORKFLOW_FILE") "$WORKFLOW_FILE" >/dev/null 2>&1; then + review_skipped_reason="differs from the copy on the default branch ($DEFAULT_BRANCH)" + fi + + if [ -n "$review_skipped_reason" ]; then + echo "::notice title=Security review not gating yet::Workflow file $review_skipped_reason; GitHub blocks claude-code-action here. The gate enforces once this workflow is on $DEFAULT_BRANCH." + { + echo "## 🔒 Security Review — skipped (not gating yet)" + echo "" + echo "\`claude-code-action\` cannot run because this workflow file $review_skipped_reason." + echo "This is expected on the PR that adds or edits the review workflow itself." + echo "" + echo "**Merge this PR; the gate enforces automatically on subsequent PRs.**" + } >> "$GITHUB_STEP_SUMMARY" + exit 0 + fi + if [ ! -f "$VERDICT" ]; then echo "::error title=Security review incomplete::No verdict file at $VERDICT. Failing closed." { From 2d8297cceb7831968efc82920fe79f8e026a4a20 Mon Sep 17 00:00:00 2001 From: GTC6244 <95836911+GTC6244@users.noreply.github.com> Date: Fri, 3 Jul 2026 11:54:02 -0400 Subject: [PATCH 3/4] ci(security-review): handle fork PRs and validate verdict schema Address PR review feedback: - Fork PRs: `pull_request` withholds secrets from forks, so the review step now skips on forked-repo PRs and the gate passes with a notice instead of failing closed and blocking every fork PR. Correct the header comment, which wrongly claimed the gate still works on forks. - Verdict schema: `jq -e .` only checked JSON validity, so `{}` passed and a missing `findings` was read as "0 findings" (false pass). Now require a findings[] array with a valid severity + confidence enum on every entry, failing closed otherwise. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/security-review.yml | 52 ++++++++++++++++++++++++--- 1 file changed, 48 insertions(+), 4 deletions(-) diff --git a/.github/workflows/security-review.yml b/.github/workflows/security-review.yml index 817f4fca..15791f8f 100644 --- a/.github/workflows/security-review.yml +++ b/.github/workflows/security-review.yml @@ -7,10 +7,11 @@ name: Security Review (gated) # Make this a required status check in branch protection (Settings -> Branches) # so a failing review blocks merge. # -# Security note: this uses `pull_request` (NOT `pull_request_target`). Fork PRs -# therefore run with a read-only GITHUB_TOKEN and cannot exfiltrate secrets, at -# the cost of the inline PR comment being skipped on forks (the gate + job -# summary still work). Internal PRs get write access and post the review comment. +# Security note: this uses `pull_request` (NOT `pull_request_target`), so secrets +# (including CLAUDE_CODE_OAUTH_TOKEN) are NOT exposed to forked-repo PRs. The +# review therefore cannot run on fork PRs; rather than fail closed and block them, +# the gate detects the fork case and passes with a notice (see "Evaluate gate"). +# Same-repo (internal) PRs get the token, run the review, and are gated normally. on: pull_request: @@ -49,6 +50,10 @@ jobs: - name: Run differential security review id: review + # Skip on forked-repo PRs: `pull_request` withholds secrets from forks, so + # the token is empty and the review cannot run. The gate step handles the + # fork case explicitly (passes with a notice) so forks are not blocked. + if: github.event.pull_request.head.repo.full_name == github.repository uses: anthropics/claude-code-action@v1 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} @@ -116,10 +121,28 @@ jobs: PR_NUMBER: ${{ github.event.pull_request.number }} DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} WORKFLOW_FILE: .github/workflows/security-review.yml + IS_FORK: ${{ github.event.pull_request.head.repo.full_name != github.repository }} run: | set -euo pipefail VERDICT="$VERDICT_FILE" + # Forked-repo PRs: `pull_request` withholds secrets, so the review step + # was skipped (no token). Do not fail closed — that would block every + # fork PR. Pass with a notice; a maintainer can re-run from a same-repo + # branch to get a gated review. + if [ "$IS_FORK" = "true" ]; then + echo "::notice title=Security review not gating (fork PR)::Secrets are unavailable to forked-repo PRs, so the automated review cannot run. Push the branch to this repository for a gated review." + { + echo "## 🔒 Security Review — skipped (fork PR)" + echo "" + echo "GitHub withholds secrets from forked-repo pull requests, so the" + echo "automated reviewer could not run. This check does not block fork PRs." + echo "" + echo "A maintainer can re-run the review from a same-repo branch." + } >> "$GITHUB_STEP_SUMMARY" + exit 0 + fi + # anthropics/claude-code-action refuses to run ("workflow validation") # unless this workflow file is byte-identical to the copy on the default # branch — so the review CANNOT run on the PR that introduces or edits @@ -165,6 +188,27 @@ jobs: exit 1 fi + # Schema validation (fail closed on anything unexpected). Valid-but-empty + # JSON like `{}` must NOT be treated as "0 findings" — require a + # `findings` array and, for every entry, a recognized severity AND + # confidence enum. A finding with a bogus/missing severity would silently + # evade the blocking filter below, so reject the whole verdict instead. + if ! jq -e ' + (type == "object") + and ((.findings | type) == "array") + and (all(.findings[]?; + (.severity as $s | ["CRITICAL","HIGH","MEDIUM","LOW","INFO"] | index($s)) + and (.confidence as $c | ["HIGH","MEDIUM","LOW"] | index($c)))) + ' "$VERDICT" >/dev/null 2>&1; then + echo "::error title=Security review invalid::$VERDICT failed schema validation (needs a findings[] array with a valid severity + confidence on every entry). Failing closed." + { + echo "## 🔒 Security Review — FAILED (invalid verdict)" + echo "" + echo "The verdict file did not match the required schema. Failing closed." + } >> "$GITHUB_STEP_SUMMARY" + exit 1 + fi + echo "Gate policy: severity in [$GATE_SEVERITIES] AND confidence in [$GATE_MIN_CONFIDENCES] blocks merge." # Split the space-separated env lists inside jq so the logic does not From db8fe00268849aecb1db987fde17088394011e87 Mon Sep 17 00:00:00 2001 From: GTC6244 <95836911+GTC6244@users.noreply.github.com> Date: Sat, 11 Jul 2026 20:49:03 -0400 Subject: [PATCH 4/4] ci(security-review): close fail-open holes from Claude review Address the Claude code review on PR #550: - HIGH: a verdict committed into the PR would let the gate read attacker-supplied findings whenever the review step fails/times out. Add a pre-step that refuses a tracked .security-review/ and wipes any working-tree copy, and gitignore .security-review/ so local runs can't commit one. - MEDIUM: the "not gating yet" workflow check compared the writable working tree, which the review step (Write/Edit) could mutate to force the skip path. Compare the committed HEAD copy instead. - LOW: fail loudly if the default branch can't be fetched, instead of silently comparing a stale FETCH_HEAD left by the base-ref fetch. - Nits: scope the .claude gitignore negations to only the vendored skill/agent; escape pipes in model-authored step-summary table cells; drop the unused `counts` object from the prompt schema; document that fork PRs pass with zero automated review once the check is required. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/security-review.yml | 39 ++++++++++++++++++++++----- .gitignore | 13 +++++++-- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/.github/workflows/security-review.yml b/.github/workflows/security-review.yml index 15791f8f..9160a85c 100644 --- a/.github/workflows/security-review.yml +++ b/.github/workflows/security-review.yml @@ -12,6 +12,8 @@ name: Security Review (gated) # review therefore cannot run on fork PRs; rather than fail closed and block them, # the gate detects the fork case and passes with a notice (see "Evaluate gate"). # Same-repo (internal) PRs get the token, run the review, and are gated normally. +# CAUTION: once this is a required check, fork PRs pass with ZERO automated +# security review by design — human review must cover forked-repo contributions. on: pull_request: @@ -48,6 +50,20 @@ jobs: - name: Fetch base ref run: git fetch --no-tags origin "${{ github.event.pull_request.base.sha }}" + - name: Clear any pre-committed verdict (prevent fail-open) + run: | + set -euo pipefail + # The gate reads the verdict from the PR-head working tree. A verdict + # committed INTO the PR would let attacker-supplied findings satisfy the + # gate whenever the review step fails/times out (or is skipped). Refuse a + # tracked .security-review/ and wipe any working-tree copy so the gate can + # only ever read THIS run's freshly generated output. + if [ -n "$(git ls-files .security-review)" ]; then + echo "::error title=Security review::.security-review/ is generated by CI and must not be committed. Remove it from the PR." + exit 1 + fi + rm -rf .security-review + - name: Run differential security review id: review # Skip on forked-repo PRs: `pull_request` withholds secrets from forks, so @@ -98,8 +114,7 @@ jobs: "description": "what is wrong and why it is exploitable", "recommendation": "concrete fix" } - ], - "counts": {"CRITICAL":0,"HIGH":0,"MEDIUM":0,"LOW":0,"INFO":0} + ] } SEVERITY/CONFIDENCE RULES: @@ -109,8 +124,8 @@ jobs: - Do NOT include a "gate" field; the gate is computed by CI from `findings` so it cannot be misjudged in prose. 4. If you have write access, post a concise PR comment summarizing the - verdict (counts + top findings). If commenting fails (e.g. fork PR), - continue — do not error out. + verdict (severity breakdown + top findings). If commenting fails + (e.g. fork PR), continue — do not error out. Output a one-line final message: "verdict written: N findings". @@ -151,11 +166,20 @@ jobs: # unmergeable, so we detect that condition exactly as GitHub does and # pass with a notice. This adds no bypass: PRs touching CI workflows are # human-reviewed, and fork PRs cannot alter privileged workflows. - git fetch --no-tags --depth=1 origin "$DEFAULT_BRANCH" 2>/dev/null || true + # + # Fail loudly if the default branch can't be fetched: a stale FETCH_HEAD + # (left over from the base-ref fetch) would silently compare the wrong + # ref. And compare the COMMITTED head copy (`HEAD:...`), never the working + # tree — the review step runs first with Write/Edit and could otherwise + # mutate the on-disk workflow to force the "skipped" path. + if ! git fetch --no-tags --depth=1 origin "$DEFAULT_BRANCH"; then + echo "::error title=Security review::Could not fetch default branch ($DEFAULT_BRANCH) to determine gate state. Failing closed." + exit 1 + fi review_skipped_reason="" if ! git cat-file -e "FETCH_HEAD:$WORKFLOW_FILE" 2>/dev/null; then review_skipped_reason="is not yet present on the default branch ($DEFAULT_BRANCH)" - elif ! diff -q <(git show "FETCH_HEAD:$WORKFLOW_FILE") "$WORKFLOW_FILE" >/dev/null 2>&1; then + elif ! diff -q <(git show "FETCH_HEAD:$WORKFLOW_FILE") <(git show "HEAD:$WORKFLOW_FILE") >/dev/null 2>&1; then review_skipped_reason="differs from the copy on the default branch ($DEFAULT_BRANCH)" fi @@ -231,7 +255,8 @@ jobs: if [ "$total" -gt 0 ]; then echo "| Severity | Confidence | File:Line | Title |" echo "|---|---|---|---|" - jq -r '.findings[]? | "| \(.severity) | \(.confidence) | \(.file // "?"):\(.line // "?") | \(.title) |"' "$VERDICT" + # Escape pipes in model-authored strings so they can't break the table. + jq -r '.findings[]? | ([.severity, .confidence, "\(.file // "?"):\(.line // "?")", (.title // "")] | map(gsub("\\|"; "\\|")) | "| \(.[0]) | \(.[1]) | \(.[2]) | \(.[3]) |")' "$VERDICT" echo "" fi echo "
Full report" diff --git a/.gitignore b/.gitignore index 493dc94d..98cde83c 100644 --- a/.gitignore +++ b/.gitignore @@ -1,11 +1,20 @@ .vscode .cursor -# Ignore local Claude state, but track the vendored security-review skill + agent -# used by .github/workflows/security-review.yml +# Ignore local Claude state, but track ONLY the vendored security-review skill + +# agent used by .github/workflows/security-review.yml (scoped so personal local +# skills/agents are not accidentally committed by `git add -A`). .claude/* !.claude/skills/ +.claude/skills/* +!.claude/skills/differential-review/ !.claude/agents/ +.claude/agents/* +!.claude/agents/adversarial-modeler.md .agents + +# Generated by the security-review workflow; must never be committed (a committed +# verdict would let the gate fail open). +.security-review/ .env docker-compose.deploy.yml .digest-*.txt