diff --git a/src/helpers/MQTTWireScratch.h b/src/helpers/MQTTWireScratch.h index c631cdf1..d5c79336 100644 --- a/src/helpers/MQTTWireScratch.h +++ b/src/helpers/MQTTWireScratch.h @@ -27,11 +27,19 @@ static_assert(1 + 4 + 1 + MAX_PATH_SIZE + MAX_PACKET_PAYLOAD <= MAX_TRANS_UNIT, // corrupt value can still leave getRawLength() inside MAX_TRANS_UNIT. // - path_len is written into a single wire byte, so anything above 255 is silently // truncated and would disagree with getPathByteLen(). -// The path bytes themselves need no check here: writePath() already refuses a -// getPathByteLen() above MAX_PATH_SIZE. +// - the path encoding must be one writePath() will actually emit. It self-guards +// against overrunning the path array, but by writing nothing and returning 0, which +// is a correctness problem rather than a safety one: getRawLength() still counts the +// path, so an over-long or reserved encoding passes a destination-size check and +// then serializes to a truncated frame that gets published as the packet. The worst +// case is path_len 0xFF with no payload — 254 counted bytes, 2 bytes emitted. +// isValidPathLen() rejects both the reserved 4-byte hash size and any +// count * size above MAX_PATH_SIZE, and is the same predicate Packet::readFrom() +// applies to every received packet, so no decodable packet is turned away. inline bool canSerialize(const mesh::Packet& packet, size_t dest_size) { if (packet.payload_len > MAX_PACKET_PAYLOAD) return false; if (packet.path_len > 0xFF) return false; + if (!mesh::Packet::isValidPathLen((uint8_t)packet.path_len)) return false; const int raw_len = packet.getRawLength(); return raw_len > 0 && (size_t)raw_len <= dest_size; } diff --git a/test/test_mqtt_wire_scratch/test_mqtt_wire_scratch.cpp b/test/test_mqtt_wire_scratch/test_mqtt_wire_scratch.cpp index b78e23bd..d8258aea 100644 --- a/test/test_mqtt_wire_scratch/test_mqtt_wire_scratch.cpp +++ b/test/test_mqtt_wire_scratch/test_mqtt_wire_scratch.cpp @@ -60,11 +60,42 @@ TEST(MQTTWireScratch, RejectsPathLenThatWouldTruncateIntoOneWireByte) { p.payload_len = 4; p.path_len = 0x100; // writeTo() stores this in a single byte EXPECT_FALSE(MQTTWireScratch::canSerialize(p, MQTTWireScratch::kWireBytes)); +} +// The case a destination-size check cannot catch, and which an earlier version of +// these tests masked by using a payload big enough to push getRawLength() over the +// limit: 0xFF encodes 63 hops of 4 bytes, so with no payload the counted length is +// 254 — inside the buffer — while writePath() refuses the 252-byte path and writeTo() +// emits only the 2-byte header. Publishing that would put 4 hex chars in the `raw` +// field and call them the packet. +TEST(MQTTWireScratch, RejectsOverlongPathEvenWhenTheCountedLengthFits) { + mesh::Packet p; + p.header = ROUTE_TYPE_FLOOD; p.path_len = 0xFF; - // Still rejected, but now on the destination check rather than truncation: - // 0xFF encodes 63 hops of 4 bytes. - EXPECT_GT(p.getRawLength(), (int)MQTTWireScratch::kWireBytes); + p.payload_len = 0; + + ASSERT_EQ(254, p.getRawLength()); + ASSERT_LE((size_t)p.getRawLength(), MQTTWireScratch::kWireBytes); + uint8_t buf[MQTTWireScratch::kWireBytes]; + ASSERT_EQ(2, (int)p.writeTo(buf)); + + EXPECT_FALSE(MQTTWireScratch::canSerialize(p, MQTTWireScratch::kWireBytes)); +} + +// hash_size 4 is reserved: isValidPathLen() and therefore Packet::readFrom() reject +// it, so serializing one produces a frame no receiver can parse back — even though the +// hop bytes fit and writePath() copies them happily. +TEST(MQTTWireScratch, RejectsReservedFourByteHashEncoding) { + mesh::Packet p; + p.header = ROUTE_TYPE_FLOOD; + p.setPathHashSizeAndCount(4, 2); + p.payload_len = 4; + + ASSERT_EQ(4, p.getPathHashSize()); + ASSERT_EQ(8, p.getPathByteLen()); // fits the path array + ASSERT_LE(p.getRawLength(), (int)MQTTWireScratch::kWireBytes); + ASSERT_FALSE(mesh::Packet::isValidPathLen((uint8_t)p.path_len)); + EXPECT_FALSE(MQTTWireScratch::canSerialize(p, MQTTWireScratch::kWireBytes)); }