From bd4ca902d5b7154ff9f690ed9c58f706831bca2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Thu, 16 Jul 2026 14:31:58 +0200 Subject: [PATCH] Router: size the fit check from the decoded Data --- src/mesh/Router.cpp | 30 ++++---- src/mesh/Router.h | 7 +- test/test_packet_signing/test_main.cpp | 97 ++++++++++++++++++++++++-- 3 files changed, 111 insertions(+), 23 deletions(-) diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index 24f7f586bd9..7ae78cbb162 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -452,7 +452,7 @@ void Router::sniffReceived(const meshtastic_MeshPacket *p, const meshtastic_Rout } #if !(MESHTASTIC_EXCLUDE_PKI) && !(MESHTASTIC_EXCLUDE_XEDDSA) -bool checkXeddsaReceivePolicy(meshtastic_MeshPacket *p, size_t encodedDataSize) +bool checkXeddsaReceivePolicy(meshtastic_MeshPacket *p) { // Only a signature we verify below may mark this packet signed; never trust an inbound flag. p->xeddsa_signed = false; @@ -483,17 +483,17 @@ bool checkXeddsaReceivePolicy(meshtastic_MeshPacket *p, size_t encodedDataSize) return false; } else { // Truly unsigned (signature size 0) - only reject the class a signing node always signs: a - // non-PKI broadcast whose signed encoding would still fit the LoRa frame. encodedDataSize is - // the size of the encoded Data exactly as the sender built it (or 0 to size p->decoded - // canonically); with no signature field present it is the unsigned base, and adding - // XEDDSA_SIGNATURE_FIELD_BYTES mirrors the sender-side signedDataFits() gate per packet, - // 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. + // 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. const meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(p->from); if (node && nodeInfoLiteHasXeddsaSigned(node) && !p->pki_encrypted && isBroadcast(p->to)) { - if (encodedDataSize == 0 && !pb_get_encoded_size(&encodedDataSize, &meshtastic_Data_msg, &p->decoded)) + size_t canonicalSize; + if (!pb_get_encoded_size(&canonicalSize, &meshtastic_Data_msg, &p->decoded)) return true; // can't size it; never drop on a sizing failure - if (encodedDataSize + XEDDSA_SIGNATURE_FIELD_BYTES + MESHTASTIC_HEADER_LENGTH <= MAX_LORA_PAYLOAD_LEN) { + 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); return false; } @@ -621,17 +621,17 @@ DecodeState perhapsDecode(meshtastic_MeshPacket *p) if (decrypted) { // parsing was successful p->channel = chIndex; // change to store the index instead of the hash - if (p->decoded.has_bitfield) - p->decoded.want_response |= p->decoded.bitfield & BITFIELD_WANT_RESPONSE_MASK; #if !(MESHTASTIC_EXCLUDE_PKI) && !(MESHTASTIC_EXCLUDE_XEDDSA) - // rawSize is the size of the encoded Data exactly as the sender built it (the PKI branch's - // MESHTASTIC_PKC_OVERHEAD subtraction preserves that, and PKI packets are unicast so the - // downgrade predicate ignores them anyway). - if (!checkXeddsaReceivePolicy(p, rawSize)) + // Runs before the bitfield merge below: that merge can set want_response, adding wire bytes + // the sender never encoded and skewing the policy's sizing of p->decoded. + if (!checkXeddsaReceivePolicy(p)) return DecodeState::DECODE_FAILURE; #endif + if (p->decoded.has_bitfield) + p->decoded.want_response |= p->decoded.bitfield & BITFIELD_WANT_RESPONSE_MASK; + /* Not actually ever used. // Decompress if needed. jm if (p->decoded.portnum == meshtastic_PortNum_TEXT_MESSAGE_COMPRESSED_APP) { diff --git a/src/mesh/Router.h b/src/mesh/Router.h index 954b88f2410..d5d4b76adb9 100644 --- a/src/mesh/Router.h +++ b/src/mesh/Router.h @@ -183,16 +183,15 @@ meshtastic_Routing_Error perhapsEncode(meshtastic_MeshPacket *p); * downgrade protection: drop a non-PKI broadcast from a known signer whose signed encoding would * still fit a LoRa frame (unicast, PKI, and oversized broadcasts always pass). * - * encodedDataSize is the wire size of the encoded Data as the sender built it; pass 0 to size - * p->decoded canonically instead (for already-decoded ingress such as plaintext-MQTT downlink, - * which bypasses perhapsDecode's crypto path). + * The fit test sizes p->decoded with the real encoder, so it measures the fields the sender + * encoded rather than any raw wire length. * * The caller MUST hold cryptLock: verification runs through the shared CryptoEngine key cache. * (perhapsDecode already holds it; other call sites must take it themselves.) * * @return false if the packet must be dropped. */ -bool checkXeddsaReceivePolicy(meshtastic_MeshPacket *p, size_t encodedDataSize = 0); +bool checkXeddsaReceivePolicy(meshtastic_MeshPacket *p); #endif extern Router *router; diff --git a/test/test_packet_signing/test_main.cpp b/test/test_packet_signing/test_main.cpp index 707d716195b..f0eb604736f 100644 --- a/test/test_packet_signing/test_main.cpp +++ b/test/test_packet_signing/test_main.cpp @@ -148,6 +148,58 @@ static bool signedEncodingFits(const meshtastic_Data *d) return encodedDataSize(©) + MESHTASTIC_HEADER_LENGTH <= MAX_LORA_PAYLOAD_LEN; } +// Append a length-delimited field whose tag this build's Data schema does not define, as a sender +// on a newer schema would emit. nanopb skips unknown fields at decode, so these bytes count toward +// the raw wire size but not the decoded struct. Returns the number of bytes appended. +static size_t appendUnknownField(uint8_t *dst, size_t dstLen, size_t contentLen) +{ + constexpr uint32_t UNKNOWN_FIELD_NUMBER = 100; // not a field of meshtastic_Data + std::vector content(contentLen, 0x77); + pb_ostream_t stream = pb_ostream_from_buffer(dst, dstLen); + TEST_ASSERT_TRUE(pb_encode_tag(&stream, PB_WT_STRING, UNKNOWN_FIELD_NUMBER)); + TEST_ASSERT_TRUE(pb_encode_string(&stream, content.data(), content.size())); + return stream.bytes_written; +} + +// Channel-encrypt raw Data bytes into a packet, exactly as perhapsEncode's non-PKI path does. +// Used to inject wire bytes perhapsEncode would never produce (it only encodes p->decoded). +static void encryptAsChannelPacket(meshtastic_MeshPacket *p, uint8_t *wire, size_t size) +{ + const int16_t hash = channels.setActiveByIndex(0); + TEST_ASSERT_GREATER_OR_EQUAL_MESSAGE(0, hash, "no usable primary channel"); + crypto->encryptPacket(getFrom(p), p->id, size, wire); + memcpy(p->encrypted.bytes, wire, size); + p->encrypted.size = size; + p->channel = hash; // on the wire the channel field carries the hash, not the index + p->which_payload_variant = meshtastic_MeshPacket_encrypted_tag; +} + +// Build A10's frame: an unsigned broadcast carrying a POSITION payload plus unknown fields, sized +// so the raw wire length exceeds the signature-fit threshold while the decoded fields stay under +// it. Channel-encrypted like a normal sender. The asserts pin that split, which is what makes A10 +// and A11 meaningful. +static meshtastic_MeshPacket makeBroadcastWithUnknownFields() +{ + meshtastic_MeshPacket p = makeDecoded(REMOTE_NODE, NODENUM_BROADCAST, meshtastic_PortNum_POSITION_APP, SMALL_PAYLOAD); + + uint8_t wire[MAX_LORA_PAYLOAD_LEN + 1]; + const size_t base = pb_encode_to_bytes(wire, sizeof(wire), &meshtastic_Data_msg, &p.decoded); + TEST_ASSERT_GREATER_THAN_MESSAGE(0, base, "failed to encode the base Data"); + const size_t raw = base + appendUnknownField(wire + base, sizeof(wire) - base, 160); + + // The decoded fields fit a signature, so a sender that signs would have signed this Data. + TEST_ASSERT_LESS_OR_EQUAL_MESSAGE(MAX_LORA_PAYLOAD_LEN, base + XEDDSA_SIGNATURE_FIELD_BYTES + MESHTASTIC_HEADER_LENGTH, + "decoded fields must fit a signature, else the test is vacuous"); + // The unknown fields put the raw size over that threshold, so the two sizings disagree here. + TEST_ASSERT_GREATER_THAN_MESSAGE(MAX_LORA_PAYLOAD_LEN, raw + XEDDSA_SIGNATURE_FIELD_BYTES + MESHTASTIC_HEADER_LENGTH, + "unknown fields must push the raw size past the fit threshold"); + // The frame is still one a radio could actually send. + TEST_ASSERT_LESS_OR_EQUAL_MESSAGE(MAX_LORA_PAYLOAD_LEN, raw + MESHTASTIC_HEADER_LENGTH, "frame must still fit a LoRa frame"); + + encryptAsChannelPacket(&p, wire, raw); + return p; +} + // --------------------------------------------------------------------------- // Unity lifecycle // --------------------------------------------------------------------------- @@ -324,6 +376,41 @@ void test_A9_unsigned_boundary_broadcast_from_signer_still_dropped(void) TEST_ASSERT_EQUAL(DECODE_FAILURE, roundTrip(&p)); } +// A10: unknown fields must not sway the downgrade decision. A sender on a newer schema can include +// Data fields this build doesn't define; nanopb skips them, so they grow the frame without changing +// what decodes. The decision sizes p->decoded, keeping it on the fields the sender encoded, so an +// unsigned broadcast from a known signer is dropped whether or not unknown fields came along. +void test_A10_unsigned_broadcast_from_signer_with_unknown_fields_dropped(void) +{ + mockNodeDB->addNode(REMOTE_NODE); + mockNodeDB->setSignerBit(REMOTE_NODE, true); // we've seen this node sign before + + meshtastic_MeshPacket p = makeBroadcastWithUnknownFields(); + + TEST_ASSERT_EQUAL_MESSAGE(DECODE_FAILURE, perhapsDecode(&p), + "unsigned broadcast from a signer must be dropped despite unknown fields"); +} + +// A11: A10's control - the same frame from a node we've never seen sign is accepted and still +// delivers its payload, so A10's DECODE_FAILURE is the downgrade drop and not a decode failure +// caused by the unknown fields. +void test_A11_unsigned_broadcast_from_nonsigner_with_unknown_fields_accepted(void) +{ + mockNodeDB->addNode(REMOTE_NODE); // signer bit clear + + meshtastic_MeshPacket p = makeBroadcastWithUnknownFields(); + const size_t rawSize = p.encrypted.size; // read before decode: encrypted/decoded share a union + + TEST_ASSERT_EQUAL_MESSAGE(DECODE_SUCCESS, perhapsDecode(&p), "frame from a non-signer must still decode"); + TEST_ASSERT_EQUAL_MESSAGE(meshtastic_PortNum_POSITION_APP, p.decoded.portnum, "unknown fields must not disturb the portnum"); + TEST_ASSERT_EQUAL_MESSAGE(SMALL_PAYLOAD, p.decoded.payload.size, "payload must survive the unknown fields"); + TEST_ASSERT_FALSE(p.xeddsa_signed); + + // The unknown fields are gone from the decoded struct: the gap the sizing basis has to account for. + TEST_ASSERT_LESS_THAN_MESSAGE(rawSize, encodedDataSize(&p.decoded), + "unknown fields must drop at decode, leaving decoded size < raw"); +} + // =========================================================================== // Group B - send-side signing policy (perhapsEncode) // =========================================================================== @@ -418,7 +505,7 @@ void test_B5_preset_signature_on_local_packet_cleared(void) // B6: the exact-fit gate tracks Data *shape*, not just payload size. A tapback-style broadcast // (want_response + reply_id + emoji) carries extra wire bytes that shift the fit boundary; the // sweep proves no dead band exists for that shape either, and - once the signer bit is learned - -// that the receiver's rawSize-driven downgrade predicate stays symmetric for it too. Window +// that the receiver's downgrade predicate stays symmetric for it too. Window // straddles this shape's boundary; capped at 200 so even the unsigned rich encoding stays well // inside the frame (at n=221 it first hits the pre-existing, signing-unrelated TOO_LARGE). void test_B6_rich_shape_sweep_no_deadband(void) @@ -558,7 +645,7 @@ void test_D1_signature_field_overhead_exact(void) // =========================================================================== // Already-decoded packets never reach perhapsDecode's crypto path (it early-returns), so // plaintext-MQTT downlink applies this policy function directly at ingress (MQTT.cpp). These -// tests drive it the same way: decoded packets, encodedDataSize = 0 (canonical sizing). +// tests drive it the same way: decoded packets, sized from p->decoded exactly as the RF path is. // End-to-end MQTT wiring is covered in test_mqtt. // E1: unsigned small broadcast from a known signer -> dropped (downgrade protection holds on @@ -616,8 +703,8 @@ void test_E4_decoded_bad_signature_dropped(void) TEST_ASSERT_FALSE(p.xeddsa_signed); } -// E5: unsigned oversized broadcast from a signer -> accepted (canonical sizing exempts packets -// whose signed encoding wouldn't fit, mirroring the RF-path rawSize rule). +// E5: unsigned oversized broadcast from a signer -> accepted (packets whose signed encoding +// wouldn't fit are exempt, identically to the RF path: both size p->decoded). void test_E5_decoded_unsigned_oversized_broadcast_from_signer_accepted(void) { mockNodeDB->addNode(REMOTE_NODE); @@ -700,6 +787,8 @@ void setup() RUN_TEST(test_A7_unsigned_oversized_broadcast_from_signer_accepted); RUN_TEST(test_A8_unsigned_deadband_broadcast_from_signer_accepted); RUN_TEST(test_A9_unsigned_boundary_broadcast_from_signer_still_dropped); + RUN_TEST(test_A10_unsigned_broadcast_from_signer_with_unknown_fields_dropped); + RUN_TEST(test_A11_unsigned_broadcast_from_nonsigner_with_unknown_fields_accepted); printf("\n=== Group B: send-side signing policy ===\n"); RUN_TEST(test_B1_local_broadcast_is_signed);