Harden XEdDSA unsigned-packet policy and add coverage (#10858)
Audit of the XEdDSA packet-signing implementation (#10478) surfaced several issues in when unsigned packets are accepted on receive or emitted on send. This fixes them and adds regression coverage. - Unicast NodeInfo exchange no longer breaks against signer nodes: the NodeInfoModule downgrade drop is gated to broadcasts, since senders never sign unicast (want_response replies, directed exchanges). - Replace the payload-size sign heuristic with an exact encoded-size gate (signedDataFits) and mirror it on the receive side, removing a dead band where 167-168 B broadcasts were signed then failed TOO_LARGE. - Extract the receive policy into checkXeddsaReceivePolicy() and apply it to plaintext-MQTT decoded downlink, which previously skipped signature verification and downgrade protection entirely. - Reject signatures whose length is neither 0 nor 64 as malformed, so a crafted partial signature can't inflate the size estimate and dodge the unsigned-downgrade drop. - Hold cryptLock on the MQTT verify path (shared Ed25519 key cache). - Clear any client-preset signature on packets we originate, on all builds. - Randomized (hedged) signing per the Signal XEdDSA spec: bump the meshtastic/Crypto pin to the build where XEdDSA::sign mixes 32 bytes of caller randomness into the nonce as Z (meshtastic/Crypto#3), and seed those bytes in xeddsa_sign from HardwareRNG (checked, with a seeded-CSPRNG fallback). test_crypto pins that repeated signs differ and both verify. Adds test coverage: test_packet_signing groups A-E (receive matrix, send policy, NodeInfo backstop, encoding invariants, decoded-ingress policy), test_mqtt end-to-end downlink cases, and a test_crypto randomization check.
This commit is contained in:
Ben Meadors
GitHub
@@ -116,8 +116,11 @@ bool CryptoEngine::xeddsa_sign(uint32_t fromNode, uint32_t packetId, uint32_t po
|
||||
size_t sigLen = buildSigningBuffer(sigBuf, sizeof(sigBuf), fromNode, packetId, portnum, payload, payloadLen);
|
||||
if (sigLen == 0)
|
||||
return false;
|
||||
// the XEdDSA::sign function requires at least the first 32 bytes of signature to be pre-filled with randomness
|
||||
HardwareRNG::fill(signature, 32);
|
||||
// XEdDSA::sign mixes signature[0..31] into the nonce as the spec's random Z (meshtastic/Crypto#3)
|
||||
// for hedged signatures, so seed it - hardware RNG, else the seeded CSPRNG. A weak Z is still
|
||||
// safe against nonce reuse (defense-in-depth only), so we never fail signing over it.
|
||||
if (!HardwareRNG::fill(signature, 32))
|
||||
CryptRNG.rand(signature, 32);
|
||||
XEdDSA::sign(signature, xeddsa_private_key, xeddsa_public_key, sigBuf, sigLen);
|
||||
return true;
|
||||
}
|
||||
|
||||
@@ -24,6 +24,9 @@ struct CryptoKey {
|
||||
#define MAX_BLOCKSIZE 256
|
||||
#define TEST_CURVE25519_FIELD_OPS // Exposes Curve25519::isWeakPoint() for testing keys
|
||||
#define XEDDSA_SIGNATURE_SIZE 64
|
||||
// Encoded size the signature adds to the Data protobuf: 1 tag byte (field 10 < 16) +
|
||||
// 1 length byte (64 < 128) + 64 signature bytes. test_packet_signing asserts this stays exact.
|
||||
#define XEDDSA_SIGNATURE_FIELD_BYTES (XEDDSA_SIGNATURE_SIZE + 2)
|
||||
|
||||
class CryptoEngine
|
||||
{
|
||||
|
||||
+83
-33
@@ -12,6 +12,7 @@
|
||||
#include "mesh-pb-constants.h"
|
||||
#include "meshUtils.h"
|
||||
#include "modules/RoutingModule.h"
|
||||
#include <pb_encode.h>
|
||||
#if HAS_TRAFFIC_MANAGEMENT
|
||||
#include "modules/TrafficManagementModule.h"
|
||||
#endif
|
||||
@@ -444,6 +445,58 @@ void Router::sniffReceived(const meshtastic_MeshPacket *p, const meshtastic_Rout
|
||||
// FIXME, update nodedb here for any packet that passes through us
|
||||
}
|
||||
|
||||
#if !(MESHTASTIC_EXCLUDE_PKI) && !(MESHTASTIC_EXCLUDE_XEDDSA)
|
||||
bool checkXeddsaReceivePolicy(meshtastic_MeshPacket *p, size_t encodedDataSize)
|
||||
{
|
||||
// Only a signature we verify below may mark this packet signed; never trust an inbound flag.
|
||||
p->xeddsa_signed = false;
|
||||
if (p->decoded.xeddsa_signature.size == XEDDSA_SIGNATURE_SIZE) {
|
||||
meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(p->from);
|
||||
if (node && node->public_key.size == 32) {
|
||||
p->xeddsa_signed =
|
||||
crypto->xeddsa_verify(node->public_key.bytes, p->from, p->id, p->decoded.portnum, p->decoded.payload.bytes,
|
||||
p->decoded.payload.size, p->decoded.xeddsa_signature.bytes);
|
||||
if (p->xeddsa_signed) {
|
||||
// Learn this node as a signer, so a later unsigned signable broadcast from it is dropped
|
||||
nodeInfoLiteSetBit(node, NODEINFO_BITFIELD_HAS_XEDDSA_SIGNED_MASK, true);
|
||||
LOG_DEBUG("Verified XEdDSA signature from 0x%08x", p->from);
|
||||
} else {
|
||||
LOG_WARN("XEdDSA signature verification failed from 0x%08x, dropping", p->from);
|
||||
return false;
|
||||
}
|
||||
} else {
|
||||
LOG_DEBUG("No public key for 0x%08x, cannot verify XEdDSA signature", p->from);
|
||||
}
|
||||
} else if (p->decoded.xeddsa_signature.size != 0) {
|
||||
// A signature field that is neither empty nor a full 64 bytes is malformed - honest
|
||||
// senders emit only those two sizes (perhapsEncode sets 0 or XEDDSA_SIGNATURE_SIZE). Drop
|
||||
// it: a crafted partial signature would otherwise land in the unsigned branch below while
|
||||
// its bytes inflated the size estimate, letting a forged broadcast dodge the downgrade drop.
|
||||
LOG_WARN("Malformed XEdDSA signature (%u bytes) from 0x%08x, dropping", (unsigned)p->decoded.xeddsa_signature.size,
|
||||
p->from);
|
||||
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.
|
||||
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))
|
||||
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) {
|
||||
LOG_WARN("Dropping unsigned broadcast from 0x%08x that previously signed", p->from);
|
||||
return false;
|
||||
}
|
||||
}
|
||||
}
|
||||
return true;
|
||||
}
|
||||
#endif
|
||||
|
||||
DecodeState perhapsDecode(meshtastic_MeshPacket *p)
|
||||
{
|
||||
concurrency::LockGuard g(cryptLock);
|
||||
@@ -542,35 +595,11 @@ DecodeState perhapsDecode(meshtastic_MeshPacket *p)
|
||||
p->decoded.want_response |= p->decoded.bitfield & BITFIELD_WANT_RESPONSE_MASK;
|
||||
|
||||
#if !(MESHTASTIC_EXCLUDE_PKI) && !(MESHTASTIC_EXCLUDE_XEDDSA)
|
||||
if (p->decoded.xeddsa_signature.size == XEDDSA_SIGNATURE_SIZE) {
|
||||
meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(p->from);
|
||||
if (node && node->public_key.size == 32) {
|
||||
p->xeddsa_signed =
|
||||
crypto->xeddsa_verify(node->public_key.bytes, p->from, p->id, p->decoded.portnum, p->decoded.payload.bytes,
|
||||
p->decoded.payload.size, p->decoded.xeddsa_signature.bytes);
|
||||
if (p->xeddsa_signed) {
|
||||
// Mark this node as a signer so future unsigned packets from it are rejected
|
||||
nodeInfoLiteSetBit(node, NODEINFO_BITFIELD_HAS_XEDDSA_SIGNED_MASK, true);
|
||||
LOG_DEBUG("Verified XEdDSA signature from 0x%08x", p->from);
|
||||
} else {
|
||||
LOG_WARN("XEdDSA signature verification failed from 0x%08x, dropping", p->from);
|
||||
return DecodeState::DECODE_FAILURE;
|
||||
}
|
||||
} else {
|
||||
LOG_DEBUG("No public key for 0x%08x, cannot verify XEdDSA signature", p->from);
|
||||
}
|
||||
} else {
|
||||
// Unsigned packet - only reject the class of packet a signing node always signs:
|
||||
// an unencrypted broadcast small enough to also carry a signature (see perhapsEncode()).
|
||||
// Unicast packets and oversized broadcasts are never signed, so they must not be
|
||||
// hard-failed here even if this node has signed before.
|
||||
const meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(p->from);
|
||||
if (node && nodeInfoLiteHasXeddsaSigned(node) && isBroadcast(p->to) &&
|
||||
p->decoded.payload.size + XEDDSA_SIGNATURE_SIZE < meshtastic_Constants_DATA_PAYLOAD_LEN) {
|
||||
LOG_WARN("Dropping unsigned broadcast from 0x%08x that previously signed", p->from);
|
||||
return DecodeState::DECODE_FAILURE;
|
||||
}
|
||||
}
|
||||
// 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))
|
||||
return DecodeState::DECODE_FAILURE;
|
||||
#endif
|
||||
|
||||
/* Not actually ever used.
|
||||
@@ -629,6 +658,20 @@ DecodeState perhapsDecode(meshtastic_MeshPacket *p)
|
||||
}
|
||||
}
|
||||
|
||||
#if !(MESHTASTIC_EXCLUDE_PKI) && !(MESHTASTIC_EXCLUDE_XEDDSA)
|
||||
/** Exact sender-side sign gate: would this Data still fit the LoRa frame with a 64-byte
|
||||
* signature attached? Sized with the real encoder so it tracks whatever fields are present. */
|
||||
static bool signedDataFits(meshtastic_Data *d)
|
||||
{
|
||||
const pb_size_t prevSize = d->xeddsa_signature.size;
|
||||
d->xeddsa_signature.size = XEDDSA_SIGNATURE_SIZE;
|
||||
size_t encodedSize;
|
||||
const bool sized = pb_get_encoded_size(&encodedSize, &meshtastic_Data_msg, d);
|
||||
d->xeddsa_signature.size = prevSize;
|
||||
return sized && encodedSize + MESHTASTIC_HEADER_LENGTH <= MAX_LORA_PAYLOAD_LEN;
|
||||
}
|
||||
#endif
|
||||
|
||||
/** Return 0 for success or a Routing_Error code for failure
|
||||
*/
|
||||
meshtastic_Routing_Error perhapsEncode(meshtastic_MeshPacket *p)
|
||||
@@ -643,11 +686,18 @@ meshtastic_Routing_Error perhapsEncode(meshtastic_MeshPacket *p)
|
||||
p->decoded.has_bitfield = true;
|
||||
p->decoded.bitfield |= (config.lora.config_ok_to_mqtt << BITFIELD_OK_TO_MQTT_SHIFT);
|
||||
p->decoded.bitfield |= (p->decoded.want_response << BITFIELD_WANT_RESPONSE_SHIFT);
|
||||
// We own signing for packets we originate; discard any signature a client preset.
|
||||
// Outside the XEdDSA guard: the field exists in the protobuf on every build, and a
|
||||
// stale/garbage signature transmitted by a non-signing build would hard-fail
|
||||
// verification at every XEdDSA-enabled receiver that knows our key.
|
||||
p->decoded.xeddsa_signature.size = 0;
|
||||
#if !(MESHTASTIC_EXCLUDE_PKI) && !(MESHTASTIC_EXCLUDE_XEDDSA)
|
||||
// Sign broadcast packets if payload + signature fits within the max Data payload.
|
||||
// The actual encoded size is checked after pb_encode (TOO_LARGE).
|
||||
if (!p->pki_encrypted && isBroadcast(p->to) &&
|
||||
p->decoded.payload.size + XEDDSA_SIGNATURE_SIZE < meshtastic_Constants_DATA_PAYLOAD_LEN) {
|
||||
// Sign broadcast packets when the Data still fits a LoRa frame with the signature
|
||||
// attached. This must be the exact encoded-size criterion, not a payload-size
|
||||
// heuristic: a heuristic band where we sign-then-fail-TOO_LARGE breaks packets that
|
||||
// were deliverable unsigned, and perhapsDecode() applies the mirror-image rule when
|
||||
// deciding whether an unsigned broadcast from a known signer is a downgrade.
|
||||
if (!p->pki_encrypted && isBroadcast(p->to) && signedDataFits(&p->decoded)) {
|
||||
if (crypto->xeddsa_sign(p->from, p->id, p->decoded.portnum, p->decoded.payload.bytes, p->decoded.payload.size,
|
||||
p->decoded.xeddsa_signature.bytes)) {
|
||||
p->decoded.xeddsa_signature.size = XEDDSA_SIGNATURE_SIZE;
|
||||
|
||||
@@ -175,6 +175,26 @@ DecodeState perhapsDecode(meshtastic_MeshPacket *p);
|
||||
*/
|
||||
meshtastic_Routing_Error perhapsEncode(meshtastic_MeshPacket *p);
|
||||
|
||||
#if !(MESHTASTIC_EXCLUDE_PKI) && !(MESHTASTIC_EXCLUDE_XEDDSA)
|
||||
/** XEdDSA receive-side signature policy. When the packet carries a 64-byte signature *and* the
|
||||
* sender's public key is known, verify it: on success learn the sender's signer bit, on failure
|
||||
* drop. If the key is unknown the signature is left unverified and the packet passes. A signature
|
||||
* of any other non-zero length is treated as malformed and dropped. For unsigned packets, enforce
|
||||
* 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 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);
|
||||
#endif
|
||||
|
||||
extern Router *router;
|
||||
|
||||
/// Generate a unique packet id
|
||||
|
||||
@@ -51,8 +51,10 @@ bool NodeInfoModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, mes
|
||||
}
|
||||
NodeNum sourceNum = getFrom(&mp);
|
||||
const meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(sourceNum);
|
||||
if (node && nodeInfoLiteHasXeddsaSigned(node) && !mp.xeddsa_signed) {
|
||||
LOG_WARN("Dropping unsigned NodeInfo from node 0x%08x that previously signed", sourceNum);
|
||||
// Broadcasts only: senders never sign unicast NodeInfo, so dropping it would break exchanges
|
||||
// with signer nodes. Backstops ingress that skips Router's downgrade drop (e.g. decoded MQTT).
|
||||
if (node && nodeInfoLiteHasXeddsaSigned(node) && !mp.xeddsa_signed && isBroadcast(mp.to)) {
|
||||
LOG_WARN("Dropping unsigned NodeInfo broadcast from node 0x%08x that previously signed", sourceNum);
|
||||
return true;
|
||||
}
|
||||
|
||||
|
||||
@@ -6,6 +6,7 @@
|
||||
#include "configuration.h"
|
||||
#include "main.h"
|
||||
#include "mesh/Channels.h"
|
||||
#include "mesh/CryptoEngine.h"
|
||||
#include "mesh/Router.h"
|
||||
#include "mesh/generated/meshtastic/mqtt.pb.h"
|
||||
#include "mesh/generated/meshtastic/telemetry.pb.h"
|
||||
@@ -131,6 +132,21 @@ inline void onReceiveProto(char *topic, byte *payload, size_t length)
|
||||
return;
|
||||
}
|
||||
p->channel = ch.index;
|
||||
#if !(MESHTASTIC_EXCLUDE_PKI) && !(MESHTASTIC_EXCLUDE_XEDDSA)
|
||||
// Already-decoded downlink skips perhapsDecode's crypto path entirely, so enforce the
|
||||
// signature policy here: verify a carried signature and apply unsigned-downgrade
|
||||
// protection for known signers. Without this, a peer on a plaintext broker could
|
||||
// impersonate a signing node with unsigned broadcasts. Hold cryptLock like the RF path
|
||||
// (perhapsDecode) does - checkXeddsaReceivePolicy -> xeddsa_verify mutates shared
|
||||
// CryptoEngine cache state, and MQTT ingress can run on a different task.
|
||||
{
|
||||
concurrency::LockGuard g(cryptLock);
|
||||
if (!checkXeddsaReceivePolicy(p.get())) {
|
||||
LOG_INFO("Ignore decoded message failing XEdDSA policy");
|
||||
return;
|
||||
}
|
||||
}
|
||||
#endif
|
||||
}
|
||||
|
||||
// PKI messages get accepted even if we can't decrypt
|
||||
|
||||
Reference in New Issue
Block a user