From 53e510199a7bf87d1029797d0c12c339b6d6b212 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Tue, 21 Jul 2026 03:29:56 +0200 Subject: [PATCH] Bound remote-initiated key verification sessions (#11101) * Bound remote-initiated key verification sessions Opening a session raises a 30 second banner and a client notification and occupies the only verification slot, all before the peer has authenticated anything, and nothing limited how often that could happen. Adds an absolute session cap that incoming packets cannot refresh, a cooldown between remote-initiated sessions measured from when the previous one ended, and refreshes the idle deadline only when the protocol actually advances rather than on any arriving packet. The busy path now sets ignoreRequest so it no longer answers with a NAK. Also replaces the getTime() - 60 timeout comparison, which underflowed before the clock passed 60. * Use elapsed-time comparison for the session timeout and condense comments --- src/modules/KeyVerificationModule.cpp | 37 +++++++++++++++++++++++---- src/modules/KeyVerificationModule.h | 6 +++++ 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/src/modules/KeyVerificationModule.cpp b/src/modules/KeyVerificationModule.cpp index f1483e90e..88410f671 100644 --- a/src/modules/KeyVerificationModule.cpp +++ b/src/modules/KeyVerificationModule.cpp @@ -6,12 +6,19 @@ #include "gps/RTC.h" #include "graphics/draw/MenuHandler.h" #include "main.h" +#include "mesh/Throttle.h" #include "meshUtils.h" #include "modules/AdminModule.h" #include "modules/NodeInfoModule.h" #include #include +#define KEY_VERIFICATION_TIMEOUT_SECS 60 +// Hard cap on one session, independent of the refreshable idle timeout above. +#define KEY_VERIFICATION_MAX_SESSION_MS (3 * 60 * 1000UL) +// Minimum spacing between remote-initiated sessions. +#define KEY_VERIFICATION_REMOTE_COOLDOWN_MS (60 * 1000UL) + KeyVerificationModule *keyVerificationModule; namespace @@ -64,7 +71,8 @@ AdminMessageHandleResult KeyVerificationModule::handleAdminMessageForModule(cons bool KeyVerificationModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshtastic_KeyVerification *r) { - updateState(); + // No refresh here: this runs before the sender is checked, so any node could hold the session open. + updateState(false); // Note: pki_encrypted is not required here. The first response (M2) may arrive channel-encrypted in // the bootstrap case; the follow-on hash1 packet (M3) is required to be PKI in its branch below. if (mp.from != currentRemoteNode) { // because the inital connection request is handled in allocReply() @@ -98,6 +106,7 @@ bool KeyVerificationModule::handleReceivedProtobuf(const meshtastic_MeshPacket & service->sendClientNotification(cn); } LOG_INFO("Received hash2"); + currentNonceTimestamp = getTime(); // the protocol advanced, so extend the deadline currentState = KEY_VERIFICATION_SENDER_AWAITING_NUMBER; return true; @@ -134,6 +143,7 @@ bool KeyVerificationModule::handleReceivedProtobuf(const meshtastic_MeshPacket & service->sendClientNotification(cn); } + currentNonceTimestamp = getTime(); // the protocol advanced, so extend the deadline currentState = KEY_VERIFICATION_RECEIVER_AWAITING_USER; return true; } @@ -160,6 +170,7 @@ bool KeyVerificationModule::sendInitialRequest(NodeNum remoteNode) CryptRNG.rand((uint8_t *)¤tNonce, sizeof(currentNonce)); } currentNonceTimestamp = getTime(); + sessionStartedMs = millis(); currentRemoteNode = remoteNode; meshtastic_KeyVerification KeyVerification = meshtastic_KeyVerification_init_zero; KeyVerification.nonce = currentNonce; @@ -190,10 +201,19 @@ meshtastic_MeshPacket *KeyVerificationModule::allocReply() SHA256 hash; NodeNum ourNodeNum = nodeDB->getNodeNum(); updateState(); - if (currentState != KEY_VERIFICATION_IDLE) { // TODO: cooldown period + if (currentState != KEY_VERIFICATION_IDLE) { LOG_WARN("Key Verification requested, but already in a request"); + ignoreRequest = true; // do not let the busy path emit a NAK back to the requester return nullptr; } + // Opening a session raises a banner and locks the only slot, before the peer has authenticated. + if (lastRemoteSessionMs != 0 && Throttle::isWithinTimespanMs(lastRemoteSessionMs, KEY_VERIFICATION_REMOTE_COOLDOWN_MS)) { + LOG_WARN("Key Verification requested, but within cooldown"); + ignoreRequest = true; + return nullptr; + } + sessionStartedMs = millis(); + sessionFromRemote = true; currentState = KEY_VERIFICATION_RECEIVER_AWAITING_HASH1; auto req = *currentRequest; @@ -357,11 +377,14 @@ void KeyVerificationModule::processSecurityNumber(uint32_t incomingNumber) void KeyVerificationModule::updateState(bool resetTimer) { if (currentState != KEY_VERIFICATION_IDLE) { - // check for the 60 second timeout - if (currentNonceTimestamp < getTime() - 60) { + uint32_t now = getTime(); + // Absolute cap first: it is millis() based, so it bounds the session even when the RTC is unset. + if (!Throttle::isWithinTimespanMs(sessionStartedMs, KEY_VERIFICATION_MAX_SESSION_MS)) { + resetToIdle(); + } else if (now - currentNonceTimestamp >= KEY_VERIFICATION_TIMEOUT_SECS) { resetToIdle(); } else if (resetTimer) { - currentNonceTimestamp = getTime(); + currentNonceTimestamp = now; } } } @@ -370,8 +393,12 @@ void KeyVerificationModule::resetToIdle() { memset(hash1, 0, 32); memset(hash2, 0, 32); + if (sessionFromRemote) + lastRemoteSessionMs = millis(); // start the cooldown when the session ends, not when it opened + sessionFromRemote = false; currentNonce = 0; currentNonceTimestamp = 0; + sessionStartedMs = 0; currentSecurityNumber = 0; currentRemoteNode = 0; currentState = KEY_VERIFICATION_IDLE; diff --git a/src/modules/KeyVerificationModule.h b/src/modules/KeyVerificationModule.h index 9496649d3..d33a89c5e 100644 --- a/src/modules/KeyVerificationModule.h +++ b/src/modules/KeyVerificationModule.h @@ -84,6 +84,12 @@ class KeyVerificationModule : public ProtobufModule private: uint64_t currentNonce = 0; uint32_t currentNonceTimestamp = 0; + // millis() the session opened, never refreshed, so a peer cannot hold the slot open indefinitely. + uint32_t sessionStartedMs = 0; + // millis() a remote-initiated session last ended. Spacing sessions bounds the slot DoS and the + // banner spam; stamped at the end rather than the start so the cooldown is a real gap. + uint32_t lastRemoteSessionMs = 0; + bool sessionFromRemote = false; NodeNum currentRemoteNode = 0; uint32_t currentSecurityNumber = 0; KeyVerificationState currentState = KEY_VERIFICATION_IDLE;