From 9d418130631e9c03bd2026069efac6acdd78ea87 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Sun, 23 Aug 2026 16:10:46 +0000 Subject: [PATCH 1/5] Nine of ten malformed frames over-read at the pin, and none of the three fixes is finished MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Research only, from issue #142. No Attadipa code changed and none should have: this repository links no MeshCore. What changes is the evidence a future pin decision is made on, and the criteria are now written down instead of implied. Three upstream pull requests claim missing length checks in MeshCore's frame and advert parsers. The issue was cautious about comparing them to our pin because they are based on `dev`, and that caution turns out to be unnecessary: every file any of them touches is byte-identical between `d929643` and both of their bases, so their diffs apply to us unchanged and a measurement on a pull request head is a measurement of our pin plus that pull request's guards. `d929643` is also upstream's current `main` tip and its newest release, so the pin is not lagging — nothing containing these guards has shipped. The findings hold. A harness compiling four upstream translation units unmodified, with each input exactly its declared length ending flush against a PROT_NONE guard page under ASan, over-reads on nine of ten cases at named lines. The first version used a tight malloc alone and ASan does not report a read at offset 0 of a malloc(0), which silently passed two real findings; the guard page is the mechanism of record and the note is in the harness so the next person does not lose the same hour. What "out-of-bounds" is worth here is smaller than it sounds for the three the pull requests describe. At every reachable call site the backing buffer is a fixed array big enough that the read stays inside the allocation -- 256 B in Dispatcher::checkRecv, 262 B and 250 B in the two bridges, the Packet object itself for adverts -- and all nine cases end in a rejected packet. The one path where Attadipa is the party supplying the bytes, CMD_IMPORT_CONTACT, cannot reach the worst of them at all: it gates on len > 98 and Packet::readFrom reaches at most byte 70. Two things the pull requests do not say, and one nobody has said. #3269's entire diff logs the condition and then performs the read -- no break, no return -- and #3270 leaves app_data[0] unguarded for app_data_len == 0, measured on its own head, so both are incomplete against their own findings and #3271 is the same commit as #3270 rather than merely equivalent. Separately, Utils::decrypt rounds its output up to whole blocks and documents the precondition its on-air caller does not honour: 192 bytes into a 184-byte stack buffer for payload_len 181..184, reproduced against the real translation unit. That one is a write, it is in no pull request, and both it and the PATH extra_len underflow sit behind a two-byte MAC, which is not the authentication gate it looks like. Reporting that upstream is the owner's call and deliberately not done here. attadipa_link's own decoder was checked rather than assumed and is not analogous -- it validates the declared length before reading, behind a length check and a CRC. Nothing in link/ changed. No ADR changed either: ADR-0008 already treats the node as a peer, and an ADR edited to say what it said is churn. What could not be established is a section of its own, not a footnote: neither arithmetic finding was driven end to end, the stale bytes they read may or may not be groomable, what the eight over-written bytes hit on an ESP32-S3 is a frame layout nobody here has compiled, and a ten-case corpus demonstrates rather than searches. M15-M18. The harness is kept because the alternative to a runnable one is a table nobody can re-derive when upstream moves; it is under docs/ so no glob can sweep it into a build. Running it is now the entry condition on T-013's pin choice, and path_arith and decrypt_bounds are not in its matrix, so a green run.sh is not a clean revision. Neither README needed a counterpart edit -- nothing in either describes MeshCore revisions or this research. Tests: cmake configure, build and ctest -- 24/24 pass, unchanged by this branch, which touches no code. Harness verified from the committed copy. Nothing ran on a radio, a node or a board: NOT EXECUTED -- HARDWARE REQUIRED. Fixes #142 Co-Authored-By: Claude Opus 5 --- STATUS.md | 20 +- TASKS.md | 65 ++ docs/research/MESHCORE_PARSER_BOUNDS.md | 577 ++++++++++++++++++ docs/research/OPEN_QUESTIONS.md | 25 + docs/research/REUSE_LEDGER.md | 38 +- docs/research/VERIFIED_FACTS.md | 78 +++ .../meshcore-parser-bounds/.gitignore | 1 + .../research/meshcore-parser-bounds/README.md | 63 ++ .../meshcore-parser-bounds/build-extras.sh | 38 ++ docs/research/meshcore-parser-bounds/build.sh | 64 ++ .../meshcore-parser-bounds/decrypt_bounds.cpp | 39 ++ .../meshcore-parser-bounds/harness.cpp | 206 +++++++ .../meshcore-parser-bounds/path_arith.cpp | 88 +++ docs/research/meshcore-parser-bounds/run.sh | 50 ++ .../meshcore-parser-bounds/shim/AES.h | 14 + .../meshcore-parser-bounds/shim/Arduino.h | 28 + .../meshcore-parser-bounds/shim/SHA256.h | 15 + .../meshcore-parser-bounds/shim/Stream.h | 2 + .../meshcore-parser-bounds/shim/shim.cpp | 2 + 19 files changed, 1411 insertions(+), 2 deletions(-) create mode 100644 docs/research/MESHCORE_PARSER_BOUNDS.md create mode 100644 docs/research/meshcore-parser-bounds/.gitignore create mode 100644 docs/research/meshcore-parser-bounds/README.md create mode 100755 docs/research/meshcore-parser-bounds/build-extras.sh create mode 100755 docs/research/meshcore-parser-bounds/build.sh create mode 100644 docs/research/meshcore-parser-bounds/decrypt_bounds.cpp create mode 100644 docs/research/meshcore-parser-bounds/harness.cpp create mode 100644 docs/research/meshcore-parser-bounds/path_arith.cpp create mode 100755 docs/research/meshcore-parser-bounds/run.sh create mode 100644 docs/research/meshcore-parser-bounds/shim/AES.h create mode 100644 docs/research/meshcore-parser-bounds/shim/Arduino.h create mode 100644 docs/research/meshcore-parser-bounds/shim/SHA256.h create mode 100644 docs/research/meshcore-parser-bounds/shim/Stream.h create mode 100644 docs/research/meshcore-parser-bounds/shim/shim.cpp diff --git a/STATUS.md b/STATUS.md index 476a4742..fbd94a27 100644 --- a/STATUS.md +++ b/STATUS.md @@ -1,6 +1,6 @@ # Status -Last updated: 2026-08-22 +Last updated: 2026-08-23 Shape fixed by [final §93](docs/master-prompt-final.md). It is a status file, not a history — what changed and why lives in git and in the ADRs. @@ -64,6 +64,24 @@ Both owner amendments of 2026-08-21 are **closed**. They are recorded as eleven small tasks rather than one large one, which is what the owner asked for in both cases. +- **T-051 — MeshCore parser bounds at the pin — done (research only).** + [`docs/research/MESHCORE_PARSER_BOUNDS.md`](docs/research/MESHCORE_PARSER_BOUNDS.md), + with a runnable harness in + [`docs/research/meshcore-parser-bounds/`](docs/research/meshcore-parser-bounds/). + Three upstream pull requests claim missing length checks in the frame and + advert parsers. They hold at `d929643` — nine of ten corpus cases over-read — + and the pin turns out to be upstream's *current* release, not a lagging one. + Three things the pull requests do not say: **#3270 leaves its own finding + open** (`app_data[0]` is still read when `app_data_len == 0`), **#3269 logs the + condition and then does the thing**, and there is a **fifth defect in + `Utils::decrypt` that none of them mentions and that is a write** — 192 bytes + into a 184-byte buffer, reproduced. Every one of the three parser findings ends + in a rejected packet with the read staying inside its allocation at every real + call site, so the blast radius is far smaller than "out-of-bounds" suggests; + the two arithmetic ones leave the buffer and are the ones that matter. **No + Attadipa code changed and none should**: we link no MeshCore. It moves the + criteria for a future local provider's pin, nothing else. `attadipa_link`'s own + decoder was checked and is not analogous. Left open as M15–M18. - **T-041 — MeshCore 1.17 upstream review — done.** [`docs/upstream/meshcore-1.17-review.md`](docs/upstream/meshcore-1.17-review.md). Ten of the thirteen owner-named pull requests are **still open**, so most of diff --git a/TASKS.md b/TASKS.md index f7c23534..559a001a 100644 --- a/TASKS.md +++ b/TASKS.md @@ -901,6 +901,14 @@ stale silently. The protocol is - **Tests:** host — the third boundary test, alongside `capability_boundary_negative` and `l10n_boundary_negative`. - **Hardware required:** no. +- **Note added 2026-08-23 (T-127):** "bumping the pin cannot require an edit + above the adapter" is a compile-time property, and it is not the whole of what + a bump costs. The pinned revision has five verified parser defects, two of + which leave a buffer + ([MESHCORE_PARSER_BOUNDS](docs/research/MESHCORE_PARSER_BOUNDS.md)), so the + adapter is also where a malformed-frame consequence stops being MeshCore's and + starts being ours. That does not change this task's acceptance; it says which + boundary the corpus in T-013's entry condition is protecting. ### T-013 · The local mesh integration spike - **Priority:** P0 @@ -926,6 +934,15 @@ stale silently. The protocol is - **Hardware required:** for a working link, yes — and **two** radio devices (A3). For the cost numbers, no. - **Constraint that is already fixed:** `Arduino.h` does not enter `core/`. +- **Entry condition added 2026-08-23 (T-127):** whichever revision this spike + proposes to pin, run the corpus in + [`docs/research/meshcore-parser-bounds/`](docs/research/meshcore-parser-bounds/) + against it first, and run `path_arith` and `decrypt_bounds` too — they are not + in the matrix, so a green `run.sh` is not a clean revision. At `d929643` the + answer is five known defects, two of which leave a buffer + ([MESHCORE_PARSER_BOUNDS](docs/research/MESHCORE_PARSER_BOUNDS.md)). That is + not a reason to avoid MeshCore; it is the thing a pin has to be chosen with + knowledge of, and a version number does not carry it. ### T-016 · Benchmark the node protocol encoding, then accept or replace it - **Priority:** P1 @@ -1817,6 +1834,54 @@ Recommended next action: ## DONE +### T-127 · MeshCore parser bounds at the pinned revision — **DONE** 2026-08-23 +Research only, from [#142](https://github.com/hleserg/Attadipa/issues/142). Full +record: [MESHCORE_PARSER_BOUNDS](docs/research/MESHCORE_PARSER_BOUNDS.md); +harness and corpus: +[`docs/research/meshcore-parser-bounds/`](docs/research/meshcore-parser-bounds/). +**No Attadipa code changed, and none should have** — we compile no MeshCore. + +- **The pin is not lagging.** `d929643` is simultaneously our pin, upstream's + `main` tip and the newest release. Every file the three pull requests touch is + byte-identical between the pin and both of their `dev` bases, so their diffs + apply to us unchanged and a measurement on a pull request head is a measurement + of our pin plus that pull request's guards. That was the caveat the issue + raised, and it dissolves rather than being worked around. +- **Nine of ten corpus cases over-read at the pin**, reproduced against upstream's + own translation units under ASan with each input flush against a guard page. + **But at every reachable call site the read stays inside its allocation** — + 256 B in `Dispatcher::checkRecv`, 262 B and 250 B in the two bridges, the + `Packet` object for adverts — and every one of the nine ends in a rejected + packet. The parsers do read past their length; nothing escapes a buffer through + them. Keeping those two sentences apart is most of the value here. +- **The companion path cannot reach the worst of the three.** `CMD_IMPORT_CONTACT` + gates on `len > 98` and `Packet::readFrom` reaches at most byte 70, so a + malformed contact blob from a client — which is what Attadipa is — cannot + trigger it. Luck rather than design, but it holds at this revision. +- **Two of the three pull requests do not close their own findings.** #3269 logs + the condition and then performs the read; #3270 leaves `app_data[0]` unguarded + at `AdvertDataHelpers.cpp:34` for `app_data_len == 0`, measured on its own head. + Only #3267 is complete, and it covers none of what #3270 covers. +- **Two arithmetic defects do leave the buffer, and neither is in any pull + request.** `Mesh.cpp:172` underflows `extra_len` — 187 of 1309 reachable + `(len, path_len)` pairs, all of them producing a window past `data[184]` — and + `Utils::decrypt` writes 192 bytes into that same 184-byte buffer for + `payload_len` 181…184, reproduced against the real translation unit. Both sit + behind a **2-byte** MAC ([M11](docs/research/OPEN_QUESTIONS.md)), which is not + the authentication gate it looks like. +- **Reporting the `Utils::decrypt` defect upstream is the owner's call.** Filing + on a third-party repository is outward-facing and this run had no authority for + it. The evidence reproduces in one command. +- **`attadipa_link`'s decoder was checked, not assumed.** It validates the + declared length before reading (`frame_codec.cpp:139`, `:148`) behind a length + check and a CRC. The finding does not transplant, and nothing in `link/` + changed. +- **Left open as M15–M18** in [OPEN_QUESTIONS](docs/research/OPEN_QUESTIONS.md): + end-to-end reachability of the two arithmetic defects, whether the stale bytes + they read can be groomed, what the eight over-written bytes hit on an + ESP32-S3, and whether a real fuzzing pass finds more. None blocks anything + while we link no MeshCore. + ### T-107 · Why agent runs died with no explanation — **DONE** 2026-08-22 - **The cause was not the model, the context or the turn ceiling.** It was `allowed_bots: ""` in `claude-agent.yml`. The hourly watchdog hands a task diff --git a/docs/research/MESHCORE_PARSER_BOUNDS.md b/docs/research/MESHCORE_PARSER_BOUNDS.md new file mode 100644 index 00000000..38990b5e --- /dev/null +++ b/docs/research/MESHCORE_PARSER_BOUNDS.md @@ -0,0 +1,577 @@ +# MeshCore parser bounds at the pinned revision + +Research for [issue #142](https://github.com/hleserg/Attadipa/issues/142). +Read on 2026-08-23. **Research only — no Attadipa production code changed, and +none should on the strength of this document alone.** + +Three upstream pull requests filed on 2026-08-21 and 2026-08-22 claim missing +length checks in MeshCore's frame and advertisement parsers, found by fuzzing. +The question this answers is not "are they right" — they are — but the three +that actually decide anything here: + +1. do the findings hold **at the revision this project pinned**, rather than at + the `dev` commits the pull requests are based on; +2. what each one can actually do, traced from the byte that triggers it to the + buffer it lands in and the caller that supplied it; +3. what any of it costs Attadipa, which today links no MeshCore code at all. + +The short answers are yes; less than the word "out-of-bounds" suggests in three +cases and rather more in a fourth that none of the pull requests mention; and +nothing today, but it moves two decisions that are not yet made. + +Everything below was executed on a host. **Nothing here was run on a radio, a +node or any physical board — NOT EXECUTED — HARDWARE REQUIRED.** Where a claim +could not be executed at all it says so in its own row rather than in a footnote. + +--- + +## 1. The revision, and why the pull request bases do not get in the way + +| | | +|---|---| +| Attadipa's pin | `d92964352441e53b93e8667b802e04f6e072b39e` | +| What that is upstream | tag `companion-v1.17.1` (also `repeater-`, `room-server-`), released 2026-08-14 | +| `meshcore-dev/MeshCore` `main`, 2026-08-23 | `d92964352441e53b93e8667b802e04f6e072b39e` — **the same commit** | +| `meshcore-dev/MeshCore` `dev`, 2026-08-23 | `9d7cee66394fffd6e8c6e9f39fe03660cb314f64`, 2026-08-22 | +| Licence | **MIT**, `license.txt` in the clone | + +So the pin is not lagging: it *is* the current release and the current `main`. +Nothing has been released that contains any of these guards. + +The pull requests are based on `dev`, which made the issue's own framing cautious +about comparing them to our pin. That caution turns out to be unnecessary, and it +is worth saying why rather than asserting it, because "the base is a different +branch" is normally a real obstacle: + +``` +$ git diff --quiet d929643 -- # for each affected file +src/Packet.cpp vs e0031870 (base of #3267) : identical +src/Packet.cpp vs 9d7cee66 (base of #3269/#3270): identical +src/Dispatcher.cpp vs e0031870 : identical +src/Dispatcher.cpp vs 9d7cee66 : identical +src/Mesh.cpp vs e0031870 : identical +src/Mesh.cpp vs 9d7cee66 : identical +src/helpers/AdvertDataHelpers.cpp vs e0031870 : identical +src/helpers/AdvertDataHelpers.cpp vs 9d7cee66 : identical +src/Utils.cpp vs e0031870 : identical +src/Utils.cpp vs 9d7cee66 : identical +``` + +Every file any of these pull requests touches is byte-identical between our pin +and both of their bases. The 29 commits `dev` is ahead by are elsewhere. So each +diff applies to `d929643` unchanged, and a measurement taken on a pull request's +head tree is a measurement of our pin plus that pull request's guards. + +### The supersession graph, checked rather than repeated + +| PR | State on 2026-08-23 | Base | Head | Files | What it is | +|---|---|---|---|---|---| +| [#3266](https://github.com/meshcore-dev/MeshCore/pull/3266) | **closed, unmerged** | `main@d929643` | `d87dd32f` | 30 (+804/−38) | the parser fix buried in board variants, `platformio.ini`, UI drivers and `armstubs.cpp` | +| [#3267](https://github.com/meshcore-dev/MeshCore/pull/3267) | **open** | `dev@e003187` | `05da523e` | 2 (+10/−0) | #3266's `Dispatcher.cpp` and `Packet.cpp` hunks, **byte-identical**, with the other 28 files dropped | +| [#3269](https://github.com/meshcore-dev/MeshCore/pull/3269) | **open** | `dev@9d7cee6` | `5ebf8ef9` | 1 (+4/−0) | a `MESH_DEBUG_PRINTLN` on the PATH length mismatch, and nothing else | +| [#3270](https://github.com/meshcore-dev/MeshCore/pull/3270) | **open** | `dev@9d7cee6` | `f80d805e` | 1 (+6/−0) | three `return` guards in `AdvertDataParser` | +| [#3271](https://github.com/meshcore-dev/MeshCore/pull/3271) | **closed, unmerged** | `dev@9d7cee6` | `f80d805e` | 1 | **the same head commit as #3270**, not merely equivalent | + +None is merged. None is in a release. Nothing below may be written up anywhere as +"upstream fixed it". + +--- + +## 2. The harness + +`docs/research/meshcore-parser-bounds/` holds it, with the shims, the build +script and the runner. It is **not** part of any Attadipa build: no CMake target +references it, and it is under `docs/` deliberately so that it cannot become one +by accident. + +What it compiles is upstream's own code. `src/Packet.cpp`, `src/Dispatcher.cpp`, +`src/helpers/AdvertDataHelpers.cpp` and `src/Utils.cpp` are taken unmodified from +a `git archive` of the revision under test and compiled against four small shim +headers — `Arduino.h`, `Stream.h`, `SHA256.h`, `AES.h` — that supply just enough +of the Arduino and Crypto surface to link on a desktop. **The `SHA256` and +`AES128` shims are not ciphers and are labelled as such in their own files**; +nothing measured here depends on what they compute, only on how many bytes the +code around them moves. + +Two independent boundary mechanisms, because one of them quietly lied: + +- **AddressSanitizer**, which names the offending source line — that is what + makes the evidence quotable; +- **a `PROT_NONE` guard page** with the input placed so that its last declared + byte is the last addressable byte before the wall. + +The guard page is not belt-and-braces. The first version of the harness used a +tight `malloc(len)` alone, and **ASan does not report a read at offset 0 of a +zero-size allocation** — which silently turned the two `len == 0` cases green on +a build that had no guard whatsoever. Both of those cases are real, and both were +recovered only when the wall replaced the redzone. It is recorded here because +the failure mode is not obvious and the next person to build a harness like this +will hit it. + +The buffer being exactly `len` bytes is the whole design. It separates *"the +parser reads past the length it was given"*, which is a property of the parser +and is what the harness measures, from *"the read left the allocation"*, which is +a property of the caller and is answered by reading the call sites. Conflating +the two is how a parser bug gets written up as a crash it cannot cause — or, as +in P4 below, how a real one gets missed. + +--- + +## 3. Findings + +Five, of which the pull requests describe three. Each row states what has to be +on the wire, where the parser is reached from, what buffer actually backs the +read at that call site, and what happens. Nothing is inferred from a pull request +description. + +### P1 · `Dispatcher::tryParsePacket` reads three fields it has not proved are there + +**Where:** `src/Dispatcher.cpp:149-189` at `d929643`. + +```cpp +pkt->header = raw[i++]; // :152 no len check +if (pkt->hasTransportCodes()) { + memcpy(&pkt->transport_codes[0], &raw[i], 2); i += 2; // :159 no len check + memcpy(&pkt->transport_codes[1], &raw[i], 2); i += 2; // :160 +} +pkt->path_len = raw[i++]; // :165 no len check +... +if (path_byte_len > MAX_PATH_SIZE || i + path_byte_len > len) return false; // :173 present +``` + +The path-bytes check at `:173` is there. The three reads before it are not. + +**Precondition:** `len < 6` with `header & 0x03 == 0x00` or `0x03` (a transport +route, four extra bytes), or `len < 2` otherwise. Maximum index reached before +the first bound check is `raw[5]`. + +**Reachable from:** the radio, and only the radio. `Dispatcher::checkRecv` +(`:190-217`) is the sole caller. + +**What actually backs `raw`:** `uint8_t raw[MAX_TRANS_UNIT+1]` — a **256-byte +stack array** in `checkRecv`, filled by `_radio->recvRaw(raw, 255)`. The furthest +this parser can reach is `raw[5]`. **The read never leaves the allocation.** It +reads stack bytes the radio did not write on this pass — the tail of an earlier +frame, or whatever the frame before that left there. + +**Outcome:** `reject`, in every case. Follow the arithmetic: whatever garbage +`path_len` picks up, `i` is already at least 2, so `i + path_byte_len > len` +holds for any `len` short enough to have triggered the over-read, and +`tryParsePacket` returns `false`. No crash, no disclosure — the parsed packet is +freed. `checkRecv` also gates on `len > 0`, so the missing `len < 1` guard that +#3267 adds is unreachable through this caller. + +**Status:** confirmed by execution (A1, A2, A3). Fixed on `#3267`'s head. +**Severity for a stock node: cosmetic — a use of uninitialised memory whose +result is discarded.** It is worth fixing because MSan-class tooling will flag it +forever and because the next caller might not have a 256-byte buffer, not because +a packet can do harm through it today. + +### P2 · `Packet::readFrom` is missing the check `tryParsePacket` has + +**Where:** `src/Packet.cpp:65-84` at `d929643`. + +```cpp +uint8_t bl = getPathByteLen(); +memcpy(path, &src[i], bl); i += bl; // :78 bl up to 64, and nothing compares i+bl to len +if (i >= len) return false; // :80 too late +``` + +Same three missing checks as P1, **plus** the path-bytes bound that +`tryParsePacket` does have. `isValidPathLen` has already capped `bl` at +`MAX_PATH_SIZE`, so `path[64]` cannot be overflowed on the write side; the read +side is unbounded against `len`. + +**Precondition:** `len < 2 + bl`, where `bl = (path_len & 63) * ((path_len >> 6) + 1)` +and `isValidPathLen` accepts it. Largest over-read 64 bytes, at `path_len == 0x40` +(32 hashes × 2) or `0x3F` (63 × 1). + +**Reachable from:** four callers, and they do not behave alike. + +| Caller | Buffer behind `src` | Can the over-read be triggered? | +|---|---|---| +| `helpers/bridges/RS232Bridge.cpp:87` | `_rx_buffer[MAX_TRANS_UNIT+1+6]` = 262 B, `src = _rx_buffer+4` | **yes** — `len` comes from the frame's own length field. Furthest reach `_rx_buffer[70]`: **inside the allocation**, stale bytes | +| `helpers/bridges/ESPNowBridge.cpp:147` | `decrypted[MAX_ESPNOW_PACKET_SIZE]` = 250 B on the stack | **yes**, same shape, same in-allocation reach | +| `helpers/BaseChatMesh.cpp:560` `importContact` | the companion command frame | **no** — see below | +| `helpers/BaseChatMesh.cpp:546` `shareContactZeroHop` | `temp_buf`, from our own stored blob | not attacker-supplied | + +**`importContact` is the one that matters to us, and it is not reachable.** It is +the companion-protocol path — `CMD_IMPORT_CONTACT`, `examples/companion_radio/MyMesh.cpp:1362-1363` +— so it is the one place a *client*, which is what Attadipa is, hands bytes to +this parser. The guard on that branch is `len > 2 + 32 + 64`, i.e. at least 99 +bytes, and the parser's furthest reach is `i + bl ≤ 6 + 64 = 70`. The minimum the +caller enforces exceeds the maximum the parser can want, so the over-read cannot +happen through it. That is luck rather than design — the constant is there to +reject a truncated identity, not to bound the parser — but it holds at this +revision, and it means **a malformed contact blob from a companion client cannot +reach P2**. + +Both bridges are optional builds and neither is on the companion path. + +**Outcome:** `reject`, again by arithmetic: after the over-read `i = 2 + bl` +exceeds the `len` that caused it, so `i >= len` returns `false`. + +**Status:** confirmed by execution (B1, B2, B3). Fixed on `#3267`'s head. + +### P3 · `PAYLOAD_TYPE_PATH` underflows `extra_len`, and #3269 does not stop it + +**Where:** `src/Mesh.cpp:161-172` at `d929643`. + +```cpp +uint8_t data[MAX_PACKET_PAYLOAD]; // :157 184 bytes, stack +int len = Utils::MACThenDecrypt(secret, data, macAndData, pkt->payload_len - i); +if (len > 0) { + if (pkt->getPayloadType() == PAYLOAD_TYPE_PATH) { + int k = 0; + uint8_t path_len = data[k++]; + if (!Packet::isValidPathLen(path_len)) break; + uint8_t hash_size = (path_len >> 6) + 1; + uint8_t hash_count = path_len & 63; + uint8_t* path = &data[k]; k += hash_size*hash_count; // k up to 65 + uint8_t extra_type = data[k++] & 0x0F; // k up to 66 + uint8_t* extra = &data[k]; + uint8_t extra_len = len - k; // :172 underflows when k > len +``` + +`isValidPathLen` bounds `k` at 66 but never compares it to `len`. When `k > len` +the subtraction is done in `int` and truncated into a `uint8_t`, so a small +negative becomes a large positive, and `extra`/`extra_len` are then handed to +`onPeerPathRecv` as a window that runs off the end of `data`. + +**The domain, computed rather than asserted.** `Utils::decrypt` returns whole +16-byte blocks — *"will always be multiple of 16"*, `src/Utils.cpp:82` — so `len` +is a multiple of 16. Over every `(len, path_len)` pair that reaches this code +with `len` in 16…176: + +``` +accepted (len, path_len) pairs : 1309 +pairs where k > len (extra_len underflows): 187 + of those, &data[k] + extra_len > 184 : 187 <- all of them +smallest underflowing input : len=16 path_len=0x0F + -> k=17, extra_len=255, + window data[17..271] against data[0..183] + (88 bytes past the end) +``` + +Every underflow produces a window past the end of the 184-byte stack buffer. +There is no benign corner of this one. + +**Reachable from:** the radio, addressed to this node, from a source hash that +matches a contact, **and past the MAC check**. That last gate is `CIPHER_MAC_SIZE` += **2 bytes** (`src/MeshCore.h:17`) — already recorded as +[M11](OPEN_QUESTIONS.md#meshcore). So it is not "authenticated peers only": a +party holding the shared secret passes it always, and a party holding nothing +passes it with probability ~1/65536 per candidate peer per packet, with up to +four candidates tried and no rate limit in this loop. `_tables->wasSeen` dedupes +identical packets, which costs an attacker one varied byte per attempt. + +**What the consumer then does — analysed, not executed.** In `companion_radio`, +the stock firmware Attadipa's node path talks to, the chain is +`BaseChatMesh::onPeerPathRecv` (`BaseChatMesh.cpp:316-326`) → +`MyMesh::onContactPathRecv` (`MyMesh.cpp:750`) → `BaseChatMesh::onContactPathRecv` +(`:328-345`) → `MyMesh::onContactResponse` (`MyMesh.cpp:676-747`), whose three +`pending_*` branches each do + +```cpp +memcpy(&out_frame[i], &data[4], len - 4); // MyMesh.cpp:722, :735, :744 +``` + +with `out_frame[MAX_FRAME_SIZE + 1]` = **177 bytes** and `i` = 8. With `len` +underflowed to 255 that writes about 82 bytes past a member array — a write, not +a read. Two things stop this being called a proven memory-corruption path and +both are load-bearing: + +- `extra_type` and the four `tag` bytes the branches match on are read from + `data[k..]`, which is **past `len`** — so they are stale stack bytes, not + anything the attacker put in this packet. Steering them means grooming the + same stack region with an earlier packet. Plausible; **not demonstrated here**; +- the branches additionally require the local app to have a matching `pending_status`, + `pending_telemetry` or `pending_req` outstanding. + +**Status of the underflow: confirmed** (exhaustive, `path_arith.cpp`). +**Status of the write overflow behind it: NOT REPRODUCED** — reaching it needs +the full node build with real AES, SHA-256 and ed25519, which this harness does +not have. See §6. + +**#3269 does not fix this.** Its entire diff is + +```cpp +if (k >= len) { + MESH_DEBUG_PRINTLN("%s PAYLOAD_TYPE_PATH, set path_len %u exceeds ..."); +} +uint8_t extra_type = data[k++] & 0x0F; // unchanged — still executes +``` + +— a log line with no `break`, no `return`, and no change to control flow, in a +build where `MESH_DEBUG_PRINTLN` expands to `{}` unless `MESH_DEBUG` is set. The +issue's reading of it is right, and this is the precise reason: it reports the +condition and then does the thing. + +### P4 · `Utils::decrypt` rounds up past the destination — not from any of the pull requests + +**Where:** `src/Utils.cpp:70-83`, reached from `src/Mesh.cpp:158`. + +```cpp +// Utils.h: "'src_len' should be multiple of block size, as returned by 'encrypt()'" +while (sp - src < src_len) { + aes.decryptBlock(dp, sp); // :77 + dp += 16; sp += 16; +} +return sp - src; // will always be multiple of 16 +``` + +A documented precondition with no enforcement, and an on-air caller that does not +honour it. `Mesh::onRecvPacket` passes `pkt->payload_len - 2` into +`MACThenDecrypt`, which passes `src_len - 2` into `decrypt`, into a **184-byte** +stack destination. `payload_len` is attacker-chosen up to 184 (`tryParsePacket` +rejects more). For `payload_len` in **181…184** the source length is 177…180, the +loop runs twelve times, and it writes **192 bytes into 184**. + +**Reproduced.** Against the real `src/Utils.cpp` from the pinned tree, with a +stub block cipher — the bound belongs to the loop, not to the cipher — and a +destination of exactly `MAX_PACKET_PAYLOAD` bytes against a guard page: + +``` +src_len = 176 : decrypt() returned 176 — wrote dest[0..175] clean +src_len = 177 : SEGV in mesh::Utils::decrypt at src/Utils.cpp:77 +src_len = 180 : SEGV in mesh::Utils::decrypt at src/Utils.cpp:77 +src_len = 182 : SEGV in mesh::Utils::decrypt at src/Utils.cpp:77 +``` + +**Reachable from:** the radio, for `PAYLOAD_TYPE_PATH`, `REQ`, `RESPONSE` and +`TXT_MSG`, behind the same 2-byte MAC as P3 — see the note there about what that +gate is and is not worth. + +**What lands where: an 8-byte stack write past `uint8_t data[184]` in +`Mesh::onRecvPacket`.** That is qualitatively different from P1, P2 and P5: it +leaves the allocation, and it is a write. On a desktop it faulted. On an +ESP32-S3 with no stack canary between `data` and its neighbours, what those eight +bytes hit is a property of the frame layout the compiler chose, and this project +has not compiled that firmware for that target — **UNKNOWN, and it stays UNKNOWN +until somebody does.** + +**Status: reproduced at function level; NOT REPRODUCED end-to-end** through +`Mesh::onRecvPacket`, for the same reason as P3. The precondition chain is read +from source and every link is quoted above. + +Not fixed, not filed, not known upstream as far as the pull request queue shows. +**Attadipa has not reported it and should not do so unilaterally** — see §5. + +### P5 · `AdvertDataParser` reads on flag bits, and #3270 misses its own first byte + +**Where:** `src/helpers/AdvertDataHelpers.cpp:31-61` at `d929643`. + +```cpp +_flags = app_data[0]; // :34 no app_data_len check +int i = 1; +if (_flags & ADV_LATLON_MASK) { memcpy(&_lat, &app_data[i], 4); i += 4; // :40 + memcpy(&_lon, &app_data[i], 4); i += 4; } // :41 +if (_flags & ADV_FEAT1_MASK) { memcpy(&_extra1, &app_data[i], 2); i += 2; } // :44 +if (_flags & ADV_FEAT2_MASK) { memcpy(&_extra2, &app_data[i], 2); i += 2; } // :47 +if (app_data_len >= i) { ... _valid = true; } // :49 the only length test +``` + +Up to `app_data[12]` is read before `app_data_len` is consulted at all. + +**Precondition:** `app_data_len < 1 + 8·[LATLON] + 2·[FEAT1] + 2·[FEAT2]`, i.e. +any advert whose flags promise fields its length does not contain. + +**Reachable from:** the radio, `src/Mesh.cpp:267-269` → +`Mesh::onAdvertRecv` → `BaseChatMesh.cpp:121`, **after** the Ed25519 signature +over `pub_key ‖ timestamp ‖ app_data` has verified. That is a weaker gate than it +sounds: the attacker signs their own advert with their own key, which anyone can +generate. Also `examples/simple_repeater/MyMesh.cpp:656`, same data. + +**What actually backs `app_data`:** `&pkt->payload[100]`, inside +`uint8_t payload[184]`, and `app_data_len` is clamped to `MAX_ADVERT_DATA_SIZE` +(32) one line earlier at `Mesh.cpp:269`. So the twelve-byte reach stays inside +the `Packet` object — **the read never leaves the allocation on this path**. + +That clamp is also what keeps a much worse bug from existing. `_name` is +`char[MAX_ADVERT_DATA_SIZE]` = 32 and the parser ends with +`memcpy(_name, &app_data[i], app_data_len - i); _name[nlen] = 0;` with no bound +of its own. Were `app_data_len` allowed past 32 — the payload has room for 84 — +that would be a straightforward stack write overflow. `Mesh.cpp:269` is the only +thing preventing it, and it is one line in one of the two callers. Worth knowing +before anyone refactors it; not a defect today. + +**Outcome:** `reject`. `_valid` requires `app_data_len >= i`, which is exactly +the condition that failed, so the parser returns an object the callers discard — +`BaseChatMesh.cpp:122` and `MyMesh.cpp:656` both test `isValid()`. + +**Status: confirmed by execution (C1, C2, C3, C4), and #3270 does not close it.** +On `#3270`'s own head `f80d805e`, case C3 — `app_data_len == 0` — still reads +`app_data[0]` at `AdvertDataHelpers.cpp:34`. The diff guards the lat/lon and +feature reads and leaves the flags byte in front of them exactly as it was. Since +`#3271` is the same commit, both of the advert pull requests are incomplete in +the same place. + +Is `app_data_len == 0` reachable? Yes: `Mesh.cpp:261` tests `i > pkt->payload_len`, +not `>=`, so a correctly signed advert with `payload_len == 100` and no app data +gives `app_data_len == 0`. It lands on a stale in-allocation byte and is rejected, +so the consequence is nil — but a fix that leaves the case it was written for +still reading out of bounds is not a fix, and this is why the harness's `len == 0` +cases had to be made to work. + +--- + +## 4. The corpus + +Ten sequences. Each is the whole input; each is fed as a buffer of exactly its own +length with a guard page behind it. `base` is `d929643`. + +| # | Parser | Bytes (hex) | `len` | Reads, and where the tool stops it | base | `#3267` head | `#3270` head | +|---|---|---|---|---|---|---|---| +| A1 | `tryParsePacket` | `01` | 1 | 1 B at `Dispatcher.cpp:165` | **OOB** | clean, `false` | **OOB** | +| A2 | `tryParsePacket` | `00` | 1 | 2 B at `Dispatcher.cpp:159` | **OOB** | clean, `false` | **OOB** | +| A3 | `tryParsePacket` | *(empty)* | 0 | 1 B at `Dispatcher.cpp:152` | **OOB** | clean, `false` | **OOB** | +| B1 | `Packet::readFrom` | `01 3F` | 2 | **63 B** at `Packet.cpp:78` | **OOB** | clean, `false` | **OOB** | +| B2 | `Packet::readFrom` | `01` | 1 | 1 B at `Packet.cpp:74` | **OOB** | clean, `false` | **OOB** | +| B3 | `Packet::readFrom` | `00` | 1 | 2 B at `Packet.cpp:69` | **OOB** | clean, `false` | **OOB** | +| C1 | `AdvertDataParser` | `91` | 1 | 4 B at `AdvertDataHelpers.cpp:40` | **OOB** | **OOB** | clean, `invalid` | +| C2 | `AdvertDataParser` | `F1` | 1 | 4 B at `AdvertDataHelpers.cpp:40` | **OOB** | **OOB** | clean, `invalid` | +| C3 | `AdvertDataParser` | *(empty)* | 0 | 1 B at `AdvertDataHelpers.cpp:34` | **OOB** | **OOB** | **OOB** | +| C4 | `AdvertDataParser` | `21` | 1 | 2 B at `AdvertDataHelpers.cpp:44` | **OOB** | **OOB** | clean, `invalid` | + +The read widths are the `memcpy` and subscript widths in the source; a +multi-field read is attributed to the statement that first crosses the boundary, +so A2 is the first of two `memcpy`s and C1 the first of two. The line numbers are +the tool's. With the guard page in place ASan words all ten as `SEGV` at those +lines; an earlier build of the same corpus against a tight `malloc(len)` instead +worded the eight non-zero-length cases as `heap-buffer-overflow` with +`READ of size N` matching the widths above — and reported nothing at all for the +two `len == 0` cases, which is why the guard page is the mechanism of record. + +Plus two experiments that are not single byte sequences: + +| # | What | Result | +|---|---|---| +| P3 | every `(len, path_len)` reaching `Mesh.cpp:161-172`, `len` ∈ 16…176 | 187 of 1309 underflow `extra_len`; **all 187** give a window past `data[184]`; smallest `len=16, path_len=0x0F` | +| P4 | `Utils::decrypt` into a 184-byte destination | clean at `src_len=176`; faults at `src_len` 177, 180, 182 at `Utils.cpp:77` | + +**Nine of ten cases over-read on the pinned revision. No head fixes all of them:** +`#3267` closes A and B and leaves C untouched; `#3270` closes C1, C2, C4 and +leaves A, B and **C3** untouched. Vendoring any one of them would buy part of the +problem. + +--- + +## 5. What this costs Attadipa + +**Today: nothing, and that is a fact about the repository rather than an +opinion.** Attadipa links no MeshCore code. There is no local provider, and the +stock-node path is a protocol client behind +[ADR-0008](../adr/0008-mesh-service-providers.md). Every one of P1–P5 executes on +the node, on the far side of the companion link. + +It changes three things anyway. + +**The node's output is a peer's output, not a trusted source.** P3 and P4 are +memory-safety defects on the device that supplies Attadipa's mesh capability, and +P4 can be provoked by a third party on the air who holds nothing but a 1-in-65536 +chance per packet. The realistic consequence for a watch is that its node +misbehaves or reboots — which is capability withdrawal, which +[ADR-0008](../adr/0008-mesh-service-providers.md) and +[ADR-0002](../adr/0002-companion-is-optional.md) already require us to survive. +The unrealistic-but-not-excluded consequence is a node whose memory has been +corrupted and which then sends us plausible frames. Our side of that boundary +must keep treating positions, headings and messages arriving over the link as +peer claims: validated on arrival, never promoted to trusted because the node +sounded confident. That is the existing rule, and this is evidence for it rather +than a change to it — no ADR is amended by this document. + +**A local MeshCore provider inherits all five.** The moment `LocalMeshProvider` +becomes real, P1–P5 stop being facts about somebody else's firmware. A pin or +upgrade decision taken then must be made against the state of these five, not +against a version number. That is the criterion this research was asked to +establish, and it is now written down: **do not pin a MeshCore revision for the +local provider without re-running the corpus in +`docs/research/meshcore-parser-bounds/` against it.** + +**Our own decoder is not analogous, and it was checked rather than assumed.** +`link/src/frame_codec.cpp` validates the declared length *before* reading — +`if (declared > kMaxPayload)` at `:139`, counted as its own error class, and +`if (size_ < needed) return 0;` at `:148` before any payload is touched — behind +a length-check byte at `:123` and a CRC. The issue's caution about not +transplanting the finding onto it is right. **No change to `attadipa_link` is +proposed by this document.** Its boundary fixtures may still be worth extending +with the shape these findings share — a declared length that outruns the frame — +and that is a separate, executable task, not this one. + +### Decision + +**ADAPT the rejection cases, MONITOR the pull requests, vendor nothing.** + +- **Do not vendor** `#3267`, `#3269` or `#3270`. Unmerged, unreleased, and two of + the three are incomplete against their own findings. Copying a guard from an + open pull request into a project that does not yet compile the file it guards + buys nothing and dates immediately. +- **Monitor** all three plus `dev`. The re-check is cheap: `build.sh ` + then `run.sh`. +- **Keep the corpus.** It is the executable form of this document and the entry + condition for any future pin. +- **P4 should be reported upstream, and that is the owner's call, not an agent's.** + It is a memory-safety defect in a third-party project, it is a write rather + than a read, and opening an issue on `meshcore-dev/MeshCore` is an outward-facing + act this run did not have authority for. The evidence needed is in §3 P4 and + reproduces in one command. Recorded as a recommendation, deliberately not acted + on. + +No ADR changes. The trust boundary these findings press on is +[ADR-0008](../adr/0008-mesh-service-providers.md)'s and it already holds; nothing +here moved it, and an ADR edited to say what it already said is churn. + +--- + +## 6. What was not established + +| Claim | Status | +|---|---| +| Any of this on a radio, a node, or a board | **NOT EXECUTED — HARDWARE REQUIRED.** Nothing here touched hardware, and no host sanitizer result may be presented as radio or HIL validation | +| The pull request authors' own claim of verification on a Heltec V4 | **not independently checked.** No crash trace, corpus or sanitizer output is attached to any of the three pull requests; taken as an unverified author statement | +| P3's write overflow in `companion_radio` | **NOT REPRODUCED.** Needs the full node build with real AES-128, SHA-256 and ed25519, which this harness does not have. The underflow it depends on is proven; the path from there to `out_frame` is read from source | +| Whether `extra_type` and `tag` in P3 can be steered by grooming the stack across packets | **UNKNOWN.** Plausible, untested, and the difference between "conditional" and "controllable" for that finding | +| P4 end-to-end through `Mesh::onRecvPacket` | **NOT REPRODUCED**, same reason. Reproduced at `Utils::decrypt` | +| What P4's eight bytes overwrite on an ESP32-S3 | **UNKNOWN.** Depends on a stack frame layout nobody here has compiled | +| Whether any of this is exploitable rather than merely wrong | **UNKNOWN, and deliberately not claimed.** P1, P2 and P5 end in a rejected packet. P3 and P4 leave a buffer, which is a necessary and not a sufficient condition for anything worse | + +The harness has no fuzzer behind it. It runs a hand-built corpus derived from +reading the parsers, so it demonstrates the findings and does not search for +more. A real fuzzing pass over the pinned tree would be a separate task and would +want the genuine crypto libraries, not the stubs used here. + +--- + +## 7. Reproducing this + +```bash +git clone --filter=blob:none https://github.com/meshcore-dev/MeshCore /tmp/meshcore-src +cd docs/research/meshcore-parser-bounds +./build.sh base d92964352441e53b93e8667b802e04f6e072b39e +./build.sh pr3267 05da523ebd32980a1c28b11f2928d351796b9737 +./build.sh pr3270 f80d805ee8b20f77ff5b3ca6bc3a9021989aafd2 +./run.sh # the ten-case matrix in §4 +./build-extras.sh +./build/path_arith # P3 +./build/decrypt_bounds 180 # P4; 176 is clean +``` + +The two pull request heads have to be fetched by SHA before they resolve — +`git -C /tmp/meshcore-src fetch origin `; `build.sh` says so if they have not +been. **`run.sh` does not cover P3 or P4**, so a green matrix is not a clean +revision. + +Needs `clang++` with AddressSanitizer. Measured on clang 18.1.3, Ubuntu 24.04, +2026-08-23. It reads the upstream clone and writes only inside its own directory. + +--- + +## References + +- Upstream: `meshcore-dev/MeshCore`, MIT, pinned `d92964352441e53b93e8667b802e04f6e072b39e` +- [`REUSE_LEDGER.md`](REUSE_LEDGER.md) — the pin and the monitored deltas +- [`OPEN_QUESTIONS.md`](OPEN_QUESTIONS.md) — M10–M14 from the first reading; M15–M17 from this one +- [`VERIFIED_FACTS.md`](VERIFIED_FACTS.md) — what is now traced to executed evidence +- [`../upstream/meshcore-1.17-review.md`](../upstream/meshcore-1.17-review.md) — T-041, the review this continues +- [`MESHCORE_COMPANION_PROTOCOL.md`](MESHCORE_COMPANION_PROTOCOL.md) — the wire format on the client side +- [ADR-0008](../adr/0008-mesh-service-providers.md) — the two providers and the trust boundary this presses on diff --git a/docs/research/OPEN_QUESTIONS.md b/docs/research/OPEN_QUESTIONS.md index 8c49d45d..f3137331 100644 --- a/docs/research/OPEN_QUESTIONS.md +++ b/docs/research/OPEN_QUESTIONS.md @@ -176,6 +176,31 @@ authentication rests on rate limiting is a protocol whose rate limiter is a security control rather than a convenience. That belongs in an ADR of its own, with someone competent reviewing it — not in a paragraph here. +### What the parser-bounds review of 2026-08-23 could not close + +Five parser defects at the pin were verified and are in +[VERIFIED_FACTS.md](VERIFIED_FACTS.md) and +[MESHCORE_PARSER_BOUNDS.md](MESHCORE_PARSER_BOUNDS.md). What follows is what that +work **failed** to establish, kept separate so that a verified over-read is never +read as a verified consequence. + +| # | Question | Status | What would resolve it | +|---|---|---|---| +| M15 | **Do P3 and P4 actually run end to end through `Mesh::onRecvPacket`?** Both are proven at the function they live in — the `extra_len` underflow exhaustively, the `Utils::decrypt` over-write against the real translation unit. Neither has been driven through the packet path that reaches it | **UNKNOWN** | a host build of MeshCore with the genuine `rweather/Crypto` AES-128 and SHA-256 and the vendored ed25519, rather than this harness's stubs. That is a day of work and it would also give the project its first real MeshCore reference vectors, which M13 says do not exist | +| M16 | **Can an attacker steer P3's `extra_type` and `tag`?** They are read from `data[k..]`, past the decrypted length, so they are stale stack bytes rather than anything in the triggering packet. Grooming them with an earlier packet is plausible and untested. This is the difference between a conditional finding and a controllable one | **UNKNOWN** | the same build as M15, plus a two-packet sequence that fills the stack region and then triggers the underflow | +| M17 | **What do P4's eight bytes overwrite on an ESP32-S3?** The over-write leaves `uint8_t data[184]` in `Mesh::onRecvPacket`. What sits after it is a property of the stack frame the compiler chose for that target, and nobody here has compiled MeshCore for it | **UNKNOWN** | build MeshCore for an ESP32-S3 target and read the frame layout. Note that answering it does **not** need a board — this one is a compiler question, not a hardware one | +| M18 | **Are there more of these?** The corpus is hand-built from reading three parsers. It demonstrates; it does not search. `Utils::decrypt` was found by following a caller, not by the corpus, which is evidence that reading finds what a ten-case corpus does not | **UNKNOWN** | a real fuzzing pass over the pinned tree with the genuine crypto libraries. Scope it as its own task; do not fold it into a pin decision | + +None of these blocks anything today, because Attadipa compiles no MeshCore code. +All four become entry conditions the moment a local MeshCore provider is real — +[MESHCORE_PARSER_BOUNDS.md](MESHCORE_PARSER_BOUNDS.md) §5. + +One more, and it is not a MeshCore question: the three pull request authors each +state they verified on a Heltec V4, and none attaches a crash trace, a corpus or +sanitizer output. That is **an unverified author claim** and it is recorded as +one. This project has no Heltec V4 and independently confirming it is +**NOT EXECUTED — HARDWARE REQUIRED**. + ## Architecture | # | Question | Status | Resolved by | diff --git a/docs/research/REUSE_LEDGER.md b/docs/research/REUSE_LEDGER.md index cbb0bab8..05d843a8 100644 --- a/docs/research/REUSE_LEDGER.md +++ b/docs/research/REUSE_LEDGER.md @@ -59,7 +59,7 @@ want to inherit the experience, not only the code. | Project | Repository | Commit at examination | Last commit | Why it is here | |---|---|---|---|---| -| `MeshCore` | github.com/meshcore-dev/MeshCore | `d92964352441e53b93e8667b802e04f6e072b39e` | 2026-08-14 | the mesh stack Attadipa builds on; T-006 | +| `MeshCore` | github.com/meshcore-dev/MeshCore | `d92964352441e53b93e8667b802e04f6e072b39e` | 2026-08-14 | the mesh stack Attadipa builds on; T-006. **Re-checked 2026-08-23**: still `main`'s tip and still the newest release (`companion-v1.17.1`), so the pin is current rather than lagging. `dev` is at `9d7cee66` (2026-08-22) and contains none of the parser guards below — [MESHCORE_PARSER_BOUNDS](MESHCORE_PARSER_BOUNDS.md) | | `meshtastic` | github.com/meshtastic/firmware | `68bfe015e6ab9ec2ab8f1657066898b7880eaf63` | 2026-08-20 | ~200 board variants, worldwide regulatory regions, nanopb phone API | | `InfiniTime` | github.com/InfiniTimeOrg/InfiniTime | `825056574f47a8187b410b860f326050566553e2` | 2026-08-19 | mature LVGL watch firmware with a real app lifecycle, on far less RAM | | `RadioLib` | github.com/jgromes/RadioLib | `510e00cfb05bbc3c2b7b524262785454944adb6e` | 2026-08-13 | radio abstraction across many chips; candidate for ADR-0003 | @@ -72,6 +72,42 @@ want to inherit the experience, not only the code. | `lv_i18n` | github.com/lvgl/lv_i18n | `08944ec6dc2faed83121c53e9cf9ba05013a6686` | 2026-03-30 | LVGL's own localization generator — the closest existing answer to T-033 | | `esp-brookesia` | github.com/espressif/esp-brookesia | `01939b5e58fd50d18339b1c35fb74c4e808962c7` | 2026-08-10 | ESP32 UI framework with an application model | +### Upstream deltas being monitored, and not taken + +A pinned revision is a decision to stop moving, not a decision to stop looking. +What sits here is upstream work that would change a pin if it landed — open, so +not takeable, and named so that the next pin decision starts from a list rather +than a search. + +**Nothing in this table has been vendored, and nothing in it may be** while it is +open: an unmerged pull request has no release behind it, and two of these three +do not close the finding they were written for. + +| Upstream | Head | State 2026-08-23 | What it would change | Our decision | +|---|---|---|---|---| +| [MeshCore #3267](https://github.com/meshcore-dev/MeshCore/pull/3267) | `05da523e` | open, unmerged, base `dev` | length checks in `src/Dispatcher.cpp::tryParsePacket` and `src/Packet.cpp::readFrom` | **MONITOR.** Verified to close all six of our A/B corpus cases on the pin. Still not taken — unreleased, and we compile neither file | +| [MeshCore #3269](https://github.com/meshcore-dev/MeshCore/pull/3269) | `5ebf8ef9` | open, unmerged, base `dev` | a `MESH_DEBUG_PRINTLN` on the `PAYLOAD_TYPE_PATH` length mismatch | **MONITOR as evidence, not as a fix.** The diff logs the condition and then executes the read anyway — no `break`, no `return`. Verified, not inferred | +| [MeshCore #3270](https://github.com/meshcore-dev/MeshCore/pull/3270) | `f80d805e` | open, unmerged, base `dev` | three guards in `AdvertDataParser` | **MONITOR.** Closes three of our four C cases and **leaves `app_data[0]` unguarded** at `AdvertDataHelpers.cpp:34` when `app_data_len == 0` — measured on its own head | +| [MeshCore #3266](https://github.com/meshcore-dev/MeshCore/pull/3266) | `d87dd32f` | **closed, unmerged** | the #3267 hunks plus 28 unrelated files | superseded by #3267, whose parser hunks are byte-identical | +| [MeshCore #3271](https://github.com/meshcore-dev/MeshCore/pull/3271) | `f80d805e` | **closed, unmerged** | — | the *same commit* as #3270, not merely equivalent | +| `meshcore-dev/MeshCore` `dev` | `9d7cee66` | 2026-08-22 | — | checked for equivalent guards arriving by another route: **none.** `readFrom` on `dev` is byte-identical to the pin | + +**Reusable as test material, not as code.** The guards in `05da523e` +(`src/Dispatcher.cpp`, `src/Packet.cpp`) and `f80d805e` +(`src/helpers/AdvertDataHelpers.cpp`) are MIT and may be read, adapted and used +to derive rejection cases. What Attadipa actually took from them is **nothing but +the shape of the inputs**: the ten-case corpus in +[`meshcore-parser-bounds/`](meshcore-parser-bounds/) is our own, written from +reading the parsers, and it is the artifact to keep. There is no upstream test or +corpus to port — MeshCore's `test/` covers none of these paths, and its `AES` and +`SHA256` test mocks are no-ops (recorded as M13 in +[OPEN_QUESTIONS.md](OPEN_QUESTIONS.md)). + +A fifth finding, in `Utils::decrypt`, is **not** in any of these pull requests and +so is not in this table. It is P4 in +[MESHCORE_PARSER_BOUNDS](MESHCORE_PARSER_BOUNDS.md), it is a write rather than a +read, and reporting it upstream is the owner's decision. + ### Licences, checked before anything was depended on Attadipa is MIT. `CLAUDE.md` says anything incompatible with MIT does not enter diff --git a/docs/research/VERIFIED_FACTS.md b/docs/research/VERIFIED_FACTS.md index b5c78d27..b05c582f 100644 --- a/docs/research/VERIFIED_FACTS.md +++ b/docs/research/VERIFIED_FACTS.md @@ -31,6 +31,84 @@ An entry that cannot name its source does not belong here. It belongs in memory requirements, LoRa abstraction, or the companion protocol. None of these have been read from source yet. +### The pinned MeshCore revision is upstream's current release, not a lagging one + +- **Claim:** `d92964352441e53b93e8667b802e04f6e072b39e` is simultaneously + Attadipa's pin, the tip of `meshcore-dev/MeshCore`'s `main`, and the newest + release (`companion-v1.17.1`, `repeater-v1.17.1`, `room-server-v1.17.1`, + published 2026-08-14). `dev` is at `9d7cee66394fffd6e8c6e9f39fe03660cb314f64`, + 2026-08-22. +- **Source:** GitHub API `repos/meshcore-dev/MeshCore/branches/{main,dev}` and + `/releases`. +- **Checked:** 2026-08-23. +- **Why it is here:** three open pull requests are based on `dev`, which normally + makes "does this apply to our pin" an open question. It is not one here — see + the next entry. + +### The three MeshCore parser pull requests all apply to our pin unchanged + +- **Claim:** `src/Packet.cpp`, `src/Dispatcher.cpp`, `src/Mesh.cpp`, + `src/helpers/AdvertDataHelpers.cpp` and `src/Utils.cpp` are **byte-identical** + between `d929643` and both pull request bases (`dev@e0031870` for #3267, + `dev@9d7cee66` for #3269/#3270). The 29 commits `dev` leads by touch none of + them. +- **Source:** `git diff --quiet -- ` against a full clone. +- **Checked:** 2026-08-23. +- **Consequence:** a measurement on a pull request's head tree is a measurement + of our pin plus that pull request's guards, and no rebasing is needed to reason + about either. + +### Nine of ten malformed-frame cases over-read on the pinned MeshCore revision + +- **Claim:** at `d929643`, `Dispatcher::tryParsePacket`, `Packet::readFrom` and + `AdvertDataParser` all read past the length they are given, on inputs of 0, 1 + and 2 bytes. Reproduced, not read: upstream's own translation units compiled + unmodified and fed buffers of exactly their declared length behind a + `PROT_NONE` guard page, under AddressSanitizer. +- **Source:** [MESHCORE_PARSER_BOUNDS.md](MESHCORE_PARSER_BOUNDS.md) §4, harness + and corpus in [`meshcore-parser-bounds/`](meshcore-parser-bounds/). +- **Checked:** 2026-08-23, clang 18.1.3, Ubuntu 24.04. +- **What it is not:** at every reachable call site in the pinned tree the buffer + behind these three parsers is a fixed array large enough that the read stays + *inside the allocation* — 256 B in `Dispatcher::checkRecv`, 262 B and 250 B in + the two bridges, the `Packet` object itself for adverts. The outcome in all + nine is a rejected packet. "Reads past its length" is verified; "leaves the + buffer" is verified **false** for these three, and the distinction is the whole + blast radius. +- **Hardware:** **NOT EXECUTED — HARDWARE REQUIRED.** Nothing here ran on a + radio, a node or a board, and no host sanitizer result may be presented as + radio or HIL validation. + +### `Utils::decrypt` writes 192 bytes into MeshCore's 184-byte packet buffer + +- **Claim:** `src/Utils.cpp:70-83` rounds its output up to whole 16-byte blocks + and documents the precondition in `Utils.h`; `Mesh::onRecvPacket` passes it a + wire-supplied length that does not honour it, into + `uint8_t data[MAX_PACKET_PAYLOAD]` (184 B). For `payload_len` 181…184 it writes + 192 bytes. Reproduced against the real `src/Utils.cpp` with a stub block cipher + — the bound belongs to the loop, not the cipher — faulting at `Utils.cpp:77` + for `src_len` 177, 180 and 182 and clean at 176. +- **Source:** [MESHCORE_PARSER_BOUNDS.md](MESHCORE_PARSER_BOUNDS.md) §3 P4. +- **Checked:** 2026-08-23. +- **Not from upstream:** none of the three pull requests mentions it, and it is + not filed upstream. Reporting it is the owner's call, not an agent's. +- **Not established:** what those eight bytes overwrite on an ESP32-S3, and + whether the end-to-end path through `Mesh::onRecvPacket` runs — both need + builds this project has not made. See + [OPEN_QUESTIONS.md](OPEN_QUESTIONS.md) M17. + +### Attadipa's own frame decoder validates length before reading + +- **Claim:** `link/src/frame_codec.cpp` rejects a declared length greater than + `kMaxPayload` at `:139` before touching a payload byte, waits rather than reads + when fewer bytes have arrived than the header declares (`:148`), and gates both + behind a length-check byte (`:123`) and a CRC. The MeshCore findings do not + transplant onto it. +- **Source:** the file, read on 2026-08-23 while answering issue #142. +- **Why it is here:** the finding that prompted the check was about a different + protocol boundary with different invariants, and "our decoder is probably fine" + is not a fact. This one was looked at. + --- ## Toolchain / host environment diff --git a/docs/research/meshcore-parser-bounds/.gitignore b/docs/research/meshcore-parser-bounds/.gitignore new file mode 100644 index 00000000..567609b1 --- /dev/null +++ b/docs/research/meshcore-parser-bounds/.gitignore @@ -0,0 +1 @@ +build/ diff --git a/docs/research/meshcore-parser-bounds/README.md b/docs/research/meshcore-parser-bounds/README.md new file mode 100644 index 00000000..abb5238a --- /dev/null +++ b/docs/research/meshcore-parser-bounds/README.md @@ -0,0 +1,63 @@ +# MeshCore parser-bounds harness + +The executable half of [`../MESHCORE_PARSER_BOUNDS.md`](../MESHCORE_PARSER_BOUNDS.md). +Read that first — this directory is the evidence, not the argument. + +**This is not part of any Attadipa build.** No CMake target references it and +none should. It lives under `docs/` so that it cannot be swept into one by a +recursive glob, and it is kept because the alternative to a runnable harness is +a table of results nobody can re-derive when the upstream revision moves. + +## What it does + +Compiles four upstream MeshCore translation units — `src/Packet.cpp`, +`src/Dispatcher.cpp`, `src/helpers/AdvertDataHelpers.cpp`, `src/Utils.cpp` — +**unmodified**, at whichever revision you name, and feeds their parsers inputs +that are exactly as long as they claim to be. + +Each input ends flush against a `PROT_NONE` guard page and the build is under +AddressSanitizer, so a read at `src[len]` is caught either way. Both mechanisms +are needed: ASan names the source line, and ASan alone **does not report a read +at offset 0 of a `malloc(0)`**, which silently passed two real findings before +the guard page was added. + +## The shims are not implementations + +`shim/` holds `Arduino.h`, `Stream.h`, `SHA256.h` and `AES.h`. They exist so the +upstream files link on a desktop. **`SHA256` returns zeros and `AES128` copies +its block through.** Neither is a cipher, neither may be used as one, and both +say so in their own headers. Nothing measured here depends on what they compute — +only on how many bytes the code around them moves, which is a property of the +loops and not of the block functions they call. + +## Running it + +```bash +git clone --filter=blob:none https://github.com/meshcore-dev/MeshCore /tmp/meshcore-src +git -C /tmp/meshcore-src fetch origin 05da523ebd32980a1c28b11f2928d351796b9737 +git -C /tmp/meshcore-src fetch origin f80d805ee8b20f77ff5b3ca6bc3a9021989aafd2 + +./build.sh base d92964352441e53b93e8667b802e04f6e072b39e # the pin +./build.sh pr3267 05da523ebd32980a1c28b11f2928d351796b9737 # PR #3267 head +./build.sh pr3270 f80d805ee8b20f77ff5b3ca6bc3a9021989aafd2 # PR #3270 head +./run.sh + +./build-extras.sh +./build/path_arith # P3, exhaustive +./build/decrypt_bounds 180 # P4, faults; 176 is clean +``` + +`MESHCORE_SRC` overrides the clone location. Output goes to `build/`, which is +ignored. Needs `clang++` with AddressSanitizer; measured on clang 18.1.3 under +Ubuntu 24.04 on 2026-08-23. + +## Checking a new revision + +That is the point of keeping it. `./build.sh && ./run.sh` answers +"does this revision still over-read" in one command, and +[`../MESHCORE_PARSER_BOUNDS.md`](../MESHCORE_PARSER_BOUNDS.md) §5 makes running +it the entry condition for pinning MeshCore into a local provider. + +Two findings are **not** in `run.sh`'s matrix and have to be checked separately — +P3 through `path_arith` and P4 through `decrypt_bounds`. A green `run.sh` is not +a clean revision. diff --git a/docs/research/meshcore-parser-bounds/build-extras.sh b/docs/research/meshcore-parser-bounds/build-extras.sh new file mode 100755 index 00000000..1e4d4a6a --- /dev/null +++ b/docs/research/meshcore-parser-bounds/build-extras.sh @@ -0,0 +1,38 @@ +#!/usr/bin/env bash +# +# The two experiments that are not part of the ten-case corpus: +# +# ./build/path_arith finding P3 — every (len, path_len) pair that +# reaches src/Mesh.cpp:161-172, and which of them +# underflow extra_len. Self-contained: an +# extraction of the index arithmetic, not a build +# of the upstream translation unit, and the file +# says so at the top. +# +# ./build/decrypt_bounds N finding P4 — the real src/Utils.cpp compiled +# against a stub block cipher, writing into a +# 184-byte destination backed by a guard page. +# N is src_len; 176 is clean, 177..180 are not. + +set -euo pipefail + +MESHCORE_SRC="${MESHCORE_SRC:-/tmp/meshcore-src}" +here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +out="$here/build" +tree="$out/tree-base" + +mkdir -p "$out" + +clang++ -std=c++17 -g -O0 "$here/path_arith.cpp" -o "$out/path_arith" +echo "built build/path_arith" + +if [ ! -d "$tree/src" ]; then + echo "build/tree-base is missing — run ./build.sh base first" >&2 + exit 66 +fi + +clang++ -std=c++17 -g -O0 -fsanitize=address -fno-omit-frame-pointer \ + -I"$tree/src" -I"$here/shim" \ + "$here/decrypt_bounds.cpp" "$here/shim/shim.cpp" "$tree/src/Utils.cpp" \ + -o "$out/decrypt_bounds" +echo "built build/decrypt_bounds" diff --git a/docs/research/meshcore-parser-bounds/build.sh b/docs/research/meshcore-parser-bounds/build.sh new file mode 100755 index 00000000..1f61468f --- /dev/null +++ b/docs/research/meshcore-parser-bounds/build.sh @@ -0,0 +1,64 @@ +#!/usr/bin/env bash +# +# Build the parser-bounds harness against one MeshCore revision. +# +# ./build.sh +# ./build.sh base d92964352441e53b93e8667b802e04f6e072b39e +# +# names the output; is anything the upstream clone can resolve. +# The sources are exported with `git archive`, so the clone is never modified and +# two revisions can be built side by side. +# +# Nothing here is part of an Attadipa build. See ../MESHCORE_PARSER_BOUNDS.md. + +set -euo pipefail + +MESHCORE_SRC="${MESHCORE_SRC:-/tmp/meshcore-src}" +here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +out="$here/build" + +if [ $# -ne 2 ]; then + echo "usage: $0 " >&2 + exit 64 +fi +tag=$1 +ref=$2 + +if [ ! -d "$MESHCORE_SRC/.git" ]; then + cat >&2 < +EOF + exit 66 +fi + +if ! git -C "$MESHCORE_SRC" rev-parse --verify --quiet "$ref^{commit}" >/dev/null; then + echo "$MESHCORE_SRC cannot resolve '$ref' — fetch it first:" >&2 + echo " git -C $MESHCORE_SRC fetch origin $ref" >&2 + exit 66 +fi + +tree="$out/tree-$tag" +rm -rf "$tree" +mkdir -p "$tree" +git -C "$MESHCORE_SRC" archive "$ref" src | tar -x -C "$tree" + +# Four upstream translation units, unmodified, plus the harness and the shim. +# -O0 so the sanitizer's line numbers name the statement rather than whatever a +# pass hoisted it into; the findings are about bounds, not about codegen. +clang++ -std=c++17 -g -O0 -fsanitize=address -fno-omit-frame-pointer \ + -I"$tree/src" -I"$here/shim" \ + "$here/harness.cpp" "$here/shim/shim.cpp" \ + "$tree/src/Packet.cpp" \ + "$tree/src/Dispatcher.cpp" \ + "$tree/src/helpers/AdvertDataHelpers.cpp" \ + -o "$out/harness-$tag" + +echo "built build/harness-$tag from $(git -C "$MESHCORE_SRC" rev-parse "$ref")" diff --git a/docs/research/meshcore-parser-bounds/decrypt_bounds.cpp b/docs/research/meshcore-parser-bounds/decrypt_bounds.cpp new file mode 100644 index 00000000..23dcdc90 --- /dev/null +++ b/docs/research/meshcore-parser-bounds/decrypt_bounds.cpp @@ -0,0 +1,39 @@ +// Utils::decrypt output bound — Attadipa issue #142, finding P4. +// +// Real upstream translation unit (src/Utils.cpp, unmodified) against a stub +// block cipher. The cipher is irrelevant: what is under test is how far the +// loop at src/Utils.cpp:76-81 walks 'dest' for a src_len that is not a multiple +// of the block size, which is exactly the shape Mesh::onRecvPacket hands it. +// +// dest is 184 bytes — sizeof(uint8_t data[MAX_PACKET_PAYLOAD]) in +// Mesh::onRecvPacket — placed flush against a PROT_NONE page. + +#include +#include +#include +#include +#include +#include + +int main(int argc, char** argv) +{ + const int src_len = argc > 1 ? atoi(argv[1]) : 180; + + const size_t page = (size_t)sysconf(_SC_PAGESIZE); + uint8_t* m = (uint8_t*)mmap(nullptr, page * 2, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + mprotect(m + page, page, PROT_NONE); + uint8_t* dest = m + page - MAX_PACKET_PAYLOAD; // 184 bytes, then the wall + + static uint8_t src[512]; + static uint8_t key[CIPHER_KEY_SIZE] = {0}; + memset(src, 0xAA, sizeof(src)); + + std::printf("dest = 184 bytes (uint8_t data[MAX_PACKET_PAYLOAD]), src_len = %d\n", src_len); + std::fflush(stdout); + + int n = mesh::Utils::decrypt(key, dest, src, src_len); + + std::printf("decrypt() returned %d — wrote dest[0..%d]\n", n, n - 1); + return 0; +} diff --git a/docs/research/meshcore-parser-bounds/harness.cpp b/docs/research/meshcore-parser-bounds/harness.cpp new file mode 100644 index 00000000..ce1547cd --- /dev/null +++ b/docs/research/meshcore-parser-bounds/harness.cpp @@ -0,0 +1,206 @@ +// Host research harness for MeshCore parser bounds — Attadipa issue #142. +// +// This file is the harness only. Every parser it exercises is compiled from +// upstream MeshCore sources unmodified; nothing here reimplements one. +// +// Design note on the input buffer. Each case gets EXACTLY the declared length, +// ending flush against a PROT_NONE guard page, and the build is also under +// AddressSanitizer. So a read at src[len] is caught whichever way. That is the +// point: it isolates "the parser reads past the length it was given" from "the +// caller happened to hand it a bigger array", which are different claims and +// only the first is a property of the parser. The second is answered by reading +// the call sites, and the report says what that reading found. + +#include +#include +#include +#include + +#include +#include + +#include +#include +#include +#include + +// ---------------------------------------------------------------- stubs ---- + +class StubClock : public mesh::MillisecondClock { +public: + unsigned long getMillis() override { return 0; } +}; + +class StubRadio : public mesh::Radio { +public: + int recvRaw(uint8_t*, int) override { return 0; } + uint32_t getEstAirtimeFor(int) override { return 0; } + float packetScore(float, int) override { return 0; } + bool startSendRaw(const uint8_t*, int) override { return true; } + bool isSendComplete() override { return true; } + void onSendFinished() override {} + bool isInRecvMode() const override { return true; } +}; + +class StubMgr : public mesh::PacketManager { +public: + mesh::Packet* allocNew() override { return new mesh::Packet(); } + void free(mesh::Packet* p) override { delete p; } + void queueOutbound(mesh::Packet*, uint8_t, uint32_t) override {} + mesh::Packet* getNextOutbound(uint32_t) override { return nullptr; } + int getOutboundCount(uint32_t) const override { return 0; } + int getOutboundTotal() const override { return 0; } + int getFreeCount() const override { return 0; } + mesh::Packet* getOutboundByIdx(int) override { return nullptr; } + mesh::Packet* removeOutboundByIdx(int) override { return nullptr; } + void queueInbound(mesh::Packet*, uint32_t) override {} + mesh::Packet* getNextInbound(uint32_t) override { return nullptr; } +}; + +class TestDispatcher : public mesh::Dispatcher { +public: + TestDispatcher(mesh::Radio& r, mesh::MillisecondClock& c, mesh::PacketManager& m) + : mesh::Dispatcher(r, c, m) {} + mesh::DispatcherAction onRecvPacket(mesh::Packet*) override { return 0; } +}; + +// ----------------------------------------------------------------- rig ----- + +// Two mechanisms, because neither alone covers every case. +// +// ASan's redzone catches a read past a heap chunk and names the source line, +// which is what makes the evidence quotable. But an ASan malloc(0) does NOT +// report a read at offset 0 — measured, and it silently turned two len=0 cases +// green on a build that had no guard at all. So the buffer is instead placed +// against a PROT_NONE guard page and ends exactly at the boundary: a read at +// src[len] faults for every len including zero, with no sanitizer in the story. +static uint8_t* tight(const std::initializer_list& bytes, size_t& len) +{ + len = bytes.size(); + const size_t page = static_cast(::sysconf(_SC_PAGESIZE)); + uint8_t* base = static_cast( + ::mmap(nullptr, page * 2, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)); + if (base == MAP_FAILED) { std::perror("mmap"); std::exit(2); } + if (::mprotect(base + page, page, PROT_NONE) != 0) { std::perror("mprotect"); std::exit(2); } + + uint8_t* p = base + page - len; // last declared byte abuts the guard page + size_t i = 0; + for (uint8_t b : bytes) p[i++] = b; + return p; +} + +static void release(uint8_t*) { /* deliberately leaked: the guard page must + outlive the case, and the process is about + to exit either way. */ } + +static void banner(const char* name, const char* what) +{ + std::printf("\n--- %s : %s\n", name, what); + std::fflush(stdout); // the sanitizer report may be the next thing written +} + +// --------------------------------------------------- one per parser under test -- + +static void case_tryParsePacket(const char* name, std::initializer_list bytes, + const char* what) +{ + banner(name, what); + size_t len = 0; + uint8_t* raw = tight(bytes, len); + + StubRadio radio; StubClock clock; StubMgr mgr; + TestDispatcher d(radio, clock, mgr); + mesh::Packet pkt; + + bool ok = d.tryParsePacket(&pkt, raw, static_cast(len)); + std::printf(" returned %s (path_len=%u payload_len=%u)\n", + ok ? "true" : "false", (unsigned)pkt.path_len, (unsigned)pkt.payload_len); + std::fflush(stdout); + release(raw); +} + +static void case_readFrom(const char* name, std::initializer_list bytes, + const char* what) +{ + banner(name, what); + size_t len = 0; + uint8_t* src = tight(bytes, len); + + mesh::Packet pkt; + bool ok = pkt.readFrom(src, static_cast(len)); + std::printf(" returned %s (path_len=%u payload_len=%u)\n", + ok ? "true" : "false", (unsigned)pkt.path_len, (unsigned)pkt.payload_len); + std::fflush(stdout); + release(src); +} + +static void case_advert(const char* name, std::initializer_list bytes, + const char* what) +{ + banner(name, what); + size_t len = 0; + uint8_t* app = tight(bytes, len); + + AdvertDataParser parser(app, static_cast(len)); + std::printf(" valid=%s type=%u hasLatLon=%s lat=%d lon=%d name=\"%s\"\n", + parser.isValid() ? "true" : "false", (unsigned)parser.getType(), + parser.hasLatLon() ? "true" : "false", + (int)parser.getIntLat(), (int)parser.getIntLon(), parser.getName()); + std::fflush(stdout); + release(app); +} + +// ------------------------------------------------------------------ main --- + +int main(int argc, char** argv) +{ + const std::string only = argc > 1 ? argv[1] : ""; + auto want = [&](const char* n) { return only.empty() || only == n; }; + + std::printf("MeshCore parser bounds harness — one case per process is the\n" + "usable mode under ASan, because the first report aborts.\n"); + + // A. Dispatcher::tryParsePacket + // header 0x01 = ROUTE_TYPE_FLOOD -> no transport codes. + // header 0x00 = ROUTE_TYPE_TRANSPORT_FLOOD -> four transport-code bytes. + if (want("A1")) + case_tryParsePacket("A1", {0x01}, + "len=1, flood route: path_len byte is read at raw[1]"); + if (want("A2")) + case_tryParsePacket("A2", {0x00}, + "len=1, transport route: four transport bytes read at raw[1..4]"); + if (want("A3")) + case_tryParsePacket("A3", {}, + "len=0: header read at raw[0] (not reachable from checkRecv, which gates on len>0)"); + + // B. Packet::readFrom + // path_len 0x3F = 63 hashes of 1 byte = 63 path bytes claimed. + if (want("B1")) + case_readFrom("B1", {0x01, 0x3F}, + "len=2, 63 path bytes claimed: 63-byte read at src[2..64]"); + if (want("B2")) + case_readFrom("B2", {0x01}, + "len=1, flood route: path_len byte read at src[1]"); + if (want("B3")) + case_readFrom("B3", {0x00}, + "len=1, transport route: transport bytes read at src[1..4]"); + + // C. AdvertDataParser + // flags 0x10 = LATLON, 0x20 = FEAT1, 0x40 = FEAT2, 0x80 = NAME. + if (want("C1")) + case_advert("C1", {0x91}, + "len=1, LATLON|NAME: eight lat/lon bytes read at app_data[1..8]"); + if (want("C2")) + case_advert("C2", {0xF1}, + "len=1, all flags: twelve bytes read at app_data[1..12]"); + if (want("C3")) + case_advert("C3", {}, + "len=0: flags byte read at app_data[0]"); + if (want("C4")) + case_advert("C4", {0x21}, + "len=1, FEAT1 only: two bytes read at app_data[1..2]"); + + std::printf("\nall requested cases ran to completion\n"); + return 0; +} diff --git a/docs/research/meshcore-parser-bounds/path_arith.cpp b/docs/research/meshcore-parser-bounds/path_arith.cpp new file mode 100644 index 00000000..77951f03 --- /dev/null +++ b/docs/research/meshcore-parser-bounds/path_arith.cpp @@ -0,0 +1,88 @@ +// PAYLOAD_TYPE_PATH length arithmetic — Attadipa issue #142, finding P3. +// +// THIS IS AN EXTRACTION, NOT AN EXECUTION OF THE UPSTREAM TRANSLATION UNIT. +// Reaching src/Mesh.cpp's PATH branch for real needs Utils::MACThenDecrypt to +// succeed, which needs the AES and SHA256 libraries MeshCore pulls from +// PlatformIO, and an Identity, which needs ed25519. None of that is built here. +// What is reproduced below is the index arithmetic, copied byte for byte from +// +// meshcore-dev/MeshCore @ d92964352441e53b93e8667b802e04f6e072b39e +// src/Mesh.cpp lines 161-172 +// +// and run over its entire input domain, so the claim "extra_len can underflow" +// is a computed result rather than an assertion about code someone read. + +#include +#include + +#define MAX_PACKET_PAYLOAD 184 +#define MAX_PATH_SIZE 64 + +// src/Packet.cpp:13-18, verbatim. +static bool isValidPathLen(uint8_t path_len) { + uint8_t hash_count = path_len & 63; + uint8_t hash_size = (path_len >> 6) + 1; + if (hash_size == 4) return false; // Reserved for future + return hash_count*hash_size <= MAX_PATH_SIZE; +} + +int main() +{ + // The decrypted plaintext length. MACThenDecrypt returns 0 for anything at + // or under CIPHER_MAC_SIZE, and cannot exceed the payload buffer. + // Utils::decrypt returns whole 16-byte blocks ("will always be multiple of + // 16", src/Utils.cpp:82), so len is a multiple of 16. 176 is the largest + // that still fits data[184]; 192 is the P4 case and is excluded here so the + // two findings do not get mixed up. + const int len_min = 16, len_max = 176, len_step = 16; + + long total = 0, underflow = 0, read_past_data = 0; + int worst_extra_len = 0, worst_len = 0, worst_path_len = 0, worst_k = 0; + int smallest_len = 1 << 30, smallest_path_len = 256; + + for (int len = len_min; len <= len_max; len += len_step) { + for (int pl = 0; pl <= 255; pl++) { + uint8_t path_len = (uint8_t)pl; + if (!isValidPathLen(path_len)) continue; // upstream rejects and breaks + total++; + + // --- src/Mesh.cpp:161-172, indices only --- + int k = 0; + k++; // uint8_t path_len = data[k++] + uint8_t hash_size = (path_len >> 6) + 1; + uint8_t hash_count = path_len & 63; + k += hash_size * hash_count; // uint8_t* path = &data[k]; k += ... + k++; // extra_type = data[k++] & 0x0F + uint8_t extra_len = (uint8_t)(len - k); // <-- the subtraction under test + // --- end extraction --- + + if (len - k < 0) { + underflow++; + if (len < smallest_len || (len == smallest_len && path_len < smallest_path_len)) { + smallest_len = len; smallest_path_len = path_len; + } + // Does the consumer's window run off the end of data[184]? + if (k + extra_len > MAX_PACKET_PAYLOAD) { + read_past_data++; + if (extra_len > worst_extra_len) { + worst_extra_len = extra_len; worst_len = len; + worst_path_len = path_len; worst_k = k; + } + } + } + } + } + + std::printf("accepted (len, path_len) pairs : %ld\n", total); + std::printf("pairs where k > len (extra_len underflows): %ld\n", underflow); + std::printf(" of those, &data[k] + extra_len > 184 : %ld\n", read_past_data); + std::printf("smallest underflowing input : len=%d path_len=0x%02X\n", + smallest_len, smallest_path_len); + std::printf("largest over-read window : len=%d path_len=0x%02X" + " -> k=%d extra_len=%d, i.e. data[%d..%d] against data[0..183]" + " (%d bytes past the end)\n", + worst_len, worst_path_len, worst_k, worst_extra_len, + worst_k, worst_k + worst_extra_len - 1, + worst_k + worst_extra_len - MAX_PACKET_PAYLOAD); + return 0; +} diff --git a/docs/research/meshcore-parser-bounds/run.sh b/docs/research/meshcore-parser-bounds/run.sh new file mode 100755 index 00000000..fc461045 --- /dev/null +++ b/docs/research/meshcore-parser-bounds/run.sh @@ -0,0 +1,50 @@ +#!/usr/bin/env bash +# +# Run the ten-case corpus against every harness that has been built, and print +# the matrix in §4 of ../MESHCORE_PARSER_BOUNDS.md. +# +# One case per process, because the first sanitizer report ends the process and +# a second case in the same run would never be reached. + +set -uo pipefail + +here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +out="$here/build" +cases=(A1 A2 A3 B1 B2 B3 C1 C2 C3 C4) + +shopt -s nullglob +harnesses=("$out"/harness-*) +if [ ${#harnesses[@]} -eq 0 ]; then + echo "nothing built yet — see build.sh" >&2 + exit 66 +fi + +for h in "${harnesses[@]}"; do + tag="${h##*/harness-}" + for c in "${cases[@]}"; do + outp=$(ASAN_OPTIONS=detect_leaks=0 "$h" "$c" 2>&1); rc=$? + if grep -q "AddressSanitizer" <<<"$outp"; then + site=$(grep -oE '(Dispatcher|Packet|AdvertDataHelpers)\.cpp:[0-9]+:[0-9]+' <<<"$outp" | head -1) + # Two wordings for one fact. A read just past a heap chunk is a + # "heap-buffer-overflow" with a size; a read into the guard page is + # a SEGV that ASan reports without one. Both are the parser reading + # past the length it was given, so both print here — but they print + # differently, because a row that hid which mechanism fired would + # make a harness bug look like a finding. + kind=$(grep -oE 'heap-buffer-overflow|SEGV|stack-buffer-overflow' <<<"$outp" | head -1) + size=$(grep -oE 'READ of size [0-9]+' <<<"$outp" | head -1) + printf '%-8s %-3s OOB %-22s at %s\n' "$tag" "$c" \ + "${kind}${size:+, $size}" "$site" + elif [ $rc -ge 128 ]; then + # The guard page without a sanitizer report: still an over-read, but + # ASan did not name the line. Reported rather than swallowed. + printf '%-8s %-3s OOB guard page, signal %d\n' "$tag" "$c" "$((rc - 128))" + elif [ $rc -ne 0 ]; then + printf '%-8s %-3s ERROR harness exited %d\n' "$tag" "$c" "$rc" + else + printf '%-8s %-3s clean %s\n' "$tag" "$c" \ + "$(grep -E 'returned|valid=' <<<"$outp" | head -1 | sed 's/^ *//')" + fi + done + echo +done diff --git a/docs/research/meshcore-parser-bounds/shim/AES.h b/docs/research/meshcore-parser-bounds/shim/AES.h new file mode 100644 index 00000000..fad2f066 --- /dev/null +++ b/docs/research/meshcore-parser-bounds/shim/AES.h @@ -0,0 +1,14 @@ +// Host shim. Not upstream code, and NOT a cipher: decryptBlock/encryptBlock +// copy their 16 bytes through. The experiment this serves measures how many +// bytes Utils::decrypt writes for a given src_len, which is a property of its +// loop and not of the block function it calls. +#pragma once +#include +#include +#include +class AES128 { +public: + void setKey(const uint8_t*, size_t) {} + void decryptBlock(uint8_t* out, const uint8_t* in) { memcpy(out, in, 16); } + void encryptBlock(uint8_t* out, const uint8_t* in) { memcpy(out, in, 16); } +}; diff --git a/docs/research/meshcore-parser-bounds/shim/Arduino.h b/docs/research/meshcore-parser-bounds/shim/Arduino.h new file mode 100644 index 00000000..657ce6d5 --- /dev/null +++ b/docs/research/meshcore-parser-bounds/shim/Arduino.h @@ -0,0 +1,28 @@ +// Host shim. Not upstream code: enough of the Arduino surface for the pinned +// MeshCore parser translation units to compile on a desktop. +#pragma once +#include +#include +#include +#include +#include +class Print { +public: + virtual size_t write(uint8_t) { return 1; } + size_t print(const char*) { return 0; } + size_t print(char) { return 0; } + size_t println(const char*) { return 0; } + int printf(const char*, ...) { return 0; } +}; +class Stream : public Print { +public: + virtual int available() { return 0; } + virtual int read() { return -1; } + virtual int peek() { return -1; } + virtual size_t readBytes(uint8_t*, size_t) { return 0; } + virtual size_t write(const uint8_t*, size_t n) { return n; } + using Print::write; +}; +extern Stream Serial; +static inline unsigned long millis() { return 0; } +static inline void delay(unsigned long) {} diff --git a/docs/research/meshcore-parser-bounds/shim/SHA256.h b/docs/research/meshcore-parser-bounds/shim/SHA256.h new file mode 100644 index 00000000..244da0a0 --- /dev/null +++ b/docs/research/meshcore-parser-bounds/shim/SHA256.h @@ -0,0 +1,15 @@ +// Host shim. Not upstream code. The parsers under test do not depend on the +// digest value; nothing here is a cryptographic implementation and it must +// never be used as one. +#pragma once +#include +#include +#include +class SHA256 { +public: + void reset() {} + void update(const void*, size_t) {} + void finalize(void* out, size_t len) { memset(out, 0, len); } + void resetHMAC(const void*, size_t) {} + void finalizeHMAC(const void*, size_t, void* out, size_t len) { memset(out, 0, len); } +}; diff --git a/docs/research/meshcore-parser-bounds/shim/Stream.h b/docs/research/meshcore-parser-bounds/shim/Stream.h new file mode 100644 index 00000000..5718c27c --- /dev/null +++ b/docs/research/meshcore-parser-bounds/shim/Stream.h @@ -0,0 +1,2 @@ +#pragma once +#include diff --git a/docs/research/meshcore-parser-bounds/shim/shim.cpp b/docs/research/meshcore-parser-bounds/shim/shim.cpp new file mode 100644 index 00000000..b6c1fd6a --- /dev/null +++ b/docs/research/meshcore-parser-bounds/shim/shim.cpp @@ -0,0 +1,2 @@ +#include +Stream Serial; From a0e652cdcf12b82841cef75c5b65198f8525c243 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Sun, 23 Aug 2026 16:14:52 +0000 Subject: [PATCH 2/5] tryParsePacket has a second caller, and it is the one on our side of the link MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correcting this branch's own first commit. The report said Dispatcher::checkRecv was the sole caller of tryParsePacket, which a grep across the tree contradicts: examples/companion_radio/MyMesh.cpp:2000 calls it too, from CMD_SEND_RAW_PACKET, on a client-supplied buffer gated only by len >= 4. That is not a detail. It is the single place in the whole review where a *client* hands bytes to a MeshCore parser, and a client is precisely what Attadipa is on the node path — so the one direction the report described as having no exposure at all turns out to have exactly one. Four bytes from any connected app make the node read four bytes of its previous command frame. The consequence is still nil, and for the same reason as everywhere else here: cmd_frame is a 177-byte member array, the parser's furthest reach is raw[5], the read stays inside the allocation and the call returns ERR_CODE_ILLEGAL_ARG. The neighbouring companion path was checked in the other direction and holds -- CMD_IMPORT_CONTACT gates on len > 98 while Packet::readFrom reaches at most byte 70, so that one cannot be triggered at all. What changes is the reachability claim, which was wrong, and what a future Attadipa client that emits raw packets has to know. Two smaller fixes found in the same pass: the three memcpys in onContactResponse are at MyMesh.cpp:722, :733 and :744, not :735, and `i` is 8 in the status and telemetry branches but 6 in the binary-response one. Found by verifying the report's own citations against the tree rather than against my notes, which is where the missing caller had been lost. Co-Authored-By: Claude Opus 5 --- TASKS.md | 13 ++++-- docs/research/MESHCORE_PARSER_BOUNDS.md | 54 +++++++++++++++++++------ docs/research/VERIFIED_FACTS.md | 17 +++++--- 3 files changed, 62 insertions(+), 22 deletions(-) diff --git a/TASKS.md b/TASKS.md index 559a001a..f5dc992f 100644 --- a/TASKS.md +++ b/TASKS.md @@ -1854,10 +1854,15 @@ harness and corpus: `Packet` object for adverts — and every one of the nine ends in a rejected packet. The parsers do read past their length; nothing escapes a buffer through them. Keeping those two sentences apart is most of the value here. -- **The companion path cannot reach the worst of the three.** `CMD_IMPORT_CONTACT` - gates on `len > 98` and `Packet::readFrom` reaches at most byte 70, so a - malformed contact blob from a client — which is what Attadipa is — cannot - trigger it. Luck rather than design, but it holds at this revision. +- **The companion path reaches one of the three and not the other.** + `CMD_IMPORT_CONTACT` gates on `len > 98` while `Packet::readFrom` reaches at + most byte 70, so a malformed contact blob from a client — which is what + Attadipa is — cannot trigger it; luck rather than design, but it holds here. + `CMD_SEND_RAW_PACKET` **does** reach `tryParsePacket`, gated only by `len >= 4`, + which makes it the single place where our side hands bytes to a MeshCore + parser. It stays inside `cmd_frame` and returns `ERR_CODE_ILLEGAL_ARG`, so it + costs nothing today — but it is the fact to have before writing a client that + emits raw packets. - **Two of the three pull requests do not close their own findings.** #3269 logs the condition and then performs the read; #3270 leaves `app_data[0]` unguarded at `AdvertDataHelpers.cpp:34` for `app_data_len == 0`, measured on its own head. diff --git a/docs/research/MESHCORE_PARSER_BOUNDS.md b/docs/research/MESHCORE_PARSER_BOUNDS.md index 38990b5e..e618e90b 100644 --- a/docs/research/MESHCORE_PARSER_BOUNDS.md +++ b/docs/research/MESHCORE_PARSER_BOUNDS.md @@ -145,21 +145,34 @@ The path-bytes check at `:173` is there. The three reads before it are not. route, four extra bytes), or `len < 2` otherwise. Maximum index reached before the first bound check is `raw[5]`. -**Reachable from:** the radio, and only the radio. `Dispatcher::checkRecv` -(`:190-217`) is the sole caller. +**Reachable from: two callers, not one, and the second is the interesting one.** -**What actually backs `raw`:** `uint8_t raw[MAX_TRANS_UNIT+1]` — a **256-byte -stack array** in `checkRecv`, filled by `_radio->recvRaw(raw, 255)`. The furthest -this parser can reach is `raw[5]`. **The read never leaves the allocation.** It -reads stack bytes the radio did not write on this pass — the tail of an earlier -frame, or whatever the frame before that left there. +| Caller | Direction | Buffer behind `raw` | +|---|---|---| +| `Dispatcher::checkRecv`, `src/Dispatcher.cpp:205` | the radio | `uint8_t raw[MAX_TRANS_UNIT+1]` — a **256-byte stack array**, filled by `_radio->recvRaw(raw, 255)` | +| `MyMesh::handleCmdFrame`, `examples/companion_radio/MyMesh.cpp:2000` | **the companion link** — `CMD_SEND_RAW_PACKET`, `tryParsePacket(pkt, &cmd_frame[2], len - 2)` | `cmd_frame[MAX_FRAME_SIZE+1]` — a **177-byte member array** of `MyMesh` | + +The second is the one place in this whole document where **a client hands bytes +to a MeshCore parser**, and a client is what Attadipa is. Its guard is `len >= 4`, +so the parser can be called with a declared length of 2, and with a transport +route in the header it then reads `cmd_frame[4..7]` having been given +`cmd_frame[2..3]`. That is reachable by any connected app, ours included, by +sending four bytes. + +**What actually backs `raw`, either way:** a fixed array far larger than this +parser's furthest reach, which is `raw[5]`. **The read never leaves the +allocation** on either caller. It picks up bytes the current frame did not write +— the tail of an earlier frame on the radio path, the tail of an earlier command +on the companion path. **Outcome:** `reject`, in every case. Follow the arithmetic: whatever garbage `path_len` picks up, `i` is already at least 2, so `i + path_byte_len > len` holds for any `len` short enough to have triggered the over-read, and `tryParsePacket` returns `false`. No crash, no disclosure — the parsed packet is -freed. `checkRecv` also gates on `len > 0`, so the missing `len < 1` guard that -#3267 adds is unreachable through this caller. +freed. Both callers also gate on a positive length (`len > 0` in `checkRecv`, +`len >= 4` on the command), so the missing `len < 1` guard that #3267 adds is +unreachable through either; case A3 exists to characterise the function, not a +reachable state. **Status:** confirmed by execution (A1, A2, A3). Fixed on `#3267`'s head. **Severity for a stock node: cosmetic — a use of uninitialised memory whose @@ -273,11 +286,12 @@ the stock firmware Attadipa's node path talks to, the chain is `pending_*` branches each do ```cpp -memcpy(&out_frame[i], &data[4], len - 4); // MyMesh.cpp:722, :735, :744 +memcpy(&out_frame[i], &data[4], len - 4); // MyMesh.cpp:722, :733, :744 ``` -with `out_frame[MAX_FRAME_SIZE + 1]` = **177 bytes** and `i` = 8. With `len` -underflowed to 255 that writes about 82 bytes past a member array — a write, not +with `out_frame[MAX_FRAME_SIZE + 1]` = **177 bytes** and `i` already at 8 in the +status and telemetry branches, 6 in the binary-response one. With `len` +underflowed to 255 that writes some 80 bytes past a member array — a write, not a read. Two things stop this being called a proven memory-corruption path and both are load-bearing: @@ -464,7 +478,21 @@ stock-node path is a protocol client behind [ADR-0008](../adr/0008-mesh-service-providers.md). Every one of P1–P5 executes on the node, on the far side of the companion link. -It changes three things anyway. +It changes four things anyway. + +**One of them is reachable from our side of the link, and it is the only one.** +`CMD_SEND_RAW_PACKET` calls `tryParsePacket` on a client-supplied buffer +(`examples/companion_radio/MyMesh.cpp:2000`), gated only by `len >= 4`, so a +four-byte command from any connected app makes the node read four bytes of its +own previous command frame. It stays inside `cmd_frame` and ends in +`ERR_CODE_ILLEGAL_ARG`, so the consequence is nil — but it is the one place where +Attadipa is the party handing bytes to a MeshCore parser rather than the party +receiving the result, and that is worth knowing before anyone writes a client +that emits raw packets. Two practical consequences, neither of them urgent: if +Attadipa ever uses that opcode it should send a complete frame or none, and a +node shared with another client is a node whose parser another client can poke. +It does **not** follow that the companion link is dangerous — P2's +`CMD_IMPORT_CONTACT` path was checked and cannot be reached at all. **The node's output is a peer's output, not a trusted source.** P3 and P4 are memory-safety defects on the device that supplies Attadipa's mesh capability, and diff --git a/docs/research/VERIFIED_FACTS.md b/docs/research/VERIFIED_FACTS.md index b05c582f..204a3589 100644 --- a/docs/research/VERIFIED_FACTS.md +++ b/docs/research/VERIFIED_FACTS.md @@ -70,11 +70,18 @@ An entry that cannot name its source does not belong here. It belongs in - **Checked:** 2026-08-23, clang 18.1.3, Ubuntu 24.04. - **What it is not:** at every reachable call site in the pinned tree the buffer behind these three parsers is a fixed array large enough that the read stays - *inside the allocation* — 256 B in `Dispatcher::checkRecv`, 262 B and 250 B in - the two bridges, the `Packet` object itself for adverts. The outcome in all - nine is a rejected packet. "Reads past its length" is verified; "leaves the - buffer" is verified **false** for these three, and the distinction is the whole - blast radius. + *inside the allocation* — 256 B in `Dispatcher::checkRecv`, 177 B in + `MyMesh::handleCmdFrame`, 262 B and 250 B in the two bridges, the `Packet` + object itself for adverts. The outcome in all nine is a rejected packet. + "Reads past its length" is verified; "leaves the buffer" is verified **false** + for these three, and the distinction is the whole blast radius. +- **Reachable from the companion link, once.** `CMD_SEND_RAW_PACKET` calls + `tryParsePacket` on a client-supplied buffer with only a `len >= 4` guard + (`examples/companion_radio/MyMesh.cpp:2000`), which is the one place a + *client* — Attadipa's role on the node path — hands bytes to a MeshCore + parser. `CMD_IMPORT_CONTACT` was checked and **cannot** reach + `Packet::readFrom`'s over-read: it gates on `len > 98` and the parser reaches + at most byte 70. - **Hardware:** **NOT EXECUTED — HARDWARE REQUIRED.** Nothing here ran on a radio, a node or a board, and no host sanitizer result may be presented as radio or HIL validation. From 05e323231285025eda998b803c94096d97c10dbc Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Sun, 23 Aug 2026 16:16:11 +0000 Subject: [PATCH 3/5] Four line numbers cited from notes rather than from the file Checked every citation in the report against the tree instead of against what I had written down while reading, which is how the missing tryParsePacket caller got in. Four were off by one or two lines: the "multiple of 16" comment is src/Utils.cpp:81, the decrypt loop is :76-79, the Attadipa decoder's short-frame wait is frame_codec.cpp:147, and simple_repeater tests isValid() at MyMesh.cpp:657 rather than at the constructor a line above. Nothing a reader would have been misled by, and all four are the kind of thing that makes the next reader stop trusting the ones that matter. Co-Authored-By: Claude Opus 5 --- TASKS.md | 2 +- docs/research/MESHCORE_PARSER_BOUNDS.md | 6 +++--- docs/research/VERIFIED_FACTS.md | 2 +- docs/research/meshcore-parser-bounds/decrypt_bounds.cpp | 2 +- docs/research/meshcore-parser-bounds/path_arith.cpp | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/TASKS.md b/TASKS.md index f5dc992f..e9359081 100644 --- a/TASKS.md +++ b/TASKS.md @@ -1878,7 +1878,7 @@ harness and corpus: on a third-party repository is outward-facing and this run had no authority for it. The evidence reproduces in one command. - **`attadipa_link`'s decoder was checked, not assumed.** It validates the - declared length before reading (`frame_codec.cpp:139`, `:148`) behind a length + declared length before reading (`frame_codec.cpp:139`, `:147`) behind a length check and a CRC. The finding does not transplant, and nothing in `link/` changed. - **Left open as M15–M18** in [OPEN_QUESTIONS](docs/research/OPEN_QUESTIONS.md): diff --git a/docs/research/MESHCORE_PARSER_BOUNDS.md b/docs/research/MESHCORE_PARSER_BOUNDS.md index e618e90b..834e1d78 100644 --- a/docs/research/MESHCORE_PARSER_BOUNDS.md +++ b/docs/research/MESHCORE_PARSER_BOUNDS.md @@ -252,7 +252,7 @@ negative becomes a large positive, and `extra`/`extra_len` are then handed to `onPeerPathRecv` as a window that runs off the end of `data`. **The domain, computed rather than asserted.** `Utils::decrypt` returns whole -16-byte blocks — *"will always be multiple of 16"*, `src/Utils.cpp:82` — so `len` +16-byte blocks — *"will always be multiple of 16"*, `src/Utils.cpp:81` — so `len` is a multiple of 16. Over every `(len, path_len)` pair that reaches this code with `len` in 16…176: @@ -411,7 +411,7 @@ before anyone refactors it; not a defect today. **Outcome:** `reject`. `_valid` requires `app_data_len >= i`, which is exactly the condition that failed, so the parser returns an object the callers discard — -`BaseChatMesh.cpp:122` and `MyMesh.cpp:656` both test `isValid()`. +`BaseChatMesh.cpp:122` and `MyMesh.cpp:657` both test `isValid()`. **Status: confirmed by execution (C1, C2, C3, C4), and #3270 does not close it.** On `#3270`'s own head `f80d805e`, case C3 — `app_data_len == 0` — still reads @@ -519,7 +519,7 @@ local provider without re-running the corpus in **Our own decoder is not analogous, and it was checked rather than assumed.** `link/src/frame_codec.cpp` validates the declared length *before* reading — `if (declared > kMaxPayload)` at `:139`, counted as its own error class, and -`if (size_ < needed) return 0;` at `:148` before any payload is touched — behind +`if (size_ < needed) return 0;` at `:147` before any payload is touched — behind a length-check byte at `:123` and a CRC. The issue's caution about not transplanting the finding onto it is right. **No change to `attadipa_link` is proposed by this document.** Its boundary fixtures may still be worth extending diff --git a/docs/research/VERIFIED_FACTS.md b/docs/research/VERIFIED_FACTS.md index 204a3589..b2234f37 100644 --- a/docs/research/VERIFIED_FACTS.md +++ b/docs/research/VERIFIED_FACTS.md @@ -108,7 +108,7 @@ An entry that cannot name its source does not belong here. It belongs in - **Claim:** `link/src/frame_codec.cpp` rejects a declared length greater than `kMaxPayload` at `:139` before touching a payload byte, waits rather than reads - when fewer bytes have arrived than the header declares (`:148`), and gates both + when fewer bytes have arrived than the header declares (`:147`), and gates both behind a length-check byte (`:123`) and a CRC. The MeshCore findings do not transplant onto it. - **Source:** the file, read on 2026-08-23 while answering issue #142. diff --git a/docs/research/meshcore-parser-bounds/decrypt_bounds.cpp b/docs/research/meshcore-parser-bounds/decrypt_bounds.cpp index 23dcdc90..ceba6d25 100644 --- a/docs/research/meshcore-parser-bounds/decrypt_bounds.cpp +++ b/docs/research/meshcore-parser-bounds/decrypt_bounds.cpp @@ -2,7 +2,7 @@ // // Real upstream translation unit (src/Utils.cpp, unmodified) against a stub // block cipher. The cipher is irrelevant: what is under test is how far the -// loop at src/Utils.cpp:76-81 walks 'dest' for a src_len that is not a multiple +// loop at src/Utils.cpp:76-79 walks 'dest' for a src_len that is not a multiple // of the block size, which is exactly the shape Mesh::onRecvPacket hands it. // // dest is 184 bytes — sizeof(uint8_t data[MAX_PACKET_PAYLOAD]) in diff --git a/docs/research/meshcore-parser-bounds/path_arith.cpp b/docs/research/meshcore-parser-bounds/path_arith.cpp index 77951f03..c4821096 100644 --- a/docs/research/meshcore-parser-bounds/path_arith.cpp +++ b/docs/research/meshcore-parser-bounds/path_arith.cpp @@ -31,7 +31,7 @@ int main() // The decrypted plaintext length. MACThenDecrypt returns 0 for anything at // or under CIPHER_MAC_SIZE, and cannot exceed the payload buffer. // Utils::decrypt returns whole 16-byte blocks ("will always be multiple of - // 16", src/Utils.cpp:82), so len is a multiple of 16. 176 is the largest + // 16", src/Utils.cpp:81), so len is a multiple of 16. 176 is the largest // that still fits data[184]; 192 is the P4 case and is excluded here so the // two findings do not get mixed up. const int len_min = 16, len_max = 176, len_step = 16; From 0ac16155c2ec1d010acd18807e6522a2433e4b36 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Sun, 23 Aug 2026 16:18:30 +0000 Subject: [PATCH 4/5] dev leads the pin by 31 commits, not the 29 the older note recorded The report reused M2's count from 2026-08-21 to say the commits dev is ahead by are elsewhere in the tree. The conclusion holds -- every file the pull requests touch is byte-identical -- but the number does not: measured today, dev at 9d7cee66 leads d929643 by 31, and 29 is #3267's base rather than dev's tip. Measured with git rev-list --count against a full clone. Worth saying how, because the first attempt returned 1: the clone had been made blobless and then deepened with --depth=1 fetches for the specific SHAs, which makes it shallow, and a shallow clone will answer a rev-list count with a confident wrong number rather than an error. Co-Authored-By: Claude Opus 5 --- docs/research/MESHCORE_PARSER_BOUNDS.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/research/MESHCORE_PARSER_BOUNDS.md b/docs/research/MESHCORE_PARSER_BOUNDS.md index 834e1d78..66111483 100644 --- a/docs/research/MESHCORE_PARSER_BOUNDS.md +++ b/docs/research/MESHCORE_PARSER_BOUNDS.md @@ -58,7 +58,10 @@ src/Utils.cpp vs 9d7cee66 : identical ``` Every file any of these pull requests touches is byte-identical between our pin -and both of their bases. The 29 commits `dev` is ahead by are elsewhere. So each +and both of their bases. `dev` leads the pin by **31** commits and `#3267`'s base +by 29 — measured on 2026-08-23, `git rev-list --count`, not carried over from +[M2](OPEN_QUESTIONS.md#meshcore)'s count of 29, which was true of `dev` on +2026-08-21 and has since moved. All of them are elsewhere in the tree. So each diff applies to `d929643` unchanged, and a measurement taken on a pull request's head tree is a measurement of our pin plus that pull request's guards. From fe906e5b9b74e75cd4175ede893f9a0a98cb4c17 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Tue, 25 Aug 2026 00:05:03 +0000 Subject: [PATCH 5/5] Close the review's three code findings, and record a second ecosystem's bounds fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two separate things, both from #142 and neither of them production code. ## The independent review of #160, findings 2, 3 and 4 **`build-extras.sh` measured the wrong revision, silently.** `tree` was hardcoded to `tree-base`, so `./build.sh cand ` followed by `./build-extras.sh` built `decrypt_bounds` from whatever `base` last held and printed only `built build/decrypt_bounds`. That is precisely the failure the T-013 entry condition exists to prevent, reintroduced inside the tool written to prevent it: a candidate where upstream fixed `Utils::decrypt` would be recorded as still broken. It now takes the tag, and `build.sh` writes the resolved SHA to `build/tree-/.revision` so both binaries can say what they speak for. **`path_arith` printed the same answer for every revision, including one where upstream had fixed the line.** It hand-copied `MAX_PACKET_PAYLOAD`, `MAX_PATH_SIZE` and `isValidPathLen`, and `build-extras.sh` compiled nothing from the tree — yet `TASKS.md`, the harness README and §4 all enrolled it as a revision check, which it could not be. The constants and the validator now come from the built tree (`Packet.cpp` is linked and `mesh::Packet::isValidPathLen` called). The eight lines of index arithmetic genuinely cannot be executed on a host — they sit behind `MACThenDecrypt`, so behind AES, SHA-256 and ed25519 — so instead of pretending, the `PAYLOAD_TYPE_PATH` branch is extracted between two anchors, whitespace-normalised, hashed and compared. A revision that has touched those lines is refused with exit 65 and told to re-read P3 by hand. `#3269`'s head is such a revision, so the check has a one-command demonstration rather than an argument: `./build-extras.sh pr3269` refuses, naming both digests. **The P4 publication question is the owner's, and it is narrower than it read.** The document said reporting upstream was their call, which is right, but left it sounding as if the disclosure decision were open. It is not: this repository is public, and opening #160 published the affected range, the faulting line and a one-command reproduction. What remains is an *ordering* decision — notify first, or accept publication-without-notice deliberately — and §5 now says so. Minors from the same review: the P3 results fence is the tool's real output instead of a paraphrase wearing its clothes; the `mmap`/`mprotect` in `decrypt_bounds.cpp` are checked, because an unchecked one faults in a way that looks like the finding and is not it; the References line said M15–M17 where four were filed. Re-run from the committed copy after the changes, all four trees: §4 reproduces exactly. `#3269`'s ten rows are byte-identical to `base`, which is the expected result and now a measured one — the report said its diff was a log line, and that was a reading of a diff rather than a run. Both say it now. Also re-checked upstream, 2026-08-24: all five pull request states, heads and bases unchanged; `main` has moved two commits ahead of the pin and both are `docs/faq.md`, so the pin is no longer the literal tip and is still upstream's newest code and newest release. The two halves are recorded separately because only one of them ages. ## The owner's upstream delta, verified rather than repeated Meshtastic `firmware#11573`, merged `ac330e6a` on 2026-08-23, replaces an `assert()` on a wire-supplied payload length with an executable rejection that releases the packet and unwinds the transmit state, and adds a unit test that asserts the rejection — return value, no half-started transmit, and the pool slot reusable. Every claim in the owner's summary was re-read from the merged diff and held; the metadata was taken from the API, not from the summary. It is **GPL-3.0** and it is **not in any release** — `master` does not contain the merge commit — so nothing is copied and nothing is called fixed downstream. What it changes for us is the weight of P4: an unchecked wire length reaching a fixed destination is now a two-instance pattern across unrelated firmwares rather than one project's defect. So the invariant and the test shape are written in our own words as entry conditions on T-013 and T-050, and the ledger carries the row with `ADAPT the invariant, IGNORE the code`. Deliberately not claimed: whether that `assert()` was compiled out in shipping builds. `NDEBUG` is in three of that repository's files and in none of its build flags, and the toolchain's own definition was not traced. The narrower claim stands without it — an `assert` is a statement about a program's own consistency, and a length that arrived from outside is not that. **NOT EXECUTED — HARDWARE REQUIRED** for everything physical, on both halves. Tests: `cmake && ctest` 33/33. `check_docs.py` clean. `shellcheck` clean on all three scripts. No Attadipa code changed. Co-Authored-By: Claude Opus 5 --- STATUS.md | 18 ++ TASKS.md | 44 ++- docs/research/MESHCORE_PARSER_BOUNDS.md | 262 +++++++++++++++--- docs/research/RECONCILIATION_2026-08-21.md | 2 +- docs/research/REUSE_LEDGER.md | 23 +- docs/research/VERIFIED_FACTS.md | 62 ++++- docs/research/WAVESHARE_ARRIVAL.md | 4 +- .../research/meshcore-parser-bounds/README.md | 30 +- .../meshcore-parser-bounds/build-extras.sh | 97 ++++++- docs/research/meshcore-parser-bounds/build.sh | 9 +- .../meshcore-parser-bounds/decrypt_bounds.cpp | 6 +- .../meshcore-parser-bounds/path_arith.cpp | 84 ++++-- 12 files changed, 545 insertions(+), 96 deletions(-) diff --git a/STATUS.md b/STATUS.md index d08e2e49..976b1f14 100644 --- a/STATUS.md +++ b/STATUS.md @@ -90,6 +90,24 @@ in both cases. Attadipa code changed and none should**: we link no MeshCore. It moves the criteria for a future local provider's pin, nothing else. `attadipa_link`'s own decoder was checked and is not analogous. Left open as M20–M23. + + **A second ecosystem reached the same invariant from the other end, and it is + not ours to copy.** The owner brought Meshtastic + [firmware#11573](https://github.com/meshtastic/firmware/pull/11573) on + 2026-08-24; it is **merged** (`ac330e6a`, 2026-08-23) and every claim about it + checks out against the merged diff. It replaces an `assert()` on an on-air + payload length with an executable rejection that also releases the packet and + unwinds the transmit state, and it lands a unit test that asserts the rejection + rather than the crash. Two things it changes for us and two it does not: the + bounds-and-cleanup invariant is now **independently confirmed** rather than + inferred from one project's parsers, and a test *shape* is available to copy; + but the code is **GPL-3.0** and cannot enter this repository, and it is + **not in any release** — `master` does not contain the merge commit, so the + shipping firmware still has the assertion. + [MESHCORE_PARSER_BOUNDS §8](docs/research/MESHCORE_PARSER_BOUNDS.md) is the + record, and the invariant it yields is an entry condition on T-013 and + T-050 rather than work to start now — there is no radio stack for it to + constrain yet. - **T-041 — MeshCore 1.17 upstream review — done.** [`docs/upstream/meshcore-1.17-review.md`](docs/upstream/meshcore-1.17-review.md). Ten of the thirteen owner-named pull requests are **still open**, so most of diff --git a/TASKS.md b/TASKS.md index 99e05a63..4c149c56 100644 --- a/TASKS.md +++ b/TASKS.md @@ -1686,6 +1686,15 @@ stale silently. The protocol is adapter is also where a malformed-frame consequence stops being MeshCore's and starts being ours. That does not change this task's acceptance; it says which boundary the corpus in T-013's entry condition is protecting. +- **Entry condition added 2026-08-24 (T-173):** the boundary test must include the + case where the far side's declared length exceeds what the receiving buffer + holds, and must assert the **rejection** — that the frame is refused, that no + partially-consumed state is left behind, and that whatever buffer the attempt + borrowed is returned — rather than only that nothing crashed. Two unrelated mesh + firmwares shipped that defect and one of them, Meshtastic, shipped the test + first ([MESHCORE_PARSER_BOUNDS §8](docs/research/MESHCORE_PARSER_BOUNDS.md)); + its code is GPL-3.0 and may not be copied, its *shape* is not copyrightable and + is what is being adopted. ### T-013 · The local mesh integration spike - **Priority:** P0 @@ -1720,6 +1729,23 @@ stale silently. The protocol is ([MESHCORE_PARSER_BOUNDS](docs/research/MESHCORE_PARSER_BOUNDS.md)). That is not a reason to avoid MeshCore; it is the thing a pin has to be chosen with knowledge of, and a version number does not carry it. + **Sharpened 2026-08-24:** those two are reached by `./build-extras.sh `, + and the tag is the point — it used to be hardcoded to the pin's tree, so the + candidate was never what got measured. It also refuses outright, exit 65, when + the `PAYLOAD_TYPE_PATH` lines `path_arith` hand-copies have moved at the + candidate; that refusal means "re-read P3 by hand here", not "the tool is + broken". +- **Entry condition added 2026-08-24 (T-173), and it is not about MeshCore:** the + radio path this spike produces must check a wire-supplied length **at the point + of use, in the shipping build, in code that rejects** — not in an `assert`, and + not only in the parser upstream of it — and the rejection must leave the caller + able to continue, with every borrowed buffer returned. Two unrelated mesh + firmwares shipped the same defect class + ([MESHCORE_PARSER_BOUNDS §8](docs/research/MESHCORE_PARSER_BOUNDS.md)), which + is why it is written as an invariant here rather than as a note about somebody + else's bug. The test that proves it should assert the *rejection* — return + value, no half-started transmit, and the buffer back in the pool — not merely + that nothing crashed. ### T-016 · Benchmark the node protocol encoding, then accept or replace it - **Priority:** P1 @@ -3003,7 +3029,23 @@ renumbered with it, M15–M18 → **M20–M23**, same collision one file along. the authentication gate it looks like. - **Reporting the `Utils::decrypt` defect upstream is the owner's call.** Filing on a third-party repository is outward-facing and this run had no authority for - it. The evidence reproduces in one command. + it. The evidence reproduces in one command. **What is not open is whether it is + public — it is**, from the moment #160 was opened, so the decision left is the + *ordering*: notify upstream first, or accept publication-without-notice + deliberately. `needs-owner`, and it is live now rather than later. +- **A second ecosystem reached the same invariant, added 2026-08-24.** The owner + brought Meshtastic + [firmware#11573](https://github.com/meshtastic/firmware/pull/11573) — merged + `ac330e6a`, base `develop`, **not in any release** — which replaces an + `assert()` on a wire-supplied payload length with an executable rejection that + releases the packet and unwinds the transmit state, plus a test asserting the + rejection rather than the absence of a crash. Every claim re-read from the + merged diff and confirmed. **GPL-3.0: read-only evidence, no code**, the same + bar as [OD-12](docs/research/OWNER_DECISIONS.md). The invariant and the test + shape are adopted as entry conditions on T-013 and T-050; + [MESHCORE_PARSER_BOUNDS §8](docs/research/MESHCORE_PARSER_BOUNDS.md) is the + record. It also upgrades P4 from one project's defect to a two-instance + pattern, which is why it is written as an invariant and not as a note. - **`attadipa_link`'s decoder was checked, not assumed.** It validates the declared length before reading (`frame_codec.cpp:139`, `:147`) behind a length check and a CRC. The finding does not transplant, and nothing in `link/` diff --git a/docs/research/MESHCORE_PARSER_BOUNDS.md b/docs/research/MESHCORE_PARSER_BOUNDS.md index 10ddb11c..747612a1 100644 --- a/docs/research/MESHCORE_PARSER_BOUNDS.md +++ b/docs/research/MESHCORE_PARSER_BOUNDS.md @@ -1,8 +1,11 @@ # MeshCore parser bounds at the pinned revision Research for [issue #142](https://github.com/hleserg/Attadipa/issues/142). -Read on 2026-08-23. **Research only — no Attadipa production code changed, and -none should on the strength of this document alone.** +Read on 2026-08-23; re-checked and extended 2026-08-24 (§1's freshness row, the +`#3269` column in §4, the harness's revision binding in §7, and §8, which is a +second ecosystem arriving at the same invariant). **Research only — no Attadipa +production code changed, and none should on the strength of this document +alone.** Three upstream pull requests filed on 2026-08-21 and 2026-08-22 claim missing length checks in MeshCore's frame and advertisement parsers, found by fuzzing. @@ -32,11 +35,19 @@ could not be executed at all it says so in its own row rather than in a footnote | Attadipa's pin | `d92964352441e53b93e8667b802e04f6e072b39e` | | What that is upstream | tag `companion-v1.17.1` (also `repeater-`, `room-server-`), released 2026-08-14 | | `meshcore-dev/MeshCore` `main`, 2026-08-23 | `d92964352441e53b93e8667b802e04f6e072b39e` — **the same commit** | +| `meshcore-dev/MeshCore` `main`, 2026-08-24 | `0679dbeffc504d562d2f09eb072fdc223f8ffc2a` — 2 commits ahead, **both `docs/faq.md`** | | `meshcore-dev/MeshCore` `dev`, 2026-08-23 | `9d7cee66394fffd6e8c6e9f39fe03660cb314f64`, 2026-08-22 | +| `meshcore-dev/MeshCore` `dev`, 2026-08-24 | `12998cba8969e4004d94ed94b5e8e5bbdfa05571` | | Licence | **MIT**, `license.txt` in the clone | -So the pin is not lagging: it *is* the current release and the current `main`. -Nothing has been released that contains any of these guards. +So the pin is not lagging: it *is* the current release, and it was the tip of +`main` when this was written. It stopped being the literal tip a day later and +that changes nothing here — `git diff d929643...0679dbe` is `docs/faq.md` and no +second file, so the pin is still upstream's newest **code**, and the newest +release is still `v1.17.1` of 2026-08-14. Nothing has been released that contains +any of these guards. The distinction is kept because "our pin is `main`" is the +kind of sentence that stays in a document after it stops being true; the way to +re-check it is `compare/...main` and to read the file list, not the count. The pull requests are based on `dev`, which made the issue's own framing cautious about comparing them to our pin. That caution turns out to be unnecessary, and it @@ -78,6 +89,10 @@ head tree is a measurement of our pin plus that pull request's guards. None is merged. None is in a release. Nothing below may be written up anywhere as "upstream fixed it". +**Re-checked 2026-08-24**, the whole row set: five states, five heads, five bases, +all unchanged, and the three open ones last touched 2026-08-22. So a day has not +moved any of it, which is worth one line and not a second table. + --- ## 2. The harness @@ -259,18 +274,32 @@ negative becomes a large positive, and `extra`/`extra_len` are then handed to is a multiple of 16. Over every `(len, path_len)` pair that reaches this code with `len` in 16…176: +`build/path_arith`, run against `d929643` on 2026-08-24, verbatim: + ``` -accepted (len, path_len) pairs : 1309 -pairs where k > len (extra_len underflows): 187 - of those, &data[k] + extra_len > 184 : 187 <- all of them -smallest underflowing input : len=16 path_len=0x0F - -> k=17, extra_len=255, - window data[17..271] against data[0..183] - (88 bytes past the end) +revision under test : d92964352441e53b93e8667b802e04f6e072b39e +MAX_PACKET_PAYLOAD : 184 (from the tree's MeshCore.h) +MAX_PATH_SIZE : 64 (from the tree's MeshCore.h) +len domain : 16..176 step 16 + +accepted (len, path_len) pairs : 1309 +pairs where k > len (extra_len underflows): 187 + of those, &data[k] + extra_len > 184 : 187 +smallest underflowing input : len=16 path_len=0x0F -> k=17 extra_len=255, i.e. data[17..271] against data[0..183] (88 bytes past the end) +largest over-read window : len=16 path_len=0x0F -> k=17 extra_len=255, i.e. data[17..271] against data[0..183] (88 bytes past the end) ``` Every underflow produces a window past the end of the 184-byte stack buffer. -There is no benign corner of this one. +There is no benign corner of this one. The smallest input and the largest window +are the same input, which is not a printing bug: `extra_len` is `uint8_t`, so +255 is its ceiling, and the shortest accepted plaintext reaches it. + +*(An earlier revision of this document paraphrased those lines inside the same +fence — the numbers were right, the labels were not the tool's, and one +continuation line was printed by nothing. In a passage arguing the result was +computed, that is the one place a reader cannot tell a transcript from a +retelling. Found by the independent review of +[#160](https://github.com/hleserg/Attadipa/pull/160).)* **Reachable from:** the radio, addressed to this node, from a source hash that matches a contact, **and past the MAC check**. That last gate is `CIPHER_MAC_SIZE` @@ -437,18 +466,25 @@ cases had to be made to work. Ten sequences. Each is the whole input; each is fed as a buffer of exactly its own length with a guard page behind it. `base` is `d929643`. -| # | Parser | Bytes (hex) | `len` | Reads, and where the tool stops it | base | `#3267` head | `#3270` head | -|---|---|---|---|---|---|---|---| -| A1 | `tryParsePacket` | `01` | 1 | 1 B at `Dispatcher.cpp:165` | **OOB** | clean, `false` | **OOB** | -| A2 | `tryParsePacket` | `00` | 1 | 2 B at `Dispatcher.cpp:159` | **OOB** | clean, `false` | **OOB** | -| A3 | `tryParsePacket` | *(empty)* | 0 | 1 B at `Dispatcher.cpp:152` | **OOB** | clean, `false` | **OOB** | -| B1 | `Packet::readFrom` | `01 3F` | 2 | **63 B** at `Packet.cpp:78` | **OOB** | clean, `false` | **OOB** | -| B2 | `Packet::readFrom` | `01` | 1 | 1 B at `Packet.cpp:74` | **OOB** | clean, `false` | **OOB** | -| B3 | `Packet::readFrom` | `00` | 1 | 2 B at `Packet.cpp:69` | **OOB** | clean, `false` | **OOB** | -| C1 | `AdvertDataParser` | `91` | 1 | 4 B at `AdvertDataHelpers.cpp:40` | **OOB** | **OOB** | clean, `invalid` | -| C2 | `AdvertDataParser` | `F1` | 1 | 4 B at `AdvertDataHelpers.cpp:40` | **OOB** | **OOB** | clean, `invalid` | -| C3 | `AdvertDataParser` | *(empty)* | 0 | 1 B at `AdvertDataHelpers.cpp:34` | **OOB** | **OOB** | **OOB** | -| C4 | `AdvertDataParser` | `21` | 1 | 2 B at `AdvertDataHelpers.cpp:44` | **OOB** | **OOB** | clean, `invalid` | +| # | Parser | Bytes (hex) | `len` | Reads, and where the tool stops it | base | `#3267` head | `#3269` head | `#3270` head | +|---|---|---|---|---|---|---|---|---| +| A1 | `tryParsePacket` | `01` | 1 | 1 B at `Dispatcher.cpp:165` | **OOB** | clean, `false` | **OOB** | **OOB** | +| A2 | `tryParsePacket` | `00` | 1 | 2 B at `Dispatcher.cpp:159` | **OOB** | clean, `false` | **OOB** | **OOB** | +| A3 | `tryParsePacket` | *(empty)* | 0 | 1 B at `Dispatcher.cpp:152` | **OOB** | clean, `false` | **OOB** | **OOB** | +| B1 | `Packet::readFrom` | `01 3F` | 2 | **63 B** at `Packet.cpp:78` | **OOB** | clean, `false` | **OOB** | **OOB** | +| B2 | `Packet::readFrom` | `01` | 1 | 1 B at `Packet.cpp:74` | **OOB** | clean, `false` | **OOB** | **OOB** | +| B3 | `Packet::readFrom` | `00` | 1 | 2 B at `Packet.cpp:69` | **OOB** | clean, `false` | **OOB** | **OOB** | +| C1 | `AdvertDataParser` | `91` | 1 | 4 B at `AdvertDataHelpers.cpp:40` | **OOB** | **OOB** | **OOB** | clean, `invalid` | +| C2 | `AdvertDataParser` | `F1` | 1 | 4 B at `AdvertDataHelpers.cpp:40` | **OOB** | **OOB** | **OOB** | clean, `invalid` | +| C3 | `AdvertDataParser` | *(empty)* | 0 | 1 B at `AdvertDataHelpers.cpp:34` | **OOB** | **OOB** | **OOB** | **OOB** | +| C4 | `AdvertDataParser` | `21` | 1 | 2 B at `AdvertDataHelpers.cpp:44` | **OOB** | **OOB** | **OOB** | clean, `invalid` | + +**The `#3269` column is ten rows of "no change", and it was measured rather than +argued.** Added 2026-08-24: the column is byte-for-byte the `base` column, +including the two lines where the sanitizer stops. That is the expected result — +its diff touches only `src/Mesh.cpp`, which the corpus does not reach — but §3 P3 +says its head does not close its own finding, and "its diff is a log line" is a +reading of a diff while this is a run. Both now say it. The read widths are the `memcpy` and subscript widths in the source; a multi-field read is attributed to the statement that first crosses the boundary, @@ -463,13 +499,22 @@ Plus two experiments that are not single byte sequences: | # | What | Result | |---|---|---| -| P3 | every `(len, path_len)` reaching `Mesh.cpp:161-172`, `len` ∈ 16…176 | 187 of 1309 underflow `extra_len`; **all 187** give a window past `data[184]`; smallest `len=16, path_len=0x0F` | +| P3 | every `(len, path_len)` reaching `Mesh.cpp:160-172`, `len` ∈ 16…176 | 187 of 1309 underflow `extra_len`; **all 187** give a window past `data[184]`; smallest `len=16, path_len=0x0F` | | P4 | `Utils::decrypt` into a 184-byte destination | clean at `src_len=176`; faults at `src_len` 177, 180, 182 at `Utils.cpp:77` | +**Neither of those two is in `run.sh`'s matrix, and neither could be measured +against an arbitrary revision until 2026-08-24.** `build-extras.sh` took no +argument and always built from `tree-base`, so running it after pointing +`build.sh` at a candidate measured the pin under the candidate's name. It now +takes the tag, prints the resolved SHA, and refuses outright when the lines +`path_arith` hand-copies have changed — see §7. A green `run.sh` is still not a +clean revision; that part was always true and is now enforced rather than +footnoted. + **Nine of ten cases over-read on the pinned revision. No head fixes all of them:** -`#3267` closes A and B and leaves C untouched; `#3270` closes C1, C2, C4 and -leaves A, B and **C3** untouched. Vendoring any one of them would buy part of the -problem. +`#3267` closes A and B and leaves C untouched; `#3269` changes nothing in this +matrix at all; `#3270` closes C1, C2, C4 and leaves A, B and **C3** untouched. +Vendoring any one of them would buy part of the problem. --- @@ -548,6 +593,18 @@ and that is a separate, executable task, not this one. reproduces in one command. Recorded as a recommendation, deliberately not acted on. + **What is not still open is whether P4 is public: it is.** This repository is + public, §3 P4 names the affected versions, the triggering range and the + faulting line, §7 gives a one-command reproduction, and `decrypt_bounds.cpp` + *is* that reproduction. Publication happened when the pull request carrying + this document was opened, and merging it does not add to that — it makes it + permanent in `main`. So the decision left to the owner is an **ordering** one + and it is live now rather than at some later point: tell upstream first, or + accept publication-without-notice as a deliberate choice. Either is defensible. + Arriving at the second by not deciding is the one outcome nobody chose. Raised + by the independent review of [#160](https://github.com/hleserg/Attadipa/pull/160) + and carried here so that it does not live only in a review comment. + No ADR changes. The trust boundary these findings press on is [ADR-0008](../adr/0008-mesh-service-providers.md)'s and it already holds; nothing here moved it, and an ADR edited to say what it already said is churn. @@ -580,26 +637,163 @@ git clone --filter=blob:none https://github.com/meshcore-dev/MeshCore /tmp/meshc cd docs/research/meshcore-parser-bounds ./build.sh base d92964352441e53b93e8667b802e04f6e072b39e ./build.sh pr3267 05da523ebd32980a1c28b11f2928d351796b9737 +./build.sh pr3269 5ebf8ef9cf1a0df28118c47460277857e0e675b2 ./build.sh pr3270 f80d805ee8b20f77ff5b3ca6bc3a9021989aafd2 ./run.sh # the ten-case matrix in §4 -./build-extras.sh +./build-extras.sh base # the tag names the tree to measure ./build/path_arith # P3 ./build/decrypt_bounds 180 # P4; 176 is clean ``` -The two pull request heads have to be fetched by SHA before they resolve — -`git -C /tmp/meshcore-src fetch origin `; `build.sh` says so if they have not -been. **`run.sh` does not cover P3 or P4**, so a green matrix is not a clean -revision. +The pull request heads have to be fetched before they resolve — +`git -C /tmp/meshcore-src fetch origin `, or +`fetch origin pull//head:pr` where the SHA alone is refused; `build.sh` +says so if they have not been. + +**`run.sh` does not cover P3 or P4**, so a green matrix is not a clean revision. +That is the one way this artifact could mislead a future pin, so both halves of +it are now mechanical rather than remembered: + +- **`build-extras.sh` takes the tag**, defaulting to `base`, and prints the SHA + it resolved from the tree's own `.revision` file. It used to hardcode + `tree-base`, which meant `./build.sh cand && ./build-extras.sh` measured + the *pin* and printed nothing to say so — the failure the entry condition + exists to prevent, reintroduced inside the tool written to prevent it. +- **It refuses a revision whose `PAYLOAD_TYPE_PATH` branch has moved.** + `path_arith` takes `MAX_PACKET_PAYLOAD`, `MAX_PATH_SIZE` and `isValidPathLen` + from the built tree, but the index arithmetic itself cannot be executed on a + host and is copied by hand — and a hand-copy cannot notice upstream *fixing* + what it copied. So those lines are extracted between two anchors, + whitespace-normalised, hashed, and compared. `#3269`'s head is exactly such a + revision and is refused by name: + + ``` + $ ./build-extras.sh pr3269 + The PAYLOAD_TYPE_PATH branch in src/Mesh.cpp has changed at 5ebf8ef9… + + expected 5eee273c0079c2e6332f42b0d3d285f7b8eaf55ecdf81602941b4eb48a09fbc2 + found e97ef1642ef638b8e1f7fff843a806ba82ea9201199b6466bbb527ef76d7251b + … + $ echo $? + 65 + ``` Needs `clang++` with AddressSanitizer. Measured on clang 18.1.3, Ubuntu 24.04, -2026-08-23. It reads the upstream clone and writes only inside its own directory. +2026-08-23; the whole sequence above re-run 2026-08-24 on clang 18.1.3 after the +`build-extras.sh` change, reproducing §4 unchanged. It reads the upstream clone +and writes only inside its own directory. + +--- + +## 8. The same invariant, reached independently, in a stack we may not copy from + +Added 2026-08-24. The owner brought +[meshtastic/firmware#11573](https://github.com/meshtastic/firmware/pull/11573) to +[issue #142](https://github.com/hleserg/Attadipa/issues/142) as an upstream delta +and asked for the invariant rather than the code. Everything below was re-read +from the merged diff rather than taken from the summary; the summary held. + +| | | +|---|---| +| Pull request | `meshtastic/firmware#11573`, *"fix(radio): MeshBeacon heap leak and runtime packet payload size check"* | +| State | **MERGED** 2026-08-23, merge commit `ac330e6a6b9fca267fe3faab27ee50c4e91bee28` | +| Base | `develop` (`05f64741`), which is that repository's default branch | +| Head | `6094d148` | +| Size | 4 files, +58 −5 | +| Licence | **GPL-3.0** — `meshtastic/firmware`'s own `LICENSE` | +| In a release? | **No.** `master` does not contain the merge commit — `compare/master...ac330e6a` answers `diverged`, ahead 828 / behind 103 — and the newest release, `v2.7.26.54e0d8d`, was published 2026-06-24 | + +**What it actually changes**, `src/mesh/RadioInterface.cpp::beginSending()`: + +```diff +- assert(p->encrypted.size <= sizeof(radioBuffer.payload)); ++ // Runtime packet payload size bounds check against radioBuffer to prevent overflow in memcpy() ++ if (static_cast(p->encrypted.size) > sizeof(radioBuffer.payload)) { ++ LOG_ERROR("Packet payload size %u exceeds radioBuffer capacity %u", …); ++ packetPool.release(p); ++ return 0; ++ } + memcpy(radioBuffer.payload, p->encrypted.bytes, p->encrypted.size); +``` + +and three consequences of that rejection which are the interesting half, because +a bounds check that leaves the caller wedged has moved the failure rather than +handled it: + +- **The caller unwinds.** `RadioLibInterface::startSend` now treats `numbytes == 0` + as an abort: `completeSending()`, `powerMon->clearState(…Lora_TXOn)`, + `startReceive()`, `return false`. +- **The radio goes home even when there was no packet.** + `MeshBeaconModule::reconfigureForBeaconTX(this, nullptr)` moved out of the + `if (p)` arm of `completeSending()` to after it. +- **The packet is not leaked.** Both `router->send(p)` sites in + `MeshBeaconModule.cpp` now release on `ERRNO_SHOULD_RELEASE`, and + `beginSending` releases the packet it rejects. + +**And a regression test that asserts the rejection rather than the crash** — +`test/test_radio/test_main.cpp::test_beginSending_oversizedPayloadAbortsSafely()`. +It sets `encrypted.size` to capacity + 10, then checks three separate things: +the return is 0, `sendingPacket` is still null, and **the pool slot is reusable** +— by allocating again and asserting the same pointer comes back. That third +assertion is the one worth copying: "it rejected" and "it rejected without losing +the buffer" are different claims, and only the second one keeps a long-running +node alive. + +### What this is evidence for, and what it is not + +**It is independent confirmation of the class.** Two unrelated mesh firmwares, on +different radios, in different code, both had a wire-supplied length reaching a +fixed-size destination with nothing executable in between — MeshCore at P4 via +`Utils::decrypt`'s block rounding, Meshtastic at `beginSending`'s `memcpy`. The +finding in §3 P4 was a single-project observation until 2026-08-23. It is now a +pattern with two instances, which changes how much weight a future +`LocalMeshProvider` design should give it. + +**It is not a fact about Attadipa's code**, which has no production radio stack +at all, and not a defect report against `attadipa_link`, whose decoder validates +the declared length before reading (§5, and `frame_codec.cpp:139`, `:147`). + +**Two things are deliberately not claimed.** Whether Meshtastic's `assert()` was +compiled out in shipping firmware is **UNKNOWN** from this reading: `NDEBUG` +appears in three files of that repository and in none of its build flags, and +whether the Arduino/ESP-IDF toolchain defines it for those environments was not +traced. And **NOT EXECUTED — HARDWARE REQUIRED** for everything physical: the +pull request's author lists Heltec LoRa32 V3, LilyGo T-Deck, Seeed T-1000E and +Wio-E5, none of which was checked here and none of which is a board this project +has. + +The claim that survives without either is narrower and enough: an `assert` is a +statement about a program's own consistency, and a length that arrived from +outside is not that. Whatever the build flags do, the pre-fix code turns a +hostile length into either a silent overflow or an abort, and neither is a +rejection. + +### Decision — `ADAPT` the invariant, `IGNORE` the code + +**The code cannot come here.** `meshtastic/firmware` is GPL-3.0 with no linking +exception, which is the same bar that closed the Meshtastic protocol path in +[OD-12](OWNER_DECISIONS.md#od-12--meshtastic-is-not-supported-and-the-reason-is-not-the-licence). +Read-only evidence, no derivation, no transcription of a hunk. + +What is adopted is an invariant and a test *shape*, both of which are ours to +state in our own words: + +> A length that arrived from outside the device is checked at the point of use, +> in the shipping build, by code that rejects — and the rejection leaves the +> caller in a state it can continue from, with every buffer it borrowed returned. + +That belongs to whichever task first gives Attadipa a radio or a local provider, +as an entry condition rather than as new work now — recorded on **T-013** and +**T-050** in [`TASKS.md`](../../TASKS.md) alongside the corpus condition. No +Attadipa code changed, and none should on the strength of this section: there is +nothing yet for it to change. --- ## References - Upstream: `meshcore-dev/MeshCore`, MIT, pinned `d92964352441e53b93e8667b802e04f6e072b39e` +- Upstream, read-only: `meshtastic/firmware`, **GPL-3.0**, `ac330e6a` — §8, evidence only, never a source of code - [`REUSE_LEDGER.md`](REUSE_LEDGER.md) — the pin and the monitored deltas - [`OPEN_QUESTIONS.md`](OPEN_QUESTIONS.md) — M10–M14 from the first reading; **M20–M23** from this one, M23 being the one that scopes the follow-up. Filed as M15–M18 and renumbered on merge, because the frame-capacity research took those on `main` in the same week - [`VERIFIED_FACTS.md`](VERIFIED_FACTS.md) — what is now traced to executed evidence diff --git a/docs/research/RECONCILIATION_2026-08-21.md b/docs/research/RECONCILIATION_2026-08-21.md index 9c87c739..c014baf1 100644 --- a/docs/research/RECONCILIATION_2026-08-21.md +++ b/docs/research/RECONCILIATION_2026-08-21.md @@ -20,7 +20,7 @@ was narrower. |---|---|---|---| | **A** | Split hardware inventory from product capabilities | **yes** | Worse than described. There was not a *mixed* model — there was only *one* layer, the hardware one, and applications were pointed straight at it. `ARCHITECTURE.md` §3 lists `Display · Touch · … · Lora · Gnss · …` as the app-facing set. No product capability such as `Position` or `MeshMessaging` existed anywhere in the repository. | | **B** | Remove ambiguous `has()` | **yes** | Then in `ARCHITECTURE.md`, `ADR-0001`, `README.md`, `TASKS.md`, `ADR-0004`, `HARDWARE_MATRIX.md` and `VERIFIED_FACTS.md`, documented as *"cheap, for gating UI"* — the exact use final §7 shows to be ambiguous. The line numbers this row carried were facts about the files on 2026-08-21 and are not repeated: `ARCHITECTURE.md:139` had already drifted onto a `HardwareFeature` enum member, and the section that records the outcome is [ARCHITECTURE.md:215](../architecture/ARCHITECTURE.md) "why `has()` is gone". | -| **C** | Radio vs LoRa | **yes** | `ADR-0001:51` — *"one of five **LoRa** chips — SX1262, SX1280, CC1101, LR1121, SI4432"*. Repeated in `ARCHITECTURE.md:113`, `CLAUDE.md:26`, `HARDWARE_MATRIX.md`, `VERIFIED_FACTS.md:264`. Two of the five have no LoRa modulator. | +| **C** | Radio vs LoRa | **yes** | `ADR-0001:51` — *"one of five **LoRa** chips — SX1262, SX1280, CC1101, LR1121, SI4432"*. Repeated in `ARCHITECTURE.md:113`, `CLAUDE.md:26`, `HARDWARE_MATRIX.md`, `VERIFIED_FACTS.md:312`. Two of the five have no LoRa modulator. | | **D** | Local T-Watch mesh vs node-only | **yes** | The contradiction was sitting in the ADR *index*: `docs/adr/README.md` said *"the watch does not run mesh, because the radio is in the node"*, while `ARCHITECTURE.md` mapped a local radio to a mesh service. `ADR-0005` built a whole protocol on the former. | | **E** | Heading reference frames | **yes**, narrower than described | Not a wrong model — **no** model. Heading appears as prose in seven documents and as a structure in none. No `reference_frame`, no `source`, no `confidence`. The specific error final §10 warns about (node magnetometer read as watch heading) had not been made yet, because there was nothing to make it with. | | **F** | EN/RU localization | **yes**, total | Zero occurrences in the repository. Not deferred, not backlogged — absent. The only mentions of Russian were "the specification documents are in Russian". | diff --git a/docs/research/REUSE_LEDGER.md b/docs/research/REUSE_LEDGER.md index 8c20a44a..ea28f704 100644 --- a/docs/research/REUSE_LEDGER.md +++ b/docs/research/REUSE_LEDGER.md @@ -59,8 +59,8 @@ want to inherit the experience, not only the code. | Project | Repository | Commit at examination | Last commit | Why it is here | |---|---|---|---|---| -| `MeshCore` | github.com/meshcore-dev/MeshCore | `d92964352441e53b93e8667b802e04f6e072b39e` | 2026-08-14 | the mesh stack Attadipa builds on; T-006. **Re-checked 2026-08-23**: still `main`'s tip and still the newest release (`companion-v1.17.1`), so the pin is current rather than lagging. `dev` is at `9d7cee66` (2026-08-22) and contains none of the parser guards below — [MESHCORE_PARSER_BOUNDS](MESHCORE_PARSER_BOUNDS.md) | -| `meshtastic` | github.com/meshtastic/firmware | `68bfe015e6ab9ec2ab8f1657066898b7880eaf63` | 2026-08-20 | ~200 board variants, worldwide regulatory regions, nanopb phone API | +| `MeshCore` | github.com/meshcore-dev/MeshCore | `d92964352441e53b93e8667b802e04f6e072b39e` | 2026-08-14 | the mesh stack Attadipa builds on; T-006. **Re-checked 2026-08-23**: still `main`'s tip and still the newest release (`companion-v1.17.1`), so the pin is current rather than lagging. `dev` is at `9d7cee66` (2026-08-22) and contains none of the parser guards below — [MESHCORE_PARSER_BOUNDS](MESHCORE_PARSER_BOUNDS.md). **Re-checked again 2026-08-24**: `main` is now `0679dbe`, two commits ahead, **both `docs/faq.md`** — so the pin is no longer the literal tip and is still upstream's newest *code* and newest release. Stated that way on purpose: "our pin is `main`" ages badly, "no code has moved" does not | +| `meshtastic` | github.com/meshtastic/firmware | `68bfe015e6ab9ec2ab8f1657066898b7880eaf63` | 2026-08-20 | ~200 board variants, worldwide regulatory regions, nanopb phone API. **GPL-3.0 — read only, always.** This is the local clone's revision and it *predates* `ac330e6a` (2026-08-23), so the bounds fix in the monitored-deltas table below is not in it; that one was read from the merged diff over the API | | `InfiniTime` | github.com/InfiniTimeOrg/InfiniTime | `825056574f47a8187b410b860f326050566553e2` | 2026-08-19 | mature LVGL watch firmware with a real app lifecycle, on far less RAM | | `RadioLib` | github.com/jgromes/RadioLib | `510e00cfb05bbc3c2b7b524262785454944adb6e` | 2026-08-13 | radio abstraction across many chips; candidate for ADR-0003 | | `lvgl` | github.com/lvgl/lvgl | `85aa60d1` (**v9.5.0**) | 2026-08-23 | the UI toolkit. **T2 is settled**: [`DEPENDENCIES.md`](DEPENDENCIES.md) pins v9.5.0 = `85aa60d1…`, verified by `git ls-remote` and observed in CI, and source **S14** in [`VERIFIED_FACTS`](VERIFIED_FACTS.md) reads that revision. This row carried `7cc13aaf…` with *"version choice is open question T2"* until 2026-08-24, so the ledger and the dependency record named two different revisions of the same dependency — the ledger being the file `CLAUDE.md` sends an agent to before implementing anything. Found in review | @@ -86,14 +86,15 @@ than a search. open: an unmerged pull request has no release behind it, and two of these three do not close the finding they were written for. -| Upstream | Head | State 2026-08-23 | What it would change | Our decision | +| Upstream | Head | State 2026-08-24 | What it would change | Our decision | |---|---|---|---|---| | [MeshCore #3267](https://github.com/meshcore-dev/MeshCore/pull/3267) | `05da523e` | open, unmerged, base `dev` | length checks in `src/Dispatcher.cpp::tryParsePacket` and `src/Packet.cpp::readFrom` | **MONITOR.** Verified to close all six of our A/B corpus cases on the pin. Still not taken — unreleased, and we compile neither file | | [MeshCore #3269](https://github.com/meshcore-dev/MeshCore/pull/3269) | `5ebf8ef9` | open, unmerged, base `dev` | a `MESH_DEBUG_PRINTLN` on the `PAYLOAD_TYPE_PATH` length mismatch | **MONITOR as evidence, not as a fix.** The diff logs the condition and then executes the read anyway — no `break`, no `return`. Verified, not inferred | | [MeshCore #3270](https://github.com/meshcore-dev/MeshCore/pull/3270) | `f80d805e` | open, unmerged, base `dev` | three guards in `AdvertDataParser` | **MONITOR.** Closes three of our four C cases and **leaves `app_data[0]` unguarded** at `AdvertDataHelpers.cpp:34` when `app_data_len == 0` — measured on its own head | | [MeshCore #3266](https://github.com/meshcore-dev/MeshCore/pull/3266) | `d87dd32f` | **closed, unmerged** | the #3267 hunks plus 28 unrelated files | superseded by #3267, whose parser hunks are byte-identical | | [MeshCore #3271](https://github.com/meshcore-dev/MeshCore/pull/3271) | `f80d805e` | **closed, unmerged** | — | the *same commit* as #3270, not merely equivalent | -| `meshcore-dev/MeshCore` `dev` | `9d7cee66` | 2026-08-22 | — | checked for equivalent guards arriving by another route: **none.** `readFrom` on `dev` is byte-identical to the pin | +| `meshcore-dev/MeshCore` `dev` | `9d7cee66` | 2026-08-22; `12998cba` on 2026-08-24 | — | checked for equivalent guards arriving by another route: **none.** `readFrom` on `dev` is byte-identical to the pin | +| [Meshtastic firmware#11573](https://github.com/meshtastic/firmware/pull/11573) | `6094d148`, merged as `ac330e6a` | **MERGED 2026-08-23** into `develop`; **not in any release** — `master` does not contain it, and `v2.7.26.54e0d8d` predates it | an `assert()` on a wire-supplied payload length becomes an executable rejection that releases the packet and unwinds the TX state, plus a unit test asserting the rejection | **ADAPT the invariant, IGNORE the code.** GPL-3.0, and a radio stack we do not have. Read-only evidence. What is taken is a sentence and a test *shape*, both restated in our own words on T-013 and T-050 — [MESHCORE_PARSER_BOUNDS §8](MESHCORE_PARSER_BOUNDS.md) | **Reusable as test material, not as code.** The guards in `05da523e` (`src/Dispatcher.cpp`, `src/Packet.cpp`) and `f80d805e` @@ -111,6 +112,20 @@ so is not in this table. It is P4 in [MESHCORE_PARSER_BOUNDS](MESHCORE_PARSER_BOUNDS.md), it is a write rather than a read, and reporting it upstream is the owner's decision. +**The Meshtastic row is a different kind of row and the difference matters.** +Every MeshCore row above is a candidate: MIT, so if one of them merged and +released, taking it would be a real option. The Meshtastic row can never become +one — GPL-3.0 with no linking exception is the same bar that closed the +Meshtastic protocol path in +[OD-12](OWNER_DECISIONS.md#od-12--meshtastic-is-not-supported-and-the-reason-is-not-the-licence), +and a merge upstream does not move it. It is in this table because a monitored +delta is not only "code we might take": it is also **evidence that changes a +decision**, and this one turned a single-project observation into a two-instance +pattern. `MONITOR` here means watching whether it reaches a release, so that the +claim "the shipping firmware still has the assertion" does not quietly go stale; +it does not mean waiting for a chance to copy it. No hunk from it is quoted +anywhere in this repository, and none may be. + ### Licences, checked before anything was depended on Attadipa is MIT. `CLAUDE.md` says anything incompatible with MIT does not enter diff --git a/docs/research/VERIFIED_FACTS.md b/docs/research/VERIFIED_FACTS.md index bb31f3df..4ceac31d 100644 --- a/docs/research/VERIFIED_FACTS.md +++ b/docs/research/VERIFIED_FACTS.md @@ -34,10 +34,10 @@ An entry that cannot name its source does not belong here. It belongs in ### The pinned MeshCore revision is upstream's current release, not a lagging one - **Claim:** `d92964352441e53b93e8667b802e04f6e072b39e` is simultaneously - Attadipa's pin, the tip of `meshcore-dev/MeshCore`'s `main`, and the newest - release (`companion-v1.17.1`, `repeater-v1.17.1`, `room-server-v1.17.1`, - published 2026-08-14). `dev` is at `9d7cee66394fffd6e8c6e9f39fe03660cb314f64`, - 2026-08-22. + Attadipa's pin, the tip of `meshcore-dev/MeshCore`'s `main` **as of 2026-08-23**, + and the newest release (`companion-v1.17.1`, `repeater-v1.17.1`, + `room-server-v1.17.1`, published 2026-08-14). `dev` was at + `9d7cee66394fffd6e8c6e9f39fe03660cb314f64`, 2026-08-22. - **Source:** GitHub API `repos/meshcore-dev/MeshCore/branches/{main,dev}`, `/releases` and `/commits?per_page=1`. - **Checked:** 2026-08-23, independently the same day by two pieces of research @@ -45,6 +45,12 @@ An entry that cannot name its source does not belong here. It belongs in ([#142](https://github.com/hleserg/Attadipa/issues/142)) and the BLE frame-capacity work ([#143](https://github.com/hleserg/Attadipa/issues/143)) — which agreed. +- **Amended 2026-08-24:** `main` has moved to + `0679dbeffc504d562d2f09eb072fdc223f8ffc2a`, two commits ahead, and + `compare/...main` lists exactly one file: `docs/faq.md`. So the pin is **no + longer the tip** and is still upstream's newest **code** and newest release. + `dev` is `12998cba8969e4004d94ed94b5e8e5bbdfa05571`. The two halves are recorded + separately because only one of them ages. - **Note:** `pushed_at` is more recent than either, because it counts pushes to any branch. It is not a claim about `main`. - **Consequence for the BLE frame-sizing finding:** there is no superseding @@ -112,6 +118,47 @@ An entry that cannot name its source does not belong here. It belongs in builds this project has not made. See [OPEN_QUESTIONS.md](OPEN_QUESTIONS.md) M22. +### Meshtastic shipped the same defect class and fixed it on 2026-08-23 + +- **Claim:** `meshtastic/firmware` PR **#11573** is **merged** — merge commit + `ac330e6a6b9fca267fe3faab27ee50c4e91bee28`, head `6094d148`, base `develop` + (`05f64741`), 4 files, +58 −5. It replaces + `assert(p->encrypted.size <= sizeof(radioBuffer.payload))` in + `src/mesh/RadioInterface.cpp::beginSending()` with a runtime check that logs, + calls `packetPool.release(p)` and returns 0 *before* the `memcpy`; makes + `RadioLibInterface::startSend` unwind on that zero (`completeSending()`, + `powerMon->clearState(…Lora_TXOn)`, `startReceive()`); moves + `reconfigureForBeaconTX(this, nullptr)` out of `completeSending()`'s `if (p)` + arm so the radio is restored even with no packet; and releases the beacon + packet on `ERRNO_SHOULD_RELEASE` at both `router->send(p)` sites in + `src/modules/MeshBeaconModule.cpp`. `test/test_radio/test_main.cpp` gains + `test_beginSending_oversizedPayloadAbortsSafely()`, which asserts the return is + 0, `sendingPacket` is null, **and the pool slot is reusable**. +- **Source:** the merged diff, `GET /repos/meshtastic/firmware/pulls/11573` with + `Accept: application/vnd.github.v3.diff`, plus `/pulls/11573` and + `/pulls/11573/files` for the metadata. +- **Checked:** 2026-08-24, independently of the owner's summary of the same + change; every claim in that summary held. +- **Not in a release.** `compare/master...ac330e6a` answers `diverged`, ahead 828 + / behind 103, so `master` does not contain it, and the newest release + `v2.7.26.54e0d8d` was published 2026-06-24. The shipping firmware still has the + assertion. +- **Licence: GPL-3.0** (`repos/meshtastic/firmware` → `license.spdx_id`). + **Read-only evidence. No code from it may enter this repository**, which is the + same bar as [OD-12](OWNER_DECISIONS.md). +- **Not established, and deliberately not claimed:** whether that `assert()` was + compiled out in shipping builds. `NDEBUG` appears in three files of that + repository and in none of its build flags; whether the Arduino/ESP-IDF + toolchain defines it for those environments was not traced. +- **Hardware:** **NOT EXECUTED — HARDWARE REQUIRED.** The pull request's author + lists Heltec LoRa32 V3, LilyGo T-Deck, Seeed T-1000E and Wio-E5. None was + checked here and this project has none of them. +- **Why it is here:** it makes the `Utils::decrypt` finding above a + **two-instance pattern** rather than one project's defect — two unrelated mesh + firmwares, different radios, different code, both with a wire-supplied length + reaching a fixed destination with nothing executable in between. See + [MESHCORE_PARSER_BOUNDS §8](MESHCORE_PARSER_BOUNDS.md). + ### Attadipa's own frame decoder validates length before reading - **Claim:** `link/src/frame_codec.cpp` rejects a declared length greater than @@ -183,9 +230,10 @@ An entry that cannot name its source does not belong here. It belongs in **Merged upward, 2026-08-25.** This entry and *"The pinned MeshCore revision is upstream's current release, not a lagging one"* were the same claim, reached by two research runs a few hours apart with different API calls. They are now one -entry near the top of this section, carrying both sources. Kept as a signpost -rather than deleted, because two entries saying the same thing is how a reader -ends up citing the one that was not updated. +entry near the top of this section, carrying both sources and the 2026-08-24 +amendment that `main` has since moved by two documentation commits. Kept as a +signpost rather than deleted, because two entries saying the same thing is how a +reader ends up citing the one that was not updated. --- diff --git a/docs/research/WAVESHARE_ARRIVAL.md b/docs/research/WAVESHARE_ARRIVAL.md index c34c9e08..39a49fd4 100644 --- a/docs/research/WAVESHARE_ARRIVAL.md +++ b/docs/research/WAVESHARE_ARRIVAL.md @@ -76,7 +76,7 @@ flatly contradicts. **The claim that the board's PSRAM is absent or undeclared is false, and was false before the advice arrived.** [HARDWARE_MATRIX.md:331](HARDWARE_MATRIX.md) "8 MB **octal**" records 8 MB of PSRAM — now also octal-VERIFIED — and -[VERIFIED_FACTS.md:697](VERIFIED_FACTS.md) "Waveshare memory: 32 MB flash, 8 MB PSRAM" +[VERIFIED_FACTS.md:745](VERIFIED_FACTS.md) "Waveshare memory: 32 MB flash, 8 MB PSRAM" records the same as the resolution of D1. No line anywhere in the repository says the part is missing — the vocabulary for absence exists and is used plainly where it is meant, as in `| Sub-GHz radio | — | **not present** | — | — | VERIFIED |` @@ -709,7 +709,7 @@ does not, kept because an uncorrected claim propagates. 1. **"PSRAM is not declared for this board."** False, and contradicted by [HARDWARE_MATRIX.md:331](HARDWARE_MATRIX.md) "8 MB **octal**" and - [VERIFIED_FACTS.md:697](VERIFIED_FACTS.md) "Waveshare memory: 32 MB flash, 8 MB PSRAM". + [VERIFIED_FACTS.md:745](VERIFIED_FACTS.md) "Waveshare memory: 32 MB flash, 8 MB PSRAM". Only the build-configuration reading is true, and it is vacuous — no target has a build configuration here. 2. **"Run `esp_psram_get_size()` on arrival."** As written this cannot do the job diff --git a/docs/research/meshcore-parser-bounds/README.md b/docs/research/meshcore-parser-bounds/README.md index abb5238a..fc6a091e 100644 --- a/docs/research/meshcore-parser-bounds/README.md +++ b/docs/research/meshcore-parser-bounds/README.md @@ -36,20 +36,22 @@ loops and not of the block functions they call. git clone --filter=blob:none https://github.com/meshcore-dev/MeshCore /tmp/meshcore-src git -C /tmp/meshcore-src fetch origin 05da523ebd32980a1c28b11f2928d351796b9737 git -C /tmp/meshcore-src fetch origin f80d805ee8b20f77ff5b3ca6bc3a9021989aafd2 +git -C /tmp/meshcore-src fetch origin pull/3269/head:pr3269 # SHA alone is refused for this one ./build.sh base d92964352441e53b93e8667b802e04f6e072b39e # the pin ./build.sh pr3267 05da523ebd32980a1c28b11f2928d351796b9737 # PR #3267 head +./build.sh pr3269 5ebf8ef9cf1a0df28118c47460277857e0e675b2 # PR #3269 head ./build.sh pr3270 f80d805ee8b20f77ff5b3ca6bc3a9021989aafd2 # PR #3270 head ./run.sh -./build-extras.sh +./build-extras.sh base # the tag names which tree to measure ./build/path_arith # P3, exhaustive ./build/decrypt_bounds 180 # P4, faults; 176 is clean ``` `MESHCORE_SRC` overrides the clone location. Output goes to `build/`, which is ignored. Needs `clang++` with AddressSanitizer; measured on clang 18.1.3 under -Ubuntu 24.04 on 2026-08-23. +Ubuntu 24.04 on 2026-08-23, re-run 2026-08-24. ## Checking a new revision @@ -59,5 +61,25 @@ That is the point of keeping it. `./build.sh && ./run.sh` answers it the entry condition for pinning MeshCore into a local provider. Two findings are **not** in `run.sh`'s matrix and have to be checked separately — -P3 through `path_arith` and P4 through `decrypt_bounds`. A green `run.sh` is not -a clean revision. +P3 through `path_arith` and P4 through `decrypt_bounds`. **A green `run.sh` is +not a clean revision.** + +Both of those go through `./build-extras.sh `, and the tag is not optional +decoration. Two ways this directory could have lied about a candidate revision, +both closed on 2026-08-24 after the independent review of +[#160](https://github.com/hleserg/Attadipa/pull/160) found them: + +- **It built from `tree-base` whatever you asked for.** `./build.sh cand ` + followed by a bare `./build-extras.sh` measured the pinned revision and + reported it without a word. The tag is now an argument, `build.sh` records the + resolved SHA in `build/tree-/.revision`, and both binaries print the + revision they speak for. +- **`path_arith` printed the same answer for every revision.** It hand-copied + `MAX_PACKET_PAYLOAD`, `MAX_PATH_SIZE` and `isValidPathLen`, so a revision where + upstream *fixed* `src/Mesh.cpp:172` scored identically to one where it had not. + The constants and the validator now come from the tree, and the eight lines + that genuinely cannot be executed on a host are fingerprinted against + `src/Mesh.cpp` before anything is built — a revision that has touched them is + refused, with exit 65, naming the digest it wanted and the one it found. + `#3269`'s head is such a revision, so `./build-extras.sh pr3269` is the + one-command demonstration that the check is real. diff --git a/docs/research/meshcore-parser-bounds/build-extras.sh b/docs/research/meshcore-parser-bounds/build-extras.sh index 1e4d4a6a..5e0d55bc 100755 --- a/docs/research/meshcore-parser-bounds/build-extras.sh +++ b/docs/research/meshcore-parser-bounds/build-extras.sh @@ -3,36 +3,107 @@ # The two experiments that are not part of the ten-case corpus: # # ./build/path_arith finding P3 — every (len, path_len) pair that -# reaches src/Mesh.cpp:161-172, and which of them -# underflow extra_len. Self-contained: an -# extraction of the index arithmetic, not a build -# of the upstream translation unit, and the file -# says so at the top. +# reaches src/Mesh.cpp:160-172, and which of them +# underflow extra_len. The constants and the +# validator come from the built tree; the eight +# lines of index arithmetic are a hand-copy, and +# are fingerprinted against the tree below. # # ./build/decrypt_bounds N finding P4 — the real src/Utils.cpp compiled # against a stub block cipher, writing into a # 184-byte destination backed by a guard page. # N is src_len; 176 is clean, 177..180 are not. +# +# Usage: +# +# ./build-extras.sh [tag] tag defaults to "base"; it names a tree already +# written by ./build.sh . +# +# WHY THE TAG IS AN ARGUMENT. Both of these are enrolled as revision checks by +# the entry condition on a future MeshCore pin. An earlier version hardcoded +# tree-base, so `./build.sh cand ` followed by ./build-extras.sh measured +# whatever tree-base happened to hold — reporting the pin's answer under the +# candidate's name, silently, in the tool written to prevent exactly that. set -euo pipefail -MESHCORE_SRC="${MESHCORE_SRC:-/tmp/meshcore-src}" here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" out="$here/build" -tree="$out/tree-base" -mkdir -p "$out" - -clang++ -std=c++17 -g -O0 "$here/path_arith.cpp" -o "$out/path_arith" -echo "built build/path_arith" +if [ $# -gt 1 ]; then + echo "usage: $0 [tag]" >&2 + exit 64 +fi +tag="${1:-base}" +tree="$out/tree-$tag" if [ ! -d "$tree/src" ]; then - echo "build/tree-base is missing — run ./build.sh base first" >&2 + echo "build/tree-$tag is missing — run ./build.sh $tag first" >&2 exit 66 fi +if [ ! -f "$tree/.revision" ]; then + echo "build/tree-$tag has no .revision — it predates this script; rebuild it" >&2 + echo " ./build.sh $tag " >&2 + exit 66 +fi +revision=$(cat "$tree/.revision") + +mkdir -p "$out" + +# --- P3's hand-copy, fingerprinted against the tree ------------------------ +# +# path_arith takes its constants and its validator from the built revision, but +# the index arithmetic in src/Mesh.cpp's PATH branch cannot be executed on a +# host (it sits behind MACThenDecrypt, so behind AES, SHA-256 and ed25519) and +# is therefore copied by hand. A hand-copy cannot notice upstream changing what +# it copied — including upstream *fixing* it, which is the case that matters. +# +# So the branch is extracted between two anchors, whitespace-normalised and +# hashed. A revision that has touched those lines fails here, loudly, instead of +# being measured with the pinned revision's arithmetic. Re-reading P3 by hand at +# that revision is then a decision somebody makes rather than one they skip +# without knowing. #3269's head is such a revision: it adds a line inside this +# region, so it is expected to fail this check. +PATH_BRANCH_SHA256_PIN=5eee273c0079c2e6332f42b0d3d285f7b8eaf55ecdf81602941b4eb48a09fbc2 + +branch_text=$( + awk '/if \(pkt->getPayloadType\(\) == PAYLOAD_TYPE_PATH\) \{/,/uint8_t extra_len = len - k;/' \ + "$tree/src/Mesh.cpp" \ + | sed -e 's/[[:space:]]\+/ /g' -e 's/^ //' -e 's/ $//' +) +if [ -z "$branch_text" ]; then + echo "could not find the PAYLOAD_TYPE_PATH branch in $tree/src/Mesh.cpp." >&2 + echo "Upstream has moved or renamed it. Re-read finding P3 by hand at $revision." >&2 + exit 65 +fi +digest=$(printf '%s\n' "$branch_text" | sha256sum | cut -d' ' -f1) +if [ "$digest" != "$PATH_BRANCH_SHA256_PIN" ]; then + cat >&2 < can only name the tag, and a tag is +# a label somebody chose rather than a revision. +resolved=$(git -C "$MESHCORE_SRC" rev-parse "$ref") +printf '%s\n' "$resolved" > "$tree/.revision" + # Four upstream translation units, unmodified, plus the harness and the shim. # -O0 so the sanitizer's line numbers name the statement rather than whatever a # pass hoisted it into; the findings are about bounds, not about codegen. @@ -61,4 +68,4 @@ clang++ -std=c++17 -g -O0 -fsanitize=address -fno-omit-frame-pointer \ "$tree/src/helpers/AdvertDataHelpers.cpp" \ -o "$out/harness-$tag" -echo "built build/harness-$tag from $(git -C "$MESHCORE_SRC" rev-parse "$ref")" +echo "built build/harness-$tag from $resolved" diff --git a/docs/research/meshcore-parser-bounds/decrypt_bounds.cpp b/docs/research/meshcore-parser-bounds/decrypt_bounds.cpp index ceba6d25..0b1abe2c 100644 --- a/docs/research/meshcore-parser-bounds/decrypt_bounds.cpp +++ b/docs/research/meshcore-parser-bounds/decrypt_bounds.cpp @@ -22,7 +22,11 @@ int main(int argc, char** argv) const size_t page = (size_t)sysconf(_SC_PAGESIZE); uint8_t* m = (uint8_t*)mmap(nullptr, page * 2, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); - mprotect(m + page, page, PROT_NONE); + // Checked, because the failure mode is indistinguishable from the finding: + // an unchecked mmap leaves dest a wild pointer and the first block write + // faults, which looks exactly like the over-write under test and is not it. + if (m == MAP_FAILED) { std::perror("mmap"); return 70; } + if (mprotect(m + page, page, PROT_NONE) != 0) { std::perror("mprotect"); return 70; } uint8_t* dest = m + page - MAX_PACKET_PAYLOAD; // 184 bytes, then the wall static uint8_t src[512]; diff --git a/docs/research/meshcore-parser-bounds/path_arith.cpp b/docs/research/meshcore-parser-bounds/path_arith.cpp index c4821096..97a15437 100644 --- a/docs/research/meshcore-parser-bounds/path_arith.cpp +++ b/docs/research/meshcore-parser-bounds/path_arith.cpp @@ -4,49 +4,67 @@ // Reaching src/Mesh.cpp's PATH branch for real needs Utils::MACThenDecrypt to // succeed, which needs the AES and SHA256 libraries MeshCore pulls from // PlatformIO, and an Identity, which needs ed25519. None of that is built here. -// What is reproduced below is the index arithmetic, copied byte for byte from +// What is reproduced below is the index arithmetic, copied by hand from // -// meshcore-dev/MeshCore @ d92964352441e53b93e8667b802e04f6e072b39e -// src/Mesh.cpp lines 161-172 +// src/Mesh.cpp lines 160-172 // // and run over its entire input domain, so the claim "extra_len can underflow" // is a computed result rather than an assertion about code someone read. +// +// WHAT REVISION IT SPEAKS FOR. Everything a hand-copy cannot notice is taken +// from the built tree instead of retyped here: MAX_PACKET_PAYLOAD and +// MAX_PATH_SIZE come from the revision's own MeshCore.h, and isValidPathLen is +// the revision's own Packet.cpp, linked in and called. What is left hand-copied +// is the eight lines of index arithmetic, and build-extras.sh fingerprints those +// against the tree before it will build this at all. So a revision where +// upstream changed the constants, the validator or the arithmetic cannot be +// silently measured with the pinned revision's answer — which is exactly the +// failure a "re-run the corpus against the candidate" entry condition is for. #include #include -#define MAX_PACKET_PAYLOAD 184 -#define MAX_PATH_SIZE 64 +#include // mesh::Packet::isValidPathLen, at the built revision +#include // MAX_PACKET_PAYLOAD, MAX_PATH_SIZE, at the built revision -// src/Packet.cpp:13-18, verbatim. -static bool isValidPathLen(uint8_t path_len) { - uint8_t hash_count = path_len & 63; - uint8_t hash_size = (path_len >> 6) + 1; - if (hash_size == 4) return false; // Reserved for future - return hash_count*hash_size <= MAX_PATH_SIZE; -} +#ifndef PARSER_BOUNDS_REV +#define PARSER_BOUNDS_REV "unknown — built outside build-extras.sh" +#endif int main() { // The decrypted plaintext length. MACThenDecrypt returns 0 for anything at // or under CIPHER_MAC_SIZE, and cannot exceed the payload buffer. // Utils::decrypt returns whole 16-byte blocks ("will always be multiple of - // 16", src/Utils.cpp:81), so len is a multiple of 16. 176 is the largest - // that still fits data[184]; 192 is the P4 case and is excluded here so the - // two findings do not get mixed up. - const int len_min = 16, len_max = 176, len_step = 16; + // 16", src/Utils.cpp:81), so len is a multiple of 16. MAX_PACKET_PAYLOAD + // rounded down to a whole block is the largest that still fits data[]; the + // next block up is the P4 case and is excluded here so the two findings do + // not get mixed up. + const int len_step = 16; + const int len_min = len_step; + const int len_max = (MAX_PACKET_PAYLOAD / len_step) * len_step; + + std::printf("revision under test : %s\n", PARSER_BOUNDS_REV); + std::printf("MAX_PACKET_PAYLOAD : %d (from the tree's MeshCore.h)\n", + (int)MAX_PACKET_PAYLOAD); + std::printf("MAX_PATH_SIZE : %d (from the tree's MeshCore.h)\n", + (int)MAX_PATH_SIZE); + std::printf("len domain : %d..%d step %d\n\n", + len_min, len_max, len_step); long total = 0, underflow = 0, read_past_data = 0; int worst_extra_len = 0, worst_len = 0, worst_path_len = 0, worst_k = 0; int smallest_len = 1 << 30, smallest_path_len = 256; + int smallest_k = 0, smallest_extra_len = 0; for (int len = len_min; len <= len_max; len += len_step) { for (int pl = 0; pl <= 255; pl++) { uint8_t path_len = (uint8_t)pl; - if (!isValidPathLen(path_len)) continue; // upstream rejects and breaks + // The revision's own validator, linked from its Packet.cpp. + if (!mesh::Packet::isValidPathLen(path_len)) continue; // upstream rejects and breaks total++; - // --- src/Mesh.cpp:161-172, indices only --- + // --- src/Mesh.cpp:160-172, indices only --- int k = 0; k++; // uint8_t path_len = data[k++] uint8_t hash_size = (path_len >> 6) + 1; @@ -60,8 +78,9 @@ int main() underflow++; if (len < smallest_len || (len == smallest_len && path_len < smallest_path_len)) { smallest_len = len; smallest_path_len = path_len; + smallest_k = k; smallest_extra_len = extra_len; } - // Does the consumer's window run off the end of data[184]? + // Does the consumer's window run off the end of data[]? if (k + extra_len > MAX_PACKET_PAYLOAD) { read_past_data++; if (extra_len > worst_extra_len) { @@ -75,14 +94,23 @@ int main() std::printf("accepted (len, path_len) pairs : %ld\n", total); std::printf("pairs where k > len (extra_len underflows): %ld\n", underflow); - std::printf(" of those, &data[k] + extra_len > 184 : %ld\n", read_past_data); - std::printf("smallest underflowing input : len=%d path_len=0x%02X\n", - smallest_len, smallest_path_len); - std::printf("largest over-read window : len=%d path_len=0x%02X" - " -> k=%d extra_len=%d, i.e. data[%d..%d] against data[0..183]" - " (%d bytes past the end)\n", - worst_len, worst_path_len, worst_k, worst_extra_len, - worst_k, worst_k + worst_extra_len - 1, - worst_k + worst_extra_len - MAX_PACKET_PAYLOAD); + std::printf(" of those, &data[k] + extra_len > %-3d : %ld\n", + (int)MAX_PACKET_PAYLOAD, read_past_data); + if (underflow > 0) { + std::printf("smallest underflowing input : len=%d path_len=0x%02X" + " -> k=%d extra_len=%d, i.e. data[%d..%d] against data[0..%d]" + " (%d bytes past the end)\n", + smallest_len, smallest_path_len, smallest_k, smallest_extra_len, + smallest_k, smallest_k + smallest_extra_len - 1, + (int)MAX_PACKET_PAYLOAD - 1, + smallest_k + smallest_extra_len - MAX_PACKET_PAYLOAD); + std::printf("largest over-read window : len=%d path_len=0x%02X" + " -> k=%d extra_len=%d, i.e. data[%d..%d] against data[0..%d]" + " (%d bytes past the end)\n", + worst_len, worst_path_len, worst_k, worst_extra_len, + worst_k, worst_k + worst_extra_len - 1, + (int)MAX_PACKET_PAYLOAD - 1, + worst_k + worst_extra_len - MAX_PACKET_PAYLOAD); + } return 0; }