fix(mqtt): reject invalid path encodings before serializing

canSerialize() validated payload_len and the destination size but not whether the
path encoding is one writePath() will actually emit. writePath() self-guards
against overrunning the path array, but it does so by writing nothing and
returning 0 — a correctness problem, not a safety one, because getRawLength()
still counts the path. An over-long or reserved encoding therefore passed the
size check and then serialized to a truncated frame that was published as the
packet.

Worst case is path_len 0xFF with no payload: 63 hops of 4 bytes counts as 254
bytes, inside the 255-byte buffer, while writeTo() emits just the 2-byte header.
The `raw` field would carry 4 hex chars presented as the frame. Reserved 4-byte
hash encodings passed too, producing frames Packet::readFrom() rejects.

Now gated on Packet::isValidPathLen(), which rejects the reserved 4-byte hash
size and any count * size above MAX_PATH_SIZE in one predicate. It is the same
check readFrom() applies to every received packet, and TX packets are built via
setPathHashSizeAndCount() with real hash sizes, so no decodable packet is turned
away.

Two tests added for the cases a destination-size check cannot reach. The existing
truncation test passed for the wrong reason -- its payload_len of 4 pushed
getRawLength() to 258 and tripped the size check, masking the hole -- so it is
split into the >0xFF truncation case and the counted-length-fits case, with the
254/2-byte asymmetry asserted explicitly so it cannot be masked again.

274/274 native tests; both observer envs and an nRF52 repeater build clean.
This commit is contained in:
agessaman
2026-08-04 14:06:32 -07:00
parent dbdf2d732f
commit 8d1a0eb333
2 changed files with 44 additions and 5 deletions
+10 -2
View File
@@ -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;
}
@@ -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));
}