From 1fca422a9a29b8cbf1038a418bf46f78d628602e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Mon, 20 Jul 2026 09:45:04 +0200 Subject: [PATCH 1/2] Strip inner-message padding when sizing unsigned broadcasts The XEdDSA unsigned-downgrade check sized the decoded Data, but Data.payload is an opaque bytes field. Unknown fields inside the inner message inflated payload.size past the signable budget while still decoding, skipping the drop. Size the canonical re-encoding of the inner message for POSITION_APP, TELEMETRY_APP, WAYPOINT_APP and NODEINFO_APP. The canonical size is used only when it is no larger than the wire size, and the original payload is restored for downstream modules. --- src/mesh/Router.cpp | 78 +++++++++++++++++++++++++- test/test_packet_signing/test_main.cpp | 60 ++++++++++++++++++++ 2 files changed, 135 insertions(+), 3 deletions(-) diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index 9b1cc3ff28d..e2344941bfd 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -452,6 +452,76 @@ void Router::sniffReceived(const meshtastic_MeshPacket *p, const meshtastic_Rout } #if !(MESHTASTIC_EXCLUDE_PKI) && !(MESHTASTIC_EXCLUDE_XEDDSA) +/** Size a decoded Data the way the sender's signedDataFits() gate would have sized it, with + * attacker-controlled padding stripped. + * + * nanopb already discards unknown fields at the Data level, but Data.payload is an opaque bytes + * field: unknown fields buried inside the *inner* message (Position/Telemetry/Waypoint/User) ride + * along in payload.size and are silently dropped later by the module's own pb_decode. Sizing the + * raw payload would let a forger pad an unsigned broadcast past the signable budget - dodging the + * downgrade drop below - while the spoofed content still decodes and applies. So for inner types + * we can parse, measure the canonical re-encoding instead. Types we cannot parse keep their wire + * size, and the canonical size is only substituted when it is no larger, so this can never make a + * packet look bigger than it is. + * + * Forward-compat note: canonical sizing measures what THIS build's schema understands. A field + * added to Position/Telemetry/Waypoint/User (or to Data) in a later release is unknown here and + * shrinks our measurement, so a legitimate unsigned broadcast that the sender declined to sign + * because it did not fit could look signable to us and be dropped. That only bites within + * XEDDSA_SIGNATURE_FIELD_BYTES of the budget, and only for a signer node, but it means the + * inner-type list below must be kept in step with the schema: when a signable message type grows, + * re-check that its legitimate maximum still lands clear of the boundary. + * + * Returns false only if the Data could not be sized at all. + */ +static bool canonicalSignableSize(meshtastic_Data *d, size_t *size) +{ + const pb_msgdesc_t *fields = nullptr; + switch (d->portnum) { + case meshtastic_PortNum_POSITION_APP: + fields = &meshtastic_Position_msg; + break; + case meshtastic_PortNum_TELEMETRY_APP: + fields = &meshtastic_Telemetry_msg; + break; + case meshtastic_PortNum_WAYPOINT_APP: + fields = &meshtastic_Waypoint_msg; + break; + case meshtastic_PortNum_NODEINFO_APP: + fields = &meshtastic_User_msg; + break; + default: + break; + } + + if (fields) { + // Scratch kept off the stack: these decoded structs are large for the smaller MCU targets. + // Safe as file-static state because both callers of checkXeddsaReceivePolicy hold cryptLock. + static union { + meshtastic_Position position; + meshtastic_Telemetry telemetry; + meshtastic_Waypoint waypoint; + meshtastic_User user; + } inner; + + memset(&inner, 0, sizeof(inner)); + size_t canonicalPayload; + if (pb_decode_from_bytes(d->payload.bytes, d->payload.size, fields, &inner) && + pb_get_encoded_size(&canonicalPayload, fields, &inner) && canonicalPayload <= d->payload.size) { + // Only the length matters to pb_get_encoded_size for a bytes field, so swap the size in + // place rather than copying the whole Data, then restore it - callers still need the + // original payload intact for the modules downstream. + const pb_size_t prevSize = d->payload.size; + d->payload.size = (pb_size_t)canonicalPayload; + const bool sized = pb_get_encoded_size(size, &meshtastic_Data_msg, d); + d->payload.size = prevSize; + return sized; + } + } + + return pb_get_encoded_size(size, &meshtastic_Data_msg, d); +} + bool checkXeddsaReceivePolicy(meshtastic_MeshPacket *p) { // Only a signature we verify below may mark this packet signed; never trust an inbound flag. @@ -486,12 +556,14 @@ bool checkXeddsaReceivePolicy(meshtastic_MeshPacket *p) // non-PKI broadcast whose signed encoding would still fit the LoRa frame. Size p->decoded // canonically so this counts the same fields the sender's signedDataFits() gate counted; // adding XEDDSA_SIGNATURE_FIELD_BYTES to that unsigned base mirrors it exactly, whatever - // fields the Data carried. Unicast/PKI packets and broadcasts too big to carry a signature - // are never signed, so they must not be hard-failed here even for a known signer. + // fields the Data carried, and canonicalSignableSize() strips padding hidden inside + // Data.payload so the budget cannot be inflated. Unicast/PKI packets and broadcasts too big + // to carry a signature are never signed, so they must not be hard-failed here even for a + // known signer. const meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(p->from); if (node && nodeInfoLiteHasXeddsaSigned(node) && !p->pki_encrypted && isBroadcast(p->to)) { size_t canonicalSize; - if (!pb_get_encoded_size(&canonicalSize, &meshtastic_Data_msg, &p->decoded)) + if (!canonicalSignableSize(&p->decoded, &canonicalSize)) return true; // can't size it; never drop on a sizing failure if (canonicalSize + XEDDSA_SIGNATURE_FIELD_BYTES + MESHTASTIC_HEADER_LENGTH <= MAX_LORA_PAYLOAD_LEN) { LOG_WARN("Dropping unsigned broadcast from 0x%08x that previously signed", p->from); diff --git a/test/test_packet_signing/test_main.cpp b/test/test_packet_signing/test_main.cpp index f0eb604736f..f8a63535f89 100644 --- a/test/test_packet_signing/test_main.cpp +++ b/test/test_packet_signing/test_main.cpp @@ -772,6 +772,64 @@ void test_E9_decoded_partial_signature_from_nonsigner_dropped(void) TEST_ASSERT_FALSE_MESSAGE(checkXeddsaReceivePolicy(&p), "partial signature must be dropped as malformed"); } +// E10: padding hidden INSIDE Data.payload must not buy an exemption. A10 closed Data-level unknown +// fields, but payload is an opaque bytes field: unknown fields within the Position ride along in +// payload.size, are discarded by PositionModule's own pb_decode, and would otherwise inflate the +// size past the signable budget while the spoofed coordinates still land. Sizing the canonical +// re-encoding of the inner message strips the padding, so the downgrade drop still fires. +void test_E10_decoded_unsigned_position_padded_inside_payload_dropped(void) +{ + mockNodeDB->addNode(REMOTE_NODE); + mockNodeDB->setSignerBit(REMOTE_NODE, true); + + meshtastic_Position pos = meshtastic_Position_init_zero; + pos.has_latitude_i = pos.has_longitude_i = true; + pos.latitude_i = 371234567; + pos.longitude_i = -1221234567; + + meshtastic_MeshPacket p = makeDecoded(REMOTE_NODE, NODENUM_BROADCAST, meshtastic_PortNum_POSITION_APP, 0); + const size_t posLen = + pb_encode_to_bytes(p.decoded.payload.bytes, sizeof(p.decoded.payload.bytes), &meshtastic_Position_msg, &pos); + TEST_ASSERT_GREATER_THAN_MESSAGE(0, posLen, "failed to encode the spoofed Position"); + p.decoded.payload.size = + posLen + appendUnknownField(p.decoded.payload.bytes + posLen, sizeof(p.decoded.payload.bytes) - posLen, 163); + + // Without canonical inner sizing the padded Data looks too big to have been signed, which is + // exactly the exemption the attacker is buying. Pin that, else the test passes vacuously. + TEST_ASSERT_FALSE_MESSAGE(signedEncodingFits(&p.decoded), "padding must push the raw size past the fit threshold"); + TEST_ASSERT_LESS_OR_EQUAL_MESSAGE(MAX_LORA_PAYLOAD_LEN, encodedDataSize(&p.decoded) + MESHTASTIC_HEADER_LENGTH, + "padded frame must still be one a radio could send"); + + TEST_ASSERT_FALSE_MESSAGE(checkXeddsaReceivePolicy(&p), "payload-padded unsigned Position from a signer must be dropped"); + TEST_ASSERT_FALSE(p.xeddsa_signed); +} + +// E11: E10's over-correction guard. Some signable types are legitimately too big to sign +// (Telemetry maxes at 272 bytes, Waypoint at 199), and their senders correctly leave them +// unsigned. Canonical sizing must not shrink such a message into the drop range: an honest +// oversized HostMetrics from a signer stays accepted. +void test_E11_decoded_unsigned_oversized_telemetry_from_signer_accepted(void) +{ + mockNodeDB->addNode(REMOTE_NODE); + mockNodeDB->setSignerBit(REMOTE_NODE, true); + + meshtastic_Telemetry t = meshtastic_Telemetry_init_zero; + t.which_variant = meshtastic_Telemetry_host_metrics_tag; + t.variant.host_metrics.uptime_seconds = 123456; + t.variant.host_metrics.has_user_string = true; + memset(t.variant.host_metrics.user_string, 'x', sizeof(t.variant.host_metrics.user_string) - 1); + + meshtastic_MeshPacket p = makeDecoded(REMOTE_NODE, NODENUM_BROADCAST, meshtastic_PortNum_TELEMETRY_APP, 0); + p.decoded.payload.size = + pb_encode_to_bytes(p.decoded.payload.bytes, sizeof(p.decoded.payload.bytes), &meshtastic_Telemetry_msg, &t); + TEST_ASSERT_GREATER_THAN_MESSAGE(0, p.decoded.payload.size, "failed to encode the oversized Telemetry"); + + // Every byte here is a field this build understands, so canonical sizing must leave it alone. + TEST_ASSERT_FALSE_MESSAGE(signedEncodingFits(&p.decoded), "telemetry must be too big to sign, else the test is vacuous"); + + TEST_ASSERT_TRUE_MESSAGE(checkXeddsaReceivePolicy(&p), "honest oversized telemetry from a signer must not be dropped"); +} + void setup() { initializeTestEnvironment(); @@ -817,6 +875,8 @@ void setup() RUN_TEST(test_E7_decoded_unsigned_pki_from_signer_accepted); RUN_TEST(test_E8_decoded_partial_signature_from_signer_dropped); RUN_TEST(test_E9_decoded_partial_signature_from_nonsigner_dropped); + RUN_TEST(test_E10_decoded_unsigned_position_padded_inside_payload_dropped); + RUN_TEST(test_E11_decoded_unsigned_oversized_telemetry_from_signer_accepted); exit(UNITY_END()); } From c4c59b64fb99e3b1dd83e51ec4c5424235c5b628 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Mon, 20 Jul 2026 10:05:06 +0200 Subject: [PATCH 2/2] Address review: condense comments, cover Waypoint and NodeInfo Shorten the new comment blocks to match the repo comment guideline. Add E12 and E13 covering the Waypoint and NodeInfo branches of the canonical-sizing switch, factoring the padded-broadcast construction into a shared helper. Suppress the cppcheck unusedStructMember warnings on the scratch union. --- src/mesh/Router.cpp | 43 +++++-------- test/test_packet_signing/test_main.cpp | 83 +++++++++++++++++++------- 2 files changed, 76 insertions(+), 50 deletions(-) diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index e2344941bfd..b21d7453e70 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -452,28 +452,11 @@ void Router::sniffReceived(const meshtastic_MeshPacket *p, const meshtastic_Rout } #if !(MESHTASTIC_EXCLUDE_PKI) && !(MESHTASTIC_EXCLUDE_XEDDSA) -/** Size a decoded Data the way the sender's signedDataFits() gate would have sized it, with - * attacker-controlled padding stripped. - * - * nanopb already discards unknown fields at the Data level, but Data.payload is an opaque bytes - * field: unknown fields buried inside the *inner* message (Position/Telemetry/Waypoint/User) ride - * along in payload.size and are silently dropped later by the module's own pb_decode. Sizing the - * raw payload would let a forger pad an unsigned broadcast past the signable budget - dodging the - * downgrade drop below - while the spoofed content still decodes and applies. So for inner types - * we can parse, measure the canonical re-encoding instead. Types we cannot parse keep their wire - * size, and the canonical size is only substituted when it is no larger, so this can never make a - * packet look bigger than it is. - * - * Forward-compat note: canonical sizing measures what THIS build's schema understands. A field - * added to Position/Telemetry/Waypoint/User (or to Data) in a later release is unknown here and - * shrinks our measurement, so a legitimate unsigned broadcast that the sender declined to sign - * because it did not fit could look signable to us and be dropped. That only bites within - * XEDDSA_SIGNATURE_FIELD_BYTES of the budget, and only for a signer node, but it means the - * inner-type list below must be kept in step with the schema: when a signable message type grows, - * re-check that its legitimate maximum still lands clear of the boundary. - * - * Returns false only if the Data could not be sized at all. - */ +/** Size a decoded Data as the sender's signedDataFits() gate would have, with padding stripped: + * unknown fields inside Data.payload survive in payload.size and would otherwise let a forger + * inflate an unsigned broadcast past the signable budget. Returns false only if sizing failed. + * Sizing only what this build's schema decodes, so a signable type that later grows needs its + * legitimate maximum re-checked against the budget or honest unsigned broadcasts get dropped. */ static bool canonicalSignableSize(meshtastic_Data *d, size_t *size) { const pb_msgdesc_t *fields = nullptr; @@ -498,9 +481,13 @@ static bool canonicalSignableSize(meshtastic_Data *d, size_t *size) // Scratch kept off the stack: these decoded structs are large for the smaller MCU targets. // Safe as file-static state because both callers of checkXeddsaReceivePolicy hold cryptLock. static union { + // cppcheck-suppress unusedStructMember ; written by pb_decode through &inner meshtastic_Position position; + // cppcheck-suppress unusedStructMember ; written by pb_decode through &inner meshtastic_Telemetry telemetry; + // cppcheck-suppress unusedStructMember ; written by pb_decode through &inner meshtastic_Waypoint waypoint; + // cppcheck-suppress unusedStructMember ; written by pb_decode through &inner meshtastic_User user; } inner; @@ -508,9 +495,8 @@ static bool canonicalSignableSize(meshtastic_Data *d, size_t *size) size_t canonicalPayload; if (pb_decode_from_bytes(d->payload.bytes, d->payload.size, fields, &inner) && pb_get_encoded_size(&canonicalPayload, fields, &inner) && canonicalPayload <= d->payload.size) { - // Only the length matters to pb_get_encoded_size for a bytes field, so swap the size in - // place rather than copying the whole Data, then restore it - callers still need the - // original payload intact for the modules downstream. + // Only the length matters when sizing a bytes field, so swap it in place instead of + // copying the whole Data; restored below because modules still need the real payload. const pb_size_t prevSize = d->payload.size; d->payload.size = (pb_size_t)canonicalPayload; const bool sized = pb_get_encoded_size(size, &meshtastic_Data_msg, d); @@ -556,10 +542,9 @@ bool checkXeddsaReceivePolicy(meshtastic_MeshPacket *p) // non-PKI broadcast whose signed encoding would still fit the LoRa frame. Size p->decoded // canonically so this counts the same fields the sender's signedDataFits() gate counted; // adding XEDDSA_SIGNATURE_FIELD_BYTES to that unsigned base mirrors it exactly, whatever - // fields the Data carried, and canonicalSignableSize() strips padding hidden inside - // Data.payload so the budget cannot be inflated. Unicast/PKI packets and broadcasts too big - // to carry a signature are never signed, so they must not be hard-failed here even for a - // known signer. + // fields the Data carried, with padding hidden inside Data.payload stripped. Unicast/PKI + // packets and broadcasts too big to carry a signature are never signed, so they must not be + // hard-failed here even for a known signer. const meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(p->from); if (node && nodeInfoLiteHasXeddsaSigned(node) && !p->pki_encrypted && isBroadcast(p->to)) { size_t canonicalSize; diff --git a/test/test_packet_signing/test_main.cpp b/test/test_packet_signing/test_main.cpp index f8a63535f89..3bd6f93fcca 100644 --- a/test/test_packet_signing/test_main.cpp +++ b/test/test_packet_signing/test_main.cpp @@ -772,11 +772,26 @@ void test_E9_decoded_partial_signature_from_nonsigner_dropped(void) TEST_ASSERT_FALSE_MESSAGE(checkXeddsaReceivePolicy(&p), "partial signature must be dropped as malformed"); } -// E10: padding hidden INSIDE Data.payload must not buy an exemption. A10 closed Data-level unknown -// fields, but payload is an opaque bytes field: unknown fields within the Position ride along in -// payload.size, are discarded by PositionModule's own pb_decode, and would otherwise inflate the -// size past the signable budget while the spoofed coordinates still land. Sizing the canonical -// re-encoding of the inner message strips the padding, so the downgrade drop still fires. +// Build an unsigned broadcast whose inner message is padded with an unknown field, and pin that the +// padding pushes the RAW size past the fit threshold - the exemption the attacker is buying - while +// the frame stays sendable. Without canonical inner sizing these packets are wrongly accepted. +static meshtastic_MeshPacket makePayloadPaddedBroadcast(meshtastic_PortNum port, const pb_msgdesc_t *fields, const void *inner, + size_t padLen) +{ + meshtastic_MeshPacket p = makeDecoded(REMOTE_NODE, NODENUM_BROADCAST, port, 0); + const size_t innerLen = pb_encode_to_bytes(p.decoded.payload.bytes, sizeof(p.decoded.payload.bytes), fields, inner); + TEST_ASSERT_GREATER_THAN_MESSAGE(0, innerLen, "failed to encode the spoofed inner message"); + p.decoded.payload.size = + innerLen + appendUnknownField(p.decoded.payload.bytes + innerLen, sizeof(p.decoded.payload.bytes) - innerLen, padLen); + + TEST_ASSERT_FALSE_MESSAGE(signedEncodingFits(&p.decoded), "padding must push the raw size past the fit threshold"); + TEST_ASSERT_LESS_OR_EQUAL_MESSAGE(MAX_LORA_PAYLOAD_LEN, encodedDataSize(&p.decoded) + MESHTASTIC_HEADER_LENGTH, + "padded frame must still be one a radio could send"); + return p; +} + +// E10: unknown fields buried inside Data.payload are discarded by the module's own pb_decode, so +// they must not sway the downgrade decision the way A10 already pins for Data-level unknown fields. void test_E10_decoded_unsigned_position_padded_inside_payload_dropped(void) { mockNodeDB->addNode(REMOTE_NODE); @@ -787,27 +802,14 @@ void test_E10_decoded_unsigned_position_padded_inside_payload_dropped(void) pos.latitude_i = 371234567; pos.longitude_i = -1221234567; - meshtastic_MeshPacket p = makeDecoded(REMOTE_NODE, NODENUM_BROADCAST, meshtastic_PortNum_POSITION_APP, 0); - const size_t posLen = - pb_encode_to_bytes(p.decoded.payload.bytes, sizeof(p.decoded.payload.bytes), &meshtastic_Position_msg, &pos); - TEST_ASSERT_GREATER_THAN_MESSAGE(0, posLen, "failed to encode the spoofed Position"); - p.decoded.payload.size = - posLen + appendUnknownField(p.decoded.payload.bytes + posLen, sizeof(p.decoded.payload.bytes) - posLen, 163); - - // Without canonical inner sizing the padded Data looks too big to have been signed, which is - // exactly the exemption the attacker is buying. Pin that, else the test passes vacuously. - TEST_ASSERT_FALSE_MESSAGE(signedEncodingFits(&p.decoded), "padding must push the raw size past the fit threshold"); - TEST_ASSERT_LESS_OR_EQUAL_MESSAGE(MAX_LORA_PAYLOAD_LEN, encodedDataSize(&p.decoded) + MESHTASTIC_HEADER_LENGTH, - "padded frame must still be one a radio could send"); + meshtastic_MeshPacket p = makePayloadPaddedBroadcast(meshtastic_PortNum_POSITION_APP, &meshtastic_Position_msg, &pos, 163); TEST_ASSERT_FALSE_MESSAGE(checkXeddsaReceivePolicy(&p), "payload-padded unsigned Position from a signer must be dropped"); TEST_ASSERT_FALSE(p.xeddsa_signed); } -// E11: E10's over-correction guard. Some signable types are legitimately too big to sign -// (Telemetry maxes at 272 bytes, Waypoint at 199), and their senders correctly leave them -// unsigned. Canonical sizing must not shrink such a message into the drop range: an honest -// oversized HostMetrics from a signer stays accepted. +// E11: over-correction guard. Telemetry (272 bytes max) and Waypoint (199) can legitimately exceed +// the signable budget, so canonical sizing must not shrink an honest one into the drop range. void test_E11_decoded_unsigned_oversized_telemetry_from_signer_accepted(void) { mockNodeDB->addNode(REMOTE_NODE); @@ -830,6 +832,43 @@ void test_E11_decoded_unsigned_oversized_telemetry_from_signer_accepted(void) TEST_ASSERT_TRUE_MESSAGE(checkXeddsaReceivePolicy(&p), "honest oversized telemetry from a signer must not be dropped"); } +// E12: E10 for the Waypoint branch of the canonical-sizing switch. +void test_E12_decoded_unsigned_waypoint_padded_inside_payload_dropped(void) +{ + mockNodeDB->addNode(REMOTE_NODE); + mockNodeDB->setSignerBit(REMOTE_NODE, true); + + meshtastic_Waypoint w = meshtastic_Waypoint_init_zero; + w.id = 42; + w.has_latitude_i = w.has_longitude_i = true; + w.latitude_i = 371234567; + w.longitude_i = -1221234567; + strcpy(w.name, "spoofed"); + + meshtastic_MeshPacket p = makePayloadPaddedBroadcast(meshtastic_PortNum_WAYPOINT_APP, &meshtastic_Waypoint_msg, &w, 150); + + TEST_ASSERT_FALSE_MESSAGE(checkXeddsaReceivePolicy(&p), "payload-padded unsigned Waypoint from a signer must be dropped"); + TEST_ASSERT_FALSE(p.xeddsa_signed); +} + +// E13: E10 for the NodeInfo/User branch. Router drops it before rebroadcast; NodeInfoModule's own +// check (Group C) is receiver-local and would not stop the packet propagating. +void test_E13_decoded_unsigned_nodeinfo_padded_inside_payload_dropped(void) +{ + mockNodeDB->addNode(REMOTE_NODE); + mockNodeDB->setSignerBit(REMOTE_NODE, true); + + meshtastic_User u = meshtastic_User_init_zero; + strcpy(u.id, "!0b0b0b0b"); + strcpy(u.long_name, "spoofed node"); + strcpy(u.short_name, "SPF"); + + meshtastic_MeshPacket p = makePayloadPaddedBroadcast(meshtastic_PortNum_NODEINFO_APP, &meshtastic_User_msg, &u, 150); + + TEST_ASSERT_FALSE_MESSAGE(checkXeddsaReceivePolicy(&p), "payload-padded unsigned NodeInfo from a signer must be dropped"); + TEST_ASSERT_FALSE(p.xeddsa_signed); +} + void setup() { initializeTestEnvironment(); @@ -877,6 +916,8 @@ void setup() RUN_TEST(test_E9_decoded_partial_signature_from_nonsigner_dropped); RUN_TEST(test_E10_decoded_unsigned_position_padded_inside_payload_dropped); RUN_TEST(test_E11_decoded_unsigned_oversized_telemetry_from_signer_accepted); + RUN_TEST(test_E12_decoded_unsigned_waypoint_padded_inside_payload_dropped); + RUN_TEST(test_E13_decoded_unsigned_nodeinfo_padded_inside_payload_dropped); exit(UNITY_END()); }