Files
meshcore-analyzer/cmd/ingestor/decoder_bounds_test.go
T
Kpa-clawbot 85e97d2f37 fix(#1211): bounds-check path length to prevent slice [218:15] panic in MQTT decode (#1214)
**RED commit:** `65d9f57b` (CI run will appear at
https://github.com/Kpa-clawbot/CoreScope/actions after PR opens)

Fixes #1211

## Root cause

`decodePath()` returns `bytesConsumed = hash_size * hash_count` where
both come straight from the wire-supplied `pathByte` (upper 2 bits →
`hash_size`, lower 6 bits → `hash_count`). Max claimable: 4 × 63 = 252
bytes.

A malformed packet on the wire claimed `pathByte=0xF6` (hash_size=4,
hash_count=54 → 216 path bytes) inside a 15-byte buffer. The inner
hop-extraction loop in `decodePath` did break early on overflow — but
`bytesConsumed` was still returned at face value (216). `DecodePacket`
then did `offset += 216` (offset=218) and `payloadBuf := buf[offset:]`
panicked with the prod-observed signature:

```
runtime error: slice bounds out of range [218:15]
```

The handler-level `defer/recover` at `cmd/ingestor/main.go:258-263`
caught it, but the message was silently dropped with no usable
diagnostic.

## Fix

Add a `if offset > len(buf)` guard at BOTH decoder sites (same pattern,
same panic potential):

- `cmd/ingestor/decoder.go` — DecodePacket after decodePath
- `cmd/server/decoder.go` — DecodePacket after decodePath

Return a descriptive error citing the claimed length and pathByte hex so
operators can reproduce.

Also: `cmd/ingestor/main.go` decode-error log now includes `topic`,
`observer`, and `rawHexLen` so future malformed packets are reproducible
without needing to attach a debugger.

## Tests (TDD red → green)

Both packages got two new tests:

- **`TestDecodePacketBoundsFromWire_Issue1211`** — feeds the exact wire
shape from the prod log (`pathByte=0xF6` inside a 15-byte buf). Asserts
`DecodePacket` does NOT panic and returns an error.
- **`TestDecodePacketFuzzTruncated_Issue1211`** — sweeps every `(header,
pathByte)` combination with tails 0..19 bytes (≈1.3M inputs). Asserts
zero panics.

### Red commit proof

On commit `65d9f57b` (RED), both tests fail with the panic:
```
=== RUN   TestDecodePacketBoundsFromWire_Issue1211
    decoder_test.go:1996: DecodePacket panicked on malformed input: runtime error: slice bounds out of range [218:15]
--- FAIL: TestDecodePacketBoundsFromWire_Issue1211 (0.00s)
=== RUN   TestDecodePacketFuzzTruncated_Issue1211
    decoder_test.go:2010: DecodePacket panicked during fuzz: runtime error: slice bounds out of range [3:2]
--- FAIL: TestDecodePacketFuzzTruncated_Issue1211 (0.01s)
```

On commit `7a6ae52c` (GREEN), full suites pass:
- `cmd/ingestor`: `ok 53.988s`
- `cmd/server`:   `ok 29.456s`

## Acceptance criteria

- [x] Identify the slice op producing `[218:15]` — `payloadBuf :=
buf[offset:]` in `DecodePacket` (decoder.go), where `offset` had been
advanced by an unchecked `bytesConsumed` from `decodePath()`.
- [x] Bounds check added at the identified site(s) — both ingestor and
server decoders.
- [x] Test with crafted payload (length-field > remaining buffer) —
`TestDecodePacketBoundsFromWire_Issue1211`.
- [x] Log topic, observer ID, payload byte length on drop — updated
`MQTT [%s] decode error` log line.
- [x] Existing tests stay green — confirmed both packages.

## Out of scope

Reconnect-after-disconnect (#1212) — handled by a separate subagent.
This PR touches NO reconnect logic.

---------

Co-authored-by: corescope-bot <bot@corescope.local>
Co-authored-by: openclaw-bot <bot@openclaw.local>
Co-authored-by: corescope-bot <bot@corescope>
2026-05-15 22:34:21 -07:00

98 lines
3.2 KiB
Go

package main
import (
"encoding/hex"
"strings"
"testing"
)
// --- Issue #1211 round-1 protocol-correctness regressions ---
// See cmd/server/decoder_bounds_test.go for full firmware citations
// (firmware/src/Packet.cpp:13-18, firmware/src/MeshCore.h:19-21).
// pathByte=0xF6 → hash_size=4 (reserved), hash_count=54.
// Buffer holds all 216 claimed bytes so the OOB guard does NOT catch.
func TestDecodePacketRejectsReservedHashSize_Issue1211(t *testing.T) {
raw := "12F6" + strings.Repeat("AB", 216) + strings.Repeat("CD", 8)
pkt, err := DecodePacket(raw, nil, false)
if err == nil {
t.Fatalf("expected error rejecting reserved hash_size=4 (firmware Packet.cpp:13-18); got nil, pkt=%+v", pkt)
}
if !strings.Contains(err.Error(), "path") {
t.Errorf("error should mention path; got %q", err)
}
}
// pathByte=0xBF → hash_size=3, hash_count=63, total=189 > MAX_PATH_SIZE=64.
func TestDecodePacketRejectsOversizedPath_Issue1211(t *testing.T) {
raw := "12BF" + strings.Repeat("AB", 189) + strings.Repeat("CD", 8)
pkt, err := DecodePacket(raw, nil, false)
if err == nil {
t.Fatalf("expected error rejecting hash_count*hash_size > 64; got nil, pkt=%+v", pkt)
}
}
// Payload > MAX_PACKET_PAYLOAD (184).
func TestDecodePacketRejectsOversizedPayload_Issue1211(t *testing.T) {
raw := "1200" + strings.Repeat("AA", 200)
pkt, err := DecodePacket(raw, nil, false)
if err == nil {
t.Fatalf("expected error rejecting payload > MAX_PACKET_PAYLOAD=184 (firmware MeshCore.h:19); got nil, pkt=%+v", pkt)
}
if !strings.Contains(err.Error(), "payload") {
t.Errorf("error should mention payload; got %q", err)
}
}
func TestDecodePath_RejectsReservedHashSize_Issue1211(t *testing.T) {
buf := make([]byte, 216)
for i := range buf {
buf[i] = 0xAB
}
_, _, err := decodePath(0xF6, buf, 0)
if err == nil {
t.Fatalf("decodePath should reject pathByte=0xF6 (hash_size=4 reserved); got nil err")
}
}
func TestDecodePath_RejectsOversizedPath_Issue1211(t *testing.T) {
buf := make([]byte, 189)
_, _, err := decodePath(0xBF, buf, 0)
if err == nil {
t.Fatalf("decodePath should reject hash_count*hash_size=189 > MAX_PATH_SIZE=64; got nil err")
}
}
func TestDecodePath_AcceptsValidEncodings_Issue1211(t *testing.T) {
buf := []byte{0x01, 0x02, 0x03, 0x04, 0x05}
path, consumed, err := decodePath(0x05, buf, 0)
if err != nil {
t.Fatalf("decodePath rejected valid encoding: %v", err)
}
if consumed != 5 {
t.Errorf("consumed=%d, want 5", consumed)
}
if path.HashCount != 5 || path.HashSize != 1 {
t.Errorf("decode wrong: hashCount=%d hashSize=%d", path.HashCount, path.HashSize)
}
}
// Kent #1 — pin tautological assertion: error MUST mention "path length"
// AND "exceeds buffer", not just non-nil. Uses firmware-valid pathByte
// that exhausts a small buffer, so the OOB guard fires (not validity).
func TestDecodePacketBoundsFromWireErrorPhrasing_Issue1211(t *testing.T) {
raw := "120A" + strings.Repeat("AA", 5)
_, err := DecodePacket(raw, nil, false)
if err == nil {
t.Fatalf("expected error, got nil")
}
if !strings.Contains(err.Error(), "path length") {
t.Errorf("error missing 'path length'; got %q", err)
}
if !strings.Contains(err.Error(), "exceeds buffer") {
t.Errorf("error missing 'exceeds buffer'; got %q", err)
}
}
var _ = hex.EncodeToString