diff --git a/.claude/skills/ipxe-security-review/SKILL.md b/.claude/skills/ipxe-security-review/SKILL.md new file mode 100644 index 000000000..e680e4543 --- /dev/null +++ b/.claude/skills/ipxe-security-review/SKILL.md @@ -0,0 +1,58 @@ +--- +name: ipxe-security-review +description: >- + Systematic memory-safety review of iPXE's attacker-facing parsers. Use + when asked to hunt for vulnerabilities, audit a network-input parser, or + review a FILE_SECBOOT(PERMITTED) file that handles DHCP / DNS / TFTP / + HTTP / TLS / EAP / PeerDist data or downloaded images, for length, + overflow, or underflow defects. +--- + +# iPXE security review + +The working conventions — threat model, scope, reporting format, and the +codebase bounds contracts — are in [`AGENTS.md`](../../../AGENTS.md). Read +them first. This skill is the step-by-step sweep procedure that applies +them. + +## The sweep loop + +Work one file at a time. Stop as soon as you find a confirmed defect. + +1. **Pick a target.** A `FILE_SECBOOT ( PERMITTED )` file that parses an + attacker input (threat model in `AGENTS.md`). Prefer wire parsers with + manual length / offset arithmetic. +2. **Find the entry point** — the `*_rx` / deliver / parse function where + attacker bytes first arrive together with a length. +3. **Establish reachability** — confirm the value is attacker-controlled + and the path is reachable from network input, not reached only + through trusted input (operator-configured settings or the + embedded/boot script). +4. **Trace the arithmetic.** Follow every length, offset, and count from + the wire to its use. Flag: + - `size_t` underflow — `a - b` where `a < b` yields ~`SIZE_MAX` + (reversed operands, or a missing header / trailer term). + - integer truncation — a length held in `int` / `unsigned int` then + used as `size_t` (LP64: 64-bit `size_t`, 32-bit `int`). + - unchecked `iob_pull` / `iob_unput` (no bounds check — see + `include/ipxe/iobuf.h`). +5. **Verify against the real allocation.** Use the documented helper + contracts (`AGENTS.md` → Codebase contracts). Account for iobuf + head / tailroom and the guaranteed end-of-image NUL. +6. **Classify.** An actual reachable defect → report. A provably in-bounds + boundary case → note privately and move on. Do not emit theoretical + findings. +7. **Report, then stop.** + +## Reporting + + ! path/file.c:NNN → + +Follow with a few sentences: how it is reached, and the consequence. +Nothing more. Stop so the maintainer can patch — do not batch defects and +do not open PRs. Keep findings private until fixed. + +## Do not flag (see `AGENTS.md` → Patterns not to misread) + +Composable-cleanup unreachable cleanup; count-then-clamp logical lengths; +end-of-image NUL reads; compiler-elided VLAs. diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 000000000..6f7b67a7e --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,177 @@ +# iPXE — agent guidance + +iPXE is a network bootloader that runs pre-OS, in ring 0 / UEFI boot +services, frequently in Secure-Boot-signed builds. A memory-safety defect +in code that parses attacker-supplied data is therefore a pre-boot +code-execution / Secure-Boot-bypass primitive, not merely a crash. Review +standards are correspondingly high. + +This file is tool-neutral: any coding agent should follow it. Claude Code +users additionally have `CLAUDE.md` and the `ipxe-security-review` skill, +which point back here. + +The authoritative description of the threat model, the `FILE_SECBOOT` +scope semantics, the ring-0 / Secure-Boot stakes, and the exclusions +(malicious hardware, USB, UEFI peers, Infiniband) lives in +`doc/threat_model.dox`. **Read it first.** The bounds contracts of the +core helpers live **at the source**, in Doxygen documentation blocks; +the Notes section of `doc/threat_model.dox` is the definitive index of +links to them. Both are authoritative; the notes below are operational +pointers for review work, not a second copy. + +## Contributions must have a human owner + +`AGENTS.md` governs all agent activity here, not just security review. +Do not open pull requests or issue reports autonomously or unattended; +anything submitted upstream must be understood, owned, and defensible by +a human contributor who has reviewed it. Per the "(Ab)use of AI" policy +in `CONTRIBUTING.md`, unsolicited or unreviewed AI-generated pull +requests and issue reports are not welcome and result in a ban. The one +sanctioned exception is a concise, verified security vulnerability +report filed via `SECURITY.md`. + +## Threat model & scope + +Authoritative: `doc/threat_model.dox`. Operational deltas for reviewers: + +- Concrete attacker inputs to hunt: DHCP / DHCPv6, DNS, TFTP, HTTP(S) + headers / bodies / redirects, TLS, 802.1X / EAP / EAPoL, PeerDist / + PCCRC, and downloaded images (ELF / PNM / …). At the weaker edge levels + the model describes: USB descriptors, inputs from consumers of the EFI + protocols iPXE *produces* (SNP, PXE Base Code, USB I/O, download), and + platform-firmware data iPXE consumes (UEFI `LoadOptions`, `Boot####` + device paths, configuration tables). +- `FILE_SECBOOT ( PERMITTED )` is the priority surface. Unmarked files are + **lower priority but still in scope** — not "out of scope". `FORBIDDEN` + is excluded by policy, not by cleanliness. +- Check each *file's own* `FILE_SECBOOT` marker, not its directory's: a + file outside a subsystem's directory can silently miss the marker its + siblings carry (an ONC-RPC helper outside `net/oncrpc` did exactly this). + +## Reporting conventions + +- One line per finding: + `! path/file.c:NNN → ` +- `!` marks a high-confidence memory-safety defect. +- Report only **actual, reachable** defects. Do the rigorous verification + privately; keep the emitted report terse. (Verbose, speculative, + AI-generated reports are actively unwelcome.) +- **One defect at a time** — stop after each confirmed defect so the + maintainer can patch it. Do not batch findings. +- **Keep findings private until fixed.** No public issue or PR may + describe an unfixed defect. Report a confirmed defect through the + process in `SECURITY.md` — as a GitHub Security Advisory, and an + AI-generated report must be filed against `ipxe/aipxe`, not + `ipxe/ipxe`. Published artefacts (commit messages, the threat-model + doc) stay descriptive: omit historical exploitation specifics that + carry only offensive value (e.g. a pre-fix corruption window), even + for already-fixed defects. +- **The maintainer writes the patch.** Identify the *correct fixed + behaviour* first: e.g. a helper named after a POSIX function owes the + POSIX validation contract, so validation belongs in that callee. + +## Verify before reporting + +A sweep produces **candidates, not findings** — however a candidate is +generated (a manual pass, or a fan-out of sub-agents partitioned by +subsystem). Every candidate must be verified first-hand against the source +(field types, the specific guard present or absent, the reachable call +path) before it is reported. A confident sub-agent write-up is an input to +that verification, never a substitute. Report only what survives. + +## Review procedure (per target file) + +1. Confirm the file's attacker input (above) and note its `FILE_SECBOOT` + status — `PERMITTED` is priority, unmarked is lower priority. +2. Find the wire entry point (the RX / deliver / parse function where + attacker bytes first arrive together with a length). +3. Confirm the suspect value is attacker-controlled and the path is + reachable from network input — not reached only through trusted + input such as operator-configured settings or the embedded/boot + script. +4. Trace every length / offset / count from the wire to its use. Watch for: + - `size_t` underflow (`a - b` with `a < b` → ~`SIZE_MAX`); + - additive overflow before a bounds check (`a + b > limit`, computed in + `uint32_t` / `size_t`, wraps and passes — recurred across SRP/FCP, + ELF program headers, and FIP descriptors; the correct idiom is the + subtraction form the tree uses elsewhere: + `if ( a > limit || b > limit - a )`); + - 32→64-bit truncation (LP64: `int` / `unsigned int` are 32-bit, + `size_t` / `long` 64-bit); + - missing header / trailer accounting. + (Signed shift into the sign bit — `1 << 31` — is now caught at build + time by `-Wshift-overflow=2`; do not hand-hunt it.) +5. Verify each access against the *real* allocation, using the documented + helper contracts — do not re-derive them. +6. Classify the finding against `doc/threat_model.dox` — reachable ≠ + interesting: + - a defect exploitable only by code already at iPXE's privilege is + uninteresting (a malformed payload handed to a ring-0 loader — `nbi`, + `elf` — can do nothing it couldn't do by simply executing; treat as + hardening); + - a driver defect triggered only by DMA-capable hardware is out of + model; a USB / EFI-peer / firmware-input defect is on the edge (fix + defensively, but rank accordingly). + State the classification in the report so the maintainer can prioritise. +7. Report terse; stop. + +## After the fix: review the PR + +The maintainer writes the patch and opens a PR; reviewing it is part of +the loop. Confirm the diff actually closes the finding (correct idiom, +ordering matched to sibling checks, no new NULL-deref or regression) **and** +that the commit message's reachability / impact claims are accurate — +inversions and typos have both slipped through and are worth catching. + +## Adjacent passes (not memory-safety) + +You may be asked for these; they use a different lens: + +- **Correctness / UB over `PERMITTED` drivers** — malicious hardware is + still out of model; check only correctness and undefined behaviour. +- **Missing little-endian conversions** (`cpu_to_leXX` / `leXX_to_cpu`), + best found by intra-file inconsistency (a wire / table / register field + converted in most sibling accesses but host-order in one). Almost always + **latent**: iPXE's hardware targets are little-endian, and the only + big-endian target (s390x) runs solely as a Linux executable with no + hardware drivers — so a driver-side miss is a correctness blemish, not + exploitable. Only core / firmware-table code reachable on s390x-linux + (e.g. an on-disk or config-table magic compared in host order) can be + live. + +## Codebase contracts (authoritative docs live at the source) + +Cross-referenced from the Notes section of `doc/threat_model.dox`. Use +them; do not re-derive: + +- `include/ipxe/iobuf.h` — the `iob_*` accessors (`iob_pull`, `iob_unput`, + …) are bare pointer arithmetic with **no** production bounds check. The + caller must validate lengths against `iob_len()` first. +- `crypto/asn1.c` — `asn1_cursor` helpers are self-checking; walking a + structure via the helper API is bounds-safe by construction (a parse + error invalidates the cursor to zero length). +- `core/xferbuf.c` — `xferbuf_*` accumulation is length-checked + (`ensure_size` before copy). +- `core/vsprintf.c` — `ssnprintf` / `vssnprintf` clamp a negative + remaining size to zero, so `used += ssnprintf(buf+used, len-used, …)` + is safe as `used` approaches `len`. +- `core/malloc.c` — allocators are safe against malicious sizes and return + NULL on failure (including a zero-size request); callers must handle + NULL. +- `include/errno.h` — the composable structured-`goto` cleanup pattern + (the cleanup that undoes a *successful* operation sits *above* that + operation's `err_*` label, so an unreachable cleanup statement + immediately before the first label is deliberate future-proofing, not + dead code). + +## Patterns not to misread as bugs + +- **Count-then-clamp** — a parser may return a logical length larger than + it actually wrote, having clamped the write to the buffer size. This is + intentional. +- **End-of-image read** — downloaded images carry a guaranteed trailing + NUL past `image->len` (see `include/ipxe/image.h`); a one-byte read + there is in-bounds. +- **Elided VLAs** — an attacker-sized local used only via `typeof` / + `sizeof` may be elided by the compiler at `-Os` / `-O2` but allocated on + the stack at `-O0`; do not conclude "unreachable" from one build. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 000000000..9af606c9e --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,16 @@ +# iPXE — Claude Code guidance + +Agent conventions for this repository are tool-neutral and live in +[`AGENTS.md`](AGENTS.md) — follow them. This file adds only the Claude +Code-specific pieces. + +- **Security review:** for vulnerability-hunting work, invoke the + **ipxe-security-review** skill + (`.claude/skills/ipxe-security-review/`). It encodes the sweep procedure + and the terse reporting format described in `AGENTS.md`. +- **Codebase knowledge is at the source.** The bounds contracts of core + helpers (`iob_*`, `asn1_cursor`, `xferbuf_*`, `ssnprintf`) and the + coding patterns (composable cleanup, count-then-clamp) are documented in + Doxygen documentation blocks at the source, indexed from the Notes + section of `doc/threat_model.dox`. Treat those as authoritative; do not + re-derive them.