Nine of ten malformed frames over-read at the pin, and none of the three fixes is finished

[claude]

Fixes #142

Research only. No Attadipa code changed, and none should have — this
repository links no MeshCore. Documentation and a runnable harness under
docs/research/.

Problem

Three open upstream pull requests (#3267, #3269, #3270, plus two closed
predecessors) claim missing length checks in MeshCore's frame and advertisement
parsers, found by fuzzing. The issue asked three things: do they hold at the
revision we pinned rather than at the dev commits they are based on; what is
the real blast radius traced from the triggering byte to the buffer it lands in;
and what does any of it cost the two Attadipa paths — an external stock companion
node today, a local MeshCore provider later.

The expensive failure mode here is not missing a finding, it is writing down
"out-of-bounds read" and letting it stand in for a consequence nobody measured.

Solution

docs/research/MESHCORE_PARSER_BOUNDS.md, with the executable half in
docs/research/meshcore-parser-bounds/.

The pin is not lagging, and the dev bases are not an obstacle. d929643 is
simultaneously our pin, upstream's main tip and its newest release. Every file
any of the three pull requests touches is byte-identical between the pin 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.

The findings hold — nine of ten corpus cases over-read at the pin. But at
every reachable call site the backing buffer 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 for adverts), and all nine end
in a rejected packet. The one direction where Attadipa supplies the bytes —
CMD_IMPORT_CONTACT — cannot reach the worst of them: it gates on len > 98 and
Packet::readFrom reaches at most byte 70.

Three things the pull requests do not say:

• #3269 does not fix its finding. Its entire diff is a MESH_DEBUG_PRINTLN
  and then the read executes anyway — no break, no return.
• #3270 does not fix its own. On its head f80d805e, app_data[0] is still
  read when app_data_len == 0 (AdvertDataHelpers.cpp:34). #3271 is the same
  commit, so both advert pull requests are incomplete in the same place. Only
  #3267 is complete, and it covers none of what #3270 covers.
• A fifth defect, in no pull request, and it is a write. Utils::decrypt
  rounds output up to whole 16-byte 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. Both it and the PAYLOAD_TYPE_PATH extra_len
  underflow sit behind a 2-byte MAC, which is not the authentication gate it
  looks like.

Reporting that upstream is the owner's call and deliberately not done here —
filing on a third-party repository is outward-facing and this run had no
authority for it. The evidence reproduces in one command.

Also updated: REUSE_LEDGER.md (pin re-check, and a monitored deltas table
that records these as watched and explicitly not vendored), VERIFIED_FACTS.md
(five entries, each separating "reads past its length" from "leaves the buffer"),
OPEN_QUESTIONS.md (M15–M18), STATUS.md, TASKS.md (T-127 done; entry
conditions added to T-013 and T-050).

No ADR changed. ADR-0008 already treats the node as a peer whose output is
validated on arrival; nothing here moved that boundary, and an ADR edited to
restate itself is churn. attadipa_link's decoder was checked, not assumed —
it validates the declared length before reading (frame_codec.cpp:139, :148)
behind a length-check byte and a CRC. Nothing in link/ changed.

Two corrections, made on this branch

Both found by checking the report's own citations against the tree rather than
against my notes, and both are in the history rather than silently amended:

• tryParsePacket has a second caller — CMD_SEND_RAW_PACKET at
  examples/companion_radio/MyMesh.cpp:2000, gated only by len >= 4. The first
  commit said Dispatcher::checkRecv was the sole caller, which was wrong in the
  one direction that matters most here: it is the single place where a client
  hands bytes to a MeshCore parser. Consequence is still nil — 177-byte
  cmd_frame, reach of raw[5], returns ERR_CODE_ILLEGAL_ARG — but the
  reachability claim was not.
• Four line numbers were off by one or two.

Upstream and reuse

meshcore-dev/MeshCore, MIT (license.txt), pinned
d92964352441e53b93e8667b802e04f6e072b39e.

Nothing was vendored. The guards in 05da523e and f80d805e are MIT and
usable, but all three pull requests are unmerged and unreleased and two do not
close their own findings, so taking any of them would buy part of a problem into
a project that compiles none of the files. What was taken is the shape of the
inputs: the ten-case corpus is ours, written from reading the parsers. There was
no upstream corpus to port — MeshCore's test/ covers none of these paths and
its AES/SHA256 test mocks are no-ops (M13).

