mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-05-19 11:55:09 +00:00
**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>
This commit is contained in:
@@ -0,0 +1,63 @@
|
||||
package main
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"log"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// TestHandleMessageDecodeErrorLog_PII — issue #1211 round-0 fix shipped without
|
||||
// a test. Asserts the decode-error log line:
|
||||
// (a) includes structured fields: topic, observer prefix, payload length
|
||||
// (b) observer substring is at most 8 chars
|
||||
// (c) full observer ID is NOT present in the output
|
||||
//
|
||||
// A bare `log.Printf("... observer=%s ...", obs)` would leak the full ID.
|
||||
func TestHandleMessageDecodeErrorLog_PII_Issue1211(t *testing.T) {
|
||||
store, source := newTestContext(t)
|
||||
|
||||
// Use a 64-char observer ID; the prefix MUST be capped at 8 chars in logs.
|
||||
observerID := "abcdef0123456789aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
|
||||
// Malformed raw — pathByte=0xF6 claims 216 path bytes in a tiny buffer.
|
||||
// This triggers the decode-error path under test.
|
||||
rawHex := "12F6AAAAAAAAAAAAAAAAAAAAAAAAAA"
|
||||
topic := "meshcore/SJC/" + observerID + "/packets"
|
||||
payload := []byte(`{"raw":"` + rawHex + `"}`)
|
||||
msg := &mockMessage{topic: topic, payload: payload}
|
||||
|
||||
var buf bytes.Buffer
|
||||
orig := log.Writer()
|
||||
log.SetOutput(&buf)
|
||||
defer log.SetOutput(orig)
|
||||
|
||||
handleMessage(store, "test", source, msg, nil, &Config{})
|
||||
|
||||
out := buf.String()
|
||||
if !strings.Contains(out, "decode error") {
|
||||
t.Fatalf("expected decode-error log; got:\n%s", out)
|
||||
}
|
||||
// (a) structured fields present
|
||||
if !strings.Contains(out, "topic=") {
|
||||
t.Errorf("log missing topic=; got:\n%s", out)
|
||||
}
|
||||
if !strings.Contains(out, "observer=") {
|
||||
t.Errorf("log missing observer=; got:\n%s", out)
|
||||
}
|
||||
if !strings.Contains(out, "rawHexLen=") {
|
||||
t.Errorf("log missing rawHexLen=; got:\n%s", out)
|
||||
}
|
||||
// (c) full observer ID must NOT appear
|
||||
if strings.Contains(out, observerID) {
|
||||
t.Errorf("log leaked full observer ID; got:\n%s", out)
|
||||
}
|
||||
// (b) observer substring capped at 8 chars — the 9th char ('2') after the
|
||||
// 8-char prefix must NOT appear adjacent to the prefix.
|
||||
if strings.Contains(out, "abcdef01234") {
|
||||
t.Errorf("log observer field longer than 8 chars; got:\n%s", out)
|
||||
}
|
||||
// Positive: 8-char prefix must be present in the log
|
||||
if !strings.Contains(out, "abcdef01") {
|
||||
t.Errorf("log missing 8-char observer prefix; got:\n%s", out)
|
||||
}
|
||||
}
|
||||
+52
-3
@@ -172,9 +172,35 @@ func decodeHeader(b byte) Header {
|
||||
}
|
||||
}
|
||||
|
||||
func decodePath(pathByte byte, buf []byte, offset int) (Path, int) {
|
||||
// Firmware-derived limits — see firmware/src/MeshCore.h:19,21.
|
||||
const (
|
||||
maxPathSize = 64 // MAX_PATH_SIZE — total path bytes allowed
|
||||
maxPacketPayload = 184 // MAX_PACKET_PAYLOAD — max raw payload bytes
|
||||
)
|
||||
|
||||
// isValidPathLen mirrors firmware Packet::isValidPathLen
|
||||
// (firmware/src/Packet.cpp:13-18). hash_size==4 is reserved; total path bytes
|
||||
// must fit within MAX_PATH_SIZE.
|
||||
func isValidPathLen(pathByte byte) bool {
|
||||
hashCount := int(pathByte & 0x3F)
|
||||
hashSize := int(pathByte>>6) + 1
|
||||
if hashSize == 4 {
|
||||
return false // reserved
|
||||
}
|
||||
return hashCount*hashSize <= maxPathSize
|
||||
}
|
||||
|
||||
func decodePath(pathByte byte, buf []byte, offset int) (Path, int, error) {
|
||||
hashSize := int(pathByte>>6) + 1
|
||||
hashCount := int(pathByte & 0x3F)
|
||||
// Exact mirror of firmware Packet::isValidPathLen (Packet.cpp:13-18).
|
||||
// hash_size==4 is reserved and is rejected by firmware regardless of
|
||||
// hash_count, so we must reject 0xC0 etc even on zero-hop packets —
|
||||
// firmware never emits them, so an on-wire pathByte with the upper
|
||||
// 2 bits set to 11 is by definition malformed/adversarial.
|
||||
if !isValidPathLen(pathByte) {
|
||||
return Path{}, 0, fmt.Errorf("invalid path encoding: pathByte 0x%02X (hash_size=%d hash_count=%d) violates firmware validity (Packet.cpp:13-18, MAX_PATH_SIZE=%d)", pathByte, hashSize, hashCount, maxPathSize)
|
||||
}
|
||||
totalBytes := hashSize * hashCount
|
||||
hops := make([]string, 0, hashCount)
|
||||
|
||||
@@ -191,7 +217,7 @@ func decodePath(pathByte byte, buf []byte, offset int) (Path, int) {
|
||||
HashSize: hashSize,
|
||||
HashCount: hashCount,
|
||||
Hops: hops,
|
||||
}, totalBytes
|
||||
}, totalBytes, nil
|
||||
}
|
||||
|
||||
// isTransportRoute delegates to packetpath.IsTransportRoute.
|
||||
@@ -300,6 +326,13 @@ func decodeAdvert(buf []byte, validateSignatures bool) Payload {
|
||||
}
|
||||
name := string(appdata[off:nameEnd])
|
||||
name = sanitizeName(name)
|
||||
// Firmware writes the node name into a 32-byte buffer
|
||||
// (MAX_ADVERT_DATA_SIZE, firmware/src/MeshCore.h:11). Truncate
|
||||
// here so adversarial on-wire adverts can't pollute Payload.Name
|
||||
// with bytes firmware would never emit.
|
||||
if len(name) > 32 {
|
||||
name = name[:32]
|
||||
}
|
||||
p.Name = name
|
||||
off = nameEnd
|
||||
// Skip null terminator(s)
|
||||
@@ -584,10 +617,26 @@ func DecodePacket(hexString string, channelKeys map[string]string, validateSigna
|
||||
pathByte := buf[offset]
|
||||
offset++
|
||||
|
||||
path, bytesConsumed := decodePath(pathByte, buf, offset)
|
||||
path, bytesConsumed, decodeErr := decodePath(pathByte, buf, offset)
|
||||
if decodeErr != nil {
|
||||
return nil, decodeErr
|
||||
}
|
||||
offset += bytesConsumed
|
||||
|
||||
// Bounds check: pathByte is wire-supplied (hash_size in upper 2 bits,
|
||||
// hash_count in lower 6 bits → up to 4*63=252 claimed path bytes). A
|
||||
// malformed packet can claim more bytes than the buffer holds — without
|
||||
// this guard `buf[offset:]` panics with `slice bounds out of range
|
||||
// [offset:len(buf)]`. See issue #1211 (prod observed [218:15]).
|
||||
if offset > len(buf) {
|
||||
return nil, fmt.Errorf("packet path length (%d bytes claimed by pathByte 0x%02X) exceeds buffer (%d bytes)", bytesConsumed, pathByte, len(buf))
|
||||
}
|
||||
|
||||
payloadBuf := buf[offset:]
|
||||
// Firmware caps payload at MAX_PACKET_PAYLOAD=184 (firmware/src/MeshCore.h:19).
|
||||
if len(payloadBuf) > maxPacketPayload {
|
||||
return nil, fmt.Errorf("packet payload (%d bytes) exceeds firmware MAX_PACKET_PAYLOAD=%d (MeshCore.h:19)", len(payloadBuf), maxPacketPayload)
|
||||
}
|
||||
payload := decodePayload(header.PayloadType, payloadBuf, channelKeys, validateSignatures)
|
||||
|
||||
// TRACE packets store hop IDs in the payload (buf[9:]) rather than the header
|
||||
|
||||
@@ -0,0 +1,97 @@
|
||||
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
|
||||
@@ -1114,7 +1114,7 @@ func TestDecodePayloadControl(t *testing.T) {
|
||||
|
||||
func TestDecodePathTruncatedBuffer(t *testing.T) {
|
||||
// path byte claims 5 hops of 2 bytes = 10 bytes, but only 4 available
|
||||
path, consumed := decodePath(0x45, []byte{0xAA, 0x11, 0xBB, 0x22}, 0)
|
||||
path, consumed, _ := decodePath(0x45, []byte{0xAA, 0x11, 0xBB, 0x22}, 0)
|
||||
if path.HashCount != 5 {
|
||||
t.Errorf("hashCount=%d, want 5", path.HashCount)
|
||||
}
|
||||
@@ -1708,15 +1708,15 @@ func TestZeroHopTransportDirectHashSize(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestZeroHopTransportDirectHashSizeWithNonZeroUpperBits(t *testing.T) {
|
||||
// TRANSPORT_DIRECT (RouteType=3) + REQ (PayloadType=0) → header byte = 0x03
|
||||
// 4 bytes transport codes + pathByte=0xC0 → hash_count=0, hash_size bits=11 → should still get HashSize=0
|
||||
// pathByte=0xC0 → hash_size bits=11 (4, reserved per firmware Packet.cpp:13-18).
|
||||
// Firmware Packet::isValidPathLen rejects this regardless of hash_count,
|
||||
// because hash_size==4 is reserved. Go decoder must mirror that — even
|
||||
// when hash_count==0, an attacker-emitted 0xC0 byte should not be
|
||||
// silently accepted; firmware never emits hash_size==4.
|
||||
hex := "03" + "11223344" + "C0" + repeatHex("AA", 20)
|
||||
pkt, err := DecodePacket(hex, nil, false)
|
||||
if err != nil {
|
||||
t.Fatalf("DecodePacket failed: %v", err)
|
||||
}
|
||||
if pkt.Path.HashSize != 0 {
|
||||
t.Errorf("TRANSPORT_DIRECT zero-hop with hash_size bits set: want HashSize=0, got %d", pkt.Path.HashSize)
|
||||
_, err := DecodePacket(hex, nil, false)
|
||||
if err == nil {
|
||||
t.Fatalf("DecodePacket(pathByte=0xC0) succeeded; want error mirroring firmware Packet.cpp:13-18 (hash_size==4 reserved)")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1976,3 +1976,107 @@ func TestDecodeTraceExtractsSNRValues(t *testing.T) {
|
||||
t.Errorf("SNRValues[1]=%v, want -2.0", pkt.Payload.SNRValues[1])
|
||||
}
|
||||
}
|
||||
|
||||
// TestDecodePacketBoundsFromWire — regression for issue #1211.
|
||||
//
|
||||
// A malformed packet on the wire claimed pathByte=0xF6 (hash_size=4, hash_count=54
|
||||
// → 216 path bytes) inside a 15-byte buffer. decodePath() returned bytesConsumed=216
|
||||
// without bounds-check, causing the outer slice `payloadBuf := buf[offset:]` to
|
||||
// blow up with `slice bounds out of range [218:15]`.
|
||||
//
|
||||
// Expected behaviour: DecodePacket MUST NOT panic on any input. If the path
|
||||
// length claimed by the wire byte exceeds the buffer, it should return a
|
||||
// clean error.
|
||||
func TestDecodePacketBoundsFromWire_Issue1211(t *testing.T) {
|
||||
// 15-byte buffer: header=0x12 (rt=DIRECT, pt=ADVERT), pathByte=0xF6
|
||||
// (hash_size=4, hash_count=54 → claims 216 path bytes), + 13 garbage bytes.
|
||||
raw := "12F6" + strings.Repeat("AA", 13)
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
t.Fatalf("DecodePacket panicked on malformed input: %v", r)
|
||||
}
|
||||
}()
|
||||
pkt, err := DecodePacket(raw, nil, false)
|
||||
if err == nil {
|
||||
t.Fatalf("expected error for malformed packet (path claims 216 bytes in 15-byte buf), got nil; pkt=%+v", pkt)
|
||||
}
|
||||
}
|
||||
|
||||
// TestDecodePacketFuzzTruncated — sweep the decoder with truncated payloads.
|
||||
// Zero panics is the acceptance bar.
|
||||
//
|
||||
// Adv M2: the original loop ran 256*256*20 = 1.3M iterations on every
|
||||
// `go test` (in both packages, so 2.6M total). That is not "fuzzing" — it
|
||||
// is an expensive deterministic sweep that runs in the default unit-test
|
||||
// path with no opt-in. We now:
|
||||
//
|
||||
// - gate the exhaustive sweep on !testing.Short() so `go test -short`
|
||||
// skips it (CI's unit gate runs short)
|
||||
// - keep the full sweep under `go test ./...` to preserve coverage
|
||||
// - prefer `go test -fuzz=FuzzDecodePacketTruncated` for actual
|
||||
// randomized fuzzing (see FuzzDecodePacketTruncated below)
|
||||
func TestDecodePacketFuzzTruncated_Issue1211(t *testing.T) {
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
t.Fatalf("DecodePacket panicked during fuzz: %v", r)
|
||||
}
|
||||
}()
|
||||
if testing.Short() {
|
||||
t.Skip("skipping exhaustive sweep in -short mode; use FuzzDecodePacketTruncated")
|
||||
}
|
||||
// Sweep every pathByte value with a short tail.
|
||||
for hdr := 0; hdr < 256; hdr++ {
|
||||
for pb := 0; pb < 256; pb++ {
|
||||
for tail := 0; tail < 20; tail++ {
|
||||
raw := hex.EncodeToString([]byte{byte(hdr), byte(pb)}) + strings.Repeat("00", tail)
|
||||
_, _ = DecodePacket(raw, nil, false)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// FuzzDecodePacketTruncated — native go fuzz target. Run with:
|
||||
//
|
||||
// go test -fuzz=FuzzDecodePacketTruncated -fuzztime=30s ./cmd/ingestor
|
||||
//
|
||||
// Zero panics regardless of input is the acceptance bar.
|
||||
func FuzzDecodePacketTruncated(f *testing.F) {
|
||||
seeds := [][]byte{
|
||||
{0x12, 0xF6, 0xAA, 0xAA, 0xAA},
|
||||
{0x12, 0x00},
|
||||
{0x03, 0x11, 0x22, 0x33, 0x44, 0xC0, 0xAA, 0xAA, 0xAA},
|
||||
}
|
||||
for _, s := range seeds {
|
||||
f.Add(s)
|
||||
}
|
||||
f.Fuzz(func(t *testing.T, data []byte) {
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
t.Fatalf("DecodePacket panicked on input %x: %v", data, r)
|
||||
}
|
||||
}()
|
||||
_, _ = DecodePacket(hex.EncodeToString(data), nil, false)
|
||||
})
|
||||
}
|
||||
|
||||
// TestDecodeAdvertOversizedNameTruncated asserts decodeAdvert truncates the
|
||||
// advert name to firmware's MAX_ADVERT_DATA_SIZE=32 (firmware/src/MeshCore.h:11).
|
||||
// Firmware writes the node name into a 32-byte buffer, so any on-wire advert
|
||||
// carrying >32 bytes of name data is adversarial — the Go decoder must not
|
||||
// surface attacker-controlled bytes beyond what firmware would ever emit.
|
||||
func TestDecodeAdvertOversizedNameTruncated(t *testing.T) {
|
||||
pubkey := repeatHex("AA", 32)
|
||||
timestamp := "78563412"
|
||||
signature := repeatHex("BB", 64)
|
||||
flags := "81" // chat(1) | hasName(0x80), no location, no feat1/2
|
||||
// 64-byte ASCII 'X' name with no null terminator (firmware buffer is 32 bytes).
|
||||
name := repeatHex("58", 64)
|
||||
hex := "1200" + pubkey + timestamp + signature + flags + name
|
||||
pkt, err := DecodePacket(hex, nil, false)
|
||||
if err != nil {
|
||||
t.Fatalf("DecodePacket: %v", err)
|
||||
}
|
||||
if got := len(pkt.Payload.Name); got > 32 {
|
||||
t.Errorf("name length=%d, want <=32 (MAX_ADVERT_DATA_SIZE firmware/src/MeshCore.h:11)", got)
|
||||
}
|
||||
}
|
||||
|
||||
+23
-1
@@ -340,7 +340,29 @@ func handleMessage(store *Store, tag string, source MQTTSource, m mqtt.Message,
|
||||
validateSigs := cfg.ShouldValidateSignatures()
|
||||
decoded, err := DecodePacket(rawHex, channelKeys, validateSigs)
|
||||
if err != nil {
|
||||
log.Printf("MQTT [%s] decode error: %v", tag, err)
|
||||
// Per #1211: include enough context to repro malformed-packet drops,
|
||||
// but NEVER log the full observer ID (PII / fingerprinting risk).
|
||||
// We log:
|
||||
// - topic prefix (with observer segment elided)
|
||||
// - 8-char observer prefix
|
||||
// - payload length, claimed length (rawHex len)
|
||||
obs := ""
|
||||
if len(parts) > 2 {
|
||||
obs = parts[2]
|
||||
}
|
||||
// Build a redacted topic that replaces parts[2] (the observer id)
|
||||
// with the 8-char prefix, so the rest of the topic is preserved
|
||||
// for debugging without leaking the full identifier.
|
||||
redactedTopic := topic
|
||||
if len(parts) > 2 {
|
||||
redactedParts := make([]string, len(parts))
|
||||
copy(redactedParts, parts)
|
||||
if len(parts[2]) > 8 {
|
||||
redactedParts[2] = parts[2][:8]
|
||||
}
|
||||
redactedTopic = strings.Join(redactedParts, "/")
|
||||
}
|
||||
log.Printf("MQTT [%s] decode error: %v (topic=%s observer=%.8s rawHexLen=%d)", tag, err, redactedTopic, obs, len(rawHex))
|
||||
return
|
||||
}
|
||||
|
||||
|
||||
+49
-3
@@ -144,9 +144,35 @@ func decodeHeader(b byte) Header {
|
||||
}
|
||||
}
|
||||
|
||||
func decodePath(pathByte byte, buf []byte, offset int) (Path, int) {
|
||||
// Firmware-derived limits — see firmware/src/MeshCore.h:19,21.
|
||||
const (
|
||||
maxPathSize = 64 // MAX_PATH_SIZE — total path bytes allowed
|
||||
maxPacketPayload = 184 // MAX_PACKET_PAYLOAD — max raw payload bytes
|
||||
)
|
||||
|
||||
// isValidPathLen mirrors firmware Packet::isValidPathLen
|
||||
// (firmware/src/Packet.cpp:13-18). hash_size==4 is reserved; total path bytes
|
||||
// must fit within MAX_PATH_SIZE.
|
||||
func isValidPathLen(pathByte byte) bool {
|
||||
hashCount := int(pathByte & 0x3F)
|
||||
hashSize := int(pathByte>>6) + 1
|
||||
if hashSize == 4 {
|
||||
return false // reserved
|
||||
}
|
||||
return hashCount*hashSize <= maxPathSize
|
||||
}
|
||||
|
||||
func decodePath(pathByte byte, buf []byte, offset int) (Path, int, error) {
|
||||
hashSize := int(pathByte>>6) + 1
|
||||
hashCount := int(pathByte & 0x3F)
|
||||
// Exact mirror of firmware Packet::isValidPathLen (Packet.cpp:13-18).
|
||||
// hash_size==4 is reserved and is rejected by firmware regardless of
|
||||
// hash_count, so we must reject 0xC0 etc even on zero-hop packets —
|
||||
// firmware never emits them, so an on-wire pathByte with the upper
|
||||
// 2 bits set to 11 is by definition malformed/adversarial.
|
||||
if !isValidPathLen(pathByte) {
|
||||
return Path{}, 0, fmt.Errorf("invalid path encoding: pathByte 0x%02X (hash_size=%d hash_count=%d) violates firmware validity (Packet.cpp:13-18, MAX_PATH_SIZE=%d)", pathByte, hashSize, hashCount, maxPathSize)
|
||||
}
|
||||
totalBytes := hashSize * hashCount
|
||||
hops := make([]string, 0, hashCount)
|
||||
|
||||
@@ -163,7 +189,7 @@ func decodePath(pathByte byte, buf []byte, offset int) (Path, int) {
|
||||
HashSize: hashSize,
|
||||
HashCount: hashCount,
|
||||
Hops: hops,
|
||||
}, totalBytes
|
||||
}, totalBytes, nil
|
||||
}
|
||||
|
||||
// isTransportRoute delegates to packetpath.IsTransportRoute.
|
||||
@@ -261,6 +287,13 @@ func decodeAdvert(buf []byte, validateSignatures bool) Payload {
|
||||
name := string(appdata[off:])
|
||||
name = strings.TrimRight(name, "\x00")
|
||||
name = sanitizeName(name)
|
||||
// Firmware writes the node name into a 32-byte buffer
|
||||
// (MAX_ADVERT_DATA_SIZE, firmware/src/MeshCore.h:11). Truncate
|
||||
// here so adversarial on-wire adverts can't pollute Payload.Name
|
||||
// with bytes firmware would never emit.
|
||||
if len(name) > 32 {
|
||||
name = name[:32]
|
||||
}
|
||||
p.Name = name
|
||||
}
|
||||
}
|
||||
@@ -385,10 +418,23 @@ func DecodePacket(hexString string, validateSignatures bool) (*DecodedPacket, er
|
||||
pathByte := buf[offset]
|
||||
offset++
|
||||
|
||||
path, bytesConsumed := decodePath(pathByte, buf, offset)
|
||||
path, bytesConsumed, decodeErr := decodePath(pathByte, buf, offset)
|
||||
if decodeErr != nil {
|
||||
return nil, decodeErr
|
||||
}
|
||||
offset += bytesConsumed
|
||||
|
||||
// Bounds check — see cmd/ingestor/decoder.go for full rationale (#1211).
|
||||
if offset > len(buf) {
|
||||
return nil, fmt.Errorf("packet path length (%d bytes claimed by pathByte 0x%02X) exceeds buffer (%d bytes)", bytesConsumed, pathByte, len(buf))
|
||||
}
|
||||
|
||||
payloadBuf := buf[offset:]
|
||||
// Firmware caps payload at MAX_PACKET_PAYLOAD=184 (firmware/src/MeshCore.h:19).
|
||||
// Anything larger cannot be a valid wire packet — drop it.
|
||||
if len(payloadBuf) > maxPacketPayload {
|
||||
return nil, fmt.Errorf("packet payload (%d bytes) exceeds firmware MAX_PACKET_PAYLOAD=%d (MeshCore.h:19)", len(payloadBuf), maxPacketPayload)
|
||||
}
|
||||
payload := decodePayload(header.PayloadType, payloadBuf, validateSignatures)
|
||||
|
||||
// TRACE packets store hop IDs in the payload (buf[9:]) rather than the header
|
||||
|
||||
@@ -0,0 +1,140 @@
|
||||
package main
|
||||
|
||||
import (
|
||||
"encoding/hex"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// --- Issue #1211 round-1 protocol-correctness regressions ---
|
||||
//
|
||||
// Background: the round-0 PR added a bounds check on `offset > len(buf)` AFTER
|
||||
// decodePath returned, but did NOT enforce the firmware-level validity rules
|
||||
// for pathByte:
|
||||
//
|
||||
// firmware/src/Packet.cpp:13-18 — isValidPathLen():
|
||||
// hash_count = path_len & 63
|
||||
// hash_size = (path_len >> 6) + 1
|
||||
// hash_size == 4 is RESERVED — invalid
|
||||
// hash_count * hash_size MUST be <= MAX_PATH_SIZE (64) [MeshCore.h:21]
|
||||
//
|
||||
// firmware/src/MeshCore.h:19 — MAX_PACKET_PAYLOAD = 184
|
||||
//
|
||||
// A malformed pathByte=0xF6 (hash_size=4, hash_count=54) inside a buffer LARGE
|
||||
// ENOUGH to hold 216 path bytes would slip past the round-0 bounds check and
|
||||
// pollute analytics with a bogus "decoded" packet. Similarly, payloads
|
||||
// exceeding MAX_PACKET_PAYLOAD should be rejected — firmware would never
|
||||
// produce them on the wire.
|
||||
|
||||
// TestDecodePacketRejectsReservedHashSize_Issue1211 — pathByte=0xF6:
|
||||
// hash_size = (0xF6 >> 6) + 1 = 3 + 1 = 4 ← RESERVED per firmware
|
||||
// hash_count = 0xF6 & 0x3F = 54
|
||||
// total path bytes = 4 * 54 = 216
|
||||
// We provide a buffer with 216 path bytes available, so the round-0 OOB
|
||||
// guard PASSES — only the firmware-derived isValidPathLen check catches this.
|
||||
func TestDecodePacketRejectsReservedHashSize_Issue1211(t *testing.T) {
|
||||
// header (1) + pathByte (1) + 216 path bytes + 8-byte payload = 226 bytes
|
||||
raw := "12F6" + strings.Repeat("AB", 216) + strings.Repeat("CD", 8)
|
||||
pkt, err := DecodePacket(raw, 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)
|
||||
}
|
||||
}
|
||||
|
||||
// TestDecodePacketRejectsOversizedPath_Issue1211 — pathByte=0xBF:
|
||||
// hash_size = (0xBF >> 6) + 1 = 2 + 1 = 3
|
||||
// hash_count = 0xBF & 0x3F = 63
|
||||
// total path bytes = 3 * 63 = 189 > MAX_PATH_SIZE (64) ← INVALID per firmware
|
||||
// Buffer holds all 189 path bytes — only the firmware check rejects.
|
||||
func TestDecodePacketRejectsOversizedPath_Issue1211(t *testing.T) {
|
||||
raw := "12BF" + strings.Repeat("AB", 189) + strings.Repeat("CD", 8)
|
||||
pkt, err := DecodePacket(raw, false)
|
||||
if err == nil {
|
||||
t.Fatalf("expected error rejecting hash_count*hash_size > 64 (firmware Packet.cpp:13-18); got nil, pkt=%+v", pkt)
|
||||
}
|
||||
}
|
||||
|
||||
// TestDecodePacketRejectsOversizedPayload_Issue1211 — payload exceeds
|
||||
// MAX_PACKET_PAYLOAD (184). Firmware (MeshCore.h:19) cannot emit such a
|
||||
// packet on the wire; the decoder should drop it rather than emit a bogus
|
||||
// "successfully decoded" record.
|
||||
func TestDecodePacketRejectsOversizedPayload_Issue1211(t *testing.T) {
|
||||
// hash_size=1, hash_count=0 → no path bytes, then 200-byte payload
|
||||
// header=0x12 (DIRECT/ADVERT), pathByte=0x00 → no hops, then payload
|
||||
// payload length 200 > 184 → must reject
|
||||
raw := "1200" + strings.Repeat("AA", 200)
|
||||
pkt, err := DecodePacket(raw, 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)
|
||||
}
|
||||
}
|
||||
|
||||
// TestDecodePath_RejectsReservedHashSize_Issue1211 — adversarial M1: push the
|
||||
// invalid-path check INTO decodePath so callers can't accidentally rely on the
|
||||
// downstream OOB guard. Direct unit test on decodePath.
|
||||
func TestDecodePath_RejectsReservedHashSize_Issue1211(t *testing.T) {
|
||||
// Plenty of buffer — 216 bytes — so the *test* doesn't hide the check
|
||||
// behind an OOB failure.
|
||||
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) {
|
||||
// hash_size=1, hash_count=5 → 5 path bytes — valid per firmware.
|
||||
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)
|
||||
}
|
||||
}
|
||||
|
||||
// Pin the round-0 tautological assertion. Specific error phrasing required.
|
||||
// (Kent #1) — `TestDecodePacketBoundsFromWire_Issue1211` used to assert only
|
||||
// `err != nil`; a generic recover would have passed. Now must contain
|
||||
// "path length" AND "exceeds buffer".
|
||||
//
|
||||
// Use pathByte=0x0A (hash_size=1, hash_count=10) — firmware-VALID encoding
|
||||
// that claims 10 path bytes; buffer only has 5 → the OOB guard fires (not
|
||||
// the validity check). This pins the OOB error string specifically.
|
||||
func TestDecodePacketBoundsFromWireErrorPhrasing_Issue1211(t *testing.T) {
|
||||
raw := "120A" + strings.Repeat("AA", 5)
|
||||
_, err := DecodePacket(raw, 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)
|
||||
}
|
||||
}
|
||||
|
||||
// silence unused import in case of future trimming
|
||||
var _ = hex.EncodeToString
|
||||
@@ -140,15 +140,15 @@ func TestZeroHopTransportDirectHashSize(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestZeroHopTransportDirectHashSizeWithNonZeroUpperBits(t *testing.T) {
|
||||
// TRANSPORT_DIRECT (RouteType=3) + REQ (PayloadType=0) → header byte = 0x03
|
||||
// 4 bytes transport codes + pathByte=0xC0 → hash_count=0, hash_size bits=11 → should still get HashSize=0
|
||||
// pathByte=0xC0 → hash_size bits=11 (4, reserved per firmware Packet.cpp:13-18).
|
||||
// Firmware Packet::isValidPathLen rejects this regardless of hash_count,
|
||||
// because hash_size==4 is reserved. Go decoder must mirror that — even
|
||||
// when hash_count==0, an attacker-emitted 0xC0 byte should not be
|
||||
// silently accepted; firmware never emits hash_size==4.
|
||||
hex := "03" + "11223344" + "C0" + repeatHex("AA", 20)
|
||||
pkt, err := DecodePacket(hex, false)
|
||||
if err != nil {
|
||||
t.Fatalf("DecodePacket failed: %v", err)
|
||||
}
|
||||
if pkt.Path.HashSize != 0 {
|
||||
t.Errorf("TRANSPORT_DIRECT zero-hop with hash_size bits set: want HashSize=0, got %d", pkt.Path.HashSize)
|
||||
_, err := DecodePacket(hex, false)
|
||||
if err == nil {
|
||||
t.Fatalf("DecodePacket(pathByte=0xC0) succeeded; want error mirroring firmware Packet.cpp:13-18 (hash_size==4 reserved)")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -488,3 +488,89 @@ func TestDecodePacket_TraceNoSNRValues(t *testing.T) {
|
||||
t.Errorf("expected empty SNRValues, got %v", pkt.Payload.SNRValues)
|
||||
}
|
||||
}
|
||||
|
||||
// TestDecodePacketBoundsFromWire_Issue1211 — mirror of ingestor test.
|
||||
// Malformed pathByte=0xF6 inside a 15-byte buffer triggered
|
||||
// `slice bounds out of range [218:15]`.
|
||||
func TestDecodePacketBoundsFromWire_Issue1211(t *testing.T) {
|
||||
raw := "12F6"
|
||||
for i := 0; i < 13; i++ {
|
||||
raw += "AA"
|
||||
}
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
t.Fatalf("DecodePacket panicked on malformed input: %v", r)
|
||||
}
|
||||
}()
|
||||
pkt, err := DecodePacket(raw, false)
|
||||
if err == nil {
|
||||
t.Fatalf("expected error for malformed packet, got nil; pkt=%+v", pkt)
|
||||
}
|
||||
}
|
||||
|
||||
// Adv M2: see cmd/ingestor/decoder_test.go — sweep gated on !testing.Short();
|
||||
// FuzzDecodePacketTruncated below is the real fuzzing target.
|
||||
func TestDecodePacketFuzzTruncated_Issue1211(t *testing.T) {
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
t.Fatalf("DecodePacket panicked during fuzz: %v", r)
|
||||
}
|
||||
}()
|
||||
if testing.Short() {
|
||||
t.Skip("skipping exhaustive sweep in -short mode; use FuzzDecodePacketTruncated")
|
||||
}
|
||||
for hdr := 0; hdr < 256; hdr++ {
|
||||
for pb := 0; pb < 256; pb++ {
|
||||
for tail := 0; tail < 20; tail++ {
|
||||
raw := hex.EncodeToString([]byte{byte(hdr), byte(pb)})
|
||||
for i := 0; i < tail; i++ {
|
||||
raw += "00"
|
||||
}
|
||||
_, _ = DecodePacket(raw, false)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// FuzzDecodePacketTruncated — native go fuzz target. Zero panics required.
|
||||
// Run with: go test -fuzz=FuzzDecodePacketTruncated -fuzztime=30s ./cmd/server
|
||||
func FuzzDecodePacketTruncated(f *testing.F) {
|
||||
seeds := [][]byte{
|
||||
{0x12, 0xF6, 0xAA, 0xAA, 0xAA},
|
||||
{0x12, 0x00},
|
||||
{0x03, 0x11, 0x22, 0x33, 0x44, 0xC0, 0xAA, 0xAA, 0xAA},
|
||||
}
|
||||
for _, s := range seeds {
|
||||
f.Add(s)
|
||||
}
|
||||
f.Fuzz(func(t *testing.T, data []byte) {
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
t.Fatalf("DecodePacket panicked on input %x: %v", data, r)
|
||||
}
|
||||
}()
|
||||
_, _ = DecodePacket(hex.EncodeToString(data), false)
|
||||
})
|
||||
}
|
||||
|
||||
// TestDecodeAdvertOversizedNameTruncated asserts decodeAdvert truncates the
|
||||
// advert name to firmware's MAX_ADVERT_DATA_SIZE=32 (firmware/src/MeshCore.h:11).
|
||||
// Firmware writes the node name into a 32-byte buffer, so any on-wire advert
|
||||
// carrying >32 bytes of name data is adversarial — the Go decoder must not
|
||||
// surface attacker-controlled bytes beyond what firmware would ever emit.
|
||||
func TestDecodeAdvertOversizedNameTruncated(t *testing.T) {
|
||||
pubkey := repeatHex("AA", 32)
|
||||
timestamp := "78563412"
|
||||
signature := repeatHex("BB", 64)
|
||||
flags := "81" // chat(1) | hasName(0x80), no location, no feat1/2
|
||||
// 64-byte ASCII 'X' name (firmware buffer is only 32 bytes).
|
||||
name := repeatHex("58", 64)
|
||||
hex := "1200" + pubkey + timestamp + signature + flags + name
|
||||
pkt, err := DecodePacket(hex, false)
|
||||
if err != nil {
|
||||
t.Fatalf("DecodePacket: %v", err)
|
||||
}
|
||||
if got := len(pkt.Payload.Name); got > 32 {
|
||||
t.Errorf("name length=%d, want <=32 (MAX_ADVERT_DATA_SIZE firmware/src/MeshCore.h:11)", got)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user