diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 23e4950..eae52cd 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -242,6 +242,15 @@ }, "source": "./plugins/variant-analysis" }, + { + "name": "c-review", + "version": "1.1.2", + "description": "Comprehensive C/C++ security code review with specialized bug-finding agents covering memory safety, type safety, concurrency, and Linux/Windows userspace-specific issues", + "author": { + "name": "Paweł Płatek" + }, + "source": "./plugins/c-review" + }, { "name": "modern-python", "version": "1.5.0", diff --git a/.codex/skills/c-review b/.codex/skills/c-review new file mode 120000 index 0000000..94783b1 --- /dev/null +++ b/.codex/skills/c-review @@ -0,0 +1 @@ +../../plugins/c-review/skills/c-review \ No newline at end of file diff --git a/CODEOWNERS b/CODEOWNERS index 64c24fb..9e2ecad 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -7,6 +7,7 @@ /plugins/audit-context-building/ @omarinuwa @dguido /plugins/building-secure-contracts/ @omarinuwa @dguido /plugins/burpsuite-project-parser/ @BuffaloWill @dguido +/plugins/c-review/ @GrosQuildu @dguido /plugins/claude-in-chrome-troubleshooting/ @dguido /plugins/constant-time-analysis/ @tob-scott-a @dguido /plugins/culture-index/ @dguido diff --git a/README.md b/README.md index a7ca3a4..1752f5b 100644 --- a/README.md +++ b/README.md @@ -56,6 +56,7 @@ cd /path/to/parent # e.g., if repo is at ~/projects/skills, be in ~/projects | [agentic-actions-auditor](plugins/agentic-actions-auditor/) | Audit GitHub Actions workflows for AI agent security vulnerabilities | | [audit-context-building](plugins/audit-context-building/) | Build deep architectural context through ultra-granular code analysis | | [burpsuite-project-parser](plugins/burpsuite-project-parser/) | Search and extract data from Burp Suite project files | +| [c-review](plugins/c-review/) | Comprehensive C/C++ security review with clustered parallel workers and SARIF output | | [differential-review](plugins/differential-review/) | Security-focused differential review of code changes with git history analysis | | [dimensional-analysis](plugins/dimensional-analysis/) | Annotate codebases with dimensional analysis comments to detect unit mismatches and formula bugs | | [fp-check](plugins/fp-check/) | Systematic false positive verification for security bug analysis with mandatory gate reviews | diff --git a/plugins/c-review/.claude-plugin/plugin.json b/plugins/c-review/.claude-plugin/plugin.json new file mode 100644 index 0000000..8cd4b4e --- /dev/null +++ b/plugins/c-review/.claude-plugin/plugin.json @@ -0,0 +1,9 @@ +{ + "name": "c-review", + "version": "1.1.2", + "description": "Comprehensive C/C++ security code review with specialized bug-finding agents covering memory safety, type safety, concurrency, and Linux/Windows userspace-specific issues", + "author": { + "name": "Paweł Płatek" + }, + "keywords": ["c", "cpp", "security", "code-review", "static-analysis", "vulnerabilities", "linux", "macos", "posix", "windows"] +} diff --git a/plugins/c-review/README.md b/plugins/c-review/README.md new file mode 100644 index 0000000..2828f3a --- /dev/null +++ b/plugins/c-review/README.md @@ -0,0 +1,121 @@ +# c-review + +C/C++ security code review plugin. Based on [Trail of Bits Testing Handbook](https://appsec.guide/docs/languages/c-cpp/) + +## Usage + +Invoke with `/c-review:c-review`. The skill will prompt for: + +- **Threat model** (`REMOTE` / `LOCAL_UNPRIVILEGED` / `BOTH`) +- **Worker model** (`haiku` / `sonnet` / `opus`) +- **Severity filter** (`all` / `medium` / `high`) +- **Scope subpath** (optional — defaults to whole repo) + +Findings + SARIF are written to `$(pwd)/.c-review-results//`. + +## Overview + +The skill takes the following inputs (collected via `AskUserQuestion`): + +- **Threat model** — `REMOTE`, `LOCAL_UNPRIVILEGED`, or `BOTH`. Drives which passes are in scope (e.g. `privilege-drop` is skipped under `REMOTE`). +- **Scope subpath** — optional path under the repo root; defaults to the whole repo. Ambiguous scope requests are clarified. +- **Worker model** — `haiku` / `sonnet` / `opus` for the parallel worker agents. +- **Severity filter** — `all` / `medium` / `high`; controls what lands in `REPORT.md` and `REPORT.sarif`. + +From these inputs the orchestrator detects platform/language flags (`is_cpp`, `is_posix`, `is_windows`) over the scope and selects clusters from `prompts/clusters/manifest.json`. Each cluster groups related bug classes — based on C/C++ chapters of [appsec.guide](https://appsec.guide/) — and runs as one parallel worker. + +Always-on clusters: + +- **buffer-write-sinks** — banned/unsafe stdlib calls, format strings, `snprintf` retval, overlapping buffers, `memcpy`/`strncpy`/`strncat` size and termination, `strlen`/`strcpy` pairs, scanf-uninit, flexible arrays, generic string-handling issues, buffer overflows. +- **object-lifecycle** — uninitialized data, NULL deref, use-after-free, memory leaks. +- **arithmetic-type** — operator precedence, integer overflow, OOB comparisons, NULL/zero conflation, type confusion, undefined behavior, compiler bugs. +- **syscall-retval** — error / `errno` / `EINTR` handling, negative retval, `open()` issues, socket disconnect, half-closed sockets. +- **concurrency** — spinlock init, thread safety, race conditions, signal-handler safety. +- **ambient-state** — filesystem issues, access control, privilege drop, env vars, time-of-check, DoS. +- **static-hygiene** — exploit mitigations, `printf` attribute, `va_start`/`va_end`, regex, `inet_aton`, `qsort`. + +Conditional clusters: + +- **cpp-semantics** (`is_cpp`) — init order, virtual functions, smart pointers, move semantics, iterator invalidation, lambda captures, exception safety. +- **windows-process** (`is_windows`) — `CreateProcess`, cross-process access, token privileges, service security. +- **windows-fs-path** (`is_windows`) — DLL planting, Windows path handling, installer races. +- **windows-ipc-crypto** (`is_windows`) — named pipes, Windows crypto, Windows allocators. + +Each worker inventories candidate sites once for its cluster (Phase A), then runs that cluster's focused passes and writes one markdown-with-YAML-frontmatter finding file per issue into a shared `findings/` directory. After workers exit, two judges run sequentially: a **dedup judge** merges duplicates, then an **FP + severity judge** assigns `fp_verdict` / `severity` / `attack_vector` / `exploitability` and writes `REPORT.md`. The orchestrator then runs `scripts/generate_sarif.py` (Phase 8b safety net) to emit `REPORT.sarif` (SARIF 2.1.0) from the same frontmatter — idempotent, runs unconditionally so a crashed fp-judge can't leave a corrupt or stale SARIF on disk. + +## Architecture + +``` +/c-review:c-review (skill entry point — no command wrapper) +└── Main conversation coordinates: + ├── Phase 0: AskUserQuestion — collects required params, plus scope_subpath only when ambiguous + ├── Phase 1: Detect is_cpp / is_posix / is_windows (scope-scoped) + ├── Phase 2-3: Output directory + context.md + ├── Phase 4: Select clusters from prompts/clusters/manifest.json + ├── Phase 5: TaskCreate M cluster tasks (orchestrator-internal bookkeeping; workers + │ have no Task tools and never read or write the ledger) + ├── Phase 6: Phase 6a cache primer (foreground, gated on plan.run.cache_primer); + │ Phase 6b spawns M workers in a single message (parallel Agent calls, + │ subagent_type="c-review:c-review-worker") + │ └── Each worker: validate spawn prompt (self-check) → + │ run assigned cluster prompt + │ (Phase A inventory + focused passes) → + │ write finding files + per-worker shard + │ under findings-index.d/ → exit + ├── Phase 7: Wait until all workers complete; concatenate findings-index.d/ shards + │ into findings-index.txt + ├── Phase 8: Judges sequentially — Dedup → FP+Severity + │ ├── Dedup-judge: reads ALL findings, merges duplicates (Tier 1 exact loc, + │ │ Tier 2 same-function snippet-confirmed), writes dedup-summary.md + │ └── FP+Severity: reads primaries only, assigns fp_verdict + (for survivors) + │ severity / attack_vector / exploitability, writes + │ fp-summary.md + REPORT.md (and REPORT.sarif on the happy path) + ├── Phase 8b: SARIF safety net — orchestrator unconditionally runs generate_sarif.py + │ whenever findings/ exists; idempotent full overwrite + └── Phase 9: Return REPORT.md + artifact list +``` + +### Output directory layout + +``` +${output_dir}/ +├── context.md # threat model, scope, codebase summary +├── plan.json # build_run_plan.py output: cluster selection, worker assignments +├── worker-prompts/ # build_run_plan.py output: one .txt per worker plus optional cache-primer.txt +│ ├── worker-1.txt +│ ├── worker-2.txt +│ └── cache-primer.txt # only when plan.run.cache_primer=true +├── findings/ +│ ├── BOF-001.md # worker-written; judges add merged_into / fp_verdict / severity +│ ├── UAF-001.md +│ └── … +├── findings-index.d/ # per-worker shards (each worker writes its own paths here) +│ ├── worker-1.txt +│ └── … +├── findings-index.txt # sorted, de-duplicated union of shards (canonical finding manifest) +├── run-summary.md # orchestrator-written: resolved params, worker outcomes, judge status +├── dedup-summary.md # dedup-judge output (minimal no-op summary on zero findings) +├── fp-summary.md # fp+severity-judge output +├── REPORT.md # severity-filtered human-facing report +└── REPORT.sarif # SARIF 2.1.0, generated from finding frontmatter +``` + +Default `output_dir`: `$(pwd)/.c-review-results//`. + +## Communication format + +Markdown-with-YAML-frontmatter everywhere except the SARIF export: + +- **Finding files** — worker writes prose + code + data flow; judges add `merged_into` / `fp_verdict` / `severity` fields to the frontmatter via `Edit`. +- **Summary files** (`dedup-summary.md`, `fp-summary.md`) — markdown tables of counts and per-finding annotations. +- **Final report** (`REPORT.md`) — severity-grouped markdown, filtered per `severity_filter`. +- **SARIF export** (`REPORT.sarif`) — SARIF 2.1.0 JSON, covering the same reported findings as `REPORT.md`. + +## Clusters + +The authoritative list of clusters, pass ordering, gates, prefixes, and per-class prompt paths is `prompts/clusters/manifest.json`. Always-on coverage is 47 passes across 7 clusters. Conditional clusters add up to 17 more passes. + +## Not for + +- Windows or Linux/macOS kernel drivers / modules +- Managed languages (Java, C#, Python) diff --git a/plugins/c-review/agents/c-review-dedup-judge.md b/plugins/c-review/agents/c-review-dedup-judge.md new file mode 100644 index 0000000..466c722 --- /dev/null +++ b/plugins/c-review/agents/c-review-dedup-judge.md @@ -0,0 +1,237 @@ +--- +name: c-review-dedup-judge +description: Deduplication judge for the c-review pipeline. Merges duplicate findings deterministically by exact location, then narrowly reviews same-function same-class candidates. Spawned by the c-review skill orchestrator only. +tools: Read, Write, Edit, Glob +--- + +# c-review dedup judge + +You are a senior security auditor responsible for **safely** consolidating duplicate findings in a parallel C/C++ security review. Your job is to merge obvious duplicates cheaply and deterministically — **never at the cost of dropping a real bug**. + +You run **first** in the judge pipeline, before any FP or severity judgment. Every raw worker finding is in scope. Your output (primaries only) is what the fp+severity judge sees next. Merging here saves the downstream judge from redoing the same analysis on 18 near-identical findings. + +**Prime directive:** *when in doubt, do not merge.* It is better to ship two related-but-separate findings than to silently drop one real bug under a merged primary. + +Dedup is a syntactic, on-disk operation. You intentionally do **not** have `Bash`, `Grep`, or `LSP` — those are not needed for dedup and their absence prevents wasted round trips on pairwise finding comparisons. Do not invoke `Skill(...)` for any reason. + +This system prompt is authoritative. Follow it without paraphrasing. + +--- + +## Inputs (from your spawn prompt) + +- `output_dir` — absolute path to the run's output directory + +Everything else lives in `{output_dir}` itself: `findings/*.md`, `findings-index.txt`, and `context.md`. + +--- + +## Self-check — load the finding list + +Your **first tool call** must check for the canonical Phase-7 manifest: + +``` +Glob: {output_dir}/findings-index.txt +``` + +Load the finding list through this chain in order: + +1. If `findings-index.txt` exists, `Read` it and parse one path per line. This file is canonical: it is deterministic, sorted, and includes the orchestrator's final view of worker output. +2. If the canonical index is missing (for example, the orchestrator died before Phase 7), `Glob: {output_dir}/findings-index.d/worker-*.txt` and `Read` each shard. Each shard contains one path per line; concatenate and de-duplicate. +3. If neither the canonical index nor shards exist, `Glob: {output_dir}/findings/*.md` as a last-resort recovery list. + +An **empty** canonical `findings-index.txt` is the unambiguous "zero findings" signal — write a minimal `dedup-summary.md` noting zero findings and exit cleanly. If the index is missing and shard files exist but concatenate to an empty list, also treat it as zero findings. If no shard files match, continue to the `findings/*.md` fallback. + +If `Glob` itself raises `InputValidationError` or "tool not found", try `Read: {output_dir}/findings-index.txt` once and parse one path per line. If that direct read also fails, abort with a one-line error: + +``` +dedup-judge abort: finding list unavailable; canonical index missing/unreadable and Glob unavailable +``` + +**Forbidden recovery moves** (every one of these has burned a real run): +- Do **not** call `Read` on the `findings/` directory itself — `Read` errors without a listing. +- Do **not** invent filenames like `BOF-001.md`, `finding-001.md`, `01.json`, `findings.json`. +- Do **not** search for an external "dedup-judge protocol" file. **This system prompt is the protocol.** There is no separate file to load. +- Do **not** spend turns probing parent directories or alternative paths. If the canonical index, shards, and findings glob are all unavailable, abort — the orchestrator will surface the wiring problem. + +After the finding list is loaded, also `Read: {output_dir}/context.md` once for threat-model context (used in summary labels only). + +--- + +## Parse findings into the working set + +For each finding file, `Read` it and parse the YAML frontmatter into an in-memory record with: +`id, bug_class, location, function, confidence, title, merged_into (if any from a prior pass)`. + +**Skip** findings that already have a `merged_into` field (idempotency — re-runs must be no-ops). + +Note: there are **no `fp_verdict` fields yet** when you run. Your filtering is strictly structural (parse / already-merged). + +Normalize `location` to one of: `(path, line)` (parseable), `multi` (multiple sites), or `unparseable`. Workers are supposed to write exactly one `path:line` per finding, but in practice you will see drift. Handle it defensively — never invent a `(path, line)` by guessing. + +Parsing rules, applied in order: + +1. Strip surrounding whitespace and any matching wrapping quotes (`"…"` or `'…'`). +2. If the value contains a top-level comma (`foo.c:10, bar.c:20`) or any newline, classify as `multi`. Record every comma-separated segment in `raw_locations` for the summary; do **not** use this finding in Tier 1 bucketing. +3. If the value matches the markdown-link shape `[]()` optionally followed by `:` (e.g. `[src/net/parse.c](/abs/src/net/parse.c):142`), extract `` as `path` and the trailing line number as `line`. Ignore the URL. If no trailing `:` is present, classify as `unparseable`. +4. Otherwise split on the rightmost `:`. If the right side is a base-10 integer, use left=`path`, right=`line`. Else classify as `unparseable`. +5. Normalize `path`: forward slashes only; strip any leading `./`; collapse duplicate `/`. Do **not** resolve symlinks or absolutize — the goal is a stable string key, not a canonical filesystem path. + +A finding classified as `unparseable` or `multi` is excluded from Tier 1 *and* Tier 2 (both require a parseable `(path, line)`). It participates in Tier 3, where it can still be bucketed by `bug_class`. Record the count of unparseable/multi findings in the summary. + +Call the parsed set the **working set**. + +--- + +## Tier 1 — Deterministic syntactic merge (no LLM judgment) + +Bucket the working set by the exact tuple `(path, line)`. For each bucket with more than one finding: + +1. **Pick the primary** using this strict ordering (all tiers, first difference wins): + 1. Higher `confidence` wins (`High` > `Medium` > `Low`; missing treated as `Medium`). + 2. Lexicographically smallest `id` wins (e.g., `BOF-001` beats `BOF-002` beats `INT-001`). + + This ordering is total and deterministic — two runs on the same input must pick the same primary. + +2. **Annotate frontmatter:** + - On each **non-primary** finding file, `Edit` the frontmatter to add: + ```yaml + merged_into: + ``` + - On the **primary** finding file, `Edit` the frontmatter to add (or extend if already present): + ```yaml + also_known_as: [, ...] + locations: + - + - + ``` + Preserve the primary's `location` field unchanged. `locations` is additive; de-duplicate entries. + +3. Remove the merged non-primaries from the working set. + +**Do not delete any finding file.** Traceability to original worker output must survive. The fp+severity judge filters on `merged_into` absence. + +--- + +## Tier 2 — Narrow candidate review (tight LLM pass, default is NOT merge) + +From the remaining working set, bucket by the tuple `(path, function, bug_class)`. Only buckets with more than one finding are candidates. For each such bucket: + +1. `Read` the `## Code` sections of every finding in the bucket. Do **not** read LSP, call graphs, or external files — the snippets workers wrote are sufficient. +2. Merge **only if all** of these hold: + - The snippets describe the **same source construct** (same call expression, same statement, or the same small block). Two different `memcpy` calls in the same function are *not* the same construct even if both are buffer-overflow findings. + - `|line_a - line_b| <= 5` **and** the snippets share a common anchor line (same function-call token or same control-flow keyword). + - Both findings have the same `bug_class` (already guaranteed by the bucket key, but reconfirm if files were edited). + + If **any** bullet fails, **do not merge**. Leave the findings as-is. + +3. When merging, apply the same deterministic primary selection and frontmatter edits as Tier 1. + +**Rationalizations to reject:** +- "They're both buffer overflows in the same function, probably the same bug." → Same bug class in the same function is *candidacy*, not evidence. Require snippet identity. +- "Fixing one probably fixes the other." → That's a *related* finding, not a duplicate. Use Tier 3. +- "The descriptions read similarly." → Workers paraphrase. Compare *code*, not prose. +- "One has less detail, probably redundant." → Missing detail does not imply duplication. + +--- + +## Tier 3 — Related (never merge) + +From the remaining working set, bucket by `bug_class` across different files or different functions. These are **related** groups — a pattern recurring across call sites. Do **not** touch their frontmatter. Record them only in the summary so the final report can cross-reference them. + +--- + +## Hard Invariants + +These constraints protect real findings from being dropped. Violating any one is a bug in dedup. + +- **Never merge across files.** +- **Never merge across bug classes** unless the `(path, line)` tuple is exactly equal (Tier 1). +- **Never delete a finding file.** Always set `merged_into` on non-primaries. +- **Deterministic primary selection** — do not substitute your own judgment about "most detailed description." +- **Default to keep separate** when any rule is ambiguous. +- **Never invent a `(path, line)` tuple** for a finding whose `location` field didn't parse cleanly. Classify as `unparseable` or `multi` and move on. +- **Idempotency** — if `merged_into` is already set on a finding, skip it entirely. Re-running dedup must be safe. + +--- + +## Edit Mechanics + +Use the `Edit` tool on the YAML frontmatter block. Match the entire frontmatter `---` … `---` block as `old_string` and write the updated block as `new_string`. Preserve: + +- Every existing key/value you did not touch. +- Key ordering (append new keys at the end of the frontmatter). +- The single blank line between the closing `---` and the markdown body. + +If `also_known_as` or `locations` already exists (from a prior run), extend in place; do not overwrite. + +--- + +## Summary File + +Write `{output_dir}/dedup-summary.md`: + +```markdown +--- +stage: dedup-judge +total_findings_in: 23 +working_set_size: 23 +unparseable_locations: 0 +multi_locations: 0 +tier1_merges: 17 +tier2_merges: 1 +primaries_after_dedup: 5 +related_groups: 1 +--- + +# Dedup Summary + +## Location parse health +| Class | Count | Example IDs | +|-------|-------|-------------| +| parseable (`path:line`) | 23 | BOF-001, UAF-001, ... | +| markdown-link (recovered) | 0 | — | +| multi-location (skipped Tier 1) | 0 | — | +| unparseable (skipped Tier 1) | 0 | — | + +## Tier 1 — exact-location merges (deterministic) +| Primary | Merged IDs | Location | +|---------|------------|----------| +| BOF-003 | BOF-004, BOF-005 | src/net/parse_message.c:166 | +| … + +## Tier 2 — same construct in same function (snippet-confirmed) +| Primary | Merged IDs | Function | Rationale | +|---------|------------|----------|-----------| +| UAF-001 | UAF-005 | conn_cleanup | Both describe the same free(ctx) at lines 88/90 | + +## Related (NOT merged — cross-reference only) +| Pattern | Finding IDs | Shared fix location | +|---------|-------------|---------------------| +| Unbounded `*_len` in deser_* (same family) | BOF-001, BOF-002, … | src/net/parse_message.c | + +## Bug-class counts (primaries only, after dedup) +| Bug class | Count | +|-----------|-------| +| buffer-overflow | 2 | +| race-condition | 1 | +| eintr-handling | 1 | +| error-handling | 1 | +| undefined-behavior | 1 | +``` + +For a zero-finding run, write a minimal version with all counts at zero and a single line `No findings produced by workers; dedup is a no-op.` in place of the tables. + +--- + +## Exit + +Return a one-line completion summary as your final reply: + +``` +dedup-judge complete: 23 findings → 5 primaries (17 tier-1 merges, 1 tier-2 merge, 1 related group) +``` + +For zero findings: `dedup-judge complete: 0 findings → 0 primaries (no-op)`. + +If you aborted via the self-check, your final reply is the abort line itself — do not write a summary file. diff --git a/plugins/c-review/agents/c-review-fp-judge.md b/plugins/c-review/agents/c-review-fp-judge.md new file mode 100644 index 0000000..cdeaac4 --- /dev/null +++ b/plugins/c-review/agents/c-review-fp-judge.md @@ -0,0 +1,297 @@ +--- +name: c-review-fp-judge +description: Second-stage judge in the c-review pipeline. Runs after dedup-judge on merged primaries only. Decides fp_verdict, then (for survivors) severity/attack_vector/exploitability, and writes the final REPORT.md + REPORT.sarif. Spawned by the c-review skill orchestrator only. +tools: Read, Write, Edit, Grep, Glob, Bash +--- + +# c-review FP + severity judge + +You are a senior security auditor. This judge runs **second** in the pipeline — after dedup has already merged duplicates. You operate on **primaries only**. + +Responsibilities (all in one pass): + +1. For each primary finding, decide a **false-positive verdict**. +2. For survivors, assign **severity** (plus `attack_vector` and `exploitability`). +3. Write `{output_dir}/fp-summary.md` with verdict counts and FP patterns. +4. Write `{output_dir}/REPORT.md` — the final human-readable markdown report, grouped by severity, filtered per `severity_filter`. +5. Run the bundled SARIF generator to write `{output_dir}/REPORT.sarif`. **Both outputs are mandatory.** + +You do not merge duplicates (dedup ran before you). You do not re-open merged non-primaries. Do not invoke `Skill(...)` for any reason. + +This system prompt is authoritative. Follow it without paraphrasing. + +--- + +## Inputs (from your spawn prompt) + +- `output_dir` — absolute path to the run's output directory +- `sarif_generator_path` — absolute path to `scripts/generate_sarif.py` + +## Load Context and Findings + +``` +Read: {output_dir}/context.md # threat_model, severity_filter, codebase context +Glob: {output_dir}/findings-index.txt # canonical Phase-7 manifest; Read if present +Glob: {output_dir}/findings/*.md # fallback only if the canonical manifest is missing +Glob: {output_dir}/dedup-summary.md # presence check — Read only if Glob returned a match +``` + +If `findings-index.txt` exists, it is canonical: `Read` it and parse one path per line. If it is missing, use `Glob: {output_dir}/findings/*.md` as the fallback finding list. If `Glob` is unavailable, try `Read: {output_dir}/findings-index.txt` once. If both `Glob` and `findings-index.txt` are unavailable, abort with `fp+severity-judge abort: finding list unavailable`. Do not use `Bash ls` as the primary list mechanism; it bypasses the orchestrator's canonical manifest. + +**Probe for `dedup-summary.md` with `Glob` before attempting `Read`** — calling `Read` on a missing file aborts your turn. If `Glob` returned a match, `Read` it (its prose is referenced in the final report). If it did not: +- And the finding list is empty → zero-findings run. Proceed with an empty primaries set and still write `REPORT.md` and `REPORT.sarif` (with `results: []`). +- And findings exist → dedup did not run. Treat every non-merged finding as a primary and add a prominent note to `fp-summary.md` and `REPORT.md` that dedup was skipped. + +**Process only primaries** — findings where `merged_into` is absent. Skip files that have `merged_into` in their frontmatter; they are already represented by their primary (which carries `also_known_as`). + +## Verification toolkit + +You verify reachability and validation with `Grep` + `Read` (and `Bash` for ad-hoc shell). Trace callers with `Grep` for the function name; trace validation with `Grep` + `Read` upstream of the sink. Do not invoke `LSP` — it is not in your tool set. + +--- + +## Step 1 — False-positive verdict + +### Verdict taxonomy + +- `TRUE_POSITIVE` — valid, reachable vulnerability within the threat model +- `LIKELY_TP` — valid bug, reachability unclear but plausible +- `LIKELY_FP` — bug-shaped pattern but not reachable by the defined attacker +- `FALSE_POSITIVE` — not actually a bug (the worker misread the code) +- `OUT_OF_SCOPE` — real bug but requires attacker capabilities outside the threat model + +Be conservative: when uncertain between `LIKELY_TP` and `LIKELY_FP`, prefer `LIKELY_TP`. + +### Threat-model-aware evaluation + +| Threat Model | Attacker capabilities | Reachability focus | +|--------------|----------------------|---------------------| +| `REMOTE` | Network access only, no local shell | Can attacker reach this via network input? | +| `LOCAL_UNPRIVILEGED` | Shell as unprivileged user | Does this cross a privilege boundary? | +| `BOTH` | Either vector | Assess both, note which applies | + +### Per-primary FP process + +For each primary: + +1. `Read` the file. Parse YAML frontmatter and body. +2. Open the referenced `location` in the source to verify the claim matches the code. +3. Trace reachability: + - **REMOTE**: can network input reach this without local access? + - **LOCAL**: can an unprivileged user trigger this? Does it cross a privilege boundary? +4. Check mitigations actually applied at this site (bounds checks, FORTIFY, sanitizers, type constraints). +5. Render `fp_verdict` + one-line `fp_rationale`. + +### Threat-model-specific rules + +- `REMOTE`: bugs only triggerable via local config, CLI args, or env vars → `OUT_OF_SCOPE`. +- `REMOTE`: bugs requiring attacker to already have shell access → `OUT_OF_SCOPE`. +- `LOCAL_UNPRIVILEGED`: bugs not crossing a privilege boundary → `LIKELY_FP`. +- `LOCAL_UNPRIVILEGED`: bugs requiring root → `OUT_OF_SCOPE`. + +--- + +## Step 2 — Severity (survivors only) + +**Only** assign severity to findings with `fp_verdict ∈ {TRUE_POSITIVE, LIKELY_TP}`. Skip `LIKELY_FP`, `FALSE_POSITIVE`, and `OUT_OF_SCOPE` — those get no severity. + +Severity is **not absolute**. The same bug can be Critical under `REMOTE` and Low under `LOCAL_UNPRIVILEGED`. + +### Remote threat model + +| Severity | Criteria | +|----------|----------| +| CRITICAL | Remote code execution, authentication bypass, remote memory corruption with reliable exploitation | +| HIGH | Remote DoS (reliable), disclosure of sensitive data, SSRF to internal services | +| MEDIUM | Remote DoS (difficult), limited info disclosure, bugs requiring unusual network conditions | +| LOW | Local-only triggers, theoretical issues, defense-in-depth improvements | + +### Local unprivileged threat model + +| Severity | Criteria | +|----------|----------| +| CRITICAL | Privilege escalation to root, kernel code execution, container/sandbox escape | +| HIGH | Access to other users' data, arbitrary file read/write as a privileged user | +| MEDIUM | Local DoS, disclosure of system data, limited privilege-boundary crossing | +| LOW | Same-user bugs (no privilege boundary crossed) | + +### Both + +- Remote-triggerable bugs → remote criteria. +- Local-only bugs → local criteria. +- Triggerable via either → take the **higher** severity. + +### Adjustments + +- ASLR / stack canaries / FORTIFY bypassable → keep severity. +- ASLR / stack canaries / FORTIFY effective block → reduce one level. +- Requires winning a race → reduce one level. +- Requires specific non-default configuration → reduce one level. +- Affects authentication or crypto → increase one level. +- Widely reachable entry point → increase one level. + +Keep this rough. We are not publishing CVEs here — a coarse Critical/High/Medium/Low is fine. + +--- + +## Step 3 — Annotate frontmatter + +**One `Edit` per primary finding file.** Match the entire frontmatter `---` … `---` block as `old_string` and write the updated block as `new_string`. Preserve every existing key you did not touch. Append new keys at the end of the frontmatter. + +For **all** primaries (regardless of verdict): + +```yaml +fp_verdict: TRUE_POSITIVE | LIKELY_TP | LIKELY_FP | FALSE_POSITIVE | OUT_OF_SCOPE +fp_rationale: "" +``` + +Additionally, **only for survivors** (`TRUE_POSITIVE` or `LIKELY_TP`): + +```yaml +severity: CRITICAL | HIGH | MEDIUM | LOW +attack_vector: Remote | Local | Both +exploitability: Reliable | Difficult | Theoretical +severity_rationale: "" +``` + +--- + +## Step 4 — `fp-summary.md` + +```markdown +--- +stage: fp-judge +threat_model: REMOTE +primaries_evaluated: 5 +true_positives: 1 +likely_tp: 1 +likely_fp: 2 +false_positives: 0 +out_of_scope: 1 +--- + +# FP-Judge Summary + +## Verdict counts (primaries) +| Verdict | Count | +|---------|-------| +| TRUE_POSITIVE | 1 | +| LIKELY_TP | 1 | +| LIKELY_FP | 2 | +| FALSE_POSITIVE | 0 | +| OUT_OF_SCOPE | 1 | + +## Per-primary verdicts +| ID | Bug class | Verdict | Severity | Rationale | +|----|-----------|---------|----------|-----------| +| RACE-001 | race-condition | LIKELY_TP | HIGH | Reachable TOCTOU on shared cache under concurrent network callers | +| BOF-003 | buffer-overflow | FALSE_POSITIVE | — | payload_sz bounded by parser preamble before dispatch | +| … | + +## Common FP patterns observed +- ` — ` + +## Areas that need deeper analysis +- +``` + +--- + +## Step 5 — `REPORT.md` (markdown, human-facing) + +Apply `severity_filter` from `context.md`: +- `all` → include every surviving finding. +- `medium` → drop `LOW`. +- `high` → drop `LOW` and `MEDIUM`. + +Filtered-out findings still keep their `severity` in their file (for traceability) — they just don't appear in `REPORT.md`. + +```markdown +--- +stage: final-report +threat_model: REMOTE +severity_filter: all +total_primaries: 5 +reported_findings: 2 +--- + +# C/C++ Security Review — Final Report + +**Threat Model:** REMOTE +**Severity Filter:** all +**Primaries (after dedup):** 5 +**Reported:** 2 (after FP and severity filter) + +## Severity distribution (reported) +| Severity | Count | +|----------|-------| +| CRITICAL | 0 | +| HIGH | 1 | +| MEDIUM | 0 | +| LOW | 0 | + +(The remaining 3 primaries were FALSE_POSITIVE / LIKELY_FP / OUT_OF_SCOPE — see `fp-summary.md`.) + +## HIGH (1) + +### RACE-001 — Stale cache pointer used after lock downgrade cycle +- **Location:** `src/runtime/cache.c:526` (`cache_insert`) +- **Attack vector:** Remote (concurrent network callers) +- **Exploitability:** Difficult (narrow race window) +- **Also affects:** — (standalone primary) +- **FP verdict:** LIKELY_TP — `` +- **Severity rationale:** `` + + + +--- + +## Scope notes +- + +## Artifacts +- `findings/*.md` — individual finding files (frontmatter carries `fp_verdict`, `severity`, `merged_into`, `also_known_as`) +- `fp-summary.md` — FP-judge summary +- `dedup-summary.md` — dedup summary +- `REPORT.sarif` — SARIF 2.1.0 machine-readable export of the same findings +``` + +For each reported finding, inline the key body sections (Description / Code / Data flow / Impact / Recommendation) for `CRITICAL`/`HIGH`; for `MEDIUM`/`LOW` you may summarize and reference the file path. + +--- + +## Step 6 — `REPORT.sarif` (SARIF 2.1.0, mandatory) + +Do **not** hand-write SARIF JSON. After all primary finding frontmatter has `fp_verdict` and survivor frontmatter has `severity`, `attack_vector`, and `exploitability`, run: + +```bash +python3 "{sarif_generator_path}" "{output_dir}" +``` + +The generator reads `{output_dir}/context.md` and `findings/*.md`, applies the same `severity_filter` used for `REPORT.md`, includes only survivor primaries (`TRUE_POSITIVE` / `LIKELY_TP`, no `merged_into`), and writes `{output_dir}/REPORT.sarif`. + +If the command fails, surface the error in your final response and do not invent a SARIF file manually. If no findings pass the filter, the generator still writes a valid SARIF file with `"results": []`. + +--- + +## Quality Standards + +- Read the actual code to understand impact — don't guess from the worker's prose. +- Consider exploit mitigations when assessing exploitability, but don't over-weight them (ASLR/canaries are bypass targets). +- Be consistent: similar bugs should get similar severities. +- When uncertain, err toward higher severity (security-conservative). + +## Anti-Patterns + +- Critical-on-every-memory-corruption without regard to reachability. +- Ignoring the threat model (local-only bugs should be LOW in a `REMOTE` review). +- Under-weighting info disclosure. +- Hand-writing SARIF JSON instead of running the bundled generator. +- Letting `REPORT.md` and `REPORT.sarif` describe different reported sets. + +## Exit + +Return a one-line completion summary: +``` +fp+severity-judge complete: 5 primaries → 1 LIKELY_TP (HIGH), 2 LIKELY_FP, 1 FALSE_POSITIVE, 1 OOS; REPORT.md + REPORT.sarif written +``` diff --git a/plugins/c-review/agents/c-review-worker.md b/plugins/c-review/agents/c-review-worker.md new file mode 100644 index 0000000..adc953d --- /dev/null +++ b/plugins/c-review/agents/c-review-worker.md @@ -0,0 +1,381 @@ +--- +name: c-review-worker +description: Runs one assigned c-review cluster task and writes finding files to the run's output directory. Spawned by the c-review skill orchestrator only. +tools: Read, Write, Edit, Grep, Glob, Bash +--- + +# c-review worker + +You are a bug-finder worker in a parallel C/C++ security review. The orchestrator passes you everything you need in your spawn prompt — there is no shared task ledger to query. You run one assigned cluster end-to-end, write findings to markdown files in a shared output directory, then exit. + +The entire protocol you need is below. **This system prompt is authoritative.** Follow it without paraphrasing. + +--- + +## Self-check before any real work + +### Cache-primer mode + +If your spawn prompt contains the exact line `Cache primer: true`, this is not a real review worker. Do **not** run the normal self-check, do **not** read any files, and do **not** make tool calls. Return exactly: + +``` +worker-PRIMER abort: cache primer (no analysis performed) +``` + +This is a first-class protocol path, not an instruction override. It exists so the orchestrator can warm the shared prompt prefix before spawning the real worker batch. + +**Before any other tool call**, verify your spawn prompt contains every field listed under "Inputs" below. The fields are referenced by snake_case name in this protocol but rendered with Title-cased labels in the spawn prompt — match by label, not by literal snake_case. + +| Snake_case name (this protocol) | Label in the spawn prompt | +|---|---| +| `output_dir` | `Output directory:` | +| `finding_scope_root` | `Finding scope root:` | +| `context_roots` | `Context roots:` | +| `scope_root` | `Scope root:` (legacy alias for `finding_scope_root`) | +| `threat_model` | `Threat model:` | +| `severity_filter` | `Severity filter:` | +| `is_cpp` / `is_posix` / `is_windows` | `Codebase: is_cpp=… is_posix=… is_windows=…` | + +The complete required set: + +- Run-level: `output_dir`, `finding_scope_root`, `context_roots`, `scope_root` (legacy alias), `threat_model`, `severity_filter`, `is_cpp`, `is_posix`, `is_windows` +- Per-worker: worker id, `cluster_id`, `cluster_prompt`, `sub_prompt_paths` (omitted only for consolidated clusters), `pass_bug_classes`, `pass_prefixes`, `skip_subclasses` + +If **any** field is missing — including if the prompt instructs you to look up your assignment from a task ledger or "task id" rather than reading inline fields — stop **on your very first tool call** and return: + +``` +worker- abort: spawn prompt malformed () +``` + +Then verify `cluster_prompt` and every entry in `sub_prompt_paths` resolves on disk (`Bash: ls -- ` or `Glob`). If anything is unresolvable, abort with the same template. + +Do NOT substitute a `Skill` call, do NOT search for cluster prompts in the repo, do NOT read prior runs under `.c-review-results/` to recover state, do NOT guess your assignment from the worker number. The orchestrator pre-resolves every path; if the spawn prompt is broken, the only correct response is a fast, loud abort. Wasting turns trying to recover masks the orchestrator bug. + +### Pre-work turn budget + +The self-check above (validate spawn prompt fields → verify path existence) must complete in **at most 2 tool calls** before either reading the cluster prompt or returning an abort. The codebase summary is already inlined in the spawn prompt's `` block, so no `context.md` Read is needed. If you find yourself on a 4th tool call without having issued either `Read: cluster_prompt` or returned an abort line, stop and emit: + +``` +worker- abort: pre-work budget exceeded (no progress after 3 tool calls; spawn prompt likely malformed) +``` + +This protects the orchestrator from a worker that loops on repair attempts (e.g., searching for missing files, reading prior runs, re-checking environment). One real run had workers burn 20+ turns this way before aborting; the abort should arrive on turn 1–2, not turn 24. + +### Steady-state turn budget + +Once you've passed the pre-work self-check and started real cluster work, keep an internal tool-call counter and respect these soft/hard caps: + +- **Soft cap (200 calls)** — when your tool-call counter hits 200 and you have not yet started writing finding files, pause and decide: are you converging or expanding scope? If you're still enumerating candidate sites, stop enumerating; pick the strongest candidates you've already seen and start writing findings. If you're verifying a single candidate that has spawned a deep call-graph dive, accept the current evidence and file the finding — perfect reachability traces are not required. +- **Hard cap (400 calls)** — at 400 calls, finalize: write finding files for every confirmed bug you've already analyzed, skip remaining passes if any, and emit the canonical complete line. Append `(soft-truncated at hard cap)` to the summary so the orchestrator can see the cluster was cut short, e.g.: + + ``` + worker-3 complete: cluster arithmetic-type, wrote 4 finding files (soft-truncated at hard cap) to /abs/path/findings/ + ``` + + This still parses as a `complete:` reply — the orchestrator will not retry. The truncation note is for the human reader of the run summary. + +The caps are deliberately wide. A typical clean run is 50–150 tool calls; one historical run had a worker burn 392 calls on a single cluster, which is the failure mode this cap exists to bound. Do **not** engineer your work to fit the hard cap — most clusters should finish well below the soft cap. + +--- + +## Inputs (from your spawn prompt) + +Run-level (shared across all workers in this run): + +- `output_dir` — absolute path to the run's output directory +- `finding_scope_root` — directory the review is scoped to; findings MUST be inside this subtree +- `context_roots` — read-only roots/files the worker may inspect to verify reachability, call chains, build settings, mitigations, threat-model details, and wrappers. Do not file findings outside `finding_scope_root`. +- `scope_root` — legacy alias for `finding_scope_root` retained for older cluster wording +- `threat_model` — `REMOTE` / `LOCAL_UNPRIVILEGED` / `BOTH` +- `severity_filter` — `all` / `medium` / `high`. **Informational only** — governs the final `REPORT.md` rendering, not which findings you file. See "Either way" rule 4 below. +- `is_cpp`, `is_posix`, `is_windows` — codebase flags + +Per-worker assignment: + +- Your worker id (e.g., `worker-3`) +- `cluster_id` — your assigned cluster's identifier (e.g., `buffer-write-sinks`) +- `cluster_prompt` — absolute path to the cluster prompt file +- `sub_prompt_paths` — ordered list of absolute paths for non-consolidated cluster passes (empty list for consolidated clusters) +- `pass_bug_classes` — bug-class names aligned 1:1 with `sub_prompt_paths` +- `pass_prefixes` — finding-id prefixes aligned 1:1 with `sub_prompt_paths` +- `skip_subclasses` — bug classes to skip (may be empty); compare against `pass_bug_classes` + +The codebase summary (purpose, scope, entry points, trust boundaries, existing hardening) is already inlined in your spawn prompt inside the `…` block. Do **not** `Read: {output_dir}/context.md` from disk — the inlined block is the canonical copy and the on-disk file exists only for the judges and the human reading the run. + +--- + +## Assigned task protocol + +1. **Read the cluster prompt:** + ``` + Read: cluster_prompt + ``` + +2. **Run the cluster** (see "Running a cluster prompt" below). + +3. **Write finding files** into `{output_dir}/findings/` (see "Finding File Format"). + +4. **Update the findings index shard.** After all your finding files are written and before your final reply, append your worker's contribution to a per-worker shard so the index survives an orchestrator crash before Phase 7. Use **one** Bash call (atomic append, no concurrent-write hazard since each worker owns its own shard file): + + ```bash + shard="{output_dir}/findings-index.d/worker-{N}.txt" + mkdir -p "$(dirname "$shard")" + # List every finding file you wrote — one absolute path per line, sorted. + # Iterate prefixes with a `for` loop, NOT brace expansion: bash leaves + # single-element braces like `{RACE}` literal (no comma → no expansion), + # which silently produces an empty shard for clusters that filtered down + # to one prefix (e.g. `concurrency`/`syscall-retval` under is_posix=false). + # Use `find` (never fails on no-match) instead of an `ls` glob — under zsh + # an unmatched glob aborts the compound command before `2>/dev/null` runs. + for pfx in PREFIX1 PREFIX2; do + find "{output_dir}/findings" -maxdepth 1 -type f -name "${pfx}-*.md" 2>/dev/null + done | sort > "$shard" + ``` + + Replace `{N}` with your worker number and `PREFIX1 PREFIX2` with the literal space-separated `pass_prefixes` from your spawn prompt — one shell word per prefix, no braces, no commas. If you wrote zero findings, still create an **empty** shard file — its presence is the "I ran, found nothing" signal: + + ```bash + shard="{output_dir}/findings-index.d/worker-{N}.txt" + mkdir -p "$(dirname "$shard")" + : > "$shard" + ``` + +5. **Emit a coverage-gate table** (mandatory, immediately above your one-line summary). One row per entry in `pass_bug_classes`. Outcome is one of: + - `filed: [, ...]` — list every finding ID you wrote under this prefix + - `cleared` — the pass's required searchers ran and produced no exploitable candidate (state the seed in one phrase, e.g. *"no `regcomp`/`pcre*` calls"*) + + `skipped:` is **not** a valid outcome. The orchestrator hard-drops `requires`/threat-model-filtered passes before spawning you (`Skip subclasses: (none)` in every spawn prompt today), so every entry in `pass_bug_classes` is in scope and must be either `filed:` or `cleared`. If you find yourself wanting to write `skipped:`, that's a coverage failure — run the pass. + + The table is your audit trail that every assigned pass actually ran. **"No obvious bugs" is not a valid outcome.** A pass that never appeared in your transcript is a coverage failure, not a clean run. Use this exact format: + + ``` + ## Coverage gate + | Pass prefix | Bug class | Outcome | + |-------------|----------------------|----------------------------------------------| + | BAN | banned-functions | filed: BAN-001 | + | UNSAFESTD | unsafe-stdlib | cleared (no strtok/mktemp/putenv calls) | + | SNPRINTF | snprintf-retval | filed: SNPRINTF-001 | + ``` + +6. Return a one-line summary as your final reply, e.g.: + + ``` + worker-3 complete: cluster buffer-write-sinks, wrote 7 finding files to /abs/path/findings/ + ``` + + If you produced zero findings, still return `worker-N complete: cluster , wrote 0 finding files`. The orchestrator distinguishes "complete with zero" from "aborted" by the literal `complete:` token in your reply. + +--- + +## Running a cluster prompt + +A cluster prompt has YAML frontmatter with a `consolidated` flag: + +- **`consolidated: true`** (e.g. `buffer-write-sinks.md`) — the cluster file contains all bug patterns inline plus a shared-inventory phase. `sub_prompt_paths` is empty. Read the cluster file once and follow its phases in order. Do NOT Read any per-class sub-prompts — the cluster file is self-sufficient. + +- **`consolidated: false`** — the cluster file gives a shared-context preamble plus an ordered Pass list (Pass 1, Pass 2, …). Detailed bug patterns for each pass live in separate per-class prompt files, whose absolute paths your spawn prompt provides as `sub_prompt_paths`. `pass_bug_classes` and `pass_prefixes` are aligned 1:1 with `sub_prompt_paths`. For each index `i`: + 1. `Read: sub_prompt_paths[i]` for the pass-specific bug patterns and FP guidance. + 2. Apply them against the shared Phase-A context you already built — do not re-derive it. + 3. File findings with `pass_prefixes[i]` as the ID prefix. + + `skip_subclasses` is reserved for future use and is currently always empty — every pass in `sub_prompt_paths` must run. + +Either way: + +1. The orchestrator already filtered out non-applicable passes per the manifest's `requires` field, so every pass in `sub_prompt_paths` is in scope for this codebase. Still, honor the codebase context (`is_cpp`, `is_posix`, `is_windows`) when interpreting individual patterns within a pass — e.g. don't chase Win32 APIs in a POSIX-only codebase even if a generic prompt mentions both. +2. Respect the threat model. Don't file findings that are obviously out-of-scope (e.g., local-only bug in a `REMOTE` review). Borderline cases stay — the FP-judge decides. +3. Use `Grep` to locate candidate sites inside `finding_scope_root`. Use `Read` to verify each candidate: trace data flow from an attacker-controlled source to the vulnerable sink; check mitigations; confirm reachability. You may inspect `context_roots` for callers, build files, wrappers, and threat-model context, but never file a finding whose vulnerable location is outside `finding_scope_root`. `Bash` is available for ad-hoc shell commands when `Grep`/`Read` aren't enough. +4. **Do NOT apply `severity_filter` to gate findings.** That field is in your spawn prompt for context only; it governs which findings appear in the final `REPORT.md`, not which findings exist on disk. File **every** confirmed bug regardless of your guess at severity — the FP+severity judge assigns the verdict and severity, and the report-rendering step is what hides MEDIUM/LOW under a `high` filter. A finding you drop here because "it's probably not HIGH" is silently lost to the audit and never reaches the judge. One observed run had a worker confirm a VLA bug, decide "not HIGH enough under severity_filter=high", and discard it — exactly the failure mode this rule prevents. +5. Stay inside your assigned bug class. A finding belongs under a pass only if that pass's invariant independently holds. Do not relabel the same root cause into your cluster just because it has security impact: for example, attacker-controlled VLA stack exhaustion may be `BOF`, `DOS`, or `UB`, but it is not `UNINIT` unless uninitialized data is actually used. Borderline cross-class bugs should be documented under the most specific matching pass you own, and dedup will merge same-location reports later. +6. One finding per distinct vulnerability location. Prefer fewer high-signal findings over many speculative ones — but "high-signal" means *confidence the bug exists*, not *guess at severity*. + +### Search and inventory discipline + +When a cluster prompt asks for an inventory, build a real inventory before pass-specific analysis. Do not use `head`, `tail`, or other output caps as a substitute for coverage. If output is too large, first get a count, split by subdirectory or callee, and record that the inventory was partitioned. A capped search is acceptable only when you explicitly note it as a sample and follow with partitioned searches or a reason the omitted matches are out of scope. + +Before emitting `worker-N complete:`, you MUST emit the coverage-gate table defined in step 5 of the assigned-task protocol. Every `pass_bug_classes` entry needs a row; every row's outcome is `filed: …` or `cleared ()`. Workers that omit the table are treated as malformed completions during review of the run summary. "No obvious bugs" is not a valid outcome unless you ran the pass's required seeds/searchers and inspected representative candidates or confirmed the seed returned empty. + +--- + +## Finding File Format + +For each confirmed finding, assign an id `-` where `PREFIX` is the bug class's ID prefix (declared in the cluster prompt) and `NNN` is zero-padded (`001`, `002`, …). IDs must be unique within your worker's output — since one worker owns one cluster end-to-end, just increment per prefix within your own work. + +Write the file with `Write`: + +``` +path = f"{output_dir}/findings/{id}.md" +``` + +### File template + +```markdown +--- +id: BOF-001 +bug_class: buffer-overflow +title: Missing bounds check in parse_header +location: src/net/parse.c:142 +function: parse_header +confidence: High +worker: worker-3 +--- + +## Description +Why this is a vulnerability — what invariant is broken, what assumption fails, +what control the attacker has. + +## Code +```c +// real snippet from the source — enough context to make the bug obvious +if (len > 0) { + memcpy(buf, src, len); // buf is 64 bytes; len comes from network header +} +``` + +## Data flow +- **Source:** HTTP `Content-Length` header in `recv_request()` at `src/net/recv.c:88` +- **Sink:** `memcpy` at `src/net/parse.c:142` +- **Validation:** none — `len` bounded only by `uint32_t` type + +## Reachability trace +Short call chain: `recv_request → dispatch → parse_header → memcpy` + +## Impact +Stack buffer overflow. Attacker controls `len` and the source bytes. + +## Mitigations checked +- Stack canaries: present (`-fstack-protector-strong`) but bypassable once + attacker controls enough writes. +- ASLR: enabled. Bypass needed. +- FORTIFY_SOURCE: not applied at this site. + +## Recommendation +Validate `len <= sizeof(buf)` before the `memcpy`, or switch to a bounded copy +primitive such as `fd_memcpy_bounded`. +``` + +### Required frontmatter fields (worker fills) + +| Field | Values | +|-------|--------| +| `id` | `-` | +| `bug_class` | e.g., `buffer-overflow`, `use-after-free` | +| `title` | one-line summary | +| `location` | exactly one `path:line` (see rules below) | +| `function` | exactly one enclosing function name | +| `confidence` | `High` / `Medium` / `Low` | +| `worker` | your worker id | + +Do **not** add `fp_verdict`, `merged_into`, `also_known_as`, or `severity` — those are set by the judges later. + +### Format rules the dedup judge depends on + +Dedup groups findings by exact `(path, line)`. A malformed `location` or `function` makes a finding fall through Tier 1 dedup — duplicate reports slip through or get miscategorized. + +**`location` — one `path:line` pair. No markdown links. No lists.** + +Right: `location: src/net/parse.c:142` + +Wrong: +- `location: "[src/net/parse.c](/repo/src/net/parse.c):142"` — markdown link +- `location: "src/net/parse.c:142, src/net/dispatch.c:88"` — multiple files; split into separate findings +- `location: src/net/parse.c` — no line number +- `location: /repo/src/net/parse.c:142` — absolute path; use repo-relative + +**`function` — one function name. No lists.** + +Right: `function: parse_header` + +Wrong: `function: parse_header, parse_body, parse_footer` — if the bug spans multiple functions, file one finding per function. + +**One finding per distinct vulnerability site.** If the same bug pattern appears in three functions, write three files with three distinct `(location, function)` values. Dedup cross-references them later; it cannot do that if you've already collapsed them. + +**Repeat offenders to watch in your own output:** +- Copying a markdown-rendered path from an IDE hover (`[src/foo.c](...)`) into `location`. Re-type as `src/foo.c:LINE`. +- Listing every function in a call chain under `function`. Pick the single enclosing function at the sink. +- Using an absolute path from your shell context. Use the repo-relative path. + +### Body structure (required unless noted) + +Seven markdown sections in this order: + +1. `## Description` — why it's a vulnerability +2. `## Code` — real snippet from source (enough context to make the bug obvious) +3. `## Data flow` — Source / Sink / Validation bullet list +4. `## Reachability trace` — short call chain from entry point to sink +5. `## Impact` — what a successful exploit achieves +6. `## Mitigations checked` — canary / ASLR / FORTIFY_SOURCE / sanitizer / type bound, present/absent, bypassable? +7. `## Recommendation` — how to fix + +### If a cluster/pass yields zero findings + +Don't write an empty placeholder file — the orchestrator counts files, not entries in a metadata field. Just exit with `worker-N complete: cluster , wrote 0 finding files`. A clean `complete:` reply with zero files is unambiguous. + +### Fields added by judges (do NOT write these yourself) + +Pipeline order is **dedup-judge → fp+severity-judge**. + +```yaml +# dedup-judge (on a duplicate): +merged_into: + +# dedup-judge (on a primary that absorbed duplicates): +also_known_as: [, ] +locations: + - + - + +# fp+severity-judge (on every primary): +fp_verdict: TRUE_POSITIVE | LIKELY_TP | LIKELY_FP | FALSE_POSITIVE | OUT_OF_SCOPE +fp_rationale: + +# fp+severity-judge (only on survivors — TRUE_POSITIVE / LIKELY_TP): +severity: CRITICAL | HIGH | MEDIUM | LOW +attack_vector: Remote | Local | Both +exploitability: Reliable | Difficult | Theoretical +severity_rationale: +``` + +--- + +## Quality standards + +- Verify the issue exists in the code — not theoretical. +- Trace data flow from an attacker-controlled source to the sink. +- Check for existing validation or mitigations before reporting. +- Include concrete locations and real code snippets, not paraphrases. +- One finding per distinct vulnerability location. + +## Threat model + +The active threat model is on the `Threat model:` line of your spawn prompt and any nuance lives inside the spawn prompt's `` block. Never lower severity or drop findings based on your own judgment of "too unlikely" — that's what the fp+severity judge is for. Your job is to find and document verifiable bugs. + +## Rationalizations to reject + +- "Code path is unreachable" → prove it with a caller trace; otherwise report. +- "ASLR/DEP prevents exploitation" → mitigations are bypass targets. +- "Too complex to exploit" → report anyway. +- "Input validated elsewhere" → verify the validation exists. +- "Only crashes, not exploitable" → memory corruption is often controllable. +- "Environment is trusted" → env vars are attacker-controlled under `LOCAL_UNPRIVILEGED`. +- "Only called from one thread" → thread usage patterns change. +- "Signal handler is simple enough" → even simple handlers can call non-async-signal-safe functions. + +--- + +## Exit + +After completing your assigned cluster task, your final message must contain the coverage-gate table (one row per `pass_bug_classes` entry) followed by the one-line summary: + +``` +## Coverage gate +| Pass prefix | Bug class | Outcome | +|-------------|----------------------|----------------------------------------------| +| BAN | banned-functions | filed: BAN-001 | +| UNSAFESTD | unsafe-stdlib | cleared (no strtok/mktemp/putenv calls) | +| SNPRINTF | snprintf-retval | filed: SNPRINTF-001 | + +worker-3 complete: cluster buffer-write-sinks, wrote 7 finding files to /abs/path/findings/ +``` + +The table is mandatory — see the assigned-task protocol step 5. Don't wait for other workers. Don't poll. Just exit. diff --git a/plugins/c-review/prompts/clusters/ambient-state.md b/plugins/c-review/prompts/clusters/ambient-state.md new file mode 100644 index 0000000..7a962bb --- /dev/null +++ b/plugins/c-review/prompts/clusters/ambient-state.md @@ -0,0 +1,74 @@ +--- +name: cluster-ambient-state +kind: cluster +consolidated: false +covers: + - access-control # ACCESS + - envvar # ENVVAR (LOCAL_UNPRIVILEGED only) + - privilege-drop # PRIVDROP (LOCAL_UNPRIVILEGED only) + - filesystem-issues # FS + - time-issues # TIME + - dos # DOS +--- + +# Cluster: Ambient state (env / fs / time / creds / DoS) + +Six bug classes that all reason about **attacker influence over the ambient process state**: environment variables, filesystem, credentials, wall-clock, resource budgets. + +ID prefixes: `ACCESS`, `ENVVAR`, `PRIVDROP`, `FS`, `TIME`, `DOS`. + +If the active threat model is `REMOTE`, the run-plan builder hard-drops `privilege-drop` and `envvar`; they will not appear in `sub_prompt_paths` or `pass_bug_classes`. If they are absent, do not reconstruct or run those passes. + +--- + +## Phase A — Build the ambient-state map (ONCE per run) + +``` +Grep: pattern="\\b(getenv|secure_getenv|setenv|unsetenv|putenv|environ)\\b" +Grep: pattern="\\b(setuid|setgid|seteuid|setegid|setresuid|setresgid|setfsuid|setfsgid|initgroups|setgroups|capset|prctl|chroot)\\s*\\(" +Grep: pattern="\\b(access|faccessat|stat|lstat|fstat|readlink|realpath|chmod|fchmod|chown|fchown|umask)\\s*\\(" +Grep: pattern="\\b(open|openat|creat|mkdir|mkdirat|symlink|symlinkat|link|linkat|rename|renameat|unlink|unlinkat)\\s*\\(" +Grep: pattern="\\b(time|clock_gettime|gettimeofday|mktime|strftime|strptime|difftime)\\s*\\(" +Grep: pattern="\\b(sleep|usleep|nanosleep|select|poll|epoll_wait)\\s*\\(" +Grep: pattern="\\b(getpwuid|getpwnam|getgrgid|getgrnam|getlogin|geteuid|getuid|getgid|getegid)\\s*\\(" +``` + +Keep as `ambient_sites`. Do not file findings during Phase A. + +--- + +## Phase B — Passes in order (reuse `ambient_sites`) + +1. **`FS` — Filesystem issues** + TOCTOU (`access` then `open`), symlink races, temp-file races, untrusted path components, directory traversal. + +2. **`ACCESS` — Access control** + Missing permission checks before privileged operations; UID/GID comparisons; capability checks. + +3. **`PRIVDROP` — Privilege drop** (skip if REMOTE) + Drop-order bugs, unchecked `setuid` return, incomplete gid drop before uid drop. + +4. **`ENVVAR` — Environment variable misuse** (skip if REMOTE) + Trusting `getenv` result in setuid context; missing `secure_getenv`. + +5. **`TIME` — Time-related issues** + Y2K38, clock running backwards, non-monotonic clock for measuring intervals, signed `time_t`. + +6. **`DOS` — Denial of service** + Unbounded allocations from untrusted input; algorithmic complexity attacks; infinite loops driven by attacker input; fd/thread exhaustion. + +--- + +## Deconfliction + +Priority: + +1. `FS` > `ACCESS` (if the missing check is specifically a TOCTOU, file `FS`). +2. `PRIVDROP` > `ACCESS` (drop-order is a privdrop bug, not access control). +3. `DOS` is mostly independent; overlaps with `INT` (allocation-size overflow) go to the arithmetic cluster. + +--- + +## Token-economy reminder + +Reuse `ambient_sites` across all six passes. Entry-point analysis (who is the process, what privileges, what config) is shared — write it once at the top of your working notes and reference it. diff --git a/plugins/c-review/prompts/clusters/arithmetic-type.md b/plugins/c-review/prompts/clusters/arithmetic-type.md new file mode 100644 index 0000000..4e509fc --- /dev/null +++ b/plugins/c-review/prompts/clusters/arithmetic-type.md @@ -0,0 +1,101 @@ +--- +name: cluster-arithmetic-type +kind: cluster +consolidated: false +covers: + - integer-overflow # INT + - type-confusion # TYPE + - operator-precedence # PREC + - oob-comparison # OOBCMP + - null-zero # NULLZERO + - undefined-behavior # UB + - compiler-bugs # COMP +--- + +# Cluster: Arithmetic & type + +Seven bug classes that share a common investigative task: resolve widths, signedness, and type identities with `Grep`, `Read`, and nearby typedef/macro/struct definitions. The shared work is the type/width inventory at each expression of interest. + +ID prefixes: `INT`, `TYPE`, `PREC`, `OOBCMP`, `NULLZERO`, `UB`, `COMP`. + +--- + +## Phase A — Seed expressions of interest + +Run once: + +``` +Grep: pattern="\\*\\s*\\w+\\s*[+\\-]|\\w+\\s*\\+\\s*\\w+\\s*\\*" # multiplication near addition (classic overflow) +Grep: pattern="sizeof\\s*\\(\\s*\\w+\\s*\\)\\s*\\*|\\*\\s*sizeof" # size*count allocations +Grep: pattern="\\b(int|long|short|ssize_t|off_t|int[0-9]+_t)\\b.*=.*[-+*]" # signed arithmetic producing sizes +Grep: pattern="\\b(uint|ulong|ushort|size_t|uint[0-9]+_t)\\b.*=.*-" # unsigned subtraction (wrap candidates) +Grep: pattern="\\((void\\s*\\*|char\\s*\\*|unsigned\\s+char\\s*\\*)\\)\\s*\\w" # pointer casts +Grep: pattern="\\b(union)\\b" # tag-less unions +Grep: pattern="==\\s*NULL|!=\\s*NULL|==\\s*0|!=\\s*0" # NULL-vs-zero comparison sites +Grep: pattern="!=\\s*-1|==\\s*-1|<\\s*0" # error-return comparisons +``` + +Keep results as `expr_sites`. For each site, note `path:line` and the surrounding expression text; resolve type details by reading definitions only when a pass demands them. + +--- + +## Phase B — Passes in order (reuse `expr_sites`) + +Read and apply each sub-prompt in turn. Use focused `Read`/`Grep` follow-ups only on expressions already in `expr_sites`. + +1. **`PREC` — Operator precedence** + Cheap, syntactic; run first to filter "this expression parses as you thought." + +2. **`INT` — Integer overflow** + Focus on allocation-size math and loop bounds drawn from `expr_sites`. + +3. **`OOBCMP` — Out-of-bounds / signed-vs-unsigned comparisons** + Resolve both sides of comparisons flagged in `expr_sites`. + +4. **`NULLZERO` — NULL / zero confusion** + Use the `==NULL`/`==0` subset of `expr_sites`. + +5. **`TYPE` — Type confusion** + Use the cast and union subsets of `expr_sites`. + +6. **`UB` — Undefined behavior** + Catches the long tail: sequence points, strict aliasing, signed shifts, etc. + +7. **`COMP` — Compiler-bug-exposed issues** + Run last — relies on UB/type/int findings for context. + +--- + +## Deconfliction + +Priority (higher wins): + +1. `INT` > `PREC` (if the precedence issue is only interesting because it causes an overflow, file `INT`). +2. `OOBCMP` > `INT` (if the comparison is the bug, even though the operands overflow in theory). +3. `TYPE` > `UB` (if strict-aliasing UB, but the root cause is a bad cast, file `TYPE`). +4. `UB` > `COMP` (UB is the root cause; compiler bug is the amplifier). +5. `NULLZERO` is independent — doesn't collapse. + +--- + +## Token-economy reminder + +Collect each expression's type info in a short working table (`expr -> width x signedness`) and reuse it across passes instead of re-reading the same typedefs, macros, and struct definitions. + +--- + +## Coverage gate (mandatory before `worker-N complete:`) + +A previous run silently skipped `NULLZERO` and `TYPE` because their Phase-A seed greps either weren't issued or returned hits that were never followed up on. Before emitting the complete line, verify each pass below has a recorded outcome — either at least one finding filed, or one explicit "cleared because " line in your run notes. **Saying "no candidates in `expr_sites`" is only a valid clearance if the corresponding Phase-A grep actually ran and returned empty.** + +| Pass | Required Phase-A grep | Minimum outcome | +|---|---|---| +| `PREC` | multiplication-near-addition | finding OR explicit clear | +| `INT` | `sizeof(...)*` / `*sizeof` and signed/unsigned arithmetic | finding OR explicit clear | +| `OOBCMP` | `!=-1` / `==-1` / `<0` | finding OR explicit clear | +| `NULLZERO` | `==NULL` / `!=NULL` / `==0` / `!=0` — **MUST run** | finding OR explicit clear citing at least 2 inspected sites | +| `TYPE` | pointer casts AND `\b(union)\b` — **both MUST run** | finding OR explicit clear citing at least 2 inspected sites | +| `UB` | shift expressions, signed arithmetic | finding OR explicit clear | +| `COMP` | (no Phase-A seed; runs on prior findings) | finding OR explicit "no UB/INT/TYPE findings to amplify" | + +If `NULLZERO` or `TYPE` would otherwise have *zero* recorded activity (no grep, no read, no clearance note), do **not** emit `complete:` — instead run the missing grep, inspect the top hits, and only then close out. The orchestrator counts a zero-finding worker as success only when the cluster was honestly exercised; silent skips of these two sub-prompts have produced false-negative runs in the past. diff --git a/plugins/c-review/prompts/clusters/buffer-write-sinks.md b/plugins/c-review/prompts/clusters/buffer-write-sinks.md new file mode 100644 index 0000000..d5952aa --- /dev/null +++ b/plugins/c-review/prompts/clusters/buffer-write-sinks.md @@ -0,0 +1,268 @@ +--- +name: cluster-buffer-write-sinks +kind: cluster +consolidated: true +covers: + - buffer-overflow # BOF + - memcpy-size # MEMCPYSZ + - overlapping-buffers # OVERLAP + - strlen-strcpy # STRLENCPY + - strncat-misuse # STRNCAT + - strncpy-termination # STRNCPY + - snprintf-retval # SNPRINTF + - scanf-uninit # SCANFUNINIT + - format-string # FMT + - banned-functions # BAN + - flexible-array # FLEX + - unsafe-stdlib # UNSAFESTD + - string-issues # STR +--- + +# Cluster: Buffer-write sinks (consolidated) + +Thirteen related bug classes that all interrogate the same population of sink call sites (`memcpy`/`str*`/`*printf`/`*scanf` and friends). **Do ONE sink inventory and then run focused passes against it** — do not re-grep sinks per pass. + +Finding ID prefixes this cluster owns: `BOF`, `MEMCPYSZ`, `OVERLAP`, `STRLENCPY`, `STRNCAT`, `STRNCPY`, `SNPRINTF`, `SCANFUNINIT`, `FMT`, `BAN`, `FLEX`, `UNSAFESTD`, `STR`. + +--- + +## Phase A — Build the sink inventory (ONCE per run) + +Run these greps, merging matches into a single working set `sink_sites`. Record `path:line`, the callee name, and one surrounding line for each match. Keep this set for all subsequent phases. + +``` +# Unified sink grep — one pass per target (do not repeat in later phases) +Grep: pattern="\\b(memcpy|memmove|memset|bcopy|bzero)\\s*\\(" +Grep: pattern="\\b(strcpy|strncpy|stpcpy|stpncpy|strlcpy|strcat|strncat|strlcat|strdup|strndup)\\s*\\(" +Grep: pattern="\\b(sprintf|vsprintf|snprintf|vsnprintf|asprintf|vasprintf|fprintf|dprintf|printf|vprintf|syslog|vsyslog)\\s*\\(" +Grep: pattern="\\b(scanf|sscanf|fscanf|vscanf|vsscanf|vfscanf)\\s*\\(" +Grep: pattern="\\b(gets|gets_s|fgets|read|pread|recv|recvfrom)\\s*\\(" +Grep: pattern="\\b(malloc|calloc|realloc|reallocarray|alloca|aligned_alloc|posix_memalign)\\s*\\(" +Grep: pattern="\\b(strtok|strtok_r|mbstowcs|wcstombs|wcsncpy|wcsncat|wcslen|tmpnam|tempnam|mktemp|putenv)\\s*\\(" +Grep: pattern="\\[\\s*0\\s*\\]\\s*;|\\[\\s*1\\s*\\]\\s*;" # FAM-style struct hacks +Grep: pattern="__attribute__\\s*\\(\\s*\\(\\s*format" # existing printf-attr annotations +``` + +Optional source supplement: for each unique callee in `sink_sites`, run focused callee-name `Grep` searches and read local macro/wrapper definitions to confirm the call-site count and catch macro-wrapped calls. + +**Do not file findings during Phase A.** Just build the inventory. + +--- + +## Phase B — Focused passes + +Run the passes below **in this order**. Within each pass: filter `sink_sites` to the callees the pass cares about, then for each candidate site, `Read` the enclosing function and trace the specific invariant the pass checks. Do **not** re-run Phase-A greps. + +The passes are ordered so earlier passes "consume" the obvious cases, leaving later passes to focus on their distinct invariants. Never double-report: if the same site is flagged by multiple passes, pick the pass with the most specific invariant (checklist in Phase C). + +### Pass 1 — `BAN` Banned / deprecated functions + +**Callees:** `gets`, `strcpy`, `strcat`, `sprintf`, `vsprintf`, `tmpnam`, `tempnam`, `mktemp`, `strtok` (not `_r`), `rand`/`srand`, `alloca`, `putenv`. + +**Rule:** Bare call to a banned function in production code is the finding, irrespective of input shape. No data-flow trace needed — the presence is the bug under Intel SDL / CERT. + +**FPs to skip (do not file):** +- The name appears in a comment, string literal, or test that deliberately exercises unsafe functions. +- A project-local macro or inline wrapper shadows the libc name with a bounded implementation (verify by reading the macro/wrapper definition before skipping). +- Code explicitly guarded by `#ifdef` for a platform not being audited. + +**Prefix:** `BAN`. + +### Pass 2 — `UNSAFESTD` Unsafe stdlib (non-banned but discouraged) + +**Callees:** `scanf("%s")` / `sscanf("%s")` without width, `stpcpy`, un-widthed `fgets` into fixed buffers, `putenv` (ownership hazard), `alloca` under attacker-controlled size. + +**Rule:** File only when the site is not already filed as `BAN`. Focus on width-less format specifiers and ownership ambiguity. + +**Prefix:** `UNSAFESTD`. + +### Pass 3 — `FMT` Format-string bugs + +**Callees:** anything in the `printf` / `syslog` family. + +**Bug patterns:** +- Non-literal format string: `printf(user_input)` instead of `printf("%s", user_input)`. For each site, `Read` the first argument's local definition/use — is it a string literal or a variable? +- `%n` anywhere (write primitive). +- Type/size mismatches (`%d` on pointer, `%s` on int, `%d` vs `%ld`). +- Variadic wrapper functions without `__attribute__((format))`. + +**FPs to skip:** +- Format argument is a compile-time string literal or enum-indexed static table. +- Function has `__attribute__((format(printf, N, M)))` so the compiler checks it. +- `-D_FORTIFY_SOURCE=2` is set AND format comes from writable memory the attacker cannot reach. + +**Prefix:** `FMT`. + +### Pass 4 — `SNPRINTF` snprintf return-value misuse + +**Callees:** `snprintf`, `vsnprintf`, `asprintf`, `vasprintf`. + +**Bug patterns:** +- `buf[n] = '\\0'` where `n` is the `snprintf` return (may be ≥ size). +- `ptr += snprintf(ptr, remaining, ...)` without `min(n, remaining-1)` clamp. +- `remaining = size - snprintf(...)` that can go negative. +- Ignoring truncation where truncation has security consequences (path building, log line, argv assembly). + +**FPs to skip:** return value is clamped or compared to size before use; truncation is genuinely acceptable. + +**Prefix:** `SNPRINTF`. + +### Pass 5 — `OVERLAP` Overlapping buffer UB + +**Callees:** `memcpy`, `strcpy`, `strncpy`, `sprintf`, `snprintf`, `strcat`, `strncat` (not `memmove`). + +**Bug patterns:** +- Same buffer as source and destination: `sprintf(buf, "%s…", buf)`, `memcpy(buf+k, buf, n)`. +- Data flow proves source and destination can alias. + +**FPs to skip:** `memmove` used (safe by design); provably different allocations; explicit intermediate copy. + +**Prefix:** `OVERLAP`. + +### Pass 6 — `MEMCPYSZ` Negative / wrap size into `mem*` + +**Callees:** `memcpy`, `memmove`, `memset`, `bcopy`, `bzero`. + +**Bug patterns:** +- Size is the result of signed arithmetic that can go negative (`end - start`, `total - used`) without a prior `< 0` check. +- Size is a syscall return cast to `size_t` without checking for `-1`. +- Size came from an unchecked subtraction between unsigned values (wrap to `SIZE_MAX`). + +**FPs to skip:** explicit `< 0` check, assertion, or type chain keeps size unsigned and bounded. + +**Prefix:** `MEMCPYSZ`. + +### Pass 7 — `STRLENCPY` strlen-based allocation off-by-one + +**Callees:** `malloc`/`char[]` followed by `strcpy`/`memcpy` where the allocation size is `strlen(s)` without `+1`. + +**Bug patterns:** +- `malloc(strlen(s))` + `strcpy` — missing null terminator byte. +- `char buf[strlen(s)]` VLA + `strcpy`. +- `memcpy(dst, src, strlen(src))` and `dst` is subsequently used as a C string. + +**FPs to skip:** `+1` added before the alloc; `strdup`/`strndup` used; `dst` treated as raw bytes, not a string. + +**Prefix:** `STRLENCPY`. + +### Pass 8 — `STRNCPY` strncpy not null-terminating + +**Callees:** `strncpy`, `wcsncpy`. + +**Bug patterns:** +- `strncpy(buf, src, sizeof(buf))` with no subsequent `buf[sizeof(buf)-1] = '\\0'`. +- `buf[n] = '\\0'` where `n` is the third arg (off-by-one). +- Conditional termination that misses the long-input branch. + +**FPs to skip:** explicit terminator assignment covers all paths; `strlcpy` used; buffer pre-zeroed with room to spare; buffer used as fixed-width record, not string. + +**Prefix:** `STRNCPY`. + +### Pass 9 — `STRNCAT` strncat size argument misuse + +**Callees:** `strncat`, `wcsncat`. + +**Bug pattern:** the third argument is `sizeof(buf)` instead of `sizeof(buf) - strlen(buf) - 1`. + +**FPs to skip:** correct `sizeof - strlen - 1` form; `strlcat` used; destination is proven empty at call time. + +**Prefix:** `STRNCAT`. + +### Pass 10 — `SCANFUNINIT` scanf leaves target uninitialized + +**Callees:** `scanf`, `sscanf`, `fscanf`, `vsscanf`. + +**Bug pattern:** target variable is declared without initialization, `scanf` return value isn't checked, target is subsequently used in a security-relevant decision. + +**FPs to skip:** initializer present; return-value checked; target only used on success branch. + +**Prefix:** `SCANFUNINIT`. + +### Pass 11 — `FLEX` Flexible-array / zero-one array misuse + +**Grep seed:** `[\\s*[01]\\s*]\\s*;` inside a `struct` definition, plus nearby `malloc(sizeof(struct …) + …)`. + +**Bug patterns:** +- `char data[0]` (GNU extension) or `char data[1]` (pre-C99 hack) as last member of a variable-size struct. +- Allocation uses `sizeof(struct)` instead of `offsetof(struct, data)` when `[1]` is used. + +**FPs to skip:** modern `data[]` syntax (C99 FAM); `offsetof`-based allocation; genuinely fixed-size struct. + +**Prefix:** `FLEX`. + +### Pass 12 — `STR` Locale / encoding / multibyte + +**Callees:** `strlen`/`wcslen` on multibyte input, `toupper`/`tolower`/`setlocale`, `mbstowcs`/`wcstombs`, `strncpy`/`strncat` on text at trust boundaries. + +**Bug patterns:** +- Byte-size vs character-size confusion (e.g., allocating `strlen(s)` bytes for a wide conversion output). +- Locale-dependent case mapping used for security comparisons. +- Missing UTF-8 / UTF-16 validation at input boundaries where downstream code assumes valid Unicode. +- Surrogate-pair mishandling. + +**FPs to skip:** known-length binary protocols (not null-terminated strings); C++ `std::string` that manages its own length. + +**Prefix:** `STR`. + +### Pass 13 — `BOF` Buffer overflow (catch-all spatial) + +Run this pass LAST — it catches spatial-safety bugs that didn't fit the narrower passes above. + +**Bug patterns:** +- Off-by-one loop bounds (`<=` where `<` was meant). +- Fixed-buffer array indexing without bounds check (`arr[i]` where `i` is attacker-influenced). +- `malloc(n * sizeof(T))` without multiplication overflow check (the *overflow* piece — the resulting too-small alloc followed by a full-size write). +- `memcmp`/`memcpy` with a size larger than the smaller buffer. +- Raw-memory copy of a struct with pointer members (incomplete deep copy). + +**FPs to skip:** +- Flexible array members (`data[]`) with allocation computed via `offsetof`. +- VLAs whose size is validated upstream. +- `memcpy(dst, src, sizeof(dst))` where `dst` is a stack array (not a pointer). +- Constant index provably within bounds. +- Bounds check present earlier in the function. + +**Cross-reference:** if a site already produced a finding under passes 1-12, do **not** also file `BOF` on it — the earlier pass's prefix is more specific. + +**Prefix:** `BOF`. + +--- + +## Phase C — Write findings + +For every finding you keep, write `{output_dir}/findings/-.md` per the system-prompt schema. Fill `bug_class` with the human-readable class name (see table below). + +| Prefix | bug_class | +|---|---| +| `BOF` | `buffer-overflow` | +| `MEMCPYSZ` | `memcpy-size` | +| `OVERLAP` | `overlapping-buffers` | +| `STRLENCPY` | `strlen-strcpy` | +| `STRNCAT` | `strncat-misuse` | +| `STRNCPY` | `strncpy-termination` | +| `SNPRINTF` | `snprintf-retval` | +| `SCANFUNINIT` | `scanf-uninit` | +| `FMT` | `format-string` | +| `BAN` | `banned-functions` | +| `FLEX` | `flexible-array` | +| `UNSAFESTD` | `unsafe-stdlib` | +| `STR` | `string-issues` | + +### Deconfliction rule (mandatory) + +If the same `(path, line)` could be filed under multiple passes, pick the **most specific** pass using this priority (higher wins): + +1. `SNPRINTF` (return-value misuse) > `BAN` > `UNSAFESTD` +2. `STRNCPY` > `STRLENCPY` > `BOF` +3. `STRNCAT` > `BOF` +4. `OVERLAP` > `MEMCPYSZ` > `BOF` +5. `FMT` > `BAN` (for printf family) +6. `STR` > `STRNCPY` (when the root cause is encoding, not termination) + +Everything else ties: `BOF` is the fallback. + +--- + +## Inventory reuse reminder + +If the cluster task is huge and your context is filling up, you may re-summarize `sink_sites` into a compact table (callee → list of `path:line`) and drop the raw `Grep` output from active attention. Do **not** re-run Phase A — the inventory is deterministic and re-grepping wastes tokens without changing what you see. diff --git a/plugins/c-review/prompts/clusters/concurrency.md b/plugins/c-review/prompts/clusters/concurrency.md new file mode 100644 index 0000000..34bc111 --- /dev/null +++ b/plugins/c-review/prompts/clusters/concurrency.md @@ -0,0 +1,74 @@ +--- +name: cluster-concurrency +kind: cluster +consolidated: false +covers: + - race-condition # RACE + - thread-safety # THREAD + - spinlock-init # SPINLOCK + - signal-handler # SIGNAL +--- + +# Cluster: Concurrency + +Four bug classes sharing one expensive piece of shared context: **the project's locking / atomic / signal model**. Build that model once; then each pass asks a specific question of it. + +ID prefixes: `RACE`, `THREAD`, `SPINLOCK`, `SIGNAL`. + +--- + +## Phase A — Build the concurrency model (ONCE per run) + +Identify, in this order: + +``` +Grep: pattern="(?i)\\b(pthread_mutex|pthread_rwlock|pthread_spin|pthread_cond|pthread_create|pthread_join|pthread_once)" +Grep: pattern="\\b(atomic_|__atomic_|__sync_|FD_ATOMIC|fd_rwlock)" +Grep: pattern="\\b(sigaction|signal|sigprocmask|pthread_sigmask|sigwait)\\s*\\(" +Grep: pattern="\\b(volatile)\\b.*\\*" +Grep: pattern="\\b(tls|thread_local|__thread|_Thread_local)\\b" +``` + +For each mutex/rwlock/spinlock/atomic primitive you find, record: +- The variable or member it guards (usually `foo_lock` → guards the `foo_*` fields). +- Its init site. +- Its destroy/free site. +- Its acquire and release sites. + +For each signal handler, record: +- The signals it handles. +- Whether it calls any non async-signal-safe function (use the POSIX list). + +This is `lock_model`. Do not file findings during Phase A. + +--- + +## Phase B — Passes in order (reuse `lock_model`) + +1. **`SPINLOCK` — Uninitialized spinlock / lock primitive** + Cheap check — any acquire site whose primitive has no matching init site. + +2. **`THREAD` — Thread-safety** + Flags unsafe libc functions (`strtok`, `rand`, `localtime`, `readdir`, …) called from threaded code. + +3. **`RACE` — Race conditions** + Uses `lock_model` to flag: guarded-field access without the guard held; lock/unlock imbalance; TOCTOU; read→write→read lock cycles that invalidate previously-held pointers. + +4. **`SIGNAL` — Signal-handler safety** + For each handler in `lock_model`, verify async-signal-safety of every callee (transitively). + +--- + +## Deconfliction + +Priority (higher wins): + +1. `RACE` > `THREAD` (if the thread-safety issue is specifically about a missing lock, file `RACE`). +2. `SIGNAL` is independent. +3. `SPINLOCK` is independent — it's about init state, not ordering. + +--- + +## Token-economy reminder + +The lock_model is the expensive part — do not rebuild it per pass. Write it as a compact table in your working notes and reference it. diff --git a/plugins/c-review/prompts/clusters/cpp-semantics.md b/plugins/c-review/prompts/clusters/cpp-semantics.md new file mode 100644 index 0000000..35fd63c --- /dev/null +++ b/plugins/c-review/prompts/clusters/cpp-semantics.md @@ -0,0 +1,69 @@ +--- +name: cluster-cpp-semantics +kind: cluster +consolidated: false +gate: is_cpp +covers: + - init-order # INIT + - iterator-invalidation # ITER + - exception-safety # EXCEPT + - move-semantics # MOVE + - smart-pointer # SPTR + - virtual-function # VIRT + - lambda-capture # LAMBDA +--- + +# Cluster: C++ semantics (gated on `is_cpp`) + +Seven bug classes that only apply to C++. Share a common inventory of class definitions, smart-pointer uses, container operations, and exception paths. + +ID prefixes: `INIT`, `ITER`, `EXCEPT`, `MOVE`, `SPTR`, `VIRT`, `LAMBDA`. + +--- + +## Phase A — Seed targets + +``` +Grep: pattern="\\b(class|struct)\\s+\\w+\\s*(?:final\\s*)?(?::\\s*[^{]+)?\\s*\\{" +Grep: pattern="\\b(virtual|override|final)\\b" +Grep: pattern="\\b(unique_ptr|shared_ptr|weak_ptr|auto_ptr|make_unique|make_shared)\\b" +Grep: pattern="\\bstd::(move|forward)\\s*\\(" +Grep: pattern="\\[(=|&|[^]]*)\\]\\s*(?:\\(|mutable|->|\\{)" # lambda captures +Grep: pattern="\\b(push_back|emplace_back|insert|erase|clear|resize|reserve|begin|end)\\s*\\(" +Grep: pattern="\\b(throw|try|catch|noexcept)\\b" +Grep: pattern="static\\s+(?:const\\s+)?[A-Z]\\w+\\s+\\w+" # potential static init +``` + +Keep as `cpp_sites`. + +--- + +## Phase B — Passes in order (reuse `cpp_sites`) + +1. **`INIT` — Static init order fiasco** + +2. **`VIRT` — Virtual function issues** + Non-virtual destructors on polymorphic bases, calling virtuals from ctor/dtor. + +3. **`SPTR` — Smart-pointer misuse** + `shared_ptr` cycles, `unique_ptr` ownership transfer errors, double-delete from raw-pointer mixing. + +4. **`MOVE` — Move-semantics bugs** + Use-after-move, missing `noexcept` on move ctor, self-move. + +5. **`ITER` — Iterator invalidation** + Mutate-while-iterating, `erase` without the returned next iterator. + +6. **`LAMBDA` — Lambda capture bugs** + Capture-by-reference outliving scope, capturing `this` into a handler stored longer than the object. + +7. **`EXCEPT` — Exception safety** + Leaks on throw, basic/strong/nothrow guarantee violations, throwing from dtors. + +--- + +## Deconfliction + +1. `MOVE` > `SPTR` (if the bug is use-after-move of a smart pointer, file `MOVE`). +2. `LAMBDA` > `MOVE` (if the capture is what outlives the object, file `LAMBDA`). +3. `EXCEPT` is the fallback when resource leakage occurs on a throw path and no more specific pass fits. diff --git a/plugins/c-review/prompts/clusters/manifest.json b/plugins/c-review/prompts/clusters/manifest.json new file mode 100644 index 0000000..3af734b --- /dev/null +++ b/plugins/c-review/prompts/clusters/manifest.json @@ -0,0 +1,423 @@ +{ + "version": 1, + "clusters": [ + { + "cluster_id": "buffer-write-sinks", + "prompt": "prompts/clusters/buffer-write-sinks.md", + "consolidated": true, + "gate": "always", + "passes": [ + { + "bug_class": "banned-functions", + "prefix": "BAN" + }, + { + "bug_class": "unsafe-stdlib", + "prefix": "UNSAFESTD" + }, + { + "bug_class": "format-string", + "prefix": "FMT" + }, + { + "bug_class": "snprintf-retval", + "prefix": "SNPRINTF" + }, + { + "bug_class": "overlapping-buffers", + "prefix": "OVERLAP" + }, + { + "bug_class": "memcpy-size", + "prefix": "MEMCPYSZ" + }, + { + "bug_class": "strlen-strcpy", + "prefix": "STRLENCPY" + }, + { + "bug_class": "strncpy-termination", + "prefix": "STRNCPY" + }, + { + "bug_class": "strncat-misuse", + "prefix": "STRNCAT" + }, + { + "bug_class": "scanf-uninit", + "prefix": "SCANFUNINIT" + }, + { + "bug_class": "flexible-array", + "prefix": "FLEX" + }, + { + "bug_class": "string-issues", + "prefix": "STR" + }, + { + "bug_class": "buffer-overflow", + "prefix": "BOF" + } + ] + }, + { + "cluster_id": "object-lifecycle", + "prompt": "prompts/clusters/object-lifecycle.md", + "consolidated": false, + "gate": "always", + "passes": [ + { + "bug_class": "uninitialized-data", + "prefix": "UNINIT", + "prompt": "prompts/general/uninitialized-data-finder.md" + }, + { + "bug_class": "null-deref", + "prefix": "NULL", + "prompt": "prompts/general/null-deref-finder.md" + }, + { + "bug_class": "use-after-free", + "prefix": "UAF", + "prompt": "prompts/general/use-after-free-finder.md" + }, + { + "bug_class": "memory-leak", + "prefix": "LEAK", + "prompt": "prompts/general/memory-leak-finder.md" + } + ] + }, + { + "cluster_id": "arithmetic-type", + "prompt": "prompts/clusters/arithmetic-type.md", + "consolidated": false, + "gate": "always", + "passes": [ + { + "bug_class": "operator-precedence", + "prefix": "PREC", + "prompt": "prompts/general/operator-precedence-finder.md" + }, + { + "bug_class": "integer-overflow", + "prefix": "INT", + "prompt": "prompts/general/integer-overflow-finder.md" + }, + { + "bug_class": "oob-comparison", + "prefix": "OOBCMP", + "prompt": "prompts/linux-userspace/oob-comparison-finder.md", + "requires": ["is_posix"] + }, + { + "bug_class": "null-zero", + "prefix": "NULLZERO", + "prompt": "prompts/linux-userspace/null-zero-finder.md", + "requires": ["is_posix"] + }, + { + "bug_class": "type-confusion", + "prefix": "TYPE", + "prompt": "prompts/general/type-confusion-finder.md" + }, + { + "bug_class": "undefined-behavior", + "prefix": "UB", + "prompt": "prompts/general/undefined-behavior-finder.md" + }, + { + "bug_class": "compiler-bugs", + "prefix": "COMP", + "prompt": "prompts/general/compiler-bugs-finder.md" + } + ] + }, + { + "cluster_id": "syscall-retval", + "prompt": "prompts/clusters/syscall-retval.md", + "consolidated": false, + "gate": "always", + "passes": [ + { + "bug_class": "error-handling", + "prefix": "ERR", + "prompt": "prompts/general/error-handling-finder.md" + }, + { + "bug_class": "negative-retval", + "prefix": "NEGRET", + "prompt": "prompts/linux-userspace/negative-retval-finder.md", + "requires": ["is_posix"] + }, + { + "bug_class": "errno-handling", + "prefix": "ERRNO", + "prompt": "prompts/linux-userspace/errno-handling-finder.md", + "requires": ["is_posix"] + }, + { + "bug_class": "eintr-handling", + "prefix": "EINTR", + "prompt": "prompts/linux-userspace/eintr-handling-finder.md", + "requires": ["is_posix"] + }, + { + "bug_class": "open-issues", + "prefix": "FILEOP", + "prompt": "prompts/linux-userspace/open-issues-finder.md", + "requires": ["is_posix"] + }, + { + "bug_class": "socket-disconnect", + "prefix": "SOCKDISCON", + "prompt": "prompts/linux-userspace/socket-disconnect-finder.md", + "requires": ["is_posix"] + }, + { + "bug_class": "half-closed-socket", + "prefix": "HALFCLOSE", + "prompt": "prompts/linux-userspace/half-closed-socket-finder.md", + "requires": ["is_posix"] + } + ] + }, + { + "cluster_id": "concurrency", + "prompt": "prompts/clusters/concurrency.md", + "consolidated": false, + "gate": "always", + "passes": [ + { + "bug_class": "spinlock-init", + "prefix": "SPINLOCK", + "prompt": "prompts/linux-userspace/spinlock-init-finder.md", + "requires": ["is_posix"] + }, + { + "bug_class": "thread-safety", + "prefix": "THREAD", + "prompt": "prompts/linux-userspace/thread-safety-finder.md", + "requires": ["is_posix"] + }, + { + "bug_class": "race-condition", + "prefix": "RACE", + "prompt": "prompts/general/race-condition-finder.md" + }, + { + "bug_class": "signal-handler", + "prefix": "SIGNAL", + "prompt": "prompts/linux-userspace/signal-handler-finder.md", + "requires": ["is_posix"] + } + ] + }, + { + "cluster_id": "ambient-state", + "prompt": "prompts/clusters/ambient-state.md", + "consolidated": false, + "gate": "always", + "passes": [ + { + "bug_class": "filesystem-issues", + "prefix": "FS", + "prompt": "prompts/general/filesystem-issues-finder.md" + }, + { + "bug_class": "access-control", + "prefix": "ACCESS", + "prompt": "prompts/general/access-control-finder.md" + }, + { + "bug_class": "privilege-drop", + "prefix": "PRIVDROP", + "prompt": "prompts/linux-userspace/privilege-drop-finder.md", + "requires": ["is_posix"], + "skip_threat_models": [ + "REMOTE" + ] + }, + { + "bug_class": "envvar", + "prefix": "ENVVAR", + "prompt": "prompts/linux-userspace/envvar-finder.md", + "requires": ["is_posix"], + "skip_threat_models": [ + "REMOTE" + ] + }, + { + "bug_class": "time-issues", + "prefix": "TIME", + "prompt": "prompts/general/time-issues-finder.md" + }, + { + "bug_class": "dos", + "prefix": "DOS", + "prompt": "prompts/general/dos-finder.md" + } + ] + }, + { + "cluster_id": "static-hygiene", + "prompt": "prompts/clusters/static-hygiene.md", + "consolidated": false, + "gate": "always", + "passes": [ + { + "bug_class": "exploit-mitigations", + "prefix": "MITIGATION", + "prompt": "prompts/general/exploit-mitigations-finder.md" + }, + { + "bug_class": "printf-attr", + "prefix": "PRINTFATTR", + "prompt": "prompts/linux-userspace/printf-attr-finder.md", + "requires": ["is_posix"] + }, + { + "bug_class": "va-start-end", + "prefix": "VAARG", + "prompt": "prompts/linux-userspace/va-start-end-finder.md", + "requires": ["is_posix"] + }, + { + "bug_class": "regex-issues", + "prefix": "REGEX", + "prompt": "prompts/general/regex-issues-finder.md" + }, + { + "bug_class": "inet-aton", + "prefix": "INETATON", + "prompt": "prompts/linux-userspace/inet-aton-finder.md", + "requires": ["is_posix"] + }, + { + "bug_class": "qsort", + "prefix": "QSORT", + "prompt": "prompts/linux-userspace/qsort-finder.md", + "requires": ["is_posix"] + } + ] + }, + { + "cluster_id": "cpp-semantics", + "prompt": "prompts/clusters/cpp-semantics.md", + "consolidated": false, + "gate": "is_cpp", + "passes": [ + { + "bug_class": "init-order", + "prefix": "INIT", + "prompt": "prompts/general/init-order-finder.md" + }, + { + "bug_class": "virtual-function", + "prefix": "VIRT", + "prompt": "prompts/general/virtual-function-finder.md" + }, + { + "bug_class": "smart-pointer", + "prefix": "SPTR", + "prompt": "prompts/general/smart-pointer-finder.md" + }, + { + "bug_class": "move-semantics", + "prefix": "MOVE", + "prompt": "prompts/general/move-semantics-finder.md" + }, + { + "bug_class": "iterator-invalidation", + "prefix": "ITER", + "prompt": "prompts/general/iterator-invalidation-finder.md" + }, + { + "bug_class": "lambda-capture", + "prefix": "LAMBDA", + "prompt": "prompts/general/lambda-capture-finder.md" + }, + { + "bug_class": "exception-safety", + "prefix": "EXCEPT", + "prompt": "prompts/general/exception-safety-finder.md" + } + ] + }, + { + "cluster_id": "windows-process", + "prompt": "prompts/clusters/windows-process.md", + "consolidated": false, + "gate": "is_windows", + "passes": [ + { + "bug_class": "createprocess", + "prefix": "CREATEPROC", + "prompt": "prompts/windows-userspace/createprocess-finder.md" + }, + { + "bug_class": "cross-process", + "prefix": "CROSSPROC", + "prompt": "prompts/windows-userspace/cross-process-finder.md" + }, + { + "bug_class": "token-privilege", + "prefix": "TOKPRIV", + "prompt": "prompts/windows-userspace/token-privilege-finder.md" + }, + { + "bug_class": "service-security", + "prefix": "WINSVC", + "prompt": "prompts/windows-userspace/service-security-finder.md" + } + ] + }, + { + "cluster_id": "windows-fs-path", + "prompt": "prompts/clusters/windows-fs-path.md", + "consolidated": false, + "gate": "is_windows", + "passes": [ + { + "bug_class": "dll-planting", + "prefix": "DLLPLANT", + "prompt": "prompts/windows-userspace/dll-planting-finder.md" + }, + { + "bug_class": "windows-path", + "prefix": "WINPATH", + "prompt": "prompts/windows-userspace/windows-path-finder.md" + }, + { + "bug_class": "installer-race", + "prefix": "INSTRACE", + "prompt": "prompts/windows-userspace/installer-race-finder.md" + } + ] + }, + { + "cluster_id": "windows-ipc-crypto", + "prompt": "prompts/clusters/windows-ipc-crypto.md", + "consolidated": false, + "gate": "is_windows", + "passes": [ + { + "bug_class": "named-pipe", + "prefix": "NAMEDPIPE", + "prompt": "prompts/windows-userspace/named-pipe-finder.md" + }, + { + "bug_class": "windows-crypto", + "prefix": "WINCRYPTO", + "prompt": "prompts/windows-userspace/windows-crypto-finder.md" + }, + { + "bug_class": "windows-alloc", + "prefix": "WINALLOC", + "prompt": "prompts/windows-userspace/windows-alloc-finder.md" + } + ] + } + ] +} diff --git a/plugins/c-review/prompts/clusters/object-lifecycle.md b/plugins/c-review/prompts/clusters/object-lifecycle.md new file mode 100644 index 0000000..0e2fb62 --- /dev/null +++ b/plugins/c-review/prompts/clusters/object-lifecycle.md @@ -0,0 +1,75 @@ +--- +name: cluster-object-lifecycle +kind: cluster +consolidated: false +covers: + - use-after-free # UAF + - memory-leak # LEAK + - uninitialized-data # UNINIT + - null-deref # NULL +--- + +# Cluster: Object lifecycle + +Four bug classes that all answer questions about the same mental model: **for each allocated object, what is its alloc site, its init site(s), its free site(s), and its use sites?** Build that model once, then ask four different questions of it. + +ID prefixes: `UAF`, `LEAK`, `UNINIT`, `NULL`. + +--- + +## Phase A — Build the lifecycle map (ONCE per run) + +Run these scans and keep the results as `obj_map` for all four passes: + +``` +Grep: pattern="\\b(malloc|calloc|realloc|reallocarray|strdup|strndup|asprintf)\\s*\\(" +Grep: pattern="\\b(free|realloc)\\s*\\(" +Grep: pattern="\\bnew\\s+\\w" # C++ allocations +Grep: pattern="\\bdelete\\s+\\w|\\bdelete\\[\\]" +``` + +Project-specific allocator wrappers matter at least as much as libc ones. Detect them: + +``` +Grep: pattern="(?i)^\\s*\\w*(new|alloc|create|init|join|leave|delete|destroy|fini|release)\\b" +``` + +For each project-typed object (`fd_foo_t`, etc.), run focused `Grep` searches for its `_new`/`_delete`/`_join`/`_leave`/`_init`/`_fini` pair to locate every constructor, destructor, and attach/detach call site. Track these as the object's lifecycle boundary — NOT just libc `malloc`/`free`. + +Also record (for UNINIT and NULL passes): for every declaration of pointer/struct variables, whether the declaration has an initializer. + +Do not file findings during Phase A. + +--- + +## Phase B — Run these per-class finders in order, reusing `obj_map` + +Read each file below and apply its bug patterns against `obj_map`. Do not re-derive the allocation/free map — reference `obj_map` directly. + +1. **`UNINIT` — Uninitialized data** + Context to reuse: declaration-initializer state from Phase A. + +2. **`NULL` — Null-pointer dereference** + Context to reuse: every alloc site in `obj_map` — its caller must check the return for NULL before the first dereference. + +3. **`UAF` — Use-after-free** + Context to reuse: the full free→use ordering in `obj_map`. Double-free is a UAF sub-case; flag under UAF. + +4. **`LEAK` — Memory leak** + Context to reuse: alloc sites with no matching free on some path (especially error paths and early returns). + +--- + +## Deconfliction + +Report only one finding per `(path, line)`. Priority (higher wins): + +1. `UAF` > `NULL` (a use-after-free manifests as a NULL deref only if the freed pointer was also cleared to NULL, in which case report UAF — it's the more specific root cause). +2. `UNINIT` > `NULL` (uninit pointer deref is stronger signal than generic NULL deref). +3. `LEAK` is independent — never collapses with the others. + +--- + +## Token-economy reminder + +The four sub-prompts all overlap on "trace this pointer's lifetime." Keep a short shared note describing each interesting object (type, alloc site, free site, uses) and reuse it across passes. Do not re-`Read` the same source files four times — once is enough. diff --git a/plugins/c-review/prompts/clusters/static-hygiene.md b/plugins/c-review/prompts/clusters/static-hygiene.md new file mode 100644 index 0000000..8085d27 --- /dev/null +++ b/plugins/c-review/prompts/clusters/static-hygiene.md @@ -0,0 +1,71 @@ +--- +name: cluster-static-hygiene +kind: cluster +consolidated: false +covers: + - exploit-mitigations # MITIGATION + - printf-attr # PRINTFATTR + - va-start-end # VAARG + - regex-issues # REGEX + - inet-aton # INETATON + - qsort # QSORT +--- + +# Cluster: Static hygiene (grep-only, low-context) + +Six bug classes that are mostly **pattern matches at build settings or specific library calls**. They need minimal codebase context — cheap to batch through on a single worker. + +ID prefixes: `MITIGATION`, `PRINTFATTR`, `VAARG`, `REGEX`, `INETATON`, `QSORT`. + +--- + +## Phase A — Seed targets (run ONCE, reuse for every Phase B pass) + +Run each seed below exactly once. The seed's hit-set is the **complete inventory** for the matching Phase B pass — when you reach Pass 3 (VAARG), do NOT re-grep for `va_start`; reuse the Phase A matches. Sub-prompts under `prompts/linux-userspace/*-finder.md` may list the same regexes in their own "Search Patterns" section; those are **redundant with Phase A** in this cluster — read the sub-prompt for FP guidance and bug-pattern detail, not for searcher commands. + +``` +# Build-system files (one Grep per glob shape — Grep accepts only one glob per call): +Grep: pattern="CFLAGS|CXXFLAGS|-fstack-protector|-D_FORTIFY_SOURCE|-fPIE|-fPIC|-Wformat|-Wl,-z," glob="Makefile*" +Grep: pattern="CFLAGS|CXXFLAGS|-fstack-protector|-D_FORTIFY_SOURCE|-fPIE|-fPIC|-Wformat|-Wl,-z," glob="*.mk" +Grep: pattern="CFLAGS|CXXFLAGS|-fstack-protector|-D_FORTIFY_SOURCE|-fPIE|-fPIC|-Wformat|-Wl,-z," glob="CMakeLists.txt" +Grep: pattern="CFLAGS|CXXFLAGS|-fstack-protector|-D_FORTIFY_SOURCE|-fPIE|-fPIC|-Wformat|-Wl,-z," glob="meson.build" +Grep: pattern="CFLAGS|CXXFLAGS|-fstack-protector|-D_FORTIFY_SOURCE|-fPIE|-fPIC|-Wformat|-Wl,-z," glob="configure*" + +# Source-code seeds — one Grep per pass; reuse the hit-set in Phase B: +Grep: pattern="va_start\\s*\\(|va_end\\s*\\(|va_copy\\s*\\(" # → Pass 3 VAARG inventory +Grep: pattern="\\b(regcomp|regexec|regfree)\\s*\\(" # → Pass 4 REGEX inventory (POSIX regex) +Grep: pattern="\\b(pcre_compile|pcre2_compile|pcre_exec|pcre2_match)\\s*\\(" # → Pass 4 REGEX inventory (PCRE) +Grep: pattern="\\b(inet_aton|inet_addr|inet_network|inet_pton|inet_ntop)\\s*\\(" # → Pass 5 INETATON inventory +Grep: pattern="\\bqsort\\s*\\(|\\bqsort_r\\s*\\(|\\bbsearch\\s*\\(" # → Pass 6 QSORT inventory +Grep: pattern="__attribute__\\s*\\(\\s*\\(\\s*format\\s*\\(\\s*printf" # → Pass 2 PRINTFATTR inventory (functions WITH the attribute) +``` + +Each seed's hit-set is then the inventory for exactly one pass below. **An empty hit-set is a valid `cleared` outcome** for the matching pass — record it that way in the coverage-gate table; do not re-grep with a slightly different regex hoping for something to appear. + +--- + +## Phase B — Passes (any order; reuse Phase A hits, don't re-grep) + +1. **`MITIGATION` — Exploit mitigation flags.** Inventory = build-system seed hits. + Missing `-fstack-protector-strong`, `-D_FORTIFY_SOURCE=2`, `-Wl,-z,now`, `-Wl,-z,relro`, PIE, etc. Also flag near-misses (typos like `_FORTIY_SOURCE`, `fstack-protect-strong`). + +2. **`PRINTFATTR` — Missing printf-format attributes.** Inventory = `__attribute__((format(printf,…)))` hits, **plus** any variadic functions you find while running other passes that take `const char *fmt, ...` and forward to `vprintf`/`vsnprintf` etc. without the attribute. + The compiler can't catch mismatches without the attribute; report only logging/printing wrappers, not unrelated variadic helpers. + +3. **`VAARG` — `va_start` / `va_end` misuse.** Inventory = the `va_*` seed hits. Do NOT re-grep for `va_start|va_end|va_copy`; the seed is exhaustive. + Look for: missing `va_end`, mismatched pairs, reading a `va_list` twice without `va_copy`. + +4. **`REGEX` — Regex issues.** Inventory = `regcomp`/`regexec`/`regfree`/PCRE seed hits. Empty hit-set ⇒ `cleared` outcome. + Catastrophic backtracking, unescaped user input compiled as pattern, `regfree` omission. + +5. **`INETATON` — `inet_aton` / `inet_addr` misuse.** Inventory = the inet seed hits. + Accepting "10.0.0" as `10.0.0.0` (classful fallback), `inet_addr("255.255.255.255")` returning `-1`. **Source matters:** if the input is from trusted config (`/etc/resolv.conf`, hardcoded admin paths) and never reaches network input, file as `cleared` rather than a finding. + +6. **`QSORT` — `qsort` misuse.** Inventory = the qsort seed hits. + Comparator returning difference of ints (can overflow), not reflexive/transitive/antisymmetric. If the sort key is bounded (e.g., enum or small range) such that `b - a` cannot overflow, file as `cleared` and note the bound. + +--- + +## Deconfliction + +These are mostly disjoint; no cross-pass dedup rules needed. If in doubt, prefer the pass whose prefix more narrowly describes the bug. diff --git a/plugins/c-review/prompts/clusters/syscall-retval.md b/plugins/c-review/prompts/clusters/syscall-retval.md new file mode 100644 index 0000000..085db20 --- /dev/null +++ b/plugins/c-review/prompts/clusters/syscall-retval.md @@ -0,0 +1,81 @@ +--- +name: cluster-syscall-retval +kind: cluster +consolidated: false +covers: + - errno-handling # ERRNO + - negative-retval # NEGRET + - error-handling # ERR + - eintr-handling # EINTR + - socket-disconnect # SOCKDISCON + - half-closed-socket # HALFCLOSE + - open-issues # FILEOP +--- + +# Cluster: Syscall / libc return handling + +Seven bug classes that all ask "what does the caller do with the return value of this syscall/libc function?" One syscall-site inventory fuels all seven. + +ID prefixes: `ERRNO`, `NEGRET`, `ERR`, `EINTR`, `SOCKDISCON`, `HALFCLOSE`, `FILEOP`. + +--- + +## Phase A — Build the syscall inventory (ONCE per run) + +``` +Grep: pattern="\\b(read|write|pread|pwrite|readv|writev|recv|recvfrom|recvmsg|send|sendto|sendmsg)\\s*\\(" +Grep: pattern="\\b(open|openat|creat|close|fopen|fclose|dup|dup2|dup3|pipe|pipe2|socketpair|socket|accept|accept4|connect|bind|listen|shutdown)\\s*\\(" +Grep: pattern="\\b(stat|fstat|lstat|access|faccessat|readlink|realpath)\\s*\\(" +Grep: pattern="\\b(malloc|calloc|realloc|reallocarray|mmap|mprotect|munmap)\\s*\\(" +Grep: pattern="\\b(ioctl|fcntl|select|poll|epoll_wait|kqueue|kevent)\\s*\\(" +Grep: pattern="\\b(fork|execve|execv|execl|waitpid|wait|kill|sigaction|signal)\\s*\\(" +Grep: pattern="\\berrno\\b" # errno reads +Grep: pattern="\\b(setsockopt|getsockopt|getaddrinfo|freeaddrinfo)\\s*\\(" +``` + +Keep the result as `syscall_sites` (callee, `path:line`, enclosing function). For each, note whether the return is assigned, whether it's checked, and whether `errno` is read on failure. + +Do not file findings during Phase A. + +--- + +## Phase B — Passes in order (reuse `syscall_sites`) + +1. **`ERR` — Error handling (coarsest)** + Flag return-value discarded entirely (`read(fd, buf, n);` with no check). + +2. **`NEGRET` — Negative return not handled** + Flag `-1` returns used as a size/index without a prior `< 0` check. + +3. **`ERRNO` — `errno` misuse** + Flag `errno` read after a non-erroring call, or after an intervening call that clobbered it. + +4. **`EINTR` — Interrupted syscalls** + Flag blocking syscalls without an `EINTR` retry loop when signals are installed. + +5. **`FILEOP` — open() / fopen() specifics** + Focus on the `open`/`openat`/`fopen` subset of `syscall_sites`: flags, mode, O_CREAT without O_EXCL, etc. + +6. **`SOCKDISCON` — Socket disconnect handling** + Focus on `recv`/`read` on sockets: return of 0 means peer closed, not "no data." + +7. **`HALFCLOSE` — Half-closed sockets** + Focus on `shutdown(SHUT_WR)` / `shutdown(SHUT_RD)` patterns and their interaction with reads/writes. + +--- + +## Deconfliction + +Priority (higher wins): + +1. `SOCKDISCON` > `ERR` (if the unchecked return is specifically `recv`-returning-0 on a socket). +2. `EINTR` > `ERR` (if the bug is specifically the missing `EINTR` loop). +3. `FILEOP` > `ERR` (for `open`/`fopen` flag issues). +4. `NEGRET` > `ERR` (when `-1` is the value causing harm). +5. `ERRNO` is independent — about `errno` reads, not return values. + +--- + +## Token-economy reminder + +Each pass cares about a specific subset of `syscall_sites`. Do not re-grep syscalls between passes; filter the inventory you already have. diff --git a/plugins/c-review/prompts/clusters/windows-fs-path.md b/plugins/c-review/prompts/clusters/windows-fs-path.md new file mode 100644 index 0000000..88083fb --- /dev/null +++ b/plugins/c-review/prompts/clusters/windows-fs-path.md @@ -0,0 +1,49 @@ +--- +name: cluster-windows-fs-path +kind: cluster +consolidated: false +gate: is_windows +covers: + - dll-planting # DLLPLANT + - windows-path # WINPATH + - installer-race # INSTRACE +--- + +# Cluster: Windows — filesystem & path + +Three Windows-only bug classes around DLL search order, path handling, and installer TOCTOU. Share one inventory of Win32 FS/path APIs. + +ID prefixes: `DLLPLANT`, `WINPATH`, `INSTRACE`. + +--- + +## Phase A — Seed targets + +``` +Grep: pattern="\\b(LoadLibrary[AW]?|LoadLibraryEx[AW]?|GetProcAddress|SetDllDirectory[AW]?|SetDefaultDllDirectories|AddDllDirectory)\\s*\\(" +Grep: pattern="\\b(GetCurrentDirectory[AW]?|SetCurrentDirectory[AW]?|GetTempPath[AW]?|GetTempFileName[AW]?|PathCombine[AW]?|PathAppend[AW]?)\\s*\\(" +Grep: pattern="\\b(CreateFile[AW]?|CreateDirectory[AW]?|DeleteFile[AW]?|MoveFile[AW]?|CopyFile[AW]?)\\s*\\(" +Grep: pattern="\\bMAX_PATH\\b" # MAX_PATH usage (truncation candidates) +``` + +Keep as `win_fs_sites`. + +--- + +## Phase B — Passes in order + +1. **`DLLPLANT` — DLL search-order hijacking** + `LoadLibrary("foo.dll")` without full path; missing `SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_SYSTEM32)`. + +2. **`WINPATH` — Windows path handling** + `MAX_PATH` truncation; missing `\\?\\` for long paths; reserved names (`CON`, `PRN`, `AUX`, `NUL`, `COM1`…). + +3. **`INSTRACE` — Installer race conditions** + Writable-by-users temp dirs used during install; signature verification before copy-to-privileged-dir. + +--- + +## Deconfliction + +1. `DLLPLANT` > `WINPATH` (if the path issue is specifically about unqualified DLL names). +2. `INSTRACE` > `WINPATH` (when the TOCTOU is the root cause). diff --git a/plugins/c-review/prompts/clusters/windows-ipc-crypto.md b/plugins/c-review/prompts/clusters/windows-ipc-crypto.md new file mode 100644 index 0000000..f5020dd --- /dev/null +++ b/plugins/c-review/prompts/clusters/windows-ipc-crypto.md @@ -0,0 +1,48 @@ +--- +name: cluster-windows-ipc-crypto +kind: cluster +consolidated: false +gate: is_windows +covers: + - named-pipe # NAMEDPIPE + - windows-crypto # WINCRYPTO + - windows-alloc # WINALLOC +--- + +# Cluster: Windows — IPC, crypto, allocator + +Three Windows-only bug classes around named-pipe security, CryptoAPI misuse, and Windows heap/alloc specifics. + +ID prefixes: `NAMEDPIPE`, `WINCRYPTO`, `WINALLOC`. + +--- + +## Phase A — Seed targets + +``` +Grep: pattern="\\b(CreateNamedPipe[AW]?|ConnectNamedPipe|ImpersonateNamedPipeClient|SetNamedPipeHandleState)\\s*\\(" +Grep: pattern="\\b(CryptAcquireContext[AW]?|CryptGenKey|CryptGenRandom|CryptEncrypt|CryptDecrypt|CryptHashData|CryptSignHash|CryptVerifySignature|BCrypt\\w+|NCrypt\\w+)\\s*\\(" +Grep: pattern="\\b(HeapAlloc|HeapFree|HeapReAlloc|HeapCreate|HeapDestroy|VirtualAlloc|VirtualFree|VirtualProtect|LocalAlloc|LocalFree|GlobalAlloc|GlobalFree)\\s*\\(" +Grep: pattern="\\bSECURITY_DESCRIPTOR\\b|\\bSetSecurityDescriptor\\w+\\s*\\(" +``` + +Keep as `win_ipc_sites`. + +--- + +## Phase B — Passes in order + +1. **`NAMEDPIPE` — Named-pipe security** + Missing `PIPE_REJECT_REMOTE_CLIENTS`, weak DACL on pipe, impersonation without `SECURITY_IDENTIFICATION`. + +2. **`WINCRYPTO` — CryptoAPI misuse** + Deprecated algorithms (`CALG_MD5`, `CALG_DES`), `rand()` for keys, missing `CryptGenRandom`/`BCryptGenRandom`. + +3. **`WINALLOC` — Windows allocator specifics** + Mismatched alloc/free pairs (`HeapAlloc` freed with `LocalFree`), `VirtualProtect` race, W^X violations. + +--- + +## Deconfliction + +Mostly disjoint. `WINCRYPTO` never merges with the others. diff --git a/plugins/c-review/prompts/clusters/windows-process.md b/plugins/c-review/prompts/clusters/windows-process.md new file mode 100644 index 0000000..1c92391 --- /dev/null +++ b/plugins/c-review/prompts/clusters/windows-process.md @@ -0,0 +1,54 @@ +--- +name: cluster-windows-process +kind: cluster +consolidated: false +gate: is_windows +covers: + - createprocess # CREATEPROC + - cross-process # CROSSPROC + - token-privilege # TOKPRIV + - service-security # WINSVC +--- + +# Cluster: Windows — process & token + +Four Windows-only bug classes around process creation, cross-process handles, privilege tokens, and service security. Share a common inventory of Win32 process/security APIs. + +ID prefixes: `CREATEPROC`, `CROSSPROC`, `TOKPRIV`, `WINSVC`. + +--- + +## Phase A — Seed targets + +``` +Grep: pattern="\\b(CreateProcess[AW]?|ShellExecute[AW]?|CreateProcessAsUser[AW]?|WinExec)\\s*\\(" +Grep: pattern="\\b(OpenProcess|DuplicateHandle|ReadProcessMemory|WriteProcessMemory|VirtualAllocEx|CreateRemoteThread)\\s*\\(" +Grep: pattern="\\b(OpenProcessToken|OpenThreadToken|AdjustTokenPrivileges|LookupPrivilegeValue|ImpersonateLoggedOnUser|SetTokenInformation)\\s*\\(" +Grep: pattern="\\b(StartServiceCtrlDispatcher|RegisterServiceCtrlHandler|CreateService|OpenSCManager|StartService)\\s*\\(" +Grep: pattern="\\bSECURITY_ATTRIBUTES\\b|\\bSECURITY_DESCRIPTOR\\b" +``` + +Keep as `win_proc_sites`. + +--- + +## Phase B — Passes in order + +1. **`CREATEPROC` — `CreateProcess` misuse** + Unquoted paths with spaces, relative lpApplicationName, bInheritHandles=TRUE leaking handles. + +2. **`CROSSPROC` — Cross-process memory / handle issues** + `OpenProcess(PROCESS_ALL_ACCESS)` to untrusted PIDs, handle duplication without access-mask narrowing. + +3. **`TOKPRIV` — Token / privilege misuse** + `AdjustTokenPrivileges` without checking `GetLastError`, impersonation not reverted. + +4. **`WINSVC` — Service security** + Weak service DACLs, `SERVICE_CHANGE_CONFIG` granted to Users, unquoted `ImagePath`. + +--- + +## Deconfliction + +1. `WINSVC` > `CREATEPROC` (if the process-creation bug lives inside a service bootstrap). +2. `TOKPRIV` > `CROSSPROC` (if the bug is specifically adjusting privileges, even across processes). diff --git a/plugins/c-review/prompts/general/access-control-finder.md b/plugins/c-review/prompts/general/access-control-finder.md new file mode 100644 index 0000000..1087016 --- /dev/null +++ b/plugins/c-review/prompts/general/access-control-finder.md @@ -0,0 +1,49 @@ +--- +name: access-control-finder +description: Detects privilege and access control vulnerabilities +--- + +**Finding ID Prefix:** `ACCESS` (e.g., ACCESS-001, ACCESS-002) + +**Bug Patterns to Find:** + +1. **Invalid Privilege Dropping** + - setuid/setgid return values not checked + - Incomplete privilege drop (saved uid) + - Wrong order (user before group) + +2. **Untrusted Data in Privileged Context** + - User data used in kernel context + - Sensitive CPU instructions with user input + - Privileged operations on user-controlled paths + +3. **Missing Authorization Checks** + - Privileged operation without check + - Race between check and use + - Capability leaks + +4. **setuid/setgid Program Issues** + - Environment variable trust + - LD_PRELOAD not cleared + - File descriptor inheritance + +5. **Capability Issues** + - Capabilities not dropped properly + - Inherited capabilities confusion + +**Common False Positives to Avoid:** + +- **Return value checked:** setuid/setgid return values are properly checked and handled +- **Non-setuid binary:** Code is not running with elevated privileges +- **Intentional privilege retention:** Some programs legitimately keep privileges +- **Capabilities properly managed:** CAP_* properly dropped after use +- **Test/development code:** Privilege code in test harnesses not deployed + +**Search Patterns:** +``` +setuid\s*\(|setgid\s*\(|seteuid\s*\(|setegid\s*\( +setresuid\s*\(|setresgid\s*\(|setgroups\s*\( +cap_set|cap_clear|prctl\s*\(.*PR_SET +execve\s*\(|execv\s*\(|system\s*\( +getuid\s*\(|geteuid\s*\(|getgid\s*\( +``` diff --git a/plugins/c-review/prompts/general/compiler-bugs-finder.md b/plugins/c-review/prompts/general/compiler-bugs-finder.md new file mode 100644 index 0000000..7646e11 --- /dev/null +++ b/plugins/c-review/prompts/general/compiler-bugs-finder.md @@ -0,0 +1,45 @@ +--- +name: compiler-bugs-finder +description: Identifies patterns that trigger compiler optimization bugs +--- + +**Finding ID Prefix:** `COMP` (e.g., COMP-001, COMP-002) + +**Bug Patterns to Find:** + +1. **Removed Bounds Checks** + - Compiler optimizes away null pointer checks + - -fdelete-null-pointer-checks removing validation + - Dead code elimination of security checks + +2. **Removed Data Zeroization** + - memset of sensitive data optimized away + - SecureZeroMemory not used + - explicit_bzero required but memset used + +3. **Constant-Time Violation** + - Compiler optimizing constant-time code + - Short-circuit evaluation breaking timing + - Branch prediction affecting security + +4. **Debug Assertion Removal** + - assert() removed in release builds + - Security-critical checks in assert + - NDEBUG removing validation + +**Common False Positives to Avoid:** + +- **explicit_bzero/SecureZeroMemory used:** Proper secure memory clearing functions in place +- **Volatile access:** volatile qualifier prevents optimization +- **Compiler barriers:** Memory barriers or asm volatile prevent reordering +- **Non-sensitive data:** memset on non-sensitive data can safely be optimized away +- **Check not security-relevant:** Null check for debug/logging purposes, not security + +**Search Patterns:** +``` +memset\s*\(.*0\s*\)|bzero\s*\( +explicit_bzero|SecureZeroMemory|volatile.*memset +assert\s*\(|ASSERT\s*\(|DEBUG_ASSERT +-O[23s]|-fdelete-null-pointer-checks +if\s*\(\s*\w+\s*!=\s*NULL\s*\).*\*\w+ +``` diff --git a/plugins/c-review/prompts/general/dos-finder.md b/plugins/c-review/prompts/general/dos-finder.md new file mode 100644 index 0000000..3bf40d2 --- /dev/null +++ b/plugins/c-review/prompts/general/dos-finder.md @@ -0,0 +1,48 @@ +--- +name: dos-finder +description: Detects denial of service vulnerabilities +--- + +**Finding ID Prefix:** `DOS` (e.g., DOS-001, DOS-002) + +**Bug Patterns to Find:** + +1. **High Resource Usage** + - Unbounded memory allocation from user input + - Unbounded loop iterations + - Exponential algorithm on attacker input + +2. **Resource Leaks** + - Memory not freed on error paths + - File descriptors not closed + - Connections not released + +3. **Inefficient Copies** + - Large container passed by value + - Unnecessary deep copies + - String concatenation in loops + +4. **Dangling References** + - Reference to moved-from object + - Reference in lambda captures after move + +5. **Algorithmic Complexity** + - O(n²) or worse on user input + - Hash collision attacks possible + - Regex backtracking (ReDoS) + +**Common False Positives to Avoid:** + +- **Bounded allocations:** Allocations with verified upper bounds (e.g., `if (size > MAX) return`) +- **Internal-only code:** Code paths not reachable from untrusted input +- **Intentional resource limits:** System-level limits (ulimits) may be in place +- **Rate limiting present:** External rate limiting may prevent exploitation +- **Streaming processing:** Code that processes data in fixed-size chunks + +**Search Patterns:** +``` +malloc\s*\(.*user|malloc\s*\(.*input|malloc\s*\(.*size +while\s*\(1\)|for\s*\(;;\)|for\s*\(.*<.*input +vector<.*>\s+\w+\s*=|string\s+\w+\s*=.*\+ +std::move\s*\( +``` diff --git a/plugins/c-review/prompts/general/error-handling-finder.md b/plugins/c-review/prompts/general/error-handling-finder.md new file mode 100644 index 0000000..c0fff3e --- /dev/null +++ b/plugins/c-review/prompts/general/error-handling-finder.md @@ -0,0 +1,48 @@ +--- +name: error-handling-finder +description: Finds missing or improper error handling +--- + +**Finding ID Prefix:** `ERR` (e.g., ERR-001, ERR-002) + +**Bug Patterns to Find:** + +1. **Unchecked Return Values** + - Ignoring malloc/fopen/socket return + - Ignoring write/read return + - Ignoring security-critical function returns + +2. **Incorrect Error Comparison** + - `if (retval != 0)` when success is 1 + - `if (retval)` when -1 is error + - Comparing wrong error codes + +3. **Exception Handling Issues** + - Catch-all hiding errors + - Exception during cleanup + - Resource leak on exception path + +4. **Partial Error Handling** + - Some errors handled, others not + - Error logged but not propagated + +5. **Error State Corruption** + - Continuing after error + - Partial operation on error + +**Common False Positives to Avoid:** + +- **Intentionally ignored:** `(void)close(fd)` - cast to void indicates intentional +- **Non-critical function:** `printf()` return rarely matters for security +- **Wrapper handles error:** Error handled in called wrapper function +- **Assert on error:** `assert(func() == 0)` catches in debug builds +- **Logging functions:** `syslog()`, `fprintf(stderr, ...)` return values rarely matter +- **Best-effort operations:** Close/cleanup operations where failure doesn't affect security + +**Search Patterns:** +``` +=\s*(malloc|calloc|fopen|socket|connect|open)\s*\( +if\s*\(.*==\s*-1|if\s*\(.*!=\s*0 +catch\s*\(|throw\s+ +errno\s*=|perror\s*\( +``` diff --git a/plugins/c-review/prompts/general/exception-safety-finder.md b/plugins/c-review/prompts/general/exception-safety-finder.md new file mode 100644 index 0000000..f6dee03 --- /dev/null +++ b/plugins/c-review/prompts/general/exception-safety-finder.md @@ -0,0 +1,51 @@ +--- +name: exception-safety-finder +description: Analyzes exception safety guarantees +--- + +**Finding ID Prefix:** `EXCEPT` (e.g., EXCEPT-001, EXCEPT-002) + +**Bug Patterns to Find:** + +1. **RAII Violations** + - Raw pointer with manual delete instead of smart pointer + - new without corresponding delete in same scope + - Resource acquired but not wrapped in RAII class + +2. **Exception-Unsafe Code** + - Destructor that throws + - noexcept function that calls throwing code + - Swap operation that can throw + +3. **Resource Leaks on Exception Path** + - Memory allocated then exception thrown before delete + - File opened then exception before close + - Lock acquired then exception before unlock + +4. **Copy/Move Assignment Issues** + - Self-assignment not handled with exceptions + - Strong exception guarantee violated + - Partial assignment on exception + +5. **Constructor Exception Issues** + - Resource acquired in constructor body (not initializer) + - Partially constructed object on exception + - Virtual function called in constructor + +**Common False Positives to Avoid:** + +- **Smart pointers used:** `unique_ptr`, `shared_ptr` handle cleanup automatically +- **RAII wrapper present:** Custom RAII class handles the resource +- **noexcept path:** If all called functions are noexcept, no exception possible +- **C code called:** extern "C" functions typically don't throw +- **Catch and handle:** Exception caught and resources cleaned up in handler +- **Finally-equivalent:** Scope guard or similar ensures cleanup + +**Search Patterns:** +``` +\bnew\s+\w+|\bdelete\s+\w+ +~\w+\s*\(.*\)\s*\{.*throw +noexcept\s*\(|noexcept\s*\{ +catch\s*\(|try\s*\{ +fopen\s*\(.*\{|open\s*\(.*\{ +``` diff --git a/plugins/c-review/prompts/general/exploit-mitigations-finder.md b/plugins/c-review/prompts/general/exploit-mitigations-finder.md new file mode 100644 index 0000000..43a2d0d --- /dev/null +++ b/plugins/c-review/prompts/general/exploit-mitigations-finder.md @@ -0,0 +1,57 @@ +--- +name: exploit-mitigations-finder +description: Checks for missing or misconfigured exploit mitigations +--- + +**Finding ID Prefix:** `MITIGATION` (e.g., MITIGATION-001, MITIGATION-002) + +**Bug Patterns to Find:** + +1. **FORTIFY_SOURCE Typos** + - `_FORTIFY_SORUCE` (missing U) + - `_FORTIFY_SOUCE` (missing R) + - `_FORTIY_SOURCE` (missing F) + - `FORTIFY_SOURCE` (missing underscore) + +2. **Stack Protector Typos** + - `-fstack-protector-stong` (missing R) + - `-fstack-protecter` (E instead of O) + - `-fstack-protector-al` (missing L) + - `-fstackprotector` (missing hyphen) + +3. **PIE/PIC Typos** + - `-fPIe` or `-fpie` (case sensitivity matters on some compilers) + - `-pie` without corresponding `-fPIE` + +4. **RELRO Typos** + - `-z,rello` (typo) + - `-z,relr` (incomplete) + - `-z relro` (space instead of comma in some contexts) + +5. **libc++ Hardening Typos** + - `_LIBCPP_HARDENING_MOD` (missing E) + - `_LIBCPP_HARDENIG_MODE` (missing N) + - `_GLIBCXX_ASSERTONS` (missing I) + +**Common False Positives to Avoid:** + +- **Intentionally disabled:** `-fno-stack-protector` is valid (though suspicious) +- **Comments:** Typos in comments don't affect compilation +- **Documentation:** Typos in docs/READMEs don't affect security + +**Files to Examine:** + +- `CMakeLists.txt`, `*.cmake` +- `Makefile`, `*.mk`, `GNUmakefile` +- `configure.ac`, `configure` +- `meson.build` +- `BUILD.bazel` +- CI/CD: `.github/workflows/*.yml`, `.gitlab-ci.yml` + +**Search Patterns:** +``` +FORTIFY_S[A-Z]*[^O]RCE|FORTIFY_SO[A-Z]*[^U]CE +fstack-protect[a-z]*[^o]r|fstack-protector-st[a-z]*[^r]ng +_LIBCPP_HARD[A-Z]*[^E]NING|_GLIBCXX_ASSERT[A-Z]*[^I]ONS +-z,[a-z]*rel[a-z]*[^r]o +``` diff --git a/plugins/c-review/prompts/general/filesystem-issues-finder.md b/plugins/c-review/prompts/general/filesystem-issues-finder.md new file mode 100644 index 0000000..29aada3 --- /dev/null +++ b/plugins/c-review/prompts/general/filesystem-issues-finder.md @@ -0,0 +1,54 @@ +--- +name: filesystem-issues-finder +description: Detects symlink attacks and temp file vulnerabilities +--- + +**Finding ID Prefix:** `FS` (e.g., FS-001, FS-002) + +**Bug Patterns to Find:** + +1. **Symlink/Softlink Issues** + - Following symlinks in privileged code + - Symlink TOCTOU attacks + - Directory traversal via symlinks + +2. **Disk Synchronization Issues** + - Missing fsync/fdatasync + - Data corruption on crash + - Write ordering bugs + +3. **Unquoted Path Issues** + - Paths with spaces not quoted + - Shell injection via paths + +4. **Missing Path Separators** + - `/path/files` vs `/path/files/` behavior + - `/path/files` vs `/path/files_sensitive` + +5. **Case and Normalization** + - Case-insensitive filesystem issues + - Unicode normalization bypasses + - Path canonicalization bugs + +6. **Predictable Temp Files** + - Using tmpnam/tempnam/mktemp + - Predictable temp file names + - Insecure temp directory permissions + +**Common False Positives to Avoid:** + +- **O_NOFOLLOW used:** Symlink following explicitly prevented +- **Hardcoded trusted paths:** Paths to system files that can't be manipulated +- **User's own directory:** Operations in user's home dir by user's own process +- **Already canonicalized:** Path passed through realpath() or similar +- **Directory fd operations:** openat() with directory fd avoids races +- **Root-only writable directory:** Symlink attacks require write access + +**Search Patterns:** +``` +open\s*\(|fopen\s*\(|stat\s*\(|lstat\s*\( +readlink\s*\(|symlink\s*\(|realpath\s*\( +tmpnam|tempnam|mktemp|mkstemp|tmpfile +fsync\s*\(|fdatasync\s*\( +O_NOFOLLOW|O_DIRECTORY +``` diff --git a/plugins/c-review/prompts/general/init-order-finder.md b/plugins/c-review/prompts/general/init-order-finder.md new file mode 100644 index 0000000..cbc7777 --- /dev/null +++ b/plugins/c-review/prompts/general/init-order-finder.md @@ -0,0 +1,42 @@ +--- +name: init-order-finder +description: Detects static initialization order fiasco +--- + +**Finding ID Prefix:** `INIT` (e.g., INIT-001, INIT-002) + +**Bug Patterns to Find:** + +1. **Static Initialization Order Fiasco** + - Global object depends on another global + - Order of initialization across translation units + - Static variable in one file uses another file's static + +2. **Construct-on-First-Use Failures** + - Singleton pattern race conditions + - Local static initialization issues + +3. **Initialization List Order** + - Members initialized out of declaration order + - Base class vs member initialization order + +4. **Thread-Unsafe Static Initialization** + - Static locals in multi-threaded code (pre-C++11) + - Global initialization races + +**Common False Positives to Avoid:** + +- **Same translation unit:** Globals in same .cpp file initialize in declaration order +- **constexpr/constinit:** Compile-time constants don't have runtime init order issues +- **POD types with literal init:** `int x = 5;` is compile-time initialized +- **C++11 thread-safe statics:** Local statics are thread-safe in C++11+ +- **Explicit init functions:** Code uses explicit init() functions instead of constructors +- **Header-only globals:** `inline` globals have defined behavior in C++17+ + +**Search Patterns:** +``` +^static\s+\w+.*=|^extern\s+\w+ +static\s+\w+\s+\w+\s*=.*:: +Singleton|GetInstance|Instance\(\) +:\s*\w+\(.*\),\s*\w+\( # Initialization lists +``` diff --git a/plugins/c-review/prompts/general/integer-overflow-finder.md b/plugins/c-review/prompts/general/integer-overflow-finder.md new file mode 100644 index 0000000..d91037b --- /dev/null +++ b/plugins/c-review/prompts/general/integer-overflow-finder.md @@ -0,0 +1,61 @@ +--- +name: integer-overflow-finder +description: Detects integer overflow and signedness issues +--- + +**Finding ID Prefix:** `INT` (e.g., INT-001, INT-002) + +**Bug Patterns to Find:** + +1. **Arithmetic Overflows** + - `a + b` where result exceeds type range + - `a * b` multiplication overflow + - Size calculations: `n * sizeof(type)` + +2. **Width Truncation** + - 64-bit value assigned to 32-bit variable + - Large value truncated to smaller type + +3. **Signedness Bugs** + - Signed/unsigned comparison + - Negative value interpreted as large unsigned + - `int` used where `size_t` expected + +4. **Implicit Conversions** + - Unexpected type promotion/demotion + - Integer to pointer conversion + +5. **Negative Assignment Overflow** + - `abs(-INT_MIN) == -INT_MIN` + - `-(-INT_MIN)` still negative + +6. **Integer Cut** + - Read 64-bit, compare 32-bit, use 64-bit + - Mask or truncate then use full value + +7. **Rounding Errors** + - Integer division truncation issues + - Lost precision in calculations + +8. **Float Imprecision** + - Direct float comparison without epsilon + - Float used for financial/precise calculations + +**Common False Positives to Avoid:** + +- **Checked arithmetic:** If overflow is explicitly checked before use (e.g., `if (a > SIZE_MAX - b)`) +- **Safe integer libraries:** `SafeInt<>`, `__builtin_add_overflow`, or similar checked operations +- **Known small values:** Constants or validated inputs that can't overflow (e.g., `argc * 4`) +- **Intentional wrapping:** Hash functions, checksums, crypto often use intentional wrapping +- **Unsigned comparison with zero:** `unsigned >= 0` is always true but not a security bug +- **Loop counters with known bounds:** `for (int i = 0; i < 100; i++)` can't overflow +- **Sizeof expressions with small n:** `sizeof(x) * n` where n is a small constant + +**Search Patterns:** +``` +\*\s*sizeof|\+\s*sizeof +\(int\)|\(unsigned\)|\(size_t\)|\(long\) +abs\s*\(|labs\s*\( +<=\s*0|>=\s*0.*unsigned +malloc\s*\(.*\*|calloc\s*\( +``` diff --git a/plugins/c-review/prompts/general/iterator-invalidation-finder.md b/plugins/c-review/prompts/general/iterator-invalidation-finder.md new file mode 100644 index 0000000..358d43e --- /dev/null +++ b/plugins/c-review/prompts/general/iterator-invalidation-finder.md @@ -0,0 +1,43 @@ +--- +name: iterator-invalidation-finder +description: Finds iterator invalidation bugs +--- + +**Finding ID Prefix:** `ITER` (e.g., ITER-001, ITER-002) + +**Bug Patterns to Find:** + +1. **Modification During Iteration** + - Inserting into vector during iteration + - Erasing from container during range-for + - Resizing container while iterating + +2. **Invalidated Iterator Use** + - Using iterator after container modification + - Storing iterator across modifying operations + - End iterator cached and invalidated + +3. **Pointer/Reference Invalidation** + - Pointer to vector element after push_back + - Reference to map value after insert + - String char* after modification + +4. **Range-Based For Issues** + - Modifying container in range-for body + - Breaking out but iterator still stored + +**Common False Positives to Avoid:** + +- **Iterator reassigned:** If iterator is reassigned after modifying operation (e.g., `it = vec.erase(it)`) +- **Non-invalidating operations:** Operations like `std::map::insert` don't invalidate existing iterators +- **Reserve before loop:** `vector::reserve` before iteration prevents reallocation invalidation +- **Index-based access:** Using indices instead of iterators doesn't have invalidation issues +- **Copy iteration:** Iterating over copy while modifying original is safe + +**Search Patterns:** +``` +for\s*\(.*begin\(\)|for\s*\(.*:\s* +\.erase\(|\.insert\(|\.push_back\(|\.clear\( +\.resize\(|\.reserve\( +iterator|::iterator|auto.*=.*begin +``` diff --git a/plugins/c-review/prompts/general/lambda-capture-finder.md b/plugins/c-review/prompts/general/lambda-capture-finder.md new file mode 100644 index 0000000..8dc3842 --- /dev/null +++ b/plugins/c-review/prompts/general/lambda-capture-finder.md @@ -0,0 +1,49 @@ +--- +name: lambda-capture-finder +description: Detects lambda capture lifetime issues +--- + +**Finding ID Prefix:** `LAMBDA` (e.g., LAMBDA-001, LAMBDA-002) + +**Bug Patterns to Find:** + +1. **Dangling Reference Capture** + - Capturing local by reference in escaping lambda + - Lambda stored outlives captured reference + - Async callback with reference capture + +2. **Dangling this Capture** + - [this] or [=] in lambda outliving object + - Lambda stored in callback then object destroyed + - Capturing this in detached thread + +3. **Capture-by-Value Issues** + - Large object captured by value unnecessarily + - Mutable lambda modifying copy not original + - Reference wrapper captured by value + +4. **Init-Capture Issues** + - Init-capture with dangling reference + - Move-capture then use original + - Init-capture evaluation order + +5. **Generic Lambda Issues** + - auto&& parameter with unexpected lifetime + - Perfect forwarding in generic lambda + +**Common False Positives to Avoid:** + +- **Lambda immediately invoked:** IIFE doesn't outlive captures +- **Lambda never escapes:** If lambda doesn't escape scope, references are safe +- **Shared ownership:** shared_ptr captured keeps object alive +- **Copy intended:** Large capture by value may be intentional for thread safety +- **Synchronous callback:** If callback is called and returns before function exits + +**Search Patterns:** +``` +\[\s*&\s*\]|\[\s*=\s*\]|\[\s*this\s*\] +\[\s*&\w+|\[\s*\w+\s*= +std::function.*=.*\[ +std::thread.*\[|async.*\[|detach.*\[ +callback.*\[|handler.*\[ +``` diff --git a/plugins/c-review/prompts/general/memory-leak-finder.md b/plugins/c-review/prompts/general/memory-leak-finder.md new file mode 100644 index 0000000..5db0d28 --- /dev/null +++ b/plugins/c-review/prompts/general/memory-leak-finder.md @@ -0,0 +1,49 @@ +--- +name: memory-leak-finder +description: Finds memory and resource leaks +--- + +**Finding ID Prefix:** `LEAK` (e.g., LEAK-001, LEAK-002) + +**Bug Patterns to Find:** + +1. **Classic Memory Leaks** + - malloc without corresponding free + - new without delete + - Reassigning pointer before free + +2. **Error Path Leaks** + - Allocation freed on success, not on error + - Early return without cleanup + - Exception path missing cleanup + +3. **Resource Leaks** + - File descriptors not closed + - Sockets not closed + - Handles not released + +4. **Uninitialized Memory Exposure** + - Sending uninitialized buffer contents + - Struct padding leaked + +5. **Pointer Exposure** + - Heap addresses leaked to attacker + - ASLR bypass via pointer disclosure + +**Common False Positives to Avoid:** + +- **Caller frees:** Function returns allocated memory that caller is responsible for +- **Global/static storage:** Intentionally long-lived allocations freed at exit +- **RAII/smart pointers:** C++ smart pointers handle deallocation automatically +- **Process exit cleanup:** Memory freed implicitly when process exits (short-lived tools) +- **Transfer of ownership:** Pointer passed to library that takes ownership +- **Pool allocators:** Memory returned to pool, not system + +**Search Patterns:** +``` +malloc\s*\(|calloc\s*\(|new\s+ +free\s*\(|delete\s+ +fopen\s*\(|open\s*\(|socket\s*\( +fclose\s*\(|close\s*\( +return.*\berr|goto\s+err +``` diff --git a/plugins/c-review/prompts/general/move-semantics-finder.md b/plugins/c-review/prompts/general/move-semantics-finder.md new file mode 100644 index 0000000..95843a5 --- /dev/null +++ b/plugins/c-review/prompts/general/move-semantics-finder.md @@ -0,0 +1,49 @@ +--- +name: move-semantics-finder +description: Identifies move semantics misuse +--- + +**Finding ID Prefix:** `MOVE` (e.g., MOVE-001, MOVE-002) + +**Bug Patterns to Find:** + +1. **Use After Move** + - Accessing object after std::move() + - Using moved-from container (size, iteration) + - Calling methods on moved-from object + +2. **Invalid Move Operations** + - Moving const object (falls back to copy) + - Moving non-moveable type + - Self-move assignment + +3. **Move in Loop** + - std::move in loop body reuses moved-from object + - Lambda capturing by move used multiple times + +4. **Forwarding Issues** + - std::forward on non-forwarding reference + - Perfect forwarding breaking value category + - Missing std::forward in template + +5. **Move Constructor Issues** + - Move constructor that copies + - Move leaves source in invalid state + - noexcept move required but missing + +**Common False Positives to Avoid:** + +- **Reassigned after move:** Object assigned new value before next use +- **Moved into function:** Object moved into function, never used again in caller +- **Valid moved-from state:** Some types (std::unique_ptr) have defined moved-from state +- **Move of primitive:** Primitives just copy, no moved-from issue +- **Conditional move:** Move only happens on certain paths, use on others is safe + +**Search Patterns:** +``` +std::move\s*\( +std::forward\s*\( +&&\s*\w+|T&&|auto&& +noexcept.*move|move.*noexcept +\w+\s*=\s*std::move\s*\( +``` diff --git a/plugins/c-review/prompts/general/null-deref-finder.md b/plugins/c-review/prompts/general/null-deref-finder.md new file mode 100644 index 0000000..6fa0f41 --- /dev/null +++ b/plugins/c-review/prompts/general/null-deref-finder.md @@ -0,0 +1,47 @@ +--- +name: null-deref-finder +description: Detects null pointer dereferences +--- + +**Finding ID Prefix:** `NULL` (e.g., NULL-001, NULL-002) + +**Bug Patterns to Find:** + +1. **Missing Null Check After Allocation** + - malloc/calloc return not checked + - new with nothrow not checked + - Factory function return not checked + +2. **Null Check After Dereference** + - `ptr->field; if (ptr == NULL)` - check too late + - Compiler may optimize away late check + +3. **Conditional Null Assignment** + - Pointer set to NULL in some paths + - Used without re-checking + +4. **Failed Lookup Returns** + - find() returning end()/NULL not checked + - Map/set lookup failure not handled + +5. **Double Pointer Issues** + - `*ptr` where ptr itself may be NULL + - Nested null checks missing + +**Common False Positives to Avoid:** + +- **assert() is present:** `assert(ptr != NULL)` in debug builds indicates assumption +- **Contract documented:** Function precondition states non-null, caller verified +- **C++ new (without nothrow):** Standard `new` throws on failure, doesn't return NULL +- **Reference parameters:** C++ references can't be NULL +- **Immediately after successful call:** `if ((p = malloc(...)) != NULL) { use(p); }` +- **Static analysis annotation:** `__attribute__((nonnull))` indicates compiler-verified +- **Known non-null source:** Return from function documented to never return NULL + +**Search Patterns:** +``` +malloc\s*\(|calloc\s*\(|realloc\s*\( +new\s+\w+|new\s*\( +->|\.find\(|\.get\( +if\s*\(\s*\w+\s*==\s*NULL|if\s*\(\s*!\w+\s*\) +``` diff --git a/plugins/c-review/prompts/general/operator-precedence-finder.md b/plugins/c-review/prompts/general/operator-precedence-finder.md new file mode 100644 index 0000000..c4c402a --- /dev/null +++ b/plugins/c-review/prompts/general/operator-precedence-finder.md @@ -0,0 +1,44 @@ +--- +name: operator-precedence-finder +description: Identifies operator precedence mistakes +--- + +**Finding ID Prefix:** `PREC` (e.g., PREC-001, PREC-002) + +**Bug Patterns to Find:** + +1. **Bitwise vs Comparison** + - `x & mask == value` (== binds tighter than &) + - `x | y < z` (< binds tighter than |) + +2. **Bitwise vs Logical** + - `x & y && z` (potential confusion) + - Mixing & and && without parens + +3. **Ternary Operator** + - `a ? b : c + d` (+ is in else only) + - Nested ternary without parens + +4. **Shift Precedence** + - `1 << n + 1` (+ happens first) + - `x >> y & mask` (& binds tighter) + +5. **Macro Expansion** + - Macro without proper parentheses + - `#define SQ(x) x*x` then SQ(1+1) + +**Common False Positives to Avoid:** + +- **Properly parenthesized:** Expression already has clarifying parentheses +- **Intentional evaluation order:** Some precedence is intentional and well-documented +- **Single-operator expressions:** No precedence issue with single operator +- **Well-known idioms:** Common patterns like `flags & MASK` without comparison +- **Compiler warnings enabled:** Many of these trigger compiler warnings that may already be addressed + +**Search Patterns:** +``` +&\s*\w+\s*==|&\s*\w+\s*!=|\|\s*\w+\s*<|\|\s*\w+\s*> +\?\s*.*:\s*\w+\s*[+\-*/] +<<\s*\w+\s*[+\-*/]|>>\s*\w+\s*[+\-*/] +#define\s+\w+\s*\([^)]*\)\s+[^(] +``` diff --git a/plugins/c-review/prompts/general/race-condition-finder.md b/plugins/c-review/prompts/general/race-condition-finder.md new file mode 100644 index 0000000..b1c1be1 --- /dev/null +++ b/plugins/c-review/prompts/general/race-condition-finder.md @@ -0,0 +1,53 @@ +--- +name: race-condition-finder +description: Detects TOCTOU and race conditions +--- + +**Finding ID Prefix:** `RACE` (e.g., RACE-001, RACE-002) + +**Bug Patterns to Find:** + +1. **Time-of-Check to Time-of-Use (TOCTOU)** + - access() followed by open() + - stat() followed by open() + - Check-then-act on shared state + +2. **Double Fetch** + - Reading shared memory twice + - Kernel reading userspace memory twice + - Value changed between reads + +3. **Over-Locking** + - Deadlock from lock order violation + - Recursive lock without recursive mutex + +4. **Under-Locking** + - Shared data accessed without lock + - Lock released too early + - Partial locking of compound operation + +5. **Non-Thread-Safe API Usage** + - Using non-thread-safe functions in threaded code + - Shared state without synchronization + +6. **Signal Safety** + - Non-async-signal-safe functions in handlers + - Signal handler race with main code + +**Common False Positives to Avoid:** + +- **Single-threaded code:** If application is provably single-threaded, no races possible +- **Read-only shared data:** Immutable data after initialization doesn't race +- **Thread-local storage:** Variables in TLS can't race between threads +- **Proper locking verified:** If lock is held for the entire critical section +- **Atomic operations:** `std::atomic`, `_Atomic`, or proper memory barriers +- **Initialization-only access:** Data written once at startup, read-only thereafter +- **Same-thread access pattern:** If analysis proves same thread does both accesses + +**Search Patterns:** +``` +pthread_mutex|pthread_rwlock|std::mutex +access\s*\(.*open\s*\(|stat\s*\(.*open\s*\( +volatile\s+|atomic|std::atomic +signal\s*\(|sigaction\s*\( +``` diff --git a/plugins/c-review/prompts/general/regex-issues-finder.md b/plugins/c-review/prompts/general/regex-issues-finder.md new file mode 100644 index 0000000..9c644a5 --- /dev/null +++ b/plugins/c-review/prompts/general/regex-issues-finder.md @@ -0,0 +1,47 @@ +--- +name: regex-issues-finder +description: Finds ReDoS and regex bypass vulnerabilities +--- + +**Finding ID Prefix:** `REGEX` (e.g., REGEX-001, REGEX-002) + +**Bug Patterns to Find:** + +1. **ReDoS (Regular Expression DoS)** + - Nested quantifiers: `(a+)+` + - Alternation with overlap: `(a|a)+` + - Backtracking explosion patterns + +2. **Newline Bypasses** + - `.` not matching newline (default) + - `^` and `$` with embedded newlines + - Missing `REG_NEWLINE` flag handling + +3. **Regex Injection** + - User input in regex pattern + - Unescaped special characters + - Metacharacter injection + +4. **Incorrect Anchoring** + - Missing `^` or `$` allows prefix/suffix attack + - Partial match when full match intended + +5. **Unicode Issues** + - Byte-based regex on UTF-8 + - Case-insensitive with Unicode + +**Common False Positives to Avoid:** + +- **Non-attacker-controlled input:** Regex matching internal/trusted data only +- **Atomic groups/possessive quantifiers:** Patterns using `(?>...)` or `++` prevent backtracking +- **Simple patterns:** Patterns without nested quantifiers or overlapping alternation +- **Timeout protection:** Regex execution has timeout/limit protection +- **Pre-validated input:** Input is sanitized before regex matching + +**Search Patterns:** +``` +regcomp\s*\(|regexec\s*\(|regex_search|regex_match +std::regex|boost::regex|pcre_ +REG_EXTENDED|REG_NEWLINE|REG_ICASE +\(\[.*\]\+\)\+|\(\.\*\)\+ # ReDoS patterns +``` diff --git a/plugins/c-review/prompts/general/smart-pointer-finder.md b/plugins/c-review/prompts/general/smart-pointer-finder.md new file mode 100644 index 0000000..451c49c --- /dev/null +++ b/plugins/c-review/prompts/general/smart-pointer-finder.md @@ -0,0 +1,49 @@ +--- +name: smart-pointer-finder +description: Detects smart pointer misuse patterns +--- + +**Finding ID Prefix:** `SPTR` (e.g., SPTR-001, SPTR-002) + +**Bug Patterns to Find:** + +1. **Circular References** + - shared_ptr cycle causing memory leak + - Parent-child with both using shared_ptr + - Observer pattern with shared_ptr + +2. **Dangling weak_ptr Issues** + - Using weak_ptr::lock() result without checking + - Storing raw pointer from weak_ptr::lock() + - Race between expired() and lock() + +3. **Ownership Problems** + - Multiple unique_ptr to same object + - shared_ptr from raw pointer to already-managed object + - Returning raw pointer from unique_ptr-managed object + +4. **Performance/Correctness** + - make_shared not used (exception safety) + - shared_ptr copy in loop (refcount overhead) + - unique_ptr where shared_ptr used + +5. **Aliasing Issues** + - shared_ptr aliasing constructor misuse + - Storing pointer to subobject that outlives parent + +**Common False Positives to Avoid:** + +- **weak_ptr used correctly:** weak_ptr breaks cycles intentionally +- **enable_shared_from_this:** Proper pattern for self-shared_ptr +- **Custom deleter:** Null deleter or custom delete is intentional +- **Aliasing for subobject:** Valid use of aliasing constructor +- **Refcount checked:** lock() result is properly checked before use + +**Search Patterns:** +``` +shared_ptr|unique_ptr|weak_ptr +make_shared|make_unique +\.get\(\)|\.release\(\) +enable_shared_from_this|shared_from_this +weak_ptr.*lock\(\) +``` diff --git a/plugins/c-review/prompts/general/time-issues-finder.md b/plugins/c-review/prompts/general/time-issues-finder.md new file mode 100644 index 0000000..4099482 --- /dev/null +++ b/plugins/c-review/prompts/general/time-issues-finder.md @@ -0,0 +1,49 @@ +--- +name: time-issues-finder +description: Identifies time-related bugs and timing attacks +--- + +**Finding ID Prefix:** `TIME` (e.g., TIME-001, TIME-002) + +**Bug Patterns to Find:** + +1. **Non-Monotonic Clocks** + - Using wall clock for duration measurement + - Time going backward breaking logic + - Clock skew between systems + +2. **Time Zone Issues** + - Local vs UTC confusion + - DST transitions breaking logic + - Midnight crossing issues + +3. **Leap Seconds** + - Assuming 86400 seconds per day + - Time comparison across leap second + +4. **Time Representation** + - 32-bit time_t (Y2038) + - Overflow in time calculations + - Loss of precision in conversion + +5. **Timing Assumptions** + - Assuming operation completes in fixed time + - Timeout calculation errors + - Sleep duration assumptions + +**Common False Positives to Avoid:** + +- **CLOCK_MONOTONIC used:** Proper monotonic clock for duration measurement +- **UTC throughout:** Code consistently uses UTC without local time confusion +- **64-bit time_t:** Modern systems with 64-bit time_t don't have Y2038 issue +- **Non-security time usage:** Logging timestamps, display purposes only +- **Explicit tolerance:** Code handles clock skew with explicit tolerance + +**Search Patterns:** +``` +time\s*\(|gettimeofday\s*\(|clock_gettime\s*\( +localtime\s*\(|gmtime\s*\(|strftime\s*\( +sleep\s*\(|usleep\s*\(|nanosleep\s*\( +difftime\s*\(|mktime\s*\( +CLOCK_MONOTONIC|CLOCK_REALTIME +``` diff --git a/plugins/c-review/prompts/general/type-confusion-finder.md b/plugins/c-review/prompts/general/type-confusion-finder.md new file mode 100644 index 0000000..2ebdd5c --- /dev/null +++ b/plugins/c-review/prompts/general/type-confusion-finder.md @@ -0,0 +1,63 @@ +--- +name: type-confusion-finder +description: Detects type confusion and unsafe casts +--- + +**Finding ID Prefix:** `TYPE` (e.g., TYPE-001, TYPE-002) + +**Bug Patterns to Find:** + +1. **Type Confusion When Casting** + - C-style casts hiding type mismatch + - reinterpret_cast to incompatible type + - static_cast of polymorphic types + - Downcasting without dynamic_cast + +2. **Type Confusion When Deserializing** + - Untrusted type tags in serialized data + - Object type determined by attacker input + - Polymorphic deserialization without validation + +3. **Pointer Dereferencing Errors** + - Pointer-to-pointer vs pointer confusion + - Wrong indirection level + - `**ptr` when `*ptr` intended + +4. **Void Pointer Misuse** + - void* cast to wrong type + - Lost type information through void* + - Callback data cast incorrectly + +5. **Union Type Safety** + - Reading wrong union member + - Type punning through unions + - Uninitialized union member access + +6. **Object Slicing** + - Derived object assigned to base value + - Loss of derived-class data + - Virtual function behavior change + +7. **Struct Size Mismatch** + - Cast to LARGER struct type than allocated buffer + - Union/struct hierarchies where some variants are larger than others + - Writing to struct members that extend past actual allocation + - Look for numbered variants (Struct vs Struct2) or size-indicating names (Large*, Extended*) + +**Common False Positives to Avoid:** + +- **Intentional type punning:** Bit manipulation, serialization where types are known +- **Tagged unions with proper checks:** If union has discriminator that's checked before access +- **void* with documented contract:** API callbacks where type is specified by design +- **C++ style casts with verification:** dynamic_cast that returns nullptr on failure (if checked) +- **Aligned memory for any type:** `alignas(max_align_t)` storage used for placement new +- **Compiler-specific type punning:** `__attribute__((may_alias))` or union-based type punning in C + +**Search Patterns:** +``` +reinterpret_cast|static_cast|dynamic_cast|\(.*\*\) +void\s*\* +union\s+\w+\s*\{ +->type|\.type|type_id|typeid +\*\*\w+|\*\s*\*\s*\w+ +``` diff --git a/plugins/c-review/prompts/general/undefined-behavior-finder.md b/plugins/c-review/prompts/general/undefined-behavior-finder.md new file mode 100644 index 0000000..e3f8ea0 --- /dev/null +++ b/plugins/c-review/prompts/general/undefined-behavior-finder.md @@ -0,0 +1,51 @@ +--- +name: undefined-behavior-finder +description: Identifies undefined behavior patterns +--- + +**Finding ID Prefix:** `UB` (e.g., UB-001, UB-002) + +**Bug Patterns to Find:** + +1. **Invalid Alignment** + - Casting pointer to misaligned type + - Packed struct access issues + - Buffer cast to stricter alignment type + +2. **Strict Aliasing Violation** + - Accessing object through wrong pointer type + - Type punning without union/memcpy + - char* to other types (usually ok, but check) + +3. **Signed Integer Overflow** + - Signed arithmetic that can overflow + - Relies on wrap-around behavior + - Compiler may optimize away overflow checks + +4. **Shift Operations** + - Shift by negative amount + - Shift by >= type width (1 << 32 on 32-bit int) + - Shifting negative values + +5. **Other Common UB** + - Multiple unsequenced modifications + - Infinite loop without side effects + - Division by zero + - Null pointer arithmetic + +**Common False Positives to Avoid:** + +- **memcpy for type punning:** Using memcpy to copy bytes between types is defined behavior +- **Union type punning:** C allows type punning through unions (C++ is stricter) +- **char* aliasing:** char/unsigned char can alias any type (standard exception) +- **Unsigned overflow:** Unsigned integers wrap around by definition (not UB) +- **Compiler extensions:** Some compilers define behavior for certain UB patterns + +**Search Patterns:** +``` +reinterpret_cast|\(\w+\s*\*\)\s*\w+ +\b<<\s*\d+|\b>>\s*\d+ +\+\+.*\+\+|--.*--|\+\+.*=.*\+\+ +\bint\b.*\+|\bint\b.*\* +__attribute__.*packed|#pragma pack +``` diff --git a/plugins/c-review/prompts/general/uninitialized-data-finder.md b/plugins/c-review/prompts/general/uninitialized-data-finder.md new file mode 100644 index 0000000..1fa7c95 --- /dev/null +++ b/plugins/c-review/prompts/general/uninitialized-data-finder.md @@ -0,0 +1,48 @@ +--- +name: uninitialized-data-finder +description: Detects use of uninitialized memory +--- + +**Finding ID Prefix:** `UNINIT` (e.g., UNINIT-001, UNINIT-002) + +**Bug Patterns to Find:** + +1. **Uninitialized Variables** + - Local variables used before assignment + - Struct members not initialized + - Array elements not set + +2. **Struct Padding Disclosure** + - Struct with padding sent over network + - Struct written to file without zeroing + - memcpy of struct leaks padding bytes + +3. **Conditional Initialization** + - Variable initialized only in some paths + - Error path skips initialization + +4. **Partial Initialization** + - Some struct members set, others not + - Array partially filled + +5. **Stack/Heap Information Disclosure** + - Returning struct with uninitialized members + - Sending buffer with uninitialized portion + +**Common False Positives to Avoid:** + +- **Compiler zero-initialization:** Static/global variables are zero-initialized by default +- **Output parameters:** Variables passed to functions that initialize them (e.g., `read()` buffer) +- **Immediately overwritten:** Variable declared then immediately assigned in next statement +- **Union active member:** Only active member matters, not all members +- **Aggregate initialization:** `struct s = {0}` zero-initializes all members including padding +- **memset before use:** If buffer is zeroed with memset before being used +- **C++ value initialization:** `Type var{}` or `Type var = Type()` zero-initializes + +**Search Patterns:** +``` +\w+\s+\w+\s*;$ # Declaration without init +struct\s+\w+\s*\{ # Struct definitions +memset\s*\(|bzero\s*\( # Initialization functions +send\s*\(|write\s*\(|fwrite\s*\( # Output functions +``` diff --git a/plugins/c-review/prompts/general/use-after-free-finder.md b/plugins/c-review/prompts/general/use-after-free-finder.md new file mode 100644 index 0000000..21a3143 --- /dev/null +++ b/plugins/c-review/prompts/general/use-after-free-finder.md @@ -0,0 +1,64 @@ +--- +name: use-after-free-finder +description: Detects use-after-free and double-free bugs +--- + +**Finding ID Prefix:** `UAF` (e.g., UAF-001, UAF-002) + +**Bug Patterns to Find:** + +1. **Classic Use-After-Free** + - Memory freed, then accessed through stale pointer + - Multiple shared_ptr to same object with incorrect refcount + +2. **Use After Scope (Dangling Pointers)** + - Heap structures storing pointers to stack variables + - Returning pointer to local variable + - Capturing local by reference in escaping lambda + +3. **Use After Return** + - `return string("").c_str()` - buffer destroyed on return + - Returning pointer to temporary object + +4. **Use After Close** + - File descriptor reused after close + - Handle accessed after release + +5. **Double Free** + - Same pointer freed twice + - Freeing in destructor and manually + +6. **Arbitrary Pointer Free** + - Freeing non-heap memory + - Freeing uninitialized pointer + +7. **Incorrect Refcounts** + - Refcount incremented incorrectly + - Object not freed when refcount hits zero + +8. **Partial Free** + - Struct field freed but struct not + - Container freed but elements not + +9. **Library Function Misuse** + - OpenSSL BN_CTX_start without BN_CTX_end + - Other allocator/deallocator mismatches + +**Common False Positives to Avoid:** + +- **Pointer reassigned before use:** If pointer is set to new allocation after free, not UAF +- **Pointer set to NULL after free:** Defensive coding; subsequent NULL check prevents use +- **Smart pointer managed lifetime:** `unique_ptr` and properly used `shared_ptr` handle lifetime +- **Pool allocators:** Object returned to pool, then same memory reused - intentional, not UAF +- **Realloc success path:** `ptr = realloc(ptr, size)` - old ptr invalid only if realloc succeeds +- **Static/global lifetime:** Pointers to static storage don't become dangling at scope exit +- **Reference counting verified:** If refcount is checked and correct, not a real UAF + +**Search Patterns:** +``` +free\s*\(|delete\s+|delete\s*\[ +shared_ptr|unique_ptr|weak_ptr +->|\.get\(\)|\.release\(\) +return.*\.c_str\(\)|return.*\.data\(\) +close\s*\(|fclose\s*\( +``` diff --git a/plugins/c-review/prompts/general/virtual-function-finder.md b/plugins/c-review/prompts/general/virtual-function-finder.md new file mode 100644 index 0000000..752199e --- /dev/null +++ b/plugins/c-review/prompts/general/virtual-function-finder.md @@ -0,0 +1,50 @@ +--- +name: virtual-function-finder +description: Finds virtual function vulnerabilities +--- + +**Finding ID Prefix:** `VIRT` (e.g., VIRT-001, VIRT-002) + +**Bug Patterns to Find:** + +1. **Missing Virtual Destructor** + - Base class with virtual methods but non-virtual destructor + - delete through base pointer leaks derived resources + - Polymorphic class without virtual destructor + +2. **Object Slicing** + - Derived object assigned to base object by value + - Base class copy/move from derived + - Vector containing Derived objects + +3. **Override Issues** + - Missing override keyword hides bug + - Signature mismatch creating new virtual + - Covariant return type issues + +4. **Virtual in Constructor/Destructor** + - Virtual call in constructor calls base version + - Pure virtual called during construction/destruction + - Dynamic type not yet established + +5. **Multiple Inheritance Diamond** + - Virtual inheritance issues + - Ambiguous base access + - Constructor order problems + +**Common False Positives to Avoid:** + +- **Final class:** Class marked final doesn't need virtual destructor +- **Non-polymorphic base:** Base without virtual functions is fine without virtual destructor +- **Intentional slicing:** Sometimes slicing is intentional (copying base portion) +- **override present:** If override keyword is there, compiler checks signature +- **CRTP pattern:** Curiously recurring template pattern intentionally non-virtual + +**Search Patterns:** +``` +virtual\s+\w+|virtual\s+~ +class\s+\w+\s*:\s*public +override|final +\(\s*\w+\s+\w+\s*\)|vector<\w+> +~\w+\s*\(\s*\)\s*[^=] +``` diff --git a/plugins/c-review/prompts/linux-userspace/eintr-handling-finder.md b/plugins/c-review/prompts/linux-userspace/eintr-handling-finder.md new file mode 100644 index 0000000..5a1b4f8 --- /dev/null +++ b/plugins/c-review/prompts/linux-userspace/eintr-handling-finder.md @@ -0,0 +1,52 @@ +--- +name: eintr-handling-finder +description: Detects missing EINTR handling +--- + +**Finding ID Prefix:** `EINTR` (e.g., EINTR-001, EINTR-002) + +**Bug Patterns to Find:** + +1. **Missing EINTR Retry** + - Most syscalls should be retried on EINTR + - `read`, `write`, `recv`, `send`, `accept`, `connect` + - `select`, `poll`, `epoll_wait` + - `waitpid`, `sem_wait`, `pthread_cond_wait` + +2. **close() Retried on EINTR** + - `close()` must NOT be retried after EINTR + - FD is already closed even if EINTR returned + - Retrying may close a different FD + +3. **Incorrect EINTR Loop** + - Not preserving partial progress + - Wrong loop termination condition + +**Correct Patterns:** + +```c +// Most syscalls - RETRY +while ((n = read(fd, buf, len)) == -1 && errno == EINTR) + ; // retry + +// close() - DO NOT RETRY +if (close(fd) == -1 && errno != EINTR) { + // handle error, but never retry +} +``` + +**Common False Positives to Avoid:** + +- **SA_RESTART set:** When SA_RESTART is used for signal handlers, most syscalls auto-restart +- **Wrapper functions:** Code may use wrappers (e.g., `safe_read`) that handle EINTR internally +- **Non-blocking I/O:** Non-blocking operations may not need EINTR handling +- **Program doesn't use signals:** If no signal handlers installed, EINTR won't occur +- **Already in retry loop:** EINTR handling may be in outer loop structure + +**Search Patterns:** +``` +read\s*\(|write\s*\(|recv\s*\(|send\s*\( +accept\s*\(|connect\s*\(|close\s*\( +select\s*\(|poll\s*\(|epoll_wait\s*\( +EINTR|while.*errno +``` diff --git a/plugins/c-review/prompts/linux-userspace/envvar-finder.md b/plugins/c-review/prompts/linux-userspace/envvar-finder.md new file mode 100644 index 0000000..647b4fe --- /dev/null +++ b/plugins/c-review/prompts/linux-userspace/envvar-finder.md @@ -0,0 +1,45 @@ +--- +name: envvar-finder +description: Identifies environment variable injection +--- + +**Finding ID Prefix:** `ENVVAR` (e.g., ENVVAR-001, ENVVAR-002) + +**Bug Patterns to Find:** + +1. **Thread Safety Issues** + - `getenv`/`setenv` not thread-safe in older glibc + - Concurrent access without synchronization + - Use secure_getenv where appropriate + +2. **Attacker-Controlled Envvars** + - Bash exported functions (Shellshock-style) + - `LIBC_FATAL_STDERR_` manipulation + - `LD_PRELOAD`, `LD_LIBRARY_PATH` in setuid + - `PATH` manipulation + +3. **Procfs Environment Leaks** + - Child process reads parent env via `/proc/$pid/environ` + - `setenv` leaves old value on stack (readable) + - Sensitive data in environment + +4. **Environment Inheritance** + - Sensitive envvars passed to child processes + - Not clearing environment before exec + +**Common False Positives to Avoid:** + +- **Non-setuid programs:** Many envvar attacks require setuid context +- **Internal configuration:** Envvars used for internal config not exposed to attackers +- **secure_getenv used:** Already using the secure version +- **Environment sanitized:** Program clears dangerous envvars at startup +- **Single-threaded:** Thread safety not a concern in single-threaded programs + +**Search Patterns:** +``` +getenv\s*\(|setenv\s*\(|putenv\s*\(|unsetenv\s*\( +secure_getenv\s*\(|clearenv\s*\( +LD_PRELOAD|LD_LIBRARY_PATH|PATH +environ\b|/proc/.*environ +execve\s*\(|execle\s*\( +``` diff --git a/plugins/c-review/prompts/linux-userspace/errno-handling-finder.md b/plugins/c-review/prompts/linux-userspace/errno-handling-finder.md new file mode 100644 index 0000000..6469fb9 --- /dev/null +++ b/plugins/c-review/prompts/linux-userspace/errno-handling-finder.md @@ -0,0 +1,45 @@ +--- +name: errno-handling-finder +description: Finds errno handling mistakes +--- + +**Finding ID Prefix:** `ERRNO` (e.g., ERRNO-001, ERRNO-002) + +**Bug Patterns to Find:** + +1. **Negative Return Values Not Handled** + - `read`/`write` can return -1 + - Network functions returning errors + - Treating negative as valid count + +2. **Partial Operations Not Handled** + - `read` may not read all requested bytes + - `write` may not write all requested bytes + - Must loop until complete or error + +3. **Functions Requiring errno Check** + - `strtoul`/`strtol` don't return error + - Must set `errno = 0` before, check after + - `atoi` has no error indication at all + +4. **Incorrect Error Comparison** + - Function returns 1 on success, code checks `!= 0` + - Function returns -1 on error, code checks `!= 1` + - Wrong error code comparison + +**Common False Positives to Avoid:** + +- **Error handled in caller:** Error propagates up and is handled at a higher level +- **Intentional ignore:** Some return values legitimately don't need checking (e.g., `printf`) +- **Wrapper function handles it:** Low-level call wrapped in function that checks +- **Loop handles partial ops:** Outer loop already handles partial read/write +- **Best-effort operations:** Some operations are intentionally fire-and-forget + +**Search Patterns:** +``` +=\s*read\s*\(|=\s*write\s*\(|=\s*recv\s*\(|=\s*send\s*\( +strtoul\s*\(|strtol\s*\(|strtod\s*\(|strtof\s*\( +atoi\s*\(|atol\s*\(|atof\s*\( +errno\s*=\s*0|if\s*\(.*errno +if\s*\(.*!=\s*0|if\s*\(.*==\s*-1 +``` diff --git a/plugins/c-review/prompts/linux-userspace/half-closed-socket-finder.md b/plugins/c-review/prompts/linux-userspace/half-closed-socket-finder.md new file mode 100644 index 0000000..213282b --- /dev/null +++ b/plugins/c-review/prompts/linux-userspace/half-closed-socket-finder.md @@ -0,0 +1,60 @@ +--- +name: half-closed-socket-finder +description: Finds half-closed socket handling issues +--- + +**Finding ID Prefix:** `HALFCLOSE` (e.g., HALFCLOSE-001, HALFCLOSE-002) + +**The Core Issue:** +`shutdown(sock, SHUT_WR)` or `shutdown(sock, SHUT_RD)` creates a half-closed socket. +This can be exploitable when: +- Remote endpoint has a bug triggered only after "connection closed" +- Data still needs to be read/written via the half-closed socket + +```c +shutdown(sock, SHUT_WR); // No more writes, but can still read +// Application may not handle this state correctly +// Attacker can exploit vulnerability window +``` + +**Bug Patterns to Find:** + +1. **Use After Partial Shutdown** + ```c + shutdown(sock, SHUT_RD); + // Later... + read(sock, buf, len); // May return unexpected results + ``` + +2. **Incomplete Shutdown Sequence** + ```c + shutdown(sock, SHUT_WR); // Send EOF to remote + // Should still drain incoming data + // But code might not handle remaining reads + ``` + +3. **Race Window After Shutdown** + ```c + shutdown(sock, SHUT_WR); + // Attacker sends data in this window + // Vulnerability in post-shutdown read handling + ``` + +4. **State Machine Confusion** + - Code expects fully closed connection + - Half-closed state not handled + +**Common False Positives to Avoid:** + +- **Intentional half-close:** Protocol requires half-close for proper shutdown sequence +- **Data drained after shutdown:** Code properly reads remaining data before close +- **No further operations:** Socket is closed immediately after shutdown +- **Well-tested protocol implementation:** Standard protocol implementations handle this +- **UDP sockets:** Half-close semantics don't apply to UDP + +**Search Patterns:** +``` +shutdown\s*\(.*SHUT_WR|shutdown\s*\(.*SHUT_RD|shutdown\s*\(.*SHUT_RDWR +shutdown\s*\(.*[12]\) # SHUT_RD=0, SHUT_WR=1, SHUT_RDWR=2 +close\s*\(.*sock|closesocket\s*\( +``` diff --git a/plugins/c-review/prompts/linux-userspace/inet-aton-finder.md b/plugins/c-review/prompts/linux-userspace/inet-aton-finder.md new file mode 100644 index 0000000..67aa391 --- /dev/null +++ b/plugins/c-review/prompts/linux-userspace/inet-aton-finder.md @@ -0,0 +1,60 @@ +--- +name: inet-aton-finder +description: Detects inet_aton/inet_addr misuse +--- + +**Finding ID Prefix:** `INETATON` (e.g., INETATON-001, INETATON-002) + +**The Core Issue:** +With glibc, `inet_aton` returns success if the string STARTS WITH a valid IP address, not if it IS a valid IP address. + +```c +inet_aton("1.1.1.1 malicious payload", &addr); // Returns 1 (success)! +inet_aton("192.168.1.1; rm -rf /", &addr); // Returns 1! +``` + +**Bug Patterns to Find:** + +1. **Using inet_aton for Validation** + ```c + if (inet_aton(user_input, &addr)) { + // Assuming user_input is a valid IP address + // But it could be "1.1.1.1 anything" + } + ``` + +2. **Security Decision Based on inet_aton** + ```c + if (inet_aton(host, &addr)) { + allow_connection(host); // host may contain extra data + } + ``` + +3. **Passing Original String After Validation** + ```c + if (inet_aton(input, &addr)) { + log("Connecting to %s", input); // Logs "1.1.1.1 malicious" + connect_to_ip(inet_ntoa(addr)); // This part is fine + } + ``` + +**Correct Approaches:** +- Use `inet_pton` (stricter parsing) +- Validate entire string is consumed +- Use the binary result, not original string + +**Common False Positives to Avoid:** + +- **Only binary result used:** If code only uses the binary addr, not original string +- **inet_pton used:** This function is stricter and doesn't have this issue +- **Additional validation:** Code validates entire string after inet_aton +- **Trusted input:** IP string comes from trusted source, not user input +- **Output only uses inet_ntoa:** Converting back to string uses clean binary + +**Search Patterns:** +``` +inet_aton\s*\( +inet_addr\s*\( # Also has issues but different +inet_pton\s*\( # This is the safer one +if\s*\(\s*inet_aton +``` diff --git a/plugins/c-review/prompts/linux-userspace/negative-retval-finder.md b/plugins/c-review/prompts/linux-userspace/negative-retval-finder.md new file mode 100644 index 0000000..233a659 --- /dev/null +++ b/plugins/c-review/prompts/linux-userspace/negative-retval-finder.md @@ -0,0 +1,52 @@ +--- +name: negative-retval-finder +description: Detects negative return value mishandling +--- + +**Finding ID Prefix:** `NEGRET` (e.g., NEGRET-001, NEGRET-002) + +**Functions That Return Negative on Error:** +- `read`, `write`, `recv`, `send` - return -1 on error +- `snprintf`, `sprintf` - return negative on error +- `open`, `socket`, `accept` - return -1 on error + +**Bug Patterns to Find:** + +1. **Negative Used as Size** + ```c + ssize_t n = read(fd, buf, len); + memcpy(dst, buf, n); // If n = -1, this is huge! + ``` + +2. **Negative Used as Index** + ```c + int idx = find_index(...); + array[idx] = value; // If idx = -1, underflow! + ``` + +3. **Negative Cast to Unsigned** + ```c + size_t len = read(fd, buf, size); // -1 becomes SIZE_MAX + ``` + +4. **Comparison After Assignment** + ```c + size_t n = read(...); // Implicit conversion + if (n == -1) {} // Never true! SIZE_MAX != -1 + ``` + +**Common False Positives to Avoid:** + +- **Error checked before use:** Code checks `if (n < 0)` or `if (n == -1)` before using value +- **Signed variable keeps signedness:** `ssize_t n = read(...)` preserves error detection +- **Wrapper handles errors:** Error checking done in wrapper function +- **Intentional sentinel:** -1 used intentionally as "not found" with proper handling +- **Immediately returned:** Error value passed up to caller who handles it + +**Search Patterns:** +``` +=\s*read\s*\(|=\s*write\s*\(|=\s*recv\s*\(|=\s*send\s*\( +size_t.*=.*read|size_t.*=.*write +memcpy.*,\s*\w+\)|memset.*,\s*\w+\) +\[\s*\w+\s*\].*= +``` diff --git a/plugins/c-review/prompts/linux-userspace/null-zero-finder.md b/plugins/c-review/prompts/linux-userspace/null-zero-finder.md new file mode 100644 index 0000000..28b9c95 --- /dev/null +++ b/plugins/c-review/prompts/linux-userspace/null-zero-finder.md @@ -0,0 +1,66 @@ +--- +name: null-zero-finder +description: Finds NULL vs zero confusion +--- + +**Finding ID Prefix:** `NULLZERO` (e.g., NULLZERO-001, NULLZERO-002) + +**The Core Issue:** +While `0` and `NULL` are often equivalent, using `0` in pointer contexts can cause issues: +- In variadic functions, `0` may not have pointer type +- On platforms where null pointer isn't all-bits-zero +- Code clarity and intent + +```c +// Problematic +execl("/bin/sh", "sh", "-c", cmd, 0); // 0 might not be null pointer + +// Correct +execl("/bin/sh", "sh", "-c", cmd, (char *)NULL); +// Or in C++: +execl("/bin/sh", "sh", "-c", cmd, nullptr); +``` + +**Bug Patterns to Find:** + +1. **Variadic Function Terminator** + ```c + execl(path, arg0, arg1, 0); // Should be (char *)NULL + execlp(file, arg0, 0); // Should be (char *)NULL + ``` + +2. **Pointer Assignment** + ```c + char *ptr = 0; // Works but unclear, use NULL + ``` + +3. **Pointer Comparison** + ```c + if (ptr == 0) // Works but unclear, use NULL + ``` + +4. **Function Pointer** + ```c + void (*fp)(void) = 0; // Should be NULL + ``` + +**Where It Actually Matters:** +- Variadic functions (exec family, etc.) - compiler doesn't know to convert 0 to pointer +- Some embedded platforms where NULL isn't 0 +- Code clarity and static analysis + +**Common False Positives to Avoid:** + +- **Non-variadic context:** In regular function calls, compiler converts 0 to null pointer +- **C++ nullptr used:** Modern C++ code using nullptr is correct +- **Explicit cast present:** `(char *)0` is equivalent to `(char *)NULL` +- **Integer context:** 0 used in integer context (not pointer) +- **Style preference:** Some codebases consistently use 0 for null with understanding + +**Search Patterns:** +``` +exec[lv]p?\s*\([^)]*,\s*0\s*\) +,\s*0\s*\)\s*; # 0 as last argument +\*\s*\w+\s*=\s*0\s*; # Pointer = 0 +==\s*0\s*[^0-9]|!=\s*0\s*[^0-9] # Comparison to 0 +``` diff --git a/plugins/c-review/prompts/linux-userspace/oob-comparison-finder.md b/plugins/c-review/prompts/linux-userspace/oob-comparison-finder.md new file mode 100644 index 0000000..5f1a042 --- /dev/null +++ b/plugins/c-review/prompts/linux-userspace/oob-comparison-finder.md @@ -0,0 +1,42 @@ +--- +name: oob-comparison-finder +description: Detects out-of-bounds comparison bugs +--- + +**Finding ID Prefix:** `OOBCMP` (e.g., OOBCMP-001, OOBCMP-002) + +**Bug Patterns to Find:** + +1. **std::equal with Unequal Lengths** + - Three-iterator form: `std::equal(a.begin(), a.end(), b.begin())` + - Reads from b even if b is shorter + - Use four-iterator form or check sizes first + +2. **memcmp Size Errors** + - Size larger than smaller buffer + - Size from wrong buffer + - Unchecked size parameter + +3. **strncmp Issues** + - Size larger than shorter string + - Comparing with size from wrong source + - Not checking string length first + +4. **bcmp Issues** + - Same problems as memcmp + - Deprecated but still used + +**Common False Positives to Avoid:** + +- **Sizes validated first:** Code checks buffer sizes before comparison +- **Equal-sized buffers:** Both buffers are known to be at least comparison size +- **Four-iterator std::equal:** `std::equal(a.begin(), a.end(), b.begin(), b.end())` is safe +- **Compile-time known sizes:** Buffers are fixed-size arrays with known dimensions +- **Size comes from smaller buffer:** Comparison size derived from minimum of both sizes + +**Search Patterns:** +``` +std::equal\s*\(|memcmp\s*\(|strncmp\s*\(|bcmp\s*\( +wcsncmp\s*\(|wmemcmp\s*\( +\.begin\(\).*\.begin\(\) # two begin() calls; verify via Read that both ends are properly bounded +``` diff --git a/plugins/c-review/prompts/linux-userspace/open-issues-finder.md b/plugins/c-review/prompts/linux-userspace/open-issues-finder.md new file mode 100644 index 0000000..bdd840f --- /dev/null +++ b/plugins/c-review/prompts/linux-userspace/open-issues-finder.md @@ -0,0 +1,49 @@ +--- +name: open-issues-finder +description: Identifies file open vulnerabilities +--- + +**Finding ID Prefix:** `FILEOP` (e.g., FILEOP-001, FILEOP-002) + +**Bug Patterns to Find:** + +1. **access() + open() TOCTOU** + - `access(path, ...)` then `open(path, ...)` + - Symlink race between check and open + - Use `faccessat` with proper flags or just open and check + +2. **rename() Race Conditions** + - Attacker control over destination + - Race between check and rename + - Use `renameat2` with `RENAME_NOREPLACE` + +3. **O_NOFOLLOW Issues** + - `O_NOFOLLOW` still follows directory symlinks + - Use `O_NOFOLLOW_ANY` (Linux 5.1+) or `openat2` + - Or resolve path component by component + +4. **Missing O_CLOEXEC** + - File descriptors leak to child processes + - Security-sensitive FDs inherited + - Always use `O_CLOEXEC` or `fcntl(F_SETFD, FD_CLOEXEC)` + +5. **Unsafe Path Operations** + - Using `realpath` without `O_NOFOLLOW` + - Trusting resolved paths + +**Common False Positives to Avoid:** + +- **Fully controlled paths:** File path is hardcoded or derived from trusted sources +- **Non-writable directories:** Directory is not writable by attacker (e.g., /etc on non-root) +- **openat used correctly:** Modern openat() with proper directory FD and flags +- **Single-user context:** No privilege difference, attacker gains nothing +- **O_CLOEXEC set elsewhere:** fcntl() called to set FD_CLOEXEC immediately after open + +**Search Patterns:** +``` +access\s*\(|faccessat\s*\( +open\s*\(|openat\s*\(|fopen\s*\( +rename\s*\(|renameat\s*\( +O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY +realpath\s*\(|readlink\s*\( +``` diff --git a/plugins/c-review/prompts/linux-userspace/printf-attr-finder.md b/plugins/c-review/prompts/linux-userspace/printf-attr-finder.md new file mode 100644 index 0000000..f77dd78 --- /dev/null +++ b/plugins/c-review/prompts/linux-userspace/printf-attr-finder.md @@ -0,0 +1,66 @@ +--- +name: printf-attr-finder +description: Finds missing printf format attributes +--- + +**Finding ID Prefix:** `PRINTFATTR` (e.g., PRINTFATTR-001, PRINTFATTR-002) + +**The Core Issue:** +Custom printf-like functions should have `__attribute__((format(printf, ...)))` so the compiler can check format strings against arguments. + +```c +// Dangerous - compiler can't check format strings +void log_error(const char *fmt, ...) { + // Forwards to vfprintf +} +log_error("%s %d", ptr); // Type mismatch not caught! + +// Safe - compiler checks format strings +__attribute__((format(printf, 1, 2))) +void log_error(const char *fmt, ...) { + // Forwards to vfprintf +} +log_error("%s %d", ptr); // Compiler warning! +``` + +**Bug Patterns to Find:** + +1. **Wrapper Functions Without Attribute** + ```c + void debug_print(const char *fmt, ...) { + va_list args; + va_start(args, fmt); + vprintf(fmt, args); + va_end(args); + } + ``` + +2. **Logging Functions Without Attribute** + ```c + void log_message(int level, const char *fmt, ...) { + // Uses vfprintf or similar + } + ``` + +3. **Error Handling Functions** + ```c + void die(const char *fmt, ...) { + // Prints error and exits + } + ``` + +**Common False Positives to Avoid:** + +- **Attribute already present:** Function already has `__attribute__((format(printf, ...)))` +- **Not a printf wrapper:** Function doesn't forward to printf family +- **Macro wrapper:** Format checking done through macro that expands to attributed function +- **Fixed format:** Function takes fixed format, not user-supplied +- **Type-safe wrapper:** C++ variadic template that's type-safe + +**Search Patterns:** +``` +\.\.\.\s*\)|va_list|va_start|va_end +vprintf|vfprintf|vsprintf|vsnprintf|vsyslog +__attribute__.*format.*printf +void\s+\w+\s*\([^)]*const\s+char\s*\*[^)]*\.\.\.\s*\) +``` diff --git a/plugins/c-review/prompts/linux-userspace/privilege-drop-finder.md b/plugins/c-review/prompts/linux-userspace/privilege-drop-finder.md new file mode 100644 index 0000000..c8d6962 --- /dev/null +++ b/plugins/c-review/prompts/linux-userspace/privilege-drop-finder.md @@ -0,0 +1,54 @@ +--- +name: privilege-drop-finder +description: Detects privilege dropping mistakes +--- + +**Finding ID Prefix:** `PRIVDROP` (e.g., PRIVDROP-001, PRIVDROP-002) + +**Bug Patterns to Find:** + +1. **Unchecked Return Values** + - `setuid(uid)` return not checked + - `setgid(gid)` return not checked + - Can fail silently, leaving privileges + +2. **Incomplete Privilege Drop** + - `seteuid(X)` followed by `setuid(X)` may not drop permanently + - Saved-set-user-ID not cleared + - Use `setresuid(uid, uid, uid)` for complete drop + +3. **Wrong Order** + - User privileges dropped before group + - Once user privileges dropped, can't change group + - Drop group privileges first, then user + +4. **Missing Verification** + - Privileges not verified after dropping + - Should call `getuid()/geteuid()` to confirm + - Check `getgroups()` for supplementary groups + +5. **Inherited Resources** + - File descriptors preserved across exec + - ioperm permissions preserved + - Capabilities inheritance complexity + +6. **vfork Caveats** + - Different privileges in same address space + - Child can corrupt parent state + +**Common False Positives to Avoid:** + +- **Return values checked:** Code checks return value and handles failure +- **setresuid used:** Using setresuid(uid, uid, uid) for complete drop +- **Correct order:** Group dropped before user +- **Verification present:** Code verifies privileges after dropping +- **Non-privileged program:** Program doesn't run with elevated privileges + +**Search Patterns:** +``` +setuid\s*\(|setgid\s*\(|seteuid\s*\(|setegid\s*\( +setresuid\s*\(|setresgid\s*\(|setgroups\s*\( +getuid\s*\(|geteuid\s*\(|getgid\s*\(|getegid\s*\( +initgroups\s*\(|setgroups\s*\( +cap_set_proc|prctl\s*\(.*CAP +``` diff --git a/plugins/c-review/prompts/linux-userspace/qsort-finder.md b/plugins/c-review/prompts/linux-userspace/qsort-finder.md new file mode 100644 index 0000000..1f5ecbb --- /dev/null +++ b/plugins/c-review/prompts/linux-userspace/qsort-finder.md @@ -0,0 +1,57 @@ +--- +name: qsort-finder +description: Identifies qsort comparison function bugs +--- + +**Finding ID Prefix:** `QSORT` (e.g., QSORT-001, QSORT-002) + +**The Core Issue:** +glibc's `qsort` with a non-transitive comparison function can cause out-of-bounds access. +This is a real vulnerability class (see Qualys advisory 2024). + +**Non-Transitive Comparator:** +A comparator is non-transitive if: `a < b` and `b < c` doesn't imply `a < c` + +```c +int bad_compare(const void *a, const void *b) { + // Compares only first byte, ignoring rest + return *(char*)a - *(char*)b; +} +// If structures differ only in later bytes, ordering is unstable +``` + +**Bug Patterns to Find:** + +1. **Partial Key Comparison** + - Only comparing part of the structure + - Inconsistent comparison logic + +2. **Floating Point Comparison** + - NaN breaks transitivity + - `a - b` doesn't handle special values + +3. **Integer Overflow in Comparison** + ```c + int compare(const void *a, const void *b) { + return *(int*)a - *(int*)b; // Can overflow! + } + ``` + +4. **Multiple Sort Keys Without Proper Chaining** + - First key doesn't distinguish, second key not checked + +**Common False Positives to Avoid:** + +- **Three-way comparison used:** `(x > y) - (x < y)` pattern is safe +- **Full structure comparison:** All relevant fields are compared +- **Small value range:** Values can't cause overflow (e.g., chars, booleans) +- **NaN explicitly handled:** Floating point comparator handles NaN case +- **Stable sort with unique keys:** Primary key is unique, no transitivity issue + +**Search Patterns:** +``` +qsort\s*\(|qsort_r\s*\( +bsearch\s*\( +int\s+\w+\s*\(.*const\s+void\s*\*.*const\s+void\s*\* +return.*-\s*\*.*\(int\s*\*\) +``` diff --git a/plugins/c-review/prompts/linux-userspace/signal-handler-finder.md b/plugins/c-review/prompts/linux-userspace/signal-handler-finder.md new file mode 100644 index 0000000..d256772 --- /dev/null +++ b/plugins/c-review/prompts/linux-userspace/signal-handler-finder.md @@ -0,0 +1,47 @@ +--- +name: signal-handler-finder +description: Finds async-signal-unsafe function calls +--- + +**Finding ID Prefix:** `SIGNAL` (e.g., SIGNAL-001, SIGNAL-002) + +**Bug Patterns to Find (async-signal-unsafe operations):** + +1. **Memory Allocation** + - `malloc`, `free`, `realloc`, `calloc` + - `new`, `delete` + +2. **Standard I/O** + - `printf`, `fprintf`, `sprintf` + - `fopen`, `fclose`, `fread`, `fwrite` + +3. **Other Unsafe Functions** + - `strtok`, `strerror` + - `getpwnam`, `getgrnam` + - `localtime`, `gmtime` + +4. **errno Modification** + - Any function that sets errno + - errno not saved/restored in handler + +**Safe Functions (async-signal-safe):** +- `write`, `read` (raw syscalls) +- `_exit`, `abort` +- `signal`, `sigaction` (careful) +- `open`, `close` (file descriptors) + +**Common False Positives to Avoid:** + +- **Handler only sets flag:** Handler just sets `volatile sig_atomic_t` flag +- **Self-pipe trick:** Handler writes to pipe, processing done elsewhere +- **signalfd used:** Using signalfd for synchronous signal handling +- **Signals blocked:** Unsafe code runs with signals blocked +- **errno saved/restored:** Handler properly saves and restores errno + +**Search Patterns:** +``` +signal\s*\(|sigaction\s*\(|sighandler_t +SIG[A-Z]+\s*,|SIGINT|SIGTERM|SIGHUP|SIGUSR +malloc\s*\(|free\s*\(|printf\s*\(|fprintf\s*\( +errno\s*=|errno\s*$ +``` diff --git a/plugins/c-review/prompts/linux-userspace/socket-disconnect-finder.md b/plugins/c-review/prompts/linux-userspace/socket-disconnect-finder.md new file mode 100644 index 0000000..0c3bef6 --- /dev/null +++ b/plugins/c-review/prompts/linux-userspace/socket-disconnect-finder.md @@ -0,0 +1,55 @@ +--- +name: socket-disconnect-finder +description: Identifies socket disconnect handling issues +--- + +**Finding ID Prefix:** `SOCKDISCON` (e.g., SOCKDISCON-001, SOCKDISCON-002) + +**The Core Issue:** +`connect(sock, AF_UNSPEC)` can disconnect an already-connected TCP socket. +The socket can then be reconnected to a different address. + +```c +// sock is connected to legitimate server +struct sockaddr sa = { .sa_family = AF_UNSPEC }; +connect(sock, &sa, sizeof(sa)); // Disconnects! +// sock can now be reconnected to attacker server +``` + +This has been used for nsjail escapes and other sandbox bypasses. + +**Bug Patterns to Find:** + +1. **Attacker Control Over connect() Arguments** + ```c + connect(sock, user_provided_addr, len); + // If user can set sa_family = AF_UNSPEC, they can disconnect + ``` + +2. **Socket Reuse After Error** + ```c + if (connect(sock, addr1, len) < 0) { + // Error path - socket might be disconnected + connect(sock, addr2, len); // Reconnecting + } + ``` + +3. **UDP Socket Address Override** + - UDP sockets can have default destination changed + - AF_UNSPEC removes the default destination + +**Common False Positives to Avoid:** + +- **Address family validated:** Code checks `addr->sa_family` before passing to connect +- **Trusted address source:** Address structure comes from trusted internal source, not user input +- **Sandbox already restricts connect:** Seccomp or other sandbox limits connect syscall +- **Socket not reused:** Socket is closed and recreated rather than reconnected +- **Intentional disconnect:** Code deliberately uses AF_UNSPEC to reset socket state + +**Search Patterns:** +``` +connect\s*\( +AF_UNSPEC +sockaddr.*sa_family +bind\s*\(|listen\s*\(|accept\s*\( +``` diff --git a/plugins/c-review/prompts/linux-userspace/spinlock-init-finder.md b/plugins/c-review/prompts/linux-userspace/spinlock-init-finder.md new file mode 100644 index 0000000..bd0ed11 --- /dev/null +++ b/plugins/c-review/prompts/linux-userspace/spinlock-init-finder.md @@ -0,0 +1,53 @@ +--- +name: spinlock-init-finder +description: Detects spinlock initialization bugs +--- + +**Finding ID Prefix:** `SPINLOCK` (e.g., SPINLOCK-001, SPINLOCK-002) + +**The Core Issue:** +Using `pthread_spin_trylock` (or any spinlock operation) on an uninitialized spinlock is undefined behavior and can cause deadlock or corruption. + +**Bug Patterns to Find:** + +1. **Missing pthread_spin_init** + ```c + pthread_spinlock_t lock; // Declared but not initialized + pthread_spin_lock(&lock); // UB! + ``` + +2. **Conditional Initialization** + ```c + if (condition) { + pthread_spin_init(&lock, PTHREAD_PROCESS_PRIVATE); + } + pthread_spin_lock(&lock); // May not be initialized + ``` + +3. **Use Before Init in Constructor Order** + - Static spinlock used before static init runs + - Similar to static initialization order fiasco + +4. **Error Path Skips Init** + ```c + if (pthread_spin_init(&lock, 0) != 0) { + // Error but continues + } + pthread_spin_lock(&lock); // May not be initialized + ``` + +**Common False Positives to Avoid:** + +- **Static zero initialization:** Static/global spinlocks are zero-initialized (may be valid on some platforms) +- **Init verified before use:** Code checks return value of pthread_spin_init and handles failure +- **Init in constructor:** C++ class initializes spinlock in constructor, use in methods +- **PTHREAD_SPINLOCK_INITIALIZER:** Static initializer macro used (if available) +- **Wrapper function initializes:** Spinlock is initialized in a wrapper/factory function + +**Search Patterns:** +``` +pthread_spinlock_t\s+\w+ +pthread_spin_init\s*\(|pthread_spin_destroy\s*\( +pthread_spin_lock\s*\(|pthread_spin_unlock\s*\( +pthread_spin_trylock\s*\( +``` diff --git a/plugins/c-review/prompts/linux-userspace/thread-safety-finder.md b/plugins/c-review/prompts/linux-userspace/thread-safety-finder.md new file mode 100644 index 0000000..d87ad6d --- /dev/null +++ b/plugins/c-review/prompts/linux-userspace/thread-safety-finder.md @@ -0,0 +1,54 @@ +--- +name: thread-safety-finder +description: Detects thread safety and concurrency bugs +--- + +**Finding ID Prefix:** `THREAD` (e.g., THREAD-001, THREAD-002) + +**Bug Patterns to Find (non-thread-safe functions):** + +1. **Network Functions** + - `gethostbyname` - Returns static struct + - `gethostbyaddr` - Returns static struct + - `inet_ntoa` - Returns static buffer + +2. **String Functions** + - `strtok` - Uses static state + - `strerror` - May use static buffer + +3. **Time Functions** + - `localtime` - Returns static struct + - `gmtime` - Returns static struct + - `ctime` - Returns static buffer + - `asctime` - Returns static buffer + +4. **User/Group Functions** + - `getpwnam` / `getpwuid` - Return static struct + - `getgrnam` / `getgrgid` - Return static struct + +5. **Other Dangerous Functions** + - `readdir` - Returns static struct + - `getenv` / `setenv` - Not thread-safe (glibc improved recently) + +**Thread-Safe Alternatives:** +- `gethostbyname_r`, `inet_ntop`, `strtok_r` +- `localtime_r`, `gmtime_r`, `ctime_r`, `asctime_r` +- `getpwnam_r`, `getpwuid_r`, `getgrnam_r`, `getgrgid_r` +- `readdir_r` (deprecated but thread-safe) + +**Common False Positives to Avoid:** + +- **Single-threaded code:** Program doesn't use pthreads, std::thread, or fork +- **Result used immediately:** Static result is copied/used before any yield point +- **Thread-local storage:** Function result stored in thread-local variable +- **Mutex protected:** Call is protected by mutex that serializes access +- **_r variant used:** Code actually uses the thread-safe _r variant + +**Search Patterns:** +``` +pthread_create|std::thread|fork\s*\(\s*\) +gethostbyname\s*\(|inet_ntoa\s*\(|\bstrtok\s*\( +\blocaltime\s*\(|\bgmtime\s*\(|\bctime\s*\( +\bgetpwnam\s*\(|\bgetpwuid\s*\( +getenv\s*\(|setenv\s*\( +``` diff --git a/plugins/c-review/prompts/linux-userspace/va-start-end-finder.md b/plugins/c-review/prompts/linux-userspace/va-start-end-finder.md new file mode 100644 index 0000000..325d171 --- /dev/null +++ b/plugins/c-review/prompts/linux-userspace/va-start-end-finder.md @@ -0,0 +1,69 @@ +--- +name: va-start-end-finder +description: Detects va_start/va_end misuse +--- + +**Finding ID Prefix:** `VAARG` (e.g., VAARG-001, VAARG-002) + +**The Core Issue:** +Every `va_start()` must have a corresponding `va_end()` before the function returns. +Missing `va_end()` is undefined behavior and may corrupt stack on some platforms. + +```c +void bad_func(const char *fmt, ...) { + va_list ap; + va_start(ap, fmt); + if (error) { + return; // Missing va_end! + } + vprintf(fmt, ap); + va_end(ap); // Only reached on success path +} +``` + +**Bug Patterns to Find:** + +1. **Early Return Without va_end** + ```c + va_start(ap, fmt); + if (check_fails) { + return; // va_end not called! + } + va_end(ap); + ``` + +2. **Exception Path Missing va_end (C++)** + ```c + va_start(ap, fmt); + may_throw(); // If throws, va_end skipped + va_end(ap); + ``` + +3. **Multiple va_start Without Matching va_end** + ```c + va_start(ap, fmt); + va_start(ap, fmt); // Second start without end + va_end(ap); + ``` + +4. **va_copy Without va_end** + ```c + va_copy(ap2, ap); // Creates new va_list + // Missing va_end(ap2) + ``` + +**Common False Positives to Avoid:** + +- **va_end on all paths:** All return paths call va_end before returning +- **goto cleanup pattern:** Code uses goto to centralized cleanup that calls va_end +- **RAII wrapper (C++):** C++ code uses RAII class that calls va_end in destructor +- **noreturn function:** Early exit is via noreturn function (abort, _exit) +- **va_copy properly paired:** Both original and copied va_list have matching va_end + +**Search Patterns:** +``` +va_start\s*\(|va_end\s*\(|va_copy\s*\( +va_list\s+\w+ +return\s*;|return\s+\w+; +throw\s+|goto\s+ +``` diff --git a/plugins/c-review/prompts/windows-userspace/createprocess-finder.md b/plugins/c-review/prompts/windows-userspace/createprocess-finder.md new file mode 100644 index 0000000..cdaa80a --- /dev/null +++ b/plugins/c-review/prompts/windows-userspace/createprocess-finder.md @@ -0,0 +1,45 @@ +--- +name: createprocess-finder +description: Identifies CreateProcess security issues +--- + +**Finding ID Prefix:** `CREATEPROC` (e.g., CREATEPROC-001, CREATEPROC-002) + +**Bug Patterns to Find:** + +1. **Unquoted Paths with Spaces** + - `lpApplicationName` is NULL and `lpCommandLine` has unquoted path with spaces + - `C:\Program Files\App\run.exe` tries `C:\Program.exe` first + - Subdirectory spaces also vulnerable: `C:\App\Some Dir\run.exe` + +2. **Handle Inheritance Leaks** + - `bInheritHandles` is TRUE when creating lower-privilege process + - Sensitive handles (files, tokens, pipes) leak to child + +3. **Console Sharing** + - Missing `DETACHED_PROCESS` or `CREATE_NEW_CONSOLE` + - Lower-privilege child shares stdin/stdout/stderr with parent + +4. **Dangerous Flags** + - `CREATE_PRESERVE_CODE_AUTHZ_LEVEL` bypasses AppLocker/SRP + - `CREATE_BREAKAWAY_FROM_JOB` escapes job sandbox + +5. **Batch File Execution** + - `.cmd`/`.bat` without full path to cmd.exe + - Vulnerable to cmd.exe planting on old systems + +**Common False Positives to Avoid:** + +- **Quoted paths:** `"C:\Program Files\App\run.exe"` is safe +- **lpApplicationName specified:** Full path in first parameter +- **Same privilege level:** Handle inheritance between same-privilege processes +- **No sensitive handles:** Process has no inheritable sensitive handles + +**Search Patterns:** +``` +CreateProcess[AW]?\s*\(|CreateProcessAsUser[AW]?\s*\( +ShellExecute[AW]?\s*\(|ShellExecuteEx[AW]?\s*\( +SHCreateProcessAsUser[AW]?\s*\( +bInheritHandles|CREATE_BREAKAWAY_FROM_JOB +CREATE_PRESERVE_CODE_AUTHZ_LEVEL|DETACHED_PROCESS +``` diff --git a/plugins/c-review/prompts/windows-userspace/cross-process-finder.md b/plugins/c-review/prompts/windows-userspace/cross-process-finder.md new file mode 100644 index 0000000..85fa7c6 --- /dev/null +++ b/plugins/c-review/prompts/windows-userspace/cross-process-finder.md @@ -0,0 +1,48 @@ +--- +name: cross-process-finder +description: Detects cross-process memory vulnerabilities +--- + +**Finding ID Prefix:** `CROSSPROC` (e.g., CROSSPROC-001, CROSSPROC-002) + +**Bug Patterns to Find:** + +1. **Arbitrary Write Primitives** + - `WriteProcessMemory` with user-controlled address + - `VirtualAllocEx` at fixed address (ASLR collision) + - Unvalidated target process handle + +2. **Information Disclosure** + - `ReadProcessMemory` data not validated + - Uninitialized source buffer in `WriteProcessMemory` + - Address/handle leaks to lower-privilege process + +3. **Remote Thread Injection** + - `CreateRemoteThread` without proper authorization checks + - Thread start address from untrusted source + - DLL injection without signature verification + +4. **Repeated Injection Issues** + - No deduplication causing memory exhaustion (DoS) + - Heap spray gadget via repeated allocations + - ROP spray via executable page allocation + +5. **Sensitive Data Exposure** + - Credentials written to lower-privilege process + - Tokens or handles shared cross-process + - Memory not cleared before cross-process operations + +**Common False Positives to Avoid:** + +- **Same process context:** Operations within same process +- **Debugger/security tool:** Legitimate debugging or security monitoring +- **Higher to lower privilege:** Already established privilege relationship +- **Validated target:** Process handle properly validated + +**Search Patterns:** +``` +VirtualAllocEx\s*\(|VirtualProtectEx\s*\(|VirtualFreeEx\s*\( +WriteProcessMemory\s*\(|ReadProcessMemory\s*\( +CreateRemoteThread\s*\(|NtCreateThreadEx\s*\( +OpenProcess\s*\(.*PROCESS_VM|PROCESS_ALL_ACCESS +``` diff --git a/plugins/c-review/prompts/windows-userspace/dll-planting-finder.md b/plugins/c-review/prompts/windows-userspace/dll-planting-finder.md new file mode 100644 index 0000000..13cbcd2 --- /dev/null +++ b/plugins/c-review/prompts/windows-userspace/dll-planting-finder.md @@ -0,0 +1,42 @@ +--- +name: dll-planting-finder +description: Finds DLL hijacking/planting vulnerabilities +--- + +**Finding ID Prefix:** `DLLPLANT` (e.g., DLLPLANT-001, DLLPLANT-002) + +**Bug Patterns to Find:** + +1. **LoadLibrary Without Full Path** + - `LoadLibrary("foo.dll")` without absolute path + - Path from untrusted source (registry, config, CWD) + - Missing `LOAD_LIBRARY_SEARCH_SYSTEM32` flag + +2. **Missing Signature Verification** + - `LoadLibrary` without `LOAD_LIBRARY_REQUIRE_SIGNED_TARGET` + - Manual signature check before load (TOCTOU) + +3. **Implicit DLL Loads** + - Delay-loaded DLLs that may not exist + - Localization DLLs (mui, resources) + - COM DLLs loaded by CLSID + +4. **Search Order Hijacking** + - Application directory writable by low-privilege user + - PATH directories writable + - Missing SafeDllSearchMode + +**Common False Positives to Avoid:** + +- **System DLLs with full path:** `LoadLibrary("C:\\Windows\\System32\\kernel32.dll")` +- **LOAD_LIBRARY_SEARCH_SYSTEM32 used:** Restricts search to System32 +- **Protected directory:** Application installed in Program Files with proper ACLs +- **Signature required:** `LOAD_LIBRARY_REQUIRE_SIGNED_TARGET` flag used + +**Search Patterns:** +``` +LoadLibrary[AW]?\s*\(|LoadLibraryEx[AW]?\s*\( +LOAD_LIBRARY_SEARCH|LOAD_LIBRARY_REQUIRE_SIGNED +GetModuleHandle[AW]?\s*\(.*NULL +delay.?load|__delayLoadHelper +``` diff --git a/plugins/c-review/prompts/windows-userspace/installer-race-finder.md b/plugins/c-review/prompts/windows-userspace/installer-race-finder.md new file mode 100644 index 0000000..aef1deb --- /dev/null +++ b/plugins/c-review/prompts/windows-userspace/installer-race-finder.md @@ -0,0 +1,49 @@ +--- +name: installer-race-finder +description: Detects installer race conditions +--- + +**Finding ID Prefix:** `INSTRACE` (e.g., INSTRACE-001, INSTRACE-002) + +**Bug Patterns to Find:** + +1. **Temp File Race Conditions** + - Extract to temp, then copy to final location + - Predictable temp file names + - Missing exclusive file access + +2. **MSI Rollback Exploitation** + - Arbitrary file deletion → privilege escalation + - Rollback restores attacker-controlled file + - Custom actions during rollback + +3. **Symlink/Junction Attacks** + - Temp directory junction to system directory + - Extracted file overwrites system file + - Missing directory validation + +4. **Permission Window** + - File extracted with weak permissions + - Permissions tightened later (TOCTOU) + - Attacker modifies file between operations + +5. **Signature Verification TOCTOU** + - Signature checked on temp file + - Different file copied to final location + - Missing re-verification after copy + +**Common False Positives to Avoid:** + +- **Atomic operations:** Single operation extracts and sets permissions +- **Locked directory:** Temp directory only accessible by SYSTEM +- **Exclusive access:** File opened with exclusive sharing +- **No privilege boundary:** Installer runs at same privilege as user + +**Search Patterns:** +``` +GetTempPath|GetTempFileName|CreateFile.*GENERIC_WRITE +MoveFile|CopyFile|DeleteFile.*temp +SetFileSecurity|SetSecurityInfo|SetNamedSecurityInfo +MsiInstall|MsiOpenPackage|MsiDoAction +INSTALLSTATE_|MsiSetProperty|MsiGetProperty +``` diff --git a/plugins/c-review/prompts/windows-userspace/named-pipe-finder.md b/plugins/c-review/prompts/windows-userspace/named-pipe-finder.md new file mode 100644 index 0000000..59e8812 --- /dev/null +++ b/plugins/c-review/prompts/windows-userspace/named-pipe-finder.md @@ -0,0 +1,47 @@ +--- +name: named-pipe-finder +description: Identifies named pipe security issues +--- + +**Finding ID Prefix:** `NAMEDPIPE` (e.g., NAMEDPIPE-001, NAMEDPIPE-002) + +**Bug Patterns to Find:** + +1. **Missing Security Descriptor** + - `lpSecurityAttributes` is NULL + - Default DACL allows Everyone access + - No explicit ACL on pipe + +2. **Remote Access Enabled** + - Missing `PIPE_REJECT_REMOTE_CLIENTS` flag + - Pipe accessible over network + +3. **Single Instance DoS** + - `nMaxInstances` is 1 + - Malicious process can claim pipe first + - Legitimate client blocked + +4. **Impersonation Without Verification** + - `ImpersonateNamedPipeClient` without checking client identity + - Privilege escalation via token impersonation + +5. **Data Validation** + - Untrusted data from pipe not validated + - Deserialization of pipe data + - Command injection via pipe input + +**Common False Positives to Avoid:** + +- **Explicit restrictive DACL:** Security descriptor properly configured +- **PIPE_REJECT_REMOTE_CLIENTS set:** Remote access blocked +- **High nMaxInstances:** Multiple instances prevent DoS +- **Server-side only:** Pipe used only for server-to-client communication + +**Search Patterns:** +``` +CreateNamedPipe[AW]?\s*\(|CallNamedPipe[AW]?\s*\( +\\\\\\\\.\\\\pipe\\\\|\\\\\\?\\\\pipe\\\\ +PIPE_REJECT_REMOTE_CLIENTS|PIPE_ACCESS +ImpersonateNamedPipeClient|RevertToSelf +lpSecurityAttributes|SECURITY_ATTRIBUTES +``` diff --git a/plugins/c-review/prompts/windows-userspace/service-security-finder.md b/plugins/c-review/prompts/windows-userspace/service-security-finder.md new file mode 100644 index 0000000..93ca523 --- /dev/null +++ b/plugins/c-review/prompts/windows-userspace/service-security-finder.md @@ -0,0 +1,50 @@ +--- +name: service-security-finder +description: Finds Windows service security problems +--- + +**Finding ID Prefix:** `WINSVC` (e.g., WINSVC-001, WINSVC-002) + +**Bug Patterns to Find:** + +1. **Excessive Service Privileges** + - Service running as `SYSTEM` unnecessarily + - Should use `LOCAL SERVICE` or `NETWORK SERVICE` + - Missing service account restrictions + +2. **Binary Path Vulnerabilities** + - Unquoted service path with spaces + - Service binary in writable directory + - Parent directory writable (DLL planting) + +3. **Registry ACL Issues** + - Service registry key writable by users + - `ImagePath` modifiable + - Manual registry entry (not SCM APIs) + +4. **Missing Protected Process** + - Security software not using PPL + - Anti-tampering bypassable + - Non-protected child processes + +5. **Service DACL Issues** + - Service modifiable by non-admin users + - `SERVICE_CHANGE_CONFIG` granted too broadly + - Missing service hardening + +**Common False Positives to Avoid:** + +- **Requires SYSTEM:** Service functionality requires SYSTEM privileges +- **Program Files location:** Binary in protected directory +- **SCM-created:** Service created via proper SCM APIs +- **Protected process light:** Security software using PPL + +**Search Patterns:** +``` +CreateService[AW]?\s*\(|ChangeServiceConfig[AW]?\s*\( +OpenService[AW]?\s*\(|StartService\s*\( +SERVICE_WIN32|SERVICE_AUTO_START|SERVICE_DEMAND_START +LocalSystem|LocalService|NetworkService +RegCreateKey|RegSetValue.*ImagePath +PROCESS_CREATION_MITIGATION_POLICY +``` diff --git a/plugins/c-review/prompts/windows-userspace/token-privilege-finder.md b/plugins/c-review/prompts/windows-userspace/token-privilege-finder.md new file mode 100644 index 0000000..17dbfde --- /dev/null +++ b/plugins/c-review/prompts/windows-userspace/token-privilege-finder.md @@ -0,0 +1,50 @@ +--- +name: token-privilege-finder +description: Detects privilege token vulnerabilities +--- + +**Finding ID Prefix:** `TOKPRIV` (e.g., TOKPRIV-001, TOKPRIV-002) + +**Bug Patterns to Find:** + +1. **Dangerous Privilege Enabling** + - `SeDebugPrivilege` - bypasses all DACLs/SACLs + - `SeBackupPrivilege` - complete filesystem access + - `SeTcbPrivilege` - create tokens for other users + - `SeAssignPrimaryTokenPrivilege` - replace process tokens + +2. **Privilege Not Dropped** + - High privilege enabled but never disabled + - Privilege enabled for longer than necessary + - Missing `AdjustTokenPrivileges` to disable + +3. **Token Impersonation Issues** + - `ImpersonateLoggedOnUser` without validation + - `SetThreadToken` with untrusted token + - Missing `RevertToSelf` after impersonation + +4. **Service Privilege Issues** + - Service running as SYSTEM when not required + - Missing `LOCAL SERVICE` or `NETWORK SERVICE` usage + - Improper service account configuration + +5. **Handle/Token Leaks** + - Token handle not closed after use + - Token inherited by child process + - Token accessible to lower-privilege code + +**Common False Positives to Avoid:** + +- **Immediately disabled:** Privilege enabled and disabled in same operation +- **Required for functionality:** Backup software needs SeBackupPrivilege +- **Security software:** Debugger/AV legitimately needs SeDebugPrivilege +- **Properly scoped:** Privilege enabled only for specific operation + +**Search Patterns:** +``` +AdjustTokenPrivileges\s*\(|LookupPrivilegeValue\s*\( +SeDebugPrivilege|SeBackupPrivilege|SeTcbPrivilege +SeAssignPrimaryTokenPrivilege|SeRestorePrivilege +ImpersonateLoggedOnUser|SetThreadToken|RevertToSelf +OpenProcessToken|OpenThreadToken|DuplicateToken +``` diff --git a/plugins/c-review/prompts/windows-userspace/windows-alloc-finder.md b/plugins/c-review/prompts/windows-userspace/windows-alloc-finder.md new file mode 100644 index 0000000..c8179af --- /dev/null +++ b/plugins/c-review/prompts/windows-userspace/windows-alloc-finder.md @@ -0,0 +1,46 @@ +--- +name: windows-alloc-finder +description: Identifies Windows memory allocation issues +--- + +**Finding ID Prefix:** `WINALLOC` (e.g., WINALLOC-001, WINALLOC-002) + +**Bug Patterns to Find:** + +1. **Uninitialized Allocations** + - `GlobalAlloc` without `GMEM_ZEROINIT` + - `LocalAlloc` without `LMEM_ZEROINIT` + - `HeapAlloc` without `HEAP_ZERO_MEMORY` + - `HeapReAlloc` without `HEAP_ZERO_MEMORY` + +2. **Mismatched Alloc/Free** + - `GlobalAlloc` freed with `LocalFree` + - `HeapAlloc` freed with `free()` + - `VirtualAlloc` freed with `HeapFree` + +3. **Sensitive Data Not Cleared** + - `memset` used for secrets (optimized out) + - `ZeroMemory` used for secrets (optimized out) + - Missing `RtlSecureZeroMemory` or `memset_s` + - Missing `CryptProtectMemory` for sensitive data + +4. **VirtualAlloc Issues** + - `MEM_RESET` without understanding zeroing behavior + - RWX pages (`PAGE_EXECUTE_READWRITE`) + - Large allocations without proper error handling + +**Common False Positives to Avoid:** + +- **Zeroing flag used:** `GMEM_ZEROINIT`, `LMEM_ZEROINIT`, `HEAP_ZERO_MEMORY` +- **Explicit memset after alloc:** Memory explicitly zeroed after allocation +- **Non-sensitive data:** Allocation for non-sensitive data structures +- **SecureZeroMemory used:** Proper secure zeroing for secrets + +**Search Patterns:** +``` +GlobalAlloc\s*\(|LocalAlloc\s*\(|HeapAlloc\s*\(|HeapReAlloc\s*\( +VirtualAlloc\s*\(|VirtualAllocEx\s*\( +GMEM_ZEROINIT|LMEM_ZEROINIT|HEAP_ZERO_MEMORY +RtlSecureZeroMemory|SecureZeroMemory|memset_s +CryptProtectMemory|CryptUnprotectMemory +``` diff --git a/plugins/c-review/prompts/windows-userspace/windows-crypto-finder.md b/plugins/c-review/prompts/windows-userspace/windows-crypto-finder.md new file mode 100644 index 0000000..42e9c11 --- /dev/null +++ b/plugins/c-review/prompts/windows-userspace/windows-crypto-finder.md @@ -0,0 +1,51 @@ +--- +name: windows-crypto-finder +description: Detects Windows crypto API misuse +--- + +**Finding ID Prefix:** `WINCRYPTO` (e.g., WINCRYPTO-001, WINCRYPTO-002) + +**Bug Patterns to Find:** + +1. **Deprecated CSP API Usage** + - `CryptAcquireContext` - deprecated + - `CryptGenRandom` - deprecated + - `CryptCreateHash` - deprecated + - `CryptGenKey` - deprecated + - Should use CNG APIs (BCrypt*, NCrypt*) + +2. **Weak Algorithms (CSP ALG_ID)** + - `CALG_MD5`, `CALG_SHA` (SHA-1) + - `CALG_RC2`, `CALG_RC4`, `CALG_DES` + - `CALG_RSA_SIGN` with small key size + +3. **Weak Algorithms (CNG)** + - `BCRYPT_MD5_ALGORITHM`, `BCRYPT_SHA1_ALGORITHM` + - `BCRYPT_RC4_ALGORITHM`, `BCRYPT_DES_ALGORITHM` + - `BCRYPT_3DES_ALGORITHM` (in most cases) + +4. **Poor Randomness** + - `rand()` or `srand()` for crypto + - Predictable seed values + - Missing `BCryptGenRandom` or `CryptGenRandom` + +5. **Key Management Issues** + - Hardcoded keys or IVs + - Key in plaintext in memory + - Missing key destruction after use + +**Common False Positives to Avoid:** + +- **Non-security use:** MD5/SHA1 for checksums, not security +- **Legacy compatibility:** Deprecated API required for interop +- **CNG used correctly:** Modern algorithms with proper parameters +- **FIPS mode:** Algorithm choice dictated by compliance + +**Search Patterns:** +``` +CryptAcquireContext|CryptGenRandom|CryptCreateHash|CryptGenKey +CryptEncrypt|CryptDecrypt|CryptDeriveKey|CryptHashData +BCryptOpenAlgorithmProvider|BCryptGenRandom|BCryptCreateHash +CALG_MD5|CALG_SHA[^2]|CALG_RC[24]|CALG_DES +BCRYPT_MD5|BCRYPT_SHA1_|BCRYPT_RC4|BCRYPT_DES_ +``` diff --git a/plugins/c-review/prompts/windows-userspace/windows-path-finder.md b/plugins/c-review/prompts/windows-userspace/windows-path-finder.md new file mode 100644 index 0000000..37404ed --- /dev/null +++ b/plugins/c-review/prompts/windows-userspace/windows-path-finder.md @@ -0,0 +1,53 @@ +--- +name: windows-path-finder +description: Finds Windows path handling vulnerabilities +--- + +**Finding ID Prefix:** `WINPATH` (e.g., WINPATH-001, WINPATH-002) + +**Bug Patterns to Find:** + +1. **Reserved DOS Device Names** + - CON, PRN, AUX, NUL + - COM1-COM9, COM¹, COM², COM³ + - LPT1-LPT9, LPT¹, LPT², LPT³ + - Reserved even with extension: `COM3.log` + +2. **8.3 Short Filename Bypass** + - `TEXTFI~1.TXT` for `TextFile.Mine.txt` + - Bypasses path/extension filters + +3. **Special Path Formats** + - UNC paths: `\\server\share\file` + - NT paths: `\\.\GLOBALROOT\Device\HarddiskVolume1\` + - Extended paths: `\\?\C:\path` + +4. **Character Encoding Issues** + - ANSI APIs (`-A` suffix) with Unicode paths + - WorstFit character fitting attacks + - Case sensitivity in UTF-16 comparison + +5. **Symlink/Junction Attacks** + - TOCTOU between check and use + - Junction to privileged directory + - Missing symlink target validation + +6. **Path Canonicalization** + - Missing PathCchCanonicalizeEx + - Inconsistent path comparison + - `..\` traversal not blocked + +**Common False Positives to Avoid:** + +- **PathCchCanonicalizeEx used:** Path properly canonicalized +- **PathIsNetworkPath checked:** UNC paths rejected +- **Hardcoded safe path:** Path constant, not user-controlled +- **Proper validation:** Reserved names explicitly blocked + +**Search Patterns:** +``` +CreateFile[AW]?\s*\(|DeleteFile[AW]?\s*\(|MoveFile[AW]?\s*\( +PathCch|PathIs|PathFind|PathAppend +\\\\\\.|\\\\\\?\\|GLOBALROOT +FILE_FLAG_POSIX_SEMANTICS|O_NOFOLLOW +``` diff --git a/plugins/c-review/scripts/build_run_plan.py b/plugins/c-review/scripts/build_run_plan.py new file mode 100644 index 0000000..a049e20 --- /dev/null +++ b/plugins/c-review/scripts/build_run_plan.py @@ -0,0 +1,518 @@ +#!/usr/bin/env python3 +# /// script +# requires-python = ">=3.13" +# dependencies = [] +# /// +"""Build a deterministic c-review run plan. + +Reads ``prompts/clusters/manifest.json`` plus run-level flags, applies gate + +per-pass filtering, verifies every referenced prompt resolves on disk, and +emits two artifacts in the run's output directory: + +* ``plan.json`` — machine-readable selection (cluster ids, prompt paths, + per-pass bug classes/prefixes, sub-prompt paths). The orchestrator reads + this to drive Phase 5 (TaskCreate metadata) and Phase 6 (worker spawn). +* ``worker-prompts/worker-N.txt`` — one ready-to-paste spawn prompt per + selected cluster, in selection order. The orchestrator passes the file + contents verbatim as the ``prompt`` argument to ``Agent``. + +The script aborts non-zero on any malformed manifest entry, missing prompt +file, or invalid flag combination — so the orchestrator can rely on its +output without further validation. + +Usage: + python3 build_run_plan.py \\ + --plugin-root /abs/plugins/c-review \\ + --output-dir /abs/.c-review-results/ \\ + --threat-model REMOTE \\ + --severity-filter medium \\ + --scope-subpath src \\ + --context-roots . \\ + --is-cpp false --is-posix true --is-windows false +""" + +from __future__ import annotations + +import argparse +import json +import sys +from pathlib import Path +from typing import Any, NoReturn + +THREAT_MODELS = {"REMOTE", "LOCAL_UNPRIVILEGED", "BOTH"} +SEVERITY_FILTERS = {"all", "medium", "high"} +GATE_VALUES = {"always", "is_cpp", "is_windows"} +KNOWN_REQUIRES = {"is_cpp", "is_posix", "is_windows"} + + +def parse_bool(value: str) -> bool: + v = value.strip().lower() + if v in ("true", "1", "yes"): + return True + if v in ("false", "0", "no"): + return False + raise argparse.ArgumentTypeError(f"expected true/false, got {value!r}") + + +def parse_args() -> argparse.Namespace: + p = argparse.ArgumentParser( + description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter + ) + p.add_argument( + "--plugin-root", + required=True, + type=Path, + help="Absolute path to the c-review plugin root (contains prompts/clusters/manifest.json)", + ) + p.add_argument( + "--output-dir", required=True, type=Path, help="Absolute path to the run's output directory" + ) + p.add_argument("--threat-model", required=True, choices=sorted(THREAT_MODELS)) + p.add_argument("--severity-filter", required=True, choices=sorted(SEVERITY_FILTERS)) + p.add_argument( + "--scope-subpath", required=True, help='Repo-relative scope directory, or "." for repo root' + ) + p.add_argument( + "--context-roots", + default=".", + help=( + "Comma-separated repo-relative read-only roots/files workers may inspect for " + "reachability, build settings, wrappers, and threat-model context. Findings " + "remain limited to --scope-subpath." + ), + ) + p.add_argument("--is-cpp", required=True, type=parse_bool) + p.add_argument("--is-posix", required=True, type=parse_bool) + p.add_argument("--is-windows", required=True, type=parse_bool) + p.add_argument( + "--manifest", + type=Path, + default=None, + help="Override manifest path (defaults to /prompts/clusters/manifest.json)", + ) + p.add_argument( + "--cache-primer", + type=parse_bool, + default=True, + help=( + "When true (default), the orchestrator spawns a small 'cache primer' worker before " + "the parallel batch so followers can hit the prompt cache on their first turn. Pass " + "false to skip and pay full cache-creation on every worker (useful for A/B testing)." + ), + ) + return p.parse_args() + + +def fail(msg: str) -> NoReturn: + print(f"build_run_plan.py: {msg}", file=sys.stderr) + sys.exit(2) + + +def gate_passes(cluster_gate: str, *, is_cpp: bool, is_windows: bool) -> bool: + if cluster_gate == "always": + return True + if cluster_gate == "is_cpp": + return is_cpp + if cluster_gate == "is_windows": + return is_windows + fail(f"unknown cluster gate {cluster_gate!r}") + + +def pass_filtered_out(p: dict[str, Any], *, flags: dict[str, bool], threat_model: str) -> bool: + """Return True if this pass should be hard-dropped from the run.""" + requires = p.get("requires", []) or [] + for req in requires: + if req not in KNOWN_REQUIRES: + fail(f"pass {p.get('bug_class', '?')!r}: unknown requires flag {req!r}") + if not flags[req]: + return True + skip_threat_models = p.get("skip_threat_models", []) or [] + return threat_model in skip_threat_models + + +def build_selection( + manifest: dict[str, Any], *, plugin_root: Path, flags: dict[str, bool], threat_model: str +) -> list[dict[str, Any]]: + if manifest.get("version") != 1: + fail(f"unsupported manifest version: {manifest.get('version')!r}") + if not isinstance(manifest.get("clusters"), list): + fail("manifest.clusters must be a list") + + selected: list[dict[str, Any]] = [] + for cluster in manifest["clusters"]: + cid = cluster.get("cluster_id") + if not cid: + fail("cluster missing cluster_id") + gate = cluster.get("gate") + if gate not in GATE_VALUES: + fail(f"cluster {cid!r}: invalid gate {gate!r}") + if not gate_passes(gate, is_cpp=flags["is_cpp"], is_windows=flags["is_windows"]): + continue + + consolidated = bool(cluster.get("consolidated", False)) + cluster_prompt_rel = cluster.get("prompt") + if not cluster_prompt_rel: + fail(f"cluster {cid!r}: missing prompt path") + cluster_prompt_abs = (plugin_root / cluster_prompt_rel).resolve() + if not cluster_prompt_abs.is_file(): + fail(f"cluster {cid!r}: prompt not found at {cluster_prompt_abs}") + + passes_in = cluster.get("passes") or [] + if not passes_in: + fail(f"cluster {cid!r}: no passes declared") + + kept_passes: list[dict[str, Any]] = [] + for raw in passes_in: + bug_class = raw.get("bug_class") + prefix = raw.get("prefix") + if not bug_class or not prefix: + fail(f"cluster {cid!r}: pass missing bug_class/prefix: {raw!r}") + if pass_filtered_out(raw, flags=flags, threat_model=threat_model): + continue + entry: dict[str, Any] = {"bug_class": bug_class, "prefix": prefix} + if consolidated: + # No per-pass prompt file; cluster prompt is self-sufficient. + if "prompt" in raw: + fail( + f"cluster {cid!r} (consolidated): " + f"pass {bug_class!r} unexpectedly has 'prompt'" + ) + else: + pass_prompt_rel = raw.get("prompt") + if not pass_prompt_rel: + fail( + f"cluster {cid!r}: non-consolidated pass {bug_class!r} missing prompt path" + ) + pass_prompt_abs = (plugin_root / pass_prompt_rel).resolve() + if not pass_prompt_abs.is_file(): + fail( + f"cluster {cid!r} pass {bug_class!r}: prompt not found at {pass_prompt_abs}" + ) + entry["prompt"] = str(pass_prompt_abs) + kept_passes.append(entry) + + if not kept_passes: + # Empty after filtering — drop the cluster entirely (Phase 4 rule). + continue + + selected.append( + { + "cluster_id": cid, + "consolidated": consolidated, + "cluster_prompt": str(cluster_prompt_abs), + "passes": kept_passes, + } + ) + + if not selected: + fail("no clusters selected after filtering — refusing to start an empty review") + return selected + + +def _render_shared_prefix_lines( + *, + output_dir: Path, + scope_root: str, + context_roots: str, + threat_model: str, + severity_filter: str, + flags: dict[str, bool], + context_md_body: str, +) -> list[str]: + """Lines that are byte-identical across all workers AND the cache primer in this run. + + Keeping this block stable is what makes the prompt cache hit cross-worker. Any change + to its shape (formatting, ordering, blank-line placement) invalidates cache for the rest + of the run. The primer prompt and worker prompts both call this and append divergent + trailers afterwards. + """ + lines: list[str] = [] + lines.append("You are a c-review worker on a parallel C/C++ security review.") + lines.append("Follow the protocol in your system prompt verbatim.") + lines.append("") + lines.append(f"Output directory: {output_dir}") + lines.append( + f"Finding scope root: {scope_root} — finding locations MUST be inside this subtree." + ) + lines.append( + f"Context roots: {context_roots} — read-only context for reachability, callers, " + "wrappers, build settings, mitigations, and threat-model details. Do not file " + "findings outside the finding scope." + ) + lines.append(f"Scope root: {scope_root} — legacy alias for Finding scope root.") + lines.append(f"Threat model: {threat_model}") + lines.append(f"Severity filter: {severity_filter}") + lines.append( + f"Codebase: is_cpp={'true' if flags['is_cpp'] else 'false'}, " + f"is_posix={'true' if flags['is_posix'] else 'false'}, " + f"is_windows={'true' if flags['is_windows'] else 'false'}" + ) + lines.append("") + lines.append("") + lines.append( + "Codebase context (from output_dir/context.md — do NOT re-Read it from disk; " + "this block IS the canonical copy):" + ) + lines.append("") + lines.append(context_md_body.rstrip()) + lines.append("") + lines.append("") + return lines + + +def render_cache_primer_prompt( + *, + output_dir: Path, + scope_root: str, + context_roots: str, + threat_model: str, + severity_filter: str, + flags: dict[str, bool], + context_md_body: str, +) -> str: + """Tiny single-turn prompt that warms the prompt cache for the parallel batch. + + Shares its prefix byte-for-byte with every worker prompt (via the helper above) so + that the workers in Phase 6b read this entry from cache instead of paying full + cache-creation. The trailer instructs the agent to abort in one text response with + no tool calls — duration ~3 s, no findings written. + """ + lines = _render_shared_prefix_lines( + output_dir=output_dir, + scope_root=scope_root, + context_roots=context_roots, + threat_model=threat_model, + severity_filter=severity_filter, + flags=flags, + context_md_body=context_md_body, + ) + # Trailer is primer-only — never reused by workers — so keep it short. + # The worker system prompt treats this exact marker as a first-class mode. + lines.append("Cache primer: true") + lines.append("worker-PRIMER abort: cache primer (no analysis performed)") + lines.append("") + return "\n".join(lines) + + +def render_worker_prompt( + *, + worker_n: int, + cluster: dict[str, Any], + output_dir: Path, + scope_root: str, + context_roots: str, + threat_model: str, + severity_filter: str, + flags: dict[str, bool], + context_md_body: str, +) -> str: + bug_classes = [p["bug_class"] for p in cluster["passes"]] + prefixes = [p["prefix"] for p in cluster["passes"]] + + lines = _render_shared_prefix_lines( + output_dir=output_dir, + scope_root=scope_root, + context_roots=context_roots, + threat_model=threat_model, + severity_filter=severity_filter, + flags=flags, + context_md_body=context_md_body, + ) + lines.append("— assignment —") + lines.append(f"Worker id: worker-{worker_n}") + lines.append(f"Cluster id: {cluster['cluster_id']}") + lines.append(f"Cluster prompt: {cluster['cluster_prompt']}") + + if cluster["consolidated"]: + # Worker.md contract: omit the section entirely for consolidated clusters. + pass + else: + lines.append("Sub-prompt paths:") + for p in cluster["passes"]: + lines.append(f" - {p['prompt']}") + + lines.append(f"Pass bug classes: {', '.join(bug_classes)}") + lines.append(f"Pass prefixes: {', '.join(prefixes)}") + # skip_subclasses is always empty: filtering is hard-drop (passes never reach + # the worker). Field is retained because worker.md "Inputs" requires it. + lines.append("Skip subclasses: (none)") + lines.append("") + return "\n".join(lines) + + +def _validate_run_inputs(args: argparse.Namespace) -> tuple[Path, Path, Path, dict[str, Any]]: + plugin_root: Path = args.plugin_root.resolve() + if not plugin_root.is_dir(): + fail(f"--plugin-root {plugin_root} is not a directory") + + output_dir: Path = args.output_dir.resolve() + if not output_dir.is_dir(): + fail(f"--output-dir {output_dir} does not exist (Phase 2 must create it first)") + + manifest_path: Path = ( + args.manifest or plugin_root / "prompts/clusters/manifest.json" + ).resolve() + if not manifest_path.is_file(): + fail(f"manifest not found at {manifest_path}") + + try: + manifest = json.loads(manifest_path.read_text()) + except json.JSONDecodeError as e: + fail(f"manifest is not valid JSON: {e}") + + return plugin_root, output_dir, manifest_path, manifest + + +def _render_workers( + selected: list[dict[str, Any]], + *, + worker_prompts_dir: Path, + output_dir: Path, + scope_subpath: str, + context_roots: str, + threat_model: str, + severity_filter: str, + flags: dict[str, bool], + context_md_body: str, +) -> list[dict[str, Any]]: + workers: list[dict[str, Any]] = [] + for i, cluster in enumerate(selected, start=1): + prompt_text = render_worker_prompt( + worker_n=i, + cluster=cluster, + output_dir=output_dir, + scope_root=scope_subpath, + context_roots=context_roots, + threat_model=threat_model, + severity_filter=severity_filter, + flags=flags, + context_md_body=context_md_body, + ) + prompt_path = worker_prompts_dir / f"worker-{i}.txt" + prompt_path.write_text(prompt_text) + workers.append( + { + "worker_n": i, + "cluster_id": cluster["cluster_id"], + "consolidated": cluster["consolidated"], + "cluster_prompt": cluster["cluster_prompt"], + "sub_prompt_paths": [p["prompt"] for p in cluster["passes"] if "prompt" in p], + "pass_bug_classes": [p["bug_class"] for p in cluster["passes"]], + "pass_prefixes": [p["prefix"] for p in cluster["passes"]], + "spawn_prompt_path": str(prompt_path), + } + ) + return workers + + +def _print_summary( + *, + plan_path: Path, + worker_prompts_dir: Path, + selected: list[dict[str, Any]], + cache_primer_path: Path | None, +) -> None: + spawn_warning = ( + "Spawn workers FOREGROUND only. Each Agent call MUST have no " + "run_in_background field (or run_in_background=false). Setting it to " + "true defeats the Phase-6a primer cache and burns ~15K cache-creation " + "tokens per worker. 'Parallel' = one assistant message with M Agent " + "calls; that is already concurrent — do not add run_in_background=true." + ) + + # Stderr banner so the warning shows up in the Bash tool result the + # orchestrator reads, not just inside the JSON summary. + print(f"WARNING: {spawn_warning}", file=sys.stderr) + + summary = { + "plan_path": str(plan_path), + "worker_prompts_dir": str(worker_prompts_dir), + "worker_count": len(selected), + "cluster_ids": [c["cluster_id"] for c in selected], + "cache_primer_path": str(cache_primer_path) if cache_primer_path else None, + "spawn_instructions": spawn_warning, + } + print(json.dumps(summary, indent=2)) + + +def main() -> int: + args = parse_args() + plugin_root, output_dir, manifest_path, manifest = _validate_run_inputs(args) + + flags = {"is_cpp": args.is_cpp, "is_posix": args.is_posix, "is_windows": args.is_windows} + selected = build_selection( + manifest, plugin_root=plugin_root, flags=flags, threat_model=args.threat_model + ) + + context_md_path = output_dir / "context.md" + if not context_md_path.is_file(): + fail( + f"context.md not found at {context_md_path} — Phase 3 must write it before Phase 4 runs" + ) + context_md_body = context_md_path.read_text() + + worker_prompts_dir = output_dir / "worker-prompts" + worker_prompts_dir.mkdir(exist_ok=True) + + workers = _render_workers( + selected, + worker_prompts_dir=worker_prompts_dir, + output_dir=output_dir, + scope_subpath=args.scope_subpath, + context_roots=args.context_roots, + threat_model=args.threat_model, + severity_filter=args.severity_filter, + flags=flags, + context_md_body=context_md_body, + ) + + cache_primer_path: Path | None = None + if args.cache_primer: + cache_primer_path = worker_prompts_dir / "cache-primer.txt" + cache_primer_path.write_text( + render_cache_primer_prompt( + output_dir=output_dir, + scope_root=args.scope_subpath, + context_roots=args.context_roots, + threat_model=args.threat_model, + severity_filter=args.severity_filter, + flags=flags, + context_md_body=context_md_body, + ) + ) + + plan: dict[str, Any] = { + "version": 1, + "run": { + "output_dir": str(output_dir), + "finding_scope_root": args.scope_subpath, + "scope_root": args.scope_subpath, + "context_roots": args.context_roots, + "threat_model": args.threat_model, + "severity_filter": args.severity_filter, + "is_cpp": args.is_cpp, + "is_posix": args.is_posix, + "is_windows": args.is_windows, + "plugin_root": str(plugin_root), + "manifest_path": str(manifest_path), + "cache_primer": args.cache_primer, + }, + "workers": workers, + } + if cache_primer_path is not None: + plan["cache_primer"] = {"spawn_prompt_path": str(cache_primer_path)} + + plan_path = output_dir / "plan.json" + plan_path.write_text(json.dumps(plan, indent=2) + "\n") + + _print_summary( + plan_path=plan_path, + worker_prompts_dir=worker_prompts_dir, + selected=selected, + cache_primer_path=cache_primer_path, + ) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/plugins/c-review/scripts/generate_sarif.py b/plugins/c-review/scripts/generate_sarif.py new file mode 100755 index 0000000..fd52ca1 --- /dev/null +++ b/plugins/c-review/scripts/generate_sarif.py @@ -0,0 +1,354 @@ +#!/usr/bin/env python3 +# /// script +# requires-python = ">=3.13" +# dependencies = [] +# /// +"""Generate c-review SARIF from finding frontmatter. + +Usage: + python3 generate_sarif.py /path/to/output_dir + python3 generate_sarif.py /path/to/output_dir --output /tmp/REPORT.sarif +""" + +from __future__ import annotations + +import argparse +import json +import re +from pathlib import Path +from typing import Any + +SEVERITY_ORDER = {"LOW": 1, "MEDIUM": 2, "HIGH": 3, "CRITICAL": 4} +SEVERITY_LEVEL = { + "CRITICAL": "error", + "HIGH": "error", + "MEDIUM": "warning", + "LOW": "note", +} +FILTER_MIN = {"all": 1, "medium": 2, "high": 3} +SURVIVOR_VERDICTS = {"TRUE_POSITIVE", "LIKELY_TP"} +# Findings with no fp_verdict (e.g. fp-judge was skipped on a partial run) +# are emitted with this synthetic verdict so the SARIF safety net still +# surfaces them. See SKILL.md Phase 8b. +UNJUDGED_FALLBACK_VERDICT = "LIKELY_TP" +CONFIDENCE_TO_SEVERITY = {"HIGH": "MEDIUM", "MEDIUM": "MEDIUM", "LOW": "LOW"} + +RULE_DESCRIPTIONS = { + "access-control": "Missing or incorrect authorization check", + "banned-functions": "Use of banned or deprecated C/C++ APIs", + "buffer-overflow": "Out-of-bounds write to a buffer", + "compiler-bugs": "Compiler-optimization-sensitive undefined behavior", + "createprocess": "Windows CreateProcess or process launch misuse", + "cross-process": "Unsafe cross-process handle or memory operation", + "dll-planting": "DLL search-order hijacking risk", + "dos": "Denial of service vulnerability", + "eintr-handling": "Missing EINTR handling", + "envvar": "Unsafe environment variable handling", + "errno-handling": "Incorrect errno handling", + "error-handling": "Missing or incorrect error handling", + "exception-safety": "C++ exception safety issue", + "exploit-mitigations": "Missing or misconfigured exploit mitigation", + "filesystem-issues": "Filesystem race or path handling issue", + "flexible-array": "Flexible array or struct-size misuse", + "format-string": "Format string vulnerability", + "half-closed-socket": "Half-closed socket handling issue", + "inet-aton": "Legacy IPv4 parsing API misuse", + "init-order": "C++ static initialization order issue", + "integer-overflow": "Integer overflow or wraparound", + "installer-race": "Windows installer filesystem race", + "iterator-invalidation": "C++ iterator invalidation", + "lambda-capture": "Unsafe C++ lambda capture lifetime", + "memcpy-size": "Incorrect memcpy or memory operation size", + "memory-leak": "Memory leak on security-relevant path", + "move-semantics": "C++ move semantics misuse", + "named-pipe": "Windows named pipe security issue", + "negative-retval": "Negative return value used unsafely", + "null-deref": "Null pointer dereference", + "null-zero": "NULL or zero confusion", + "oob-comparison": "Out-of-bounds comparison", + "open-issues": "File open or creation misuse", + "operator-precedence": "Operator precedence issue", + "overlapping-buffers": "Overlapping memory operation buffers", + "printf-attr": "Missing printf-format attribute", + "privilege-drop": "Privilege drop flaw", + "qsort": "qsort or comparator misuse", + "race-condition": "Race condition or inconsistent synchronization", + "regex-issues": "Regex safety issue", + "scanf-uninit": "scanf leaves target uninitialized", + "service-security": "Windows service security issue", + "signal-handler": "Unsafe signal handler", + "smart-pointer": "C++ smart pointer misuse", + "snprintf-retval": "snprintf return value misuse", + "socket-disconnect": "Socket disconnect handling issue", + "spinlock-init": "Uninitialized lock primitive", + "string-issues": "String encoding or termination issue", + "strlen-strcpy": "strlen/strcpy size mismatch", + "strncat-misuse": "strncat size argument misuse", + "strncpy-termination": "strncpy missing termination", + "thread-safety": "Thread safety issue", + "time-issues": "Time handling issue", + "token-privilege": "Windows token or privilege misuse", + "type-confusion": "Type confusion or unsafe cast", + "undefined-behavior": "Undefined behavior", + "unsafe-stdlib": "Unsafe standard library use", + "use-after-free": "Use-after-free or double free", + "va-start-end": "va_list lifecycle misuse", + "virtual-function": "C++ virtual function misuse", + "windows-alloc": "Windows allocation API misuse", + "windows-crypto": "Windows cryptography API misuse", + "windows-path": "Windows path handling issue", +} + + +def split_frontmatter(text: str) -> tuple[dict[str, Any], str]: + if not text.startswith("---\n"): + return {}, text + end = text.find("\n---", 4) + if end == -1: + return {}, text + frontmatter = text[4:end] + body = text[end + len("\n---") :].lstrip("\n") + return parse_frontmatter(frontmatter), body + + +def parse_frontmatter(frontmatter: str) -> dict[str, Any]: + result: dict[str, Any] = {} + current_key: str | None = None + for raw_line in frontmatter.splitlines(): + line = raw_line.rstrip() + if not line: + continue + if line.startswith(" - ") and current_key: + result.setdefault(current_key, []).append(parse_scalar(line[4:])) + continue + if ":" not in line or line.startswith((" ", "\t")): + current_key = None + continue + key, value = line.split(":", 1) + key = key.strip() + value = value.strip() + current_key = key + if value == "": + result[key] = [] + else: + result[key] = parse_scalar(value) + return result + + +def _split_inline_list(inner: str) -> list[str]: + """Split a YAML flow-list body on commas, respecting quoted strings.""" + parts: list[str] = [] + buf: list[str] = [] + quote: str | None = None + for ch in inner: + if quote: + buf.append(ch) + if ch == quote: + quote = None + elif ch in ('"', "'"): + quote = ch + buf.append(ch) + elif ch == ",": + parts.append("".join(buf).strip()) + buf = [] + else: + buf.append(ch) + if buf: + parts.append("".join(buf).strip()) + return parts + + +def parse_scalar(value: str) -> Any: + value = value.strip() + if (value.startswith('"') and value.endswith('"')) or ( + value.startswith("'") and value.endswith("'") + ): + return value[1:-1] + if value.startswith("[") and value.endswith("]"): + inner = value[1:-1].strip() + if not inner: + return [] + return [parse_scalar(part) for part in _split_inline_list(inner)] + if value.lower() == "true": + return True + if value.lower() == "false": + return False + return value + + +def parse_context(output_dir: Path) -> dict[str, Any]: + path = output_dir / "context.md" + if not path.exists(): + return {} + frontmatter, _ = split_frontmatter(path.read_text(encoding="utf-8")) + return frontmatter + + +def iter_findings(output_dir: Path) -> list[dict[str, Any]]: + findings = [] + for path in sorted((output_dir / "findings").glob("*.md")): + frontmatter, _ = split_frontmatter(path.read_text(encoding="utf-8")) + frontmatter["_path"] = str(path) + findings.append(frontmatter) + return findings + + +def location_parts(location: Any) -> tuple[str, int]: + value = str(location or "") + if "," in value or "\n" in value: + return value, 1 + match = re.match(r"^\[([^\]]+)\]\([^)]+\):(\d+)$", value) + if match: + return normalize_path(match.group(1)), int(match.group(2)) + path, sep, line = value.rpartition(":") + if sep and line.isdecimal(): + return normalize_path(path), int(line) + if sep and not line: + return normalize_path(path), 1 + return normalize_path(value), 1 + + +def normalize_path(path: str) -> str: + path = path.replace("\\", "/") + while path.startswith("./"): + path = path[2:] + while "//" in path: + path = path.replace("//", "/") + return path + + +def severity_allowed(severity: str, severity_filter: str) -> bool: + return SEVERITY_ORDER.get(severity.upper(), 0) >= FILTER_MIN.get(severity_filter, 1) + + +def sarif_level(severity: str) -> str: + return SEVERITY_LEVEL.get(severity.upper(), "warning") + + +def rule_level(findings: list[dict[str, Any]], bug_class: str) -> str: + max_severity = "LOW" + for finding in findings: + if finding.get("bug_class") == bug_class: + severity = str(finding.get("severity", "LOW")).upper() + if SEVERITY_ORDER.get(severity, 0) > SEVERITY_ORDER.get(max_severity, 0): + max_severity = severity + return sarif_level(max_severity) + + +def build_sarif(output_dir: Path) -> dict[str, Any]: + context = parse_context(output_dir) + severity_filter = str(context.get("severity_filter", "all")).lower() + threat_model = str(context.get("threat_model", "UNKNOWN")) + findings = [] + for finding in iter_findings(output_dir): + if "merged_into" in finding: + continue + verdict = str(finding.get("fp_verdict", "")).upper() + if not verdict: + # Unjudged finding — fp-judge was skipped (partial run). Treat as + # LIKELY_TP and infer severity from worker-assigned confidence. + finding["fp_verdict"] = UNJUDGED_FALLBACK_VERDICT + finding.setdefault( + "severity", + CONFIDENCE_TO_SEVERITY.get(str(finding.get("confidence", "")).upper(), "MEDIUM"), + ) + finding["unjudged"] = True + elif verdict not in SURVIVOR_VERDICTS: + continue + if not severity_allowed(str(finding.get("severity", "")).upper(), severity_filter): + continue + findings.append(finding) + + rules = [] + for bug_class in sorted({str(finding.get("bug_class", "unknown")) for finding in findings}): + rules.append( + { + "id": bug_class, + "shortDescription": { + "text": RULE_DESCRIPTIONS.get(bug_class, bug_class.replace("-", " ").title()) + }, + "defaultConfiguration": {"level": rule_level(findings, bug_class)}, + } + ) + + results = [] + for finding in findings: + location, line = location_parts(finding.get("location")) + severity = str(finding.get("severity", "MEDIUM")).upper() + also_known_as = finding.get("also_known_as", []) + if not isinstance(also_known_as, list): + also_known_as = [str(also_known_as)] + results.append( + { + "ruleId": str(finding.get("bug_class", "unknown")), + "level": sarif_level(severity), + "message": { + "text": str(finding.get("title") or finding.get("id") or "c-review finding") + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": location, + "uriBaseId": "%SRCROOT%", + }, + "region": {"startLine": line}, + } + } + ], + "properties": { + "finding_id": str(finding.get("id", "")), + "bug_class": str(finding.get("bug_class", "unknown")), + "severity": severity, + "attack_vector": str(finding.get("attack_vector", "")), + "exploitability": str(finding.get("exploitability", "")), + "fp_verdict": str(finding.get("fp_verdict", "")), + "unjudged": bool(finding.get("unjudged", False)), + "also_known_as": also_known_as, + }, + } + ) + + return { + "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json", + "version": "2.1.0", + "runs": [ + { + "tool": { + "driver": { + "name": "c-review", + "informationUri": "https://github.com/trailofbits/skills/tree/main/plugins/c-review", + "rules": rules, + } + }, + "invocations": [ + { + "executionSuccessful": True, + "properties": { + "threat_model": threat_model, + "severity_filter": severity_filter, + }, + } + ], + "results": results, + } + ], + } + + +def main() -> int: + parser = argparse.ArgumentParser(description="Generate REPORT.sarif for a c-review output dir") + parser.add_argument("output_dir", type=Path) + parser.add_argument("--output", type=Path, default=None) + args = parser.parse_args() + + output_dir = args.output_dir.resolve() + output_path = args.output or output_dir / "REPORT.sarif" + sarif = build_sarif(output_dir) + output_path.write_text(json.dumps(sarif, indent=2) + "\n", encoding="utf-8") + print(f"wrote {output_path}") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/plugins/c-review/scripts/pyproject.toml b/plugins/c-review/scripts/pyproject.toml new file mode 100644 index 0000000..27fcf2d --- /dev/null +++ b/plugins/c-review/scripts/pyproject.toml @@ -0,0 +1,11 @@ +[project] +name = "c-review-scripts" +version = "0.1.0" +description = "Build-plan + SARIF helpers for the c-review plugin" +requires-python = ">=3.13" +dependencies = [] + +[tool.ruff] +line-length = 100 + +[tool.ty.rules] diff --git a/plugins/c-review/skills/c-review/SKILL.md b/plugins/c-review/skills/c-review/SKILL.md new file mode 100644 index 0000000..7963814 --- /dev/null +++ b/plugins/c-review/skills/c-review/SKILL.md @@ -0,0 +1,313 @@ +--- +name: c-review +description: Performs comprehensive C/C++ security review for memory corruption, integer overflows, race conditions, and platform-specific vulnerabilities. Use when auditing native C/C++ applications, reviewing daemons or services for memory safety, or hunting integer overflow / use-after-free / race conditions in userspace code. +allowed-tools: Agent AskUserQuestion SendMessage TaskCreate TaskUpdate TaskList TaskGet Grep Glob Read Write Bash +--- + +# C/C++ Security Review + +Runs in the main conversation (invoke via `/c-review:c-review`). Orchestrator owns the `Task*` ledger as bookkeeping for retries; workers and judges have no Task tools. Workers and judges are named plugin subagents (`c-review:c-review-worker`, `c-review:c-review-dedup-judge`, `c-review:c-review-fp-judge`); tool sets are declared in `plugins/c-review/agents/*.md`. Findings are exchanged via markdown-with-YAML files in a shared output directory. + +## When to Use + +Native C/C++ application security review: memory safety, integer overflow, races, type confusion, Linux/macOS daemons, Windows userspace services. + +## When NOT to Use + +- Kernel drivers/modules (Linux, Windows, macOS). +- Managed languages (Java, C#, Python, Go, Rust). +- Embedded/bare-metal code without libc. + +## Subagents + +| Subagent type | Purpose | Tool set | +|---|---|---| +| `c-review:c-review-worker` | Run assigned cluster, write findings | Read, Write, Edit, Grep, Glob, Bash | +| `c-review:c-review-dedup-judge` | Merge duplicates (runs **first**) | Read, Write, Edit, Glob | +| `c-review:c-review-fp-judge` | FP + severity + final reports (runs **second**) | Read, Write, Edit, Grep, Glob, Bash | + +Tools come from each agent's frontmatter at spawn time. The orchestrator's `Task*`/`Agent`/`Bash`/etc. come from this skill's `allowed-tools`. + +--- + +## Architecture + +``` +coordinator: write context.md → build_run_plan.py → TaskCreate × M + → spawn primer (foreground) → spawn M workers (parallel) + → classify Phase-7 outcomes + write findings-index.txt + → dedup-judge → fp-judge → SARIF safety net → return REPORT.md +``` + +Output directory contains: `context.md`, `plan.json`, `worker-prompts/`, `findings/`, `findings-index.d/` (per-worker shards), `findings-index.txt`, `run-summary.md`, `dedup-summary.md`, `fp-summary.md`, `REPORT.md`, `REPORT.sarif`. + +**Path convention:** set `${C_REVIEW_PLUGIN_ROOT}=${CLAUDE_PLUGIN_ROOT}` if that resolves (`Bash: ls "${CLAUDE_PLUGIN_ROOT}/prompts/clusters/buffer-write-sinks.md"`), otherwise `Bash: find ~/.claude -path '*/plugins/c-review/prompts/clusters/buffer-write-sinks.md' -print -quit`. + +**Scope convention:** keep two scopes separate throughout the run: + +- `finding_scope_root` — the user-requested audit subtree. Workers may only file findings whose vulnerable location is inside this subtree. +- `context_roots` — read-only repo roots/files workers and judges may inspect to verify reachability, callers, wrappers, build flags, mitigations, and threat-model details. Default to `.` unless the user explicitly forbids broader context. Reading context outside `finding_scope_root` is allowed; filing findings there is not. + +--- + +## Rationalizations to Reject + +- **"Background spawns parallelize the workers."** They do not — `Agent` calls in a single assistant message already run concurrently. `run_in_background=true` defeats the Phase 6a primer cache, so every worker pays full cache-creation (`cache_read_input_tokens=0`) and the ~15 K-token primer is wasted M times. This is the single most common defect — multiple recent runs spawned 7-of-8 (or all) workers with `bg=true`. Default: omit `run_in_background` from worker spawns. +- **"I'll re-derive the cluster list / paths / pass prefixes inline instead of running `build_run_plan.py`."** The script is the only authority for selection and rendering. Paraphrasing it drops fields that the worker self-check requires, producing `worker-N abort: spawn prompt malformed`. Always run the script and `Read plan.json`. +- **"The run partially succeeded — I'll just write `REPORT.md` from what completed."** Hiding partial runs behind a successful report is a correctness bug. If any Phase-5 cluster task is not `completed`, surface it prominently in `run-summary.md` and the final response. +- **"Zero findings — skip Phase 8."** Always run both judges and Phase 8b: dedup-judge writes a minimal no-op `dedup-summary.md` on an empty index, fp-judge writes empty `REPORT.md`/`REPORT.sarif`, and Phase 8b's SARIF generator emits `results: []` for the empty case. SARIF consumers depend on a stable artifact set. +- **"`Bash: ls README*` is fine for the preflight."** Under zsh, an unmatched glob aborts the whole compound command before `2>/dev/null` runs. Use `Glob` (preferred) or `find` (never fails on no-match). + +--- + +## Orchestration Workflow + +Run these phases **in the main conversation**. + +### Phase 0: Parameter Collection + +**Entry:** skill invoked. **Exit:** `threat_model`, `worker_model`, `severity_filter` resolved; `scope_subpath` resolved or set to `"."`; `finding_scope_root=scope_subpath`; `context_roots` resolved. + +The skill is invoked directly (no command wrapper). Parse any free-text arguments the user passed on the `/c-review:c-review` line (e.g. `flamenco only`, `high severity only`, `use haiku`) and pre-fill the answers they imply — then ask for any missing required parameters with **one** `AskUserQuestion` call. Never silently default the required parameters. + +Required parameters: + +| Parameter | Values | How to infer from args | +|---|---|---| +| `threat_model` | `REMOTE` / `LOCAL_UNPRIVILEGED` / `BOTH` | Words like "remote", "network", "attacker" → `REMOTE`; "local", "unprivileged" → `LOCAL_UNPRIVILEGED`; otherwise ask. | +| `worker_model` | `haiku` / `sonnet` / `opus` | Explicit model name in args. Otherwise ask (no silent default). | +| `severity_filter` | `all` / `medium` / `high` | "all", "every", "noisy" → `all`; "medium and above" → `medium`; "high only", "criticals only" → `high`. Otherwise ask — **no silent default**. | +| `scope_subpath` | repo-relative directory (optional) | Phrases like "X only", "just audit X/", "review subdirectory X" → `src/X/` or the matching subdir. Apply fuzzy matching against top-level subdirectories of the repo. If absent, set `"."`; if ambiguous, ask. | + +Call `AskUserQuestion` exactly once with only unresolved required parameters (`threat_model`, `worker_model`, `severity_filter`) plus `scope_subpath` only when the user explicitly requested a narrowed scope but it is ambiguous. If the required parameters were all pre-filled and scope is absent or resolved, skip the question. + +After resolving `scope_subpath`, set `finding_scope_root="${scope_subpath:-.}"`. Set `context_roots="."` by default so workers can verify callers/build settings outside a narrowed subtree without filing out-of-scope findings. If the user explicitly asks to forbid broader context, set `context_roots="${finding_scope_root}"` and note that reachability confidence may be lower. + +### Phase 1: Prerequisites + +**Entry:** Phase 0 complete. **Exit:** `is_cpp`, `is_posix`, `is_windows` flags determined. + +Probe within `${finding_scope_root:-.}`. Prefer `Glob`/`Grep` when available in the orchestrator's tool set; some sessions only expose `Bash`, so fall back to the equivalents below — both forms produce identical signals (non-empty output ⇒ flag true): + +```bash +# is_cpp +find "${finding_scope_root:-.}" -type f \( -name '*.cpp' -o -name '*.cxx' -o -name '*.cc' -o -name '*.hpp' -o -name '*.hh' \) -print -quit +# is_posix +grep -rlE '#include[[:space:]]*<(pthread|signal|sys/(socket|stat|types|wait)|unistd|errno)\.h>' \ + --include='*.c' --include='*.h' \ + --include='*.cpp' --include='*.cxx' --include='*.cc' --include='*.hpp' --include='*.hh' \ + "${finding_scope_root:-.}" | head -1 +# is_windows +grep -rlE '#include[[:space:]]*<(windows|winbase|winnt|winuser|winsock|ntdef|ntstatus)\.h>' \ + --include='*.c' --include='*.h' \ + --include='*.cpp' --include='*.cxx' --include='*.cc' --include='*.hpp' --include='*.hh' \ + "${finding_scope_root:-.}" | head -1 +``` + +`compile_commands.json` is informational (no agent currently uses LSP), but the probe is mandatory so the run summary records whether richer local tooling is available. Probe via `Glob: **/compile_commands.json` under `${context_roots}`. If `Glob` is unavailable, use: + +```bash +printf '%s\n' "${context_roots:-.}" | tr ',' '\n' | while IFS= read -r root; do + [ -n "$root" ] && find "$root" -name compile_commands.json -print -quit +done | head -1 +# `find "$root"` is quoted intentionally so a context root containing spaces +# (e.g. "/Users/me/My Repo") survives word-splitting. Do not unquote it. +``` + +If absent, suggest CMake `-DCMAKE_EXPORT_COMPILE_COMMANDS=ON`/Bear/compiledb to the user but continue. + +### Phase 2: Output Directory + +**Entry:** Phase 1 flags set. **Exit:** absolute `output_dir` resolved; `${output_dir}/findings/` exists. + +Resolve an absolute path for `output_dir` (default: `$(pwd)/.c-review-results/$(date -u +%Y%m%dT%H%M%SZ)/`): + +```bash +mkdir -p "${output_dir}/findings" +``` + +### Phase 3: Codebase Context + +**Entry:** `${output_dir}` exists. **Exit:** `${output_dir}/context.md` written. + +Skim `README.{md,rst,txt}` and any build file (`Makefile`, `CMakeLists.txt`, `meson.build`, `configure.ac`) — preflight with the `Glob` tool before any `Read` (a `Read` on a missing file aborts the turn). Do **not** use `Bash: ls README*` for the preflight: under zsh, an unmatched glob aborts the whole compound command before `2>/dev/null` runs (observed: a Phase-3 `ls src/X/README*` call failed with `no matches found` and dropped the entire preflight). If you must use `Bash`, use `find . -maxdepth 2 -name 'README*' -o -name 'Makefile' -o -name 'CMakeLists.txt' -o -name 'meson.build'`, which never fails on no-match. + +Write `${output_dir}/context.md` with: YAML frontmatter (`threat_model`, `severity_filter`, `scope_subpath`, `finding_scope_root`, `context_roots`, `is_cpp`, `is_posix`, `is_windows`, `output_dir`, `compile_commands` as `present`/`absent` plus path when present), then a short markdown body with five sections — **Purpose** (1-3 sentences), **Scope** (what's in `finding_scope_root`, and that findings outside it are out of scope), **Entry points** (where untrusted data enters: network, files, CLI, IPC), **Trust boundaries** (sandboxed vs trusted peers vs arbitrary remote), **Existing hardening** (fuzzing corpora, sanitizers, privilege separation). + +### Phase 4: Build Run Plan (deterministic) + +**Entry:** language flags + `threat_model` known; `${output_dir}/findings/` exists. **Exit:** `${output_dir}/plan.json` and `${output_dir}/worker-prompts/*.txt` written; `M = worker_count` known. + +Selection, filtering, path resolution, and spawn-prompt rendering are **delegated to the script** to prevent the "orchestrator paraphrases the spawn template and drops fields" failure mode: + +```bash +python3 "${C_REVIEW_PLUGIN_ROOT}/scripts/build_run_plan.py" \ + --plugin-root "${C_REVIEW_PLUGIN_ROOT}" --output-dir "${output_dir}" \ + --threat-model "${threat_model}" --severity-filter "${severity_filter}" \ + --scope-subpath "${finding_scope_root:-.}" --context-roots "${context_roots:-.}" \ + --is-cpp "${is_cpp}" --is-posix "${is_posix}" --is-windows "${is_windows}" +``` + +The script writes `plan.json` + `worker-prompts/worker-N.txt` + (if `--cache-primer=true`, the default) `worker-prompts/cache-primer.txt`, and prints a JSON summary on stdout. Exits non-zero on any missing prompt — surface the message and stop. Typical M: 7 (C POSIX), 8 (C++ POSIX), 10 (C POSIX + Windows), 11 (C++ POSIX + Windows). After it returns, `Read plan.json` for the structured selection — never re-derive filtering or paths. + +### Phase 5: Create Bookkeeping Tasks (orchestrator-internal) + +**Entry:** `${output_dir}/plan.json` exists; `M = plan.workers.length`. **Exit:** `cluster_task_ids[]` created (1:1 with `plan.workers`), all `pending`. + +The task ledger is **orchestrator bookkeeping only** (TUI visibility + Phase-7 retry tracking) — workers never read or write it. One `TaskCreate` per worker, populating `metadata` with `kind="cluster"`, `worker_n`, `cluster_id`, `spawn_prompt_path`, `pass_prefixes`, `attempt=1` — all values copied verbatim from `plan.workers[i]`. Track `cluster_task_ids[]` in `plan.workers` order. + +### Phase 6: Spawn workers (optional cache-primer first, then M in parallel) + +**Entry:** `cluster_task_ids[]` populated; per-worker spawn prompt files exist at `${output_dir}/worker-prompts/worker-N.txt`. **Exit:** all M `Agent` calls have returned (the parallel spawn block completed). + +#### Phase 6a: Cache primer (gated on `plan.run.cache_primer`) + +A parallel batch from cold start cannot share cache (all M requests dispatch simultaneously, none has finished writing). To warm the prefix, spawn a tiny primer first — **foreground** (background spawns don't share cache with subsequent foreground spawns). + +If `plan.run.cache_primer == true`, `build_run_plan.py` has written `${output_dir}/worker-prompts/cache-primer.txt`. Spawn it in its own assistant message: `Read` the file, pass verbatim as `Agent` `prompt` with `subagent_type=c-review:c-review-worker`, `model=${worker_model}`, `description="C review cache primer"`, no `run_in_background`. The script wrote the prefix byte-identical to `worker-1.txt` through the `` block — that byte-identity is what gives the parallel workers their cache hit. The primer trailer contains `Cache primer: true`, which the worker system prompt treats as a first-class mode and returns exactly `worker-PRIMER abort: cache primer (no analysis performed)` in one text response with zero tool calls. Discard the abort line — Phase 7 ignores it (no `worker-N` id). + +Foreground spawn already serializes — no `sleep` needed before Phase 6b. Skip Phase 6a entirely if `plan.run.cache_primer == false`. + +#### Phase 6b: Spawn M real workers in ONE message + +> **STOP — read this before composing the spawn message.** +> +> Workers MUST be spawned **foreground** (no `run_in_background` field, or `run_in_background=false`). +> "Parallel" here means *one assistant message containing M `Agent` calls* — that already runs them concurrently. **Background spawns are NOT how you parallelize this skill.** +> +> Background spawns defeat Phase 6a's primer cache: every worker pays full cache-creation on its first turn (`cache_read_input_tokens=0`), and the primer's ~15 K tokens are wasted M times over. Two real runs (audit logs available) had exactly this symptom — every worker started with `first_cr=0`. +> +> Before sending the spawn message, audit your draft: every `Agent` call must have **no** `run_in_background` key. If you wrote `run_in_background=true`, delete it. + +**Required spawn shape:** emit a single assistant message containing M `Agent` tool invocations. Sequential spawning serializes the review and is also wrong, but that failure is loud (timing); the background-spawn failure is silent (cost). + +For each worker `N ∈ [1..M]`: + +1. `Read: ${output_dir}/worker-prompts/worker-N.txt` +2. Pass the file contents **verbatim** as the `Agent` tool's `prompt` argument: + +| Parameter | Value | +|-----------|-------| +| `subagent_type` | `c-review:c-review-worker` | +| `model` | `${worker_model}` (haiku / sonnet / opus) | +| `description` | `C review worker N` | +| `prompt` | the full text of `worker-N.txt` (no edits) | +| `run_in_background` | **field MUST be omitted, OR set to `false`.** Never `true`. See the foreground-spawn warning above. | + +The spawn prompt is the single authority. Pass it verbatim — every field is required by the worker's self-check; any deviation triggers `worker-N abort: spawn prompt malformed`. + +**Anti-patterns to reject:** + +- **Passing `run_in_background=true`** (the dominant historical defect — see warning above). +- Hand-typing the spawn prompt instead of reading `worker-N.txt`. +- Inserting Task-related instructions ("first call TaskList", "Assigned task id: "). Workers have no Task tools. +- Editing the rendered prompt before passing it (trimming "redundant" fields, collapsing pass lists). + +### Phase 7: Wait for Workers and Classify Outcomes + +**Entry:** all M Phase-6 `Agent` calls have returned. **Exit:** every cluster has either succeeded or been retried up to the cap; `${output_dir}/findings-index.txt` written. + +The Phase-6 `Agent` invocations block until each worker returns. Inspect each worker's return text and apply this classifier in order — first match wins: + +| # | Match (in return text) | Outcome | Action | +|---|---|---|---| +| 1 | `worker-N complete:` | **success** | `TaskUpdate` to `completed`. | +| 2 | `abort: spawn prompt malformed`, `abort: pre-work budget exceeded`, or `abort: TaskList unavailable` (legacy) | **non-retryable orchestrator bug** | Stop the run, surface the abort + spawn-prompt path. Re-running the same prompt repeats the failure — pre-work-budget exhaustion always means the worker couldn't pass its self-check, which a retry won't fix. | +| 3 | other `worker-N abort:` | **retryable** | Mark `pending`, set `metadata.abort_reason`, `needs_respawn=true`, increment `attempt`. | +| 4 | `Agent` errored or no `complete:`/`abort:` token | **retryable** | Same as #3 (transient worker crash). | + +If any non-retryable, stop. Otherwise re-spawn each `pending` retryable with `attempt < 2` in one parallel block (cap = 2 attempts per cluster). Replacement workers can safely overwrite partial files — finding IDs are deterministic per prefix. + +#### Sanity-check + write index + +For every `complete:` cluster, list `${output_dir}/findings/${prefix}-*.md` for each `pass_prefix` (from `plan.json`). A worker that says "wrote N finding files" with N>0 but zero files on disk is **suspicious** — treat as retryable (classifier row #4). Zero claimed + zero on disk is fine. + +Then build the index — workers wrote per-worker shards under `${output_dir}/findings-index.d/`, prefer those: + +```bash +# Use `find` rather than a `worker-*.txt` glob: zsh aborts the compound command on no-match +# even with `2>/dev/null`, so an empty findings-index.d would otherwise drop the index file. +# `awk 1` (vs `cat`) normalizes a missing trailing newline on any shard, so a future +# worker that writes shards via Write/printf instead of `ls -1 | sort` can't silently glue +# the last path of one shard onto the first of the next when sort -u dedupes. +if [ -d "${output_dir}/findings-index.d" ]; then + find "${output_dir}/findings-index.d" -maxdepth 1 -type f -name 'worker-*.txt' -exec awk 1 {} + 2>/dev/null \ + | sort -u > "${output_dir}/findings-index.txt" +else + find "${output_dir}/findings" -maxdepth 1 -type f -name '*.md' 2>/dev/null | sort > "${output_dir}/findings-index.txt" +fi +``` + +`sort -u` collapses duplicates from Phase-7 retries. Empty file is the unambiguous "zero findings" signal. Cross-check the line count against the sum of `wrote N` worker claims; log mismatches but don't abort. + +After task updates and index creation, run `TaskList` and write `${output_dir}/run-summary.md` with: + +- resolved parameters (`threat_model`, `severity_filter`, `finding_scope_root`, `context_roots`, language/platform flags, compile-commands status) +- worker outcome table (`worker_n`, `cluster_id`, claimed finding count, shard line count, task status, retry/abort state) +- `findings-index.txt` line count and any mismatch against worker claims +- judge status once Phase 8 finishes, or the reason a judge was skipped/failed + +If any Phase-5 cluster task is not `completed`, include it prominently in `run-summary.md` and the final response. Do not hide a partial run behind a successful report. + +**Always run Phase 8 even on zero findings** — both judges short-circuit on an empty index: dedup-judge writes a minimal no-op `dedup-summary.md`, and fp-judge writes empty `REPORT.md`/`REPORT.sarif` so SARIF consumers get a stable artifact set. + +### Phase 8: Judge Pipeline (sequential, dedup → fp+severity) + +**Entry:** `findings-index.txt` exists. **Exit:** dedup-judge and fp-judge have returned; `dedup-summary.md`, `fp-summary.md`, `REPORT.md`, and ideally `REPORT.sarif` are written. + +Each judge's full protocol is its system prompt (`agents/c-review-{dedup,fp}-judge.md`); spawn prompts pass only per-run variables. Do **not** reference `prompts/internal/judges/` — those files don't exist. + +Spawn sequentially (dedup first, fp-judge sees only merged primaries): + +- `Agent(subagent_type="c-review:c-review-dedup-judge", description="Dedup judge", prompt=f"output_dir: {output_dir}")` +- `Agent(subagent_type="c-review:c-review-fp-judge", description="FP + severity judge", prompt=f"output_dir: {output_dir}\nsarif_generator_path: {sarif_generator_path}")` — resolve `sarif_generator_path` to `${C_REVIEW_PLUGIN_ROOT}/scripts/generate_sarif.py`. + +**Judge failure handling.** Same shape as Phase 7's classifier, applied to judge return text: + +- `… complete:` → **success.** +- `… abort:` → **non-retryable.** Surface the abort line plus `ls -l ${output_dir}/findings-index.txt`; stop. +- No `complete:` (help message / error / question) → **retryable once.** `SendMessage(to=, …)` rather than a fresh spawn (the agent already paid the protocol-parse cost). Include the explicit finding paths from `findings-index.txt`. If the second try still fails, surface the transcript and continue to Phase 8b. + +### Phase 8b: SARIF safety net + +**Entry:** fp-judge returned, or the run aborted early. **Exit:** `${output_dir}/REPORT.sarif` exists. + +```bash +test -d "${output_dir}/findings" && python3 "${C_REVIEW_PLUGIN_ROOT}/scripts/generate_sarif.py" "${output_dir}" +``` + +Run unconditionally whenever `findings/` exists — generator is idempotent (full overwrite), emits `results: []` for zero-survivor runs, and handles partial runs (findings without `fp_verdict` are emitted as `LIKELY_TP` rather than being silently dropped). Always overwriting protects against the case where fp-judge crashed mid-write and left a corrupt `REPORT.sarif` on disk. Skip only if `${output_dir}/findings/` doesn't exist (Phase 2 failed). After this phase, update `${output_dir}/run-summary.md` with judge/SARIF status. + +### Phase 9: Return Report + +**Entry:** Phase 8b complete. **Exit:** every item in [Success Criteria](#success-criteria) verified true; `REPORT.md` returned to the caller. + +Before composing the response, walk the [Success Criteria](#success-criteria) checklist below and confirm each bullet against on-disk artifacts (`TaskList` for cluster tasks, `ls`/`Read` for the files). If any criterion fails, surface the failure prominently in the response — do **not** hide a partial run behind a successful report. + +Then `Read ${output_dir}/REPORT.md` and return its content to the caller. Append an Artifacts list pointing at `findings/`, `findings-index.txt`, `run-summary.md`, `dedup-summary.md`, `fp-summary.md`, `REPORT.md`, `REPORT.sarif`. + +--- + +## Finding file frontmatter — three stages + +Authoritative schema: `agents/c-review-worker.md` ("Finding File Format"). Three-stage write: + +1. **Worker** — base fields (`id`, `bug_class`, `title`, `location`, `function`, `confidence`, `worker`) + seven body sections. +2. **Dedup-judge** — adds `merged_into` on duplicates, or `also_known_as` + `locations` on primaries that absorbed. +3. **FP+Severity judge** — adds `fp_verdict` + `fp_rationale` on every primary; on survivors (`TRUE_POSITIVE`/`LIKELY_TP`) also adds `severity`, `attack_vector`, `exploitability`, `severity_rationale`. + +## Bug classes / clusters + +Authoritative: `prompts/clusters/manifest.json`. 47 always-on bug classes, up to 64 with all conditional clusters enabled. `buffer-write-sinks` is fully consolidated (its sub-prompts are not re-read at runtime). + +--- + +## Success Criteria + +The phase exits already cover most of this; the orchestrator-visible end-state is: + +- Every Phase-5 cluster task is `completed` (verify via `TaskList`). +- `${output_dir}/run-summary.md` exists and records resolved scope/context, compile-commands probe result, worker claims vs index count, task status, and judge/SARIF status. +- Every primary finding (no `merged_into`) has `fp_verdict` + `fp_rationale`; every survivor (`TRUE_POSITIVE`/`LIKELY_TP`) also has `severity`, `attack_vector`, `exploitability`, `severity_rationale`. +- `REPORT.md` exists, severity-filtered per `severity_filter`. +- `REPORT.sarif` exists (Phase 8b safety net guarantees this).