The harness compiles four upstream translation units unmodified at whichever
revision it is pointed at, against four shim headers. The SHA256 and AES128
shims are not ciphers and say so in their own files; nothing measured depends
on what they compute, only on how many bytes the surrounding loops move.

Tests

• cmake -S . -B build && cmake --build build && ctest --test-dir build --output-on-failure
  — 24/24 pass. This branch touches no code, so that is a statement that
  nothing regressed, not evidence for any claim in the report.
  (python3-pil must be installed or the build registers a deliberately failing
  placeholder, as CI's own step notes.)
• shellcheck clean on the three new scripts. Relative links in every edited
  document resolve.
• The harness was re-run from the committed copy, not from the scratch
  directory it was written in, and reproduces the matrix in §4 exactly:
  base 10/10 over-read → pr3267 closes A and B, leaves C → pr3270 closes
  C1/C2/C4, leaves A, B and C3.
• path_arith: 187 of 1309 reachable (len, path_len) pairs underflow
  extra_len, and all 187 produce a window past data[184].
• decrypt_bounds: clean at src_len=176, faults at 177/180/182 at
  Utils.cpp:77.

One method note, because it changed a result. The first harness used a tight
malloc(len) alone and ASan does not report a read at offset 0 of a
malloc(0) — which silently turned two genuine findings green, including the
one that shows #3270 is incomplete. The guard page is the mechanism of record and
the trap is documented in the harness.

Hardware

NOT EXECUTED — HARDWARE REQUIRED. Nothing here ran on a radio, a node or any
board, and no host sanitizer result may be presented as radio or HIL validation.

To go further would need: a Heltec V4/V4.3 or SX1262 node to confirm the pull
request authors' own (unverified, trace-free) claim of hardware verification, and
a second radio to inject malformed frames on air. Neither is available and
neither is needed for anything asserted here.

Risks

• The report could be read as licence to relax our own validation. It is the
  opposite: the node stays a peer, its output stays validated on arrival, and the
  reason is now stronger than it was.
• The harness can rot if upstream restructures those files. It fails loudly
  (a compile error naming the file) rather than quietly, and build.sh refuses a
  ref the clone cannot resolve.
• A green run.sh is not a clean revision — P3 and P4 are outside its matrix.
  Said in the README, in the report, and in the T-013 entry condition, because
  this is the one way the artifact could mislead a future pin.
• P4 is unreported upstream for as long as the owner has not decided. It is a
  memory-safety defect in shipping third-party firmware; the delay is a real cost
  and it is the owner's to weigh, not an agent's to pre-empt.

Remaining blockers

None blocking. Four things this work could not establish, recorded as M15–M18
in OPEN_QUESTIONS.md: whether the two arithmetic defects run end to end
(needs a host build with the genuine AES/SHA-256/ed25519, which would also give
the project its first real MeshCore reference vectors); whether the stale stack
bytes they read can be groomed; what P4's eight over-written bytes hit on an
ESP32-S3 (a compiler question, not a hardware one); and whether a real fuzzing
pass finds more — the corpus demonstrates, it does not search, and Utils::decrypt
was found by following a caller rather than by the corpus.

None of the four blocks anything while Attadipa compiles no MeshCore. All four
become entry conditions the moment a local provider is real.

🤖 Generated with Claude Code

[claude]

Independent review — PR #160

Read the diff, CLAUDE.md, master-prompt-final.md §1, ADR-0002/0008, frame_codec.cpp, check_docs.py, ci.yml, codeql.yml, merge-candidate.sh. Research and a harness under docs/; no production code changed, so most regression axes genuinely do not apply — named at the end rather than padded. Four findings, one blocking, and the blocking one is not about MeshCore.

Caveat bounding the rest: no network to raw.githubusercontent.com in this run, so I could not re-read MeshCore at d929643. Claims about upstream code rest on the author's execution.

1 · BLOCKING — STATUS.md:67 marks a live P1 GNSS task as done

- **T-051 — MeshCore parser bounds at the pin — done (research only).**

T-051 is not this work. TASKS.md:674 is "T-051 · What the MIA-M10Q actually does, from u-blox's own documents" — P1, "it gates the GNSS driver, and nothing before it", Research status: not started. This branch never touches it. The work done is T-127 everywhere else in the diff (TASKS.md:1837 DONE entry, :904 and :937 both stamped (T-127), and the PR body). T-127 appears nowhere in STATUS.md.

CLAUDE.md sends the next agent to STATUS.md for "where things actually are". After merge the two files disagree about a P1 task in the direction that loses work: an agent sees T-051 closed and never opens the MIA-M10Q datasheets, while the research that was done has no entry under its real ID.

Reproduces: grep -n "T-051" STATUS.md TASKS.md — one says done, the other not started, different subjects.

CI cannot catch it. check_docs.py checks links, code spans, duplicate IDs within TASKS.md, empty bodies, stray root files (main(), 347-352). An ID consistent in TASKS.md and wrong in STATUS.md passes all five; the docs job will be green. One identifier — but it must precede merge, because STATUS.md steers the queue.

2 · needs-owner — P4 is published here before the people who can fix it are told

MESHCORE_PARSER_BOUNDS.md:513-518. Not filing on a third-party repository unasked is right and matches CLAUDE.md — but that sentence is about notifying, and reads as if the whole disclosure decision were open. It is not: this repository is public, §3 P4 gives affected versions, the triggering range (payload_len 181…184) and the faulting line, §7 gives a one-command reproduction, and decrypt_bounds.cpp is the reproduction. Merge puts that in main permanently while upstream has not been told it exists.

In fairness, opening this PR already published it, so blocking merge un-publishes nothing. The ask is not a code fix: the owner should decide the ordering while it is still theirs — notify upstream first, or accept publication-without-notice deliberately. Either is defensible; drifting into the second by default is not. Add needs-owner; reword §5's P4 bullet to say publication has already occurred.

3 · build-extras.sh:22 cannot check the revision the entry condition names

tree="$out/tree-base" is hardcoded, used at :36, so decrypt_bounds is always built from whatever ./build.sh base <ref> last wrote — whichever revision is under evaluation. TASKS.md:937 makes exactly this T-013's entry condition: "run path_arith and decrypt_bounds too — they are not in the matrix, so a green run.sh is not a clean revision."

Reproduces: ./build.sh base d929643…; ./build.sh cand <future-sha>; ./run.sh (correctly shows cand rows); ./build-extras.sh && ./build/decrypt_bounds 180 → measures d929643. Silently: build.sh:64 echoes the SHA it built, build-extras.sh:38 echoes only built build/decrypt_bounds. A candidate where upstream fixed Utils::decrypt is recorded as still broken; a worse candidate is recorded as fine. The only loud failure is tree-base missing (:29-32). This is the failure the document warns about, reintroduced in the tool written to prevent it. Fix: ./build-extras.sh <tag>, defaulting to base, printing the resolved SHA the way build.sh:64 does.

4 · path_arith cannot answer a question about any revision, including the one it names

path_arith.cpp:18-19 and :22-27 hand-copy MAX_PACKET_PAYLOAD, MAX_PATH_SIZE and isValidPathLen; build-extras.sh:26 compiles nothing from the tree. Its output is byte-identical for every MeshCore revision, including one where upstream fixed Mesh.cpp:172. The file's own header is honest about being an extraction — the defect is downstream: TASKS.md:937, README.md:61-63 and MESHCORE_PARSER_BOUNDS.md:489 all enrol it as a revision check, which it cannot be.

Reproduces: ./build/path_arith prints 1309 / 187 / 187 / len=16 path_len=0x0F whatever build.sh was last pointed at. Either read the constants from build/tree-<tag>/src, or have the entry condition say P3 must be re-read by hand at the candidate revision.

Minor

• MESHCORE_PARSER_BOUNDS.md:573 — References says "M15–M17 from this one"; four were filed (OPEN_QUESTIONS.md:189-192 = M15–M18), and STATUS.md, TASKS.md and the PR body all say M15–M18. A reader following the reference misses M18, the one scoping the follow-up.
• MESHCORE_PARSER_BOUNDS.md:246-254 — the block under "computed rather than asserted" reads as captured output and is not: path_arith.cpp:76-86 uses different labels and emits two separate rows, and the -> k=17, extra_len=255, window data[17..271]… continuation is printed by nothing. Numbers are right, so presentation only — but in a section arguing the result was computed, a paraphrase inside a results fence is the one place a reader cannot tell which they have.
• decrypt_bounds.cpp:23-25 — mmap/mprotect unchecked, unlike harness.cpp:84-85. On failure dest is a wild pointer and the first block write faults: a SEGV that looks like the finding and is not it. Rig hygiene, not a reason to doubt P4.

What I verified, and what I could not

P3's arithmetic is exactly right, re-derived by hand: isValidPathLen accepts 64+33+22 = 119 values × 11 values of len = 1309 ✓; underflows 91+61+32+3 = 187 ✓; every underflow gives k + extra_len = 256 + len ≥ 272 > 184, so "all 187" is exact rather than empirical ✓; smallest len=16, path_len=0x0F → k=17, extra_len=255, 88 past ✓. The guard-page rig is correct: harness.cpp:87 puts the last declared byte at base+page-1, so p[len] is the first guarded byte for every len including 0 — which is what makes A3 and C3 observable at all — and decrypt_bounds.cpp:26 lands dest+184 exactly on the guard. "Not part of any Attadipa build" holds: the only file(GLOB …) is tests/CMakeLists.txt:304, scoped to replay/scenarios/*.trace, and CodeQL builds the CMake project manually (codeql.yml:45-52), so the hostile harness never enters its database. The attadipa_link claim is accurate line for line: frame_codec.cpp:123, :139 declared > kMaxPayload, :148 size_ < needed before any payload byte is touched. All relative links resolve including the #meshcore anchor; §3 ↔ §4 line numbers and the masks at harness.cpp:190-202 are internally consistent.

Not verified: anything in MeshCore's own source — P1, P2, P4, P5, the call-site buffer sizes (256/262/250 B), the CMD_IMPORT_CONTACT len > 98 gate and the byte-identical-bases table are the author's execution, unre-run here. The harness has never run in CI and no transcript is committed, so §4 is trusted rather than checked — and finding 3 is how it stops being reproducible later. CI does not shellcheck these scripts (ci.yml:233 covers .github/scripts and .github/tests only).

Axes that do not apply

Architecture — nothing under core/, apps/, boards/, platform/, ui/, link/; no #ifdef BOARD_*; no capability-registry surface. The one architectural claim, that ADR-0008's trust boundary already covers this and needs no edit, is correct: the node stays a peer and P1–P5 all execute on the far side of the companion link. Lifetime/memory — nothing ships; the harness leaks deliberately and says why (harness.cpp:93-95). Concurrency — no task, ISR, queue or LVGL. Power — no rail, wake source or sleep path, and no power number asserted. Persistence — no stored structure or format. Protocol — link/ unchanged, non-analogy verified above. GNSS — nothing, except by accident: finding 1 lands on the GNSS queue, which is what makes it blocking. Product — no user-facing string; neither README needed a counterpart edit and the commit says so. Evidence — the discipline holds where dropping it would have been easy: §6 separates "reads past its length" from "leaves the buffer" and marks P3/P4 NOT REPRODUCED end to end; the shims declare in their own headers that they are not ciphers; the authors' unevidenced Heltec V4 claim is recorded as unverified.

Process note: merge-candidate.sh:119 allowlists nine docs/* prefixes; STATUS.md and TASKS.md are at the root and are not on it, so the half-hourly sweep will correctly decline this even once green — it needs an orchestrator session. Not a defect, just so nobody waits on it.

Label ai-review:blocking, for finding 1 alone — one identifier that tells the next agent a not-started P1 GNSS task is finished. Findings 3 and 4 belong in the same push: cheap now, expensive at T-013, which is exactly when nobody will re-read the harness. Finding 2 needs the owner, not a commit.

The research itself is good work. The base-equivalence argument dissolves the issue's caveat instead of working around it; the buffer-versus-length distinction is held everywhere, including where it makes the finding less dramatic; and catching that two of three upstream fixes do not close their own findings is what "never trust, verify" is for.

[github-actions]

This pull request has a merge conflict, so nothing is building it

Head 0ac1615 reports mergeable: false (mergeable_state: dirty), and GET /commits/0ac1615/check-runs answers total_count: 12.

That is not CI being slow. GitHub builds refs/pull/160/merge to run pull_request workflows against, and it does not run them at all when that ref cannot be built:

Workflows will not run on pull_request activity if the pull request has a merge conflict. The merge conflict must be resolved first.

Every check this repository produces on a pull request — CI, CodeQL and the review — is pull_request-triggered, and there is no pull_request_target workflow here, which is the one exception that page names. So none of them will run against this head while the conflict stands. That count is not zero, so read what is actually on this head before trusting it — a status left by a third-party app, and runs against an earlier head listed under the same branch, both look like coverage and are neither.

Resolve the conflict and push; the checks start on the next commit.

git fetch origin && git rebase origin/main   # or merge, either starts CI

Issue #142 carried agent:review, which says the work is with the reviewers. It is not — there is no verdict for them to have reached — so it is back to agent:ready and the next watchdog tick can pick it up.

Said once per head commit, not once per hour: push again while still conflicted and this appears again, because that push will not have been built either.