diff --git a/cmd/ingestor/decode_error_log_test.go b/cmd/ingestor/decode_error_log_test.go new file mode 100644 index 00000000..909154a3 --- /dev/null +++ b/cmd/ingestor/decode_error_log_test.go @@ -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) + } +} diff --git a/cmd/ingestor/decoder.go b/cmd/ingestor/decoder.go index 8821d828..7b8f0941 100644 --- a/cmd/ingestor/decoder.go +++ b/cmd/ingestor/decoder.go @@ -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 diff --git a/cmd/ingestor/decoder_bounds_test.go b/cmd/ingestor/decoder_bounds_test.go new file mode 100644 index 00000000..071d9980 --- /dev/null +++ b/cmd/ingestor/decoder_bounds_test.go @@ -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 diff --git a/cmd/ingestor/decoder_test.go b/cmd/ingestor/decoder_test.go index 92e1c5db..779a9062 100644 --- a/cmd/ingestor/decoder_test.go +++ b/cmd/ingestor/decoder_test.go @@ -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) + } +} diff --git a/cmd/ingestor/main.go b/cmd/ingestor/main.go index 391a6c79..4beae31f 100644 --- a/cmd/ingestor/main.go +++ b/cmd/ingestor/main.go @@ -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 } diff --git a/cmd/server/decoder.go b/cmd/server/decoder.go index 24c62af5..2e004002 100644 --- a/cmd/server/decoder.go +++ b/cmd/server/decoder.go @@ -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 diff --git a/cmd/server/decoder_bounds_test.go b/cmd/server/decoder_bounds_test.go new file mode 100644 index 00000000..be83313d --- /dev/null +++ b/cmd/server/decoder_bounds_test.go @@ -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 diff --git a/cmd/server/decoder_test.go b/cmd/server/decoder_test.go index caf016d9..5d96deac 100644 --- a/cmd/server/decoder_test.go +++ b/cmd/server/decoder_test.go @@ -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) + } +}