Router: size the fit check from the decoded Data

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) {

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;

test/test_packet_signing/test_main.cpp

@@ -148,6 +148,58 @@ static bool signedEncodingFits(const meshtastic_Data *d)
     return encodedDataSize(&copy) + 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<uint8_t> 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);