More fuzz tests and small fixes for the findings (#10864)
* first pass tests * more tests * Fix two crafted-admin-packet crashes found by the E5 fuzzer Both are reachable from an authorized admin (local from==0, admin channel, or PKC) - remote DoS: 1. SIGFPE in LoRa config validation. A set_config LoRaConfig with use_preset=false and bandwidth=0 makes freqSlotWidth 0, so numFreqSlots is 0 and `hash(name) % numFreqSlots` (RadioInterface.cpp) divides by zero. Guard the modulo; the existing channel_num check then rejects/ clamps the config. 2. Stack overflow in Channels::getKey. A SECONDARY channel at the primary slot with an empty PSK recursed into getKey(primaryIndex) forever. Skip the primary-key borrow when chIndex == primaryIndex. Re-enable the E5 admin fuzzer to hit both triggers again (use_preset both ways incl. bandwidth 0, plus the set_channel tag) as regression guards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Correct fuzz-test invariants after the crash fixes - E5 admin fuzz: node eviction under a filling NodeDB is legitimate, so assert only the bounded-count invariant, not that a specific seed node survives 6000 mutating ops. - TMM blitz: scope off the nodeinfo direct-response send path (it needs a fully-wired MeshService/phone queue the fixture doesn't provide; the deterministic directResponse tests cover it). The crafted-nodenum rate/unknown/position cache stress is unchanged. clod helped too * realistic tests * test: dedup fuzz RNG into shared test/support/DeterministicRng.h The four in-tree fuzz suites (test_fuzz_decode, test_fuzz_packets, test_hop_scaling, test_traffic_management) each carried a byte-identical copy of the seeded 64-bit LCG (rngSeed/rngNext/rngByte/rngRange). Hoist it into one shared header so there is a single generator to reason about and no risk of the copies drifting. static inline keeps per-suite state per translation unit and avoids -Wunused-function for suites that don't use every helper. Also corrects a stale comment in test_traffic_management (the blitz's nodeinfo direct-response path is intentionally left off). No behavioral change: same constants, same per-suite seeds. clod helped too * test: fuzz uncovered ProtobufModule handlers and the MQTT downlink ingress Extend the in-tree fuzz coverage to packet sources that previously had none: - test_fuzz_packets E8/E9/E10: drive PositionModule, DeviceTelemetryModule and NeighborInfoModule at handleReceivedProtobuf directly (via using-shims, bypassing the ProtobufModule reply/send path so no router is needed). The fixture already stands up nodeDB/service/channels, and nodeStatus/powerStatus are auto-initialized in main.cpp, so no new globals are required. Adds a shared fuzzRxHeader() helper for crafting adversarial RX packet headers. - test_fuzz_decode: add meshtastic_KeyVerification to the decode table. The KeyVerification and StoreForward handler paths are documented as decode-level only, with the concrete reason each is intrinsic (private-state gating / PSRAM + self-pointer wiring), not a fixture gap. - test_mqtt: test_receiveFuzzServiceEnvelope blitzes the non-RF broker-push ingress (onReceiveProto) two ways - raw garbage bytes that must fail envelope decode cleanly, and a well-formed ServiceEnvelope wrapping a crafted inner MeshPacket over crafted channel_id/gateway_id - exercising the channel match, isFromUs, XEdDSA receive policy and perhapsDecode chain. Adds a deliverRaw() passthrough to MQTTUnitTest. All under the coverage env (ASan/LSan). No firmware/src changes. Full sweep GREEN 27/27, 544 cases. clod helped too * Harden LoRa/channel config against crafted admin messages; consolidate test helpers Production (review findings on the hot-fuzz crash fixes): - Clamp bandwidth at the source (clampBandwidthKHz) in checkOrClampConfigLora and applyModemConfig so numFreqSlots can never be 0 for any consumer; a bandwidth-0 set_config previously passed validation and re-armed the SIGFPE on the next applyModemConfig. - Guard applyModemConfig's hash % numFreqSlots (the validator's sibling modulo was fixed earlier but this one was still unguarded). - Enforce the primary-channel invariant in Channels::onConfigChanged: a config demoting every slot now re-promotes the stale SECONDARY slot (keeping its key) or restores the default channel if the slot is DISABLED, instead of leaving every getPrimaryIndex() reader on a non-primary slot. The getKey recursion guard stays as defense-in-depth. Tests: - New test/support/MockMeshService.h and AdminModuleTestShim.h replace four byte-identical mocks and three divergent admin shims (test_mqtt's capturing mock is genuinely different and stays). - DeterministicRng.h: add rngFill() (replaces 14 hand-rolled fill loops) and rngEdgeNodeNum() (unifies the three NodeNum boundary pools). - Extract fuzzChannelSettings() shared by the set_channel case and fuzzBeacon. - fuzzBeacon: the un-terminated branch now fills the whole buffer with non-NUL bytes so the strnlen bound is actually stressed (~50% of iterations, not ~4%). - E6 beacon fuzz: replace the TEST_ASSERT_TRUE(true) tautology with real invariants (handler never consumes; offers land in lastReceivedOffer keyed to the sender). - Trim the seven over-long comment blocks flagged against the 1-2 line rule; the FINDINGS trailer moves to this commit message (see production notes). Full native suite GREEN 27/27 under the coverage (ASan/LSan) env. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Clamp UTF-8 char length in the emote walkers; add test_fuzz_emotes A TEXT_MESSAGE payload is opaque protobuf bytes, so PB_VALIDATE_UTF8 never screens it - invalid UTF-8 and truncated multi-byte lead bytes reach the emote/width render path verbatim. EmoteRenderer's walkers advanced by utf8CharLen(lead) without clamping to the bytes actually remaining, so a truncated lead (e.g. a lone 0xF0, which claims 4 bytes) near the end of the buffer made getUtf8ChunkWidth's memcpy read past the string. ASan confirms a heap-buffer-overflow READ from measureStringWithEmotes. Add utf8CharLenClamped() and use it at every walk site (width measure, truncation cut-loop, and the draw-path text-run/chunk builders); the one already-guarded site (matchAtIgnoringModifiers) is unchanged. New test/test_fuzz_emotes drives measureStringWithEmotes and truncateToWidth over adversarial byte strings (biased to embed/end in truncated multi-byte leads) in exact-sized heap buffers so any over-read is a hard ASan fault. Its headless display uses a synthetic font (firstChar 0, fontData centered in a large buffer) so the stock OLEDDisplay::getStringWidth - which indexes the font jump table with a signed char and over-reads for any byte >= 0x80 - does not mask the finding. native-suite-count bumped 27 -> 28. Full native suite GREEN 28/28 under the coverage (ASan/LSan) env. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Keep emote width measurement in-bounds for non-ASCII bytes OLEDDisplay::getStringWidth (the utf8=false path EmoteRenderer uses on default builds) indexes the font jump table by (c - firstChar) with a signed char and no bounds check, so any byte outside printable ASCII - high bytes from UTF-8 text, but also a stray control byte like 0x0A - reads outside the font array. On-device this reads adjacent flash and returns a garbage width; under ASan the test_fuzz_emotes fuzzer flags it as a global-buffer-overflow, and it made the non-ASCII width measurement meaningless either way. The OLED driver is a pinned upstream dependency, so guard it firmware-side in EmoteRenderer's getStringWidth helper: measure a sanitized copy where any byte outside [0x20, 0x7E] counts as a '?' placeholder. Printable ASCII is unchanged and the UA/RU lookup path is untouched. test_fuzz_emotes now drives a real ArialMT font instead of the synthetic in-bounds font it needed before this fix, so the suite exercises the true production width path (utf8CharLen clamp + this sanitizer) end to end. The same fuzzer tripped the global-buffer-overflow before this change. Full native suite GREEN 28/28 under the coverage (ASan/LSan) env. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Tom
co-authored by
GitHub
Claude Fable 5
GitHub
Claude Fable 5
parent
6c7ee8afc7
commit
d846780a9b
@@ -12,6 +12,8 @@
|
||||
#include "mqtt/MQTT.h"
|
||||
#include "mqtt/ServiceEnvelope.h"
|
||||
|
||||
#include "support/DeterministicRng.h" // rngSeed/rngNext/rngByte/rngRange - shared seeded LCG (fuzz group)
|
||||
|
||||
#include <PubSubClient.h>
|
||||
#include <WiFiClient.h>
|
||||
|
||||
@@ -275,6 +277,13 @@ class MQTTUnitTest : public MQTT
|
||||
size_t numBytes = pb_encode_to_bytes(bytes, sizeof(bytes), &meshtastic_ServiceEnvelope_msg, &env);
|
||||
mqttCallback(const_cast<char *>(topic.str().c_str()), bytes, numBytes);
|
||||
}
|
||||
// Feed arbitrary bytes straight into the subscription callback - the non-RF ingress a malicious or
|
||||
// broken broker could push. Mirrors publish()'s final mqttCallback() call but with an unconstrained
|
||||
// payload, so it exercises DecodedServiceEnvelope decode + onReceiveProto with garbage.
|
||||
void deliverRaw(const std::string &topic, const uint8_t *bytes, size_t n)
|
||||
{
|
||||
mqttCallback(const_cast<char *>(topic.c_str()), const_cast<uint8_t *>(bytes), (unsigned int)n);
|
||||
}
|
||||
static void restart()
|
||||
{
|
||||
if (mqtt != NULL) {
|
||||
@@ -963,6 +972,65 @@ void test_configWithTLSEnabled(void)
|
||||
#endif
|
||||
}
|
||||
|
||||
// ===========================================================================
|
||||
// Fuzz - adversarial MQTT downlink ingress (the non-RF path a broker can push)
|
||||
// ===========================================================================
|
||||
// Blitzes the onReceiveProto() chain with (a) raw garbage that must fail envelope decode cleanly and
|
||||
// (b) well-formed envelopes wrapping crafted inner packets. Contract: no crash, at most one enqueue per envelope.
|
||||
constexpr uint64_t MQTT_FUZZ_SEED = 0x00E3A71C0FULL;
|
||||
|
||||
void test_receiveFuzzServiceEnvelope(void)
|
||||
{
|
||||
printf(" seed=0x%llx\n", (unsigned long long)MQTT_FUZZ_SEED);
|
||||
rngSeed(MQTT_FUZZ_SEED);
|
||||
|
||||
const char *channelIds[] = {"test", "PKI", "nope", ""};
|
||||
const char *gatewayIds[] = {"!12345678", "!87654321", "!00000000"}; // [0] == our node id -> self path
|
||||
|
||||
for (unsigned k = 0; k < 4000; k++) {
|
||||
if (rngRange(3) == 0) {
|
||||
// (a) Raw bytes: mostly random, must be rejected at DecodedServiceEnvelope without crashing.
|
||||
uint8_t raw[128];
|
||||
size_t n = rngRange(sizeof(raw) + 1);
|
||||
rngFill(raw, n);
|
||||
unitTest->deliverRaw("msh/2/e/test/!87654321", raw, n);
|
||||
} else {
|
||||
// (b) Well-formed envelope around a crafted inner packet.
|
||||
meshtastic_MeshPacket p = meshtastic_MeshPacket_init_zero;
|
||||
p.from = (rngRange(3) == 0) ? myNodeInfo.my_node_num : rngNext(); // self origin hits isFromUs
|
||||
p.to = (rngRange(2)) ? myNodeInfo.my_node_num : rngNext();
|
||||
p.id = rngNext();
|
||||
p.channel = (uint8_t)rngByte();
|
||||
p.hop_limit = (uint8_t)rngRange(10); // includes > HOP_MAX (the invalid-hop reject path)
|
||||
p.hop_start = (uint8_t)rngRange(10);
|
||||
p.want_ack = (rngRange(2) == 0);
|
||||
p.pki_encrypted = (rngRange(2) == 0);
|
||||
if (rngRange(2) == 0) {
|
||||
p.which_payload_variant = meshtastic_MeshPacket_decoded_tag;
|
||||
p.decoded.portnum = (rngRange(6) == 0) ? meshtastic_PortNum_ADMIN_APP : (meshtastic_PortNum)rngRange(80);
|
||||
p.decoded.want_response = (rngRange(2) == 0);
|
||||
p.decoded.has_bitfield = (rngRange(2) == 0);
|
||||
p.decoded.bitfield = (uint32_t)rngNext();
|
||||
p.decoded.payload.size = rngRange(64);
|
||||
rngFill(p.decoded.payload.bytes, p.decoded.payload.size);
|
||||
} else {
|
||||
p.which_payload_variant = meshtastic_MeshPacket_encrypted_tag;
|
||||
p.encrypted.size = rngRange(64);
|
||||
rngFill(p.encrypted.bytes, p.encrypted.size);
|
||||
}
|
||||
unitTest->publish(&p, gatewayIds[rngRange(3)], channelIds[rngRange(4)]);
|
||||
}
|
||||
|
||||
// A single envelope reaches at most one enqueueReceivedMessage; more would be a routing bug.
|
||||
TEST_ASSERT_TRUE_MESSAGE(mockRouter->packets_.size() <= 1, "MQTT downlink enqueued >1 packet per envelope");
|
||||
// Drain capture lists so 4000 iterations stay bounded (values already released to their pools).
|
||||
mockRouter->packets_.clear();
|
||||
mockRoutingModule->ackNacks_.clear();
|
||||
mockMeshService->messages_.clear();
|
||||
mockMeshService->notifications_.clear();
|
||||
}
|
||||
}
|
||||
|
||||
void setup()
|
||||
{
|
||||
initializeTestEnvironment();
|
||||
@@ -998,6 +1066,7 @@ void setup()
|
||||
#endif
|
||||
RUN_TEST(test_receiveIgnoresUnexpectedFields);
|
||||
RUN_TEST(test_receiveIgnoresInvalidHopLimit);
|
||||
RUN_TEST(test_receiveFuzzServiceEnvelope);
|
||||
RUN_TEST(test_publishTextMessageDirect);
|
||||
RUN_TEST(test_publishTextMessageWithProxy);
|
||||
RUN_TEST(test_reportToMapDefaultImprecise);
|
||||
|
||||
Reference in New Issue
Block a user