Accept a pending key only for the key-verification exchange (#11107)
perhapsDecode fell back to the not-yet-verified key held during a key verification handshake for any incoming PKI unicast. That key is supplied by whoever opened the handshake and proves only that they hold it, not that they are the node they claim to be, so until the session ended they could send DMs on any port that decrypted and were marked pki_encrypted. perhapsEncode already restricts the pending key to KEY_VERIFICATION_APP. The receive path now applies the same rule.
This commit is contained in:
co-authored by
GitHub
parent
61750d9118
commit
d9f8839241
+16
-1
@@ -577,7 +577,14 @@ DecodeState perhapsDecode(meshtastic_MeshPacket *p)
|
||||
// Resolve the sender's public key: prefer the one stored in NodeDB (hot store or warm tier), else
|
||||
// fall back to a not-yet-committed key held during an in-progress key-verification handshake.
|
||||
meshtastic_NodeInfoLite_public_key_t remotePublic = {0, {0}};
|
||||
bool haveRemoteKey = nodeDB->copyPublicKey(p->from, remotePublic) || crypto->getPendingPublicKey(p->from, remotePublic);
|
||||
bool haveRemoteKey = nodeDB->copyPublicKey(p->from, remotePublic);
|
||||
// A pending key is an unverified identity claim supplied by whoever opened the handshake, so it is
|
||||
// accepted only for the exchange itself (checked after decode). perhapsEncode applies the same rule.
|
||||
bool havePendingKey = false;
|
||||
if (!haveRemoteKey) {
|
||||
havePendingKey = crypto->getPendingPublicKey(p->from, remotePublic);
|
||||
haveRemoteKey = havePendingKey;
|
||||
}
|
||||
|
||||
meshtastic_NodeInfoLite *ourNode = nullptr;
|
||||
if (p->channel == 0 && isToUs(p) && p->to > 0 && !isBroadcast(p->to) && rawSize > MESHTASTIC_PKC_OVERHEAD &&
|
||||
@@ -585,8 +592,10 @@ DecodeState perhapsDecode(meshtastic_MeshPacket *p)
|
||||
// Try the sender's known key first, then each configured admin key so an authorized admin can
|
||||
// reach a node that has not yet learned their key. AES-CCM AEAD rejects wrong candidates.
|
||||
bool viaAdminKey = false;
|
||||
bool viaPendingKey = false;
|
||||
if (haveRemoteKey && crypto->decryptCurve25519(p->from, remotePublic, p->id, rawSize, p->encrypted.bytes, bytes)) {
|
||||
decrypted = true;
|
||||
viaPendingKey = havePendingKey;
|
||||
}
|
||||
for (int i = 0; i < 3 && !decrypted; i++) {
|
||||
if (config.security.admin_key[i].size != 32)
|
||||
@@ -607,6 +616,12 @@ DecodeState perhapsDecode(meshtastic_MeshPacket *p)
|
||||
size_t payloadSize = rawSize - MESHTASTIC_PKC_OVERHEAD;
|
||||
if (pb_decode_from_bytes(bytes, payloadSize, &meshtastic_Data_msg, &decodedtmp) &&
|
||||
decodedtmp.portnum != meshtastic_PortNum_UNKNOWN_APP) {
|
||||
if (viaPendingKey && decodedtmp.portnum != meshtastic_PortNum_KEY_VERIFICATION_APP) {
|
||||
// The pending key only proves the handshake initiator holds it, not that they are
|
||||
// p->from. Beyond the exchange it would let them send DMs that look authenticated.
|
||||
LOG_WARN("Refusing pending-key decrypt of port %u from 0x%08x", (unsigned)decodedtmp.portnum, p->from);
|
||||
return DecodeState::DECODE_FAILURE;
|
||||
}
|
||||
decrypted = true;
|
||||
rawSize = payloadSize; // commit the overhead subtraction only on full success
|
||||
LOG_INFO("Packet decrypted using PKI!");
|
||||
|
||||
@@ -202,6 +202,33 @@ void test_wrong_admin_key_does_not_decode(void)
|
||||
TEST_ASSERT_NULL(mockNodeDB->getMeshNode(ADMIN_NODE));
|
||||
}
|
||||
|
||||
// A pending key is an unverified identity claim from whoever opened a key-verification handshake, so it
|
||||
// must decrypt only the exchange itself. Otherwise they could send DMs that look PKI-authenticated as a
|
||||
// node they never proved they are.
|
||||
void test_pending_key_decrypts_only_key_verification(void)
|
||||
{
|
||||
// PEER is unknown to us; the handshake stashed its claimed key as pending.
|
||||
static constexpr NodeNum PEER = 0x0C0C0C0C;
|
||||
uint8_t peerPub[32], peerPriv[32];
|
||||
crypto->generateKeyPair(peerPub, peerPriv);
|
||||
crypto->setDHPrivateKey(ourPriv); // generateKeyPair changed it
|
||||
crypto->setPendingPublicKey(PEER, peerPub);
|
||||
|
||||
// A DM on any other port must not decode, even though the pending key would decrypt it.
|
||||
meshtastic_MeshPacket spoofed = makePkiPacket(PEER, meshtastic_PortNum_TEXT_MESSAGE_APP, 16, peerPriv);
|
||||
TEST_ASSERT_NOT_EQUAL_MESSAGE(DECODE_SUCCESS, perhapsDecode(&spoofed), "pending key must not decrypt a text DM");
|
||||
TEST_ASSERT_FALSE_MESSAGE(spoofed.pki_encrypted, "spoofed DM must not be marked PKI-authenticated");
|
||||
|
||||
// The key-verification exchange itself still works, so bootstrapping is unaffected.
|
||||
meshtastic_MeshPacket handshake = makePkiPacket(PEER, meshtastic_PortNum_KEY_VERIFICATION_APP, 16, peerPriv);
|
||||
TEST_ASSERT_EQUAL_MESSAGE(DECODE_SUCCESS, perhapsDecode(&handshake), "key verification must still decrypt");
|
||||
TEST_ASSERT_TRUE(handshake.pki_encrypted);
|
||||
|
||||
// A pending key is never persisted, so the peer stays unknown until verification commits it.
|
||||
TEST_ASSERT_NULL_MESSAGE(mockNodeDB->getMeshNode(PEER), "pending key must not be learned into NodeDB");
|
||||
crypto->clearPendingPublicKey();
|
||||
}
|
||||
|
||||
#endif // !(MESHTASTIC_EXCLUDE_PKI)
|
||||
|
||||
void setup()
|
||||
@@ -216,6 +243,7 @@ void setup()
|
||||
RUN_TEST(test_admin_key_slot2_only_decrypts);
|
||||
RUN_TEST(test_no_admin_key_unknown_sender_not_decoded);
|
||||
RUN_TEST(test_wrong_admin_key_does_not_decode);
|
||||
RUN_TEST(test_pending_key_decrypts_only_key_verification);
|
||||
#endif
|
||||
exit(UNITY_END());
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user