# DF-0414 — PPPoE discovery `ph->length` unchecked → OOB heap read

## Verdict

**REPRODUCED.** Remote-kernel memory-safety bug confirmed by kernel panic in
`get_tag()` at `sys/netgraph/pppoe/ng_pppoe.c:297`. **Fix validated**: the
single-line bounds check closes the bug deterministically.

## Important note about the cited file

The finding cites `sys/netgraph7/pppoe/ng_pppoe.c` (the new netgraph7 PPPoE).
**`netgraph7` is NOT built by default** — `sys/conf/files` lists
`netgraph7/pppoe/ng_pppoe.c optional netgraph7_pppoe`, and
`X86_64_GENERIC` does not enable `NETGRAPH7_PPPOE`. The module actually
shipped in `/boot/kernel/ng_pppoe.ko` and reachable from userland via
`ng_socket` is the **old netgraph** one at `sys/netgraph/pppoe/ng_pppoe.c`
(verified: `strings /boot/kernel/ng_pppoe.ko | grep netgraph/pppoe`).
The same bug pattern is present there; this PoC and fix target the
old-netgraph file because that is the production-reachable code.

## Mechanism (trigger → primitive → effect)

`ng_pppoe_rcvdata()` (`sys/netgraph/pppoe/ng_pppoe.c:881`) receives an mbuf on
the node's `ethernet` hook. For discovery frames (`ether_type == 0x8863`) it
enters the discovery branch (`:943`) which:

1. Computes `length = ntohs(wh->ph.length)` at `:925` —
   **attacker-controlled 16-bit value taken straight from the packet header**.
2. `m_pullup`s the mbuf to `m_pkthdr.len` so the data is contiguous
   (`:952`–`:968`).
3. Calls `get_tag(ph, PTT_SRV_NAME)` at `:980`.

`get_tag()` (`:283`) computes its walk bound from `ph->length`:
```c
const char *const end = (const char *)next_tag(ph);   // &ph->tag[0] + ntohs(ph->length)
```
(`next_tag()` at `:271`–`:276`). The walker then iterates
`pt = &ph->tag[0]`, advancing by `sizeof(tag_hdr) + ntohs(pt->tag_len)` each
iteration, until `pt+1 > end` or `ptn > end`:

```c
while((const char*)(pt + 1) <= end) {
    ptn = (((const char *)(pt + 1)) + ntohs(pt->tag_len));   // <-- OOB read here
    if(ptn > end) return NULL;
    if(pt->tag_type == idx) return pt;
    pt = (const struct pppoe_tag*)ptn;
}
```

If `ph->length > m_pkthdr.len - sizeof(*wh)` (i.e. the claimed payload exceeds
the real payload), `end` is past the mbuf. After the first non-matching tag
the walker sets `pt = ptn` past the mbuf, then on the next iteration reads
`pt->tag_len` (`movzwl 0x2(%rax),%edx` at `get_tag+0x12`) **from kernel heap
past the mbuf data**.

The session branch (`:1210`) **does** validate `m->m_pkthdr.len < length`
at `:1221` (after `m_adj(sizeof(*wh))`). The discovery branch omits this
check — the asymmetry is the bug.

### Observable effects

* **Panic / DoS** — if the OOB walk crosses into an unmapped page, the kernel
  takes a page fault in `get_tag`. Reproduced (see `panic.txt`).
* **Silent OOB read** — if the OOB region is mapped (mbuf zone is large and
  contiguous), the walk reads kernel heap without faulting. 30+ such walks
  were observed per run as `"no service tag"` prints in dmesg (that kprintf
  at `:982` is only reachable if `get_tag()` returns NULL, which with
  `ph->length=0xFFFF` and a non-matching first tag requires the walker to
  advance past the mbuf).
* **Info leak (reachable, not demonstrated end-to-end)** — `scan_tags()`
  (`:1622`) uses the same `end` calculation and calls `insert_tag()` for any
  OOB byte sequence that happens to look like `tag_type == PTT_RELAY_SID`.
  In the PADR/PADO code paths this would copy OOB heap bytes into outgoing
  discovery responses. A malicious PADO/PADR with `ph->length > mbuf` could
  therefore leak heap bytes back to the attacker.

## Trigger

`trigger.c` builds the netgraph topology from userland (no admin wiring
beyond `kldload ng_socket ng_pppoe`):
- creates an `ng_socket` node and an `ng_pppoe` peer connected as
  `mydata <-> pppoe:ethernet` via `NGM_MKPEER`;
- injects a 24-byte Ethernet/PPPoE frame via `NgSendData()` with
  `ether_type=0x8863`, `code=PADI`, `ph->length=0xFFFF`, and a single
  4-byte tag with `tag_type=0x0001` (not `PTT_SRV_NAME`) and `tag_len=0`.

`trigger_spray.c` additionally opens 20 000 sockets and maps 64 anon pages
before triggering 50 times, in order to pressurize the mbuf zone so the
trigger mbuf is allocated near a slab/page boundary and the OOB walk is
more likely to hit unmapped memory.

### Panic signature (both baseline runs, identical IP `0xffffffff82656022`)

```
Fatal trap 12: page fault while in kernel mode
fault virtual address	= 0xfffff8011c100002     (1st run)
fault virtual address	= 0xfffff8011b400002     (2nd run, after vm reset)
fault code		= supervisor read data, page not present
instruction pointer	= 0x8:0xffffffff82656022
current process		= Idle
Stopped at      get_tag+0x12:   movzwl  0x2(%rax),%edx     <- read pt->tag_len
db>
```

`get_tag+0x12` is exactly the `ntohs(pt->tag_len)` read at `:297`, i.e. the
out-of-bounds read the finding predicts.

## Exploit chain / escalation discussion

This bug is a **read-only OOB** primitive — the immediate effect is a kernel
**read** of `pt->tag_type`/`pt->tag_len` from heap past the mbuf. There is
no corruption of any victim object (no write, no UAF, no type confusion). Per
Phase 6, a read-only primitive has no `uid=0` chain derivable from itself; the
deliverable impact is the realistic ceiling:

* **DoS / panic** (demonstrated — single malicious PADI on a host whose
  admin has wired ng_pppoe to an interface).
* **Kernel heap info leak** up to `65535 - 4` bytes per packet (reachable via
  `scan_tags`/`insert_tag` in PADR/PADO; would leak kernel heap pointers,
  defeating KASLR, and mbuf/slab metadata). On this guest KASLR is already
  off, so the leak's main value on other systems is KASLR bypass and heap
  layout disclosure for use by a separate write-class bug.

So **`impact=panic`** (the demonstrated ceiling on this guest); the read-only
nature is why no `uid=0` chain was pursued.

## Fix

`fix.diff` — adds the missing bounds check in the discovery branch,
mirroring the session branch's `:1221` check. One logical change, 18 lines
including comment:

```c
+			if (m->m_pkthdr.len - sizeof(*wh) < length) {
+				kprintf("pppoe: discovery ph->length too large\n");
+				LEAVE(EMSGSIZE);
+			}
```

Placed after the `m_pullup` block that guarantees `m_pkthdr.len >= sizeof(*wh)`
(so the subtraction cannot underflow), immediately before `switch(code)` at
`:970` and therefore before any `get_tag()`/`scan_tags()` call in the
discovery branch.

### Fix validation (Phase 8)

| Kernel | Build | Trigger result |
|---|---|---|
| `#0` unpatched baseline (`with-src`) | — | **PANIC** `get_tag+0x12` after ~10–40 trigger iterations |
| `#1` patched (same source + `fix.diff`) | `make -j6 nativekernel` `rc=0` | 50× `EMSGSIZE` per spray run, **no panic**, 3 spray runs in a row clean |

Sanity check (`sanity.c`): a well-formed PADI with `ph->length == 4` matching
the actual 4-byte tag payload is **accepted** (returns `ENETUNREACH` for
"no service hook configured", **not** `EMSGSIZE`) — the fix is precise and
does not break legitimate PPPoE traffic.

The same `ph->length` validation should also be applied to the
`netgraph7/pppoe/ng_pppoe.c` discovery branch for defense-in-depth, but since
netgraph7 is not built/loaded by default on DragonFly, that file is not the
production-reachable target.

## Files

* `trigger.c` — minimal reproduction (single inject, observes ENETUNREACH or panic)
* `trigger_spray.c` — heap-pressure variant that reliably panics the baseline
* `sanity.c` — well-formed-packet regression test for the fix
* `build.sh` / `run.sh` — exact build & run commands
* `fix.diff` — git-apply-able patch closing the bug
* `panic.txt`, `baseline_panic.log`, `boot_full.log` — crash evidence
* `env.txt` — guest environment