From b863db9e9472b446c1d0f196392db58b518ebb49 Mon Sep 17 00:00:00 2001 From: nightjoker7 Date: Wed, 22 Apr 2026 21:55:01 -0500 Subject: [PATCH 1/2] KeyVerificationModule: null-check getMeshNode() before dereference Six sites dereferenced nodeDB->getMeshNode(currentRemoteNode)->user or ->bitfield without null check. Four of them were flagged with the comment "should really check for nulls, etc" in the original commit. NodeDB can evict a node between key-verification state transitions (e.g. if the DB fills up with other heard nodes while we wait for the user to punch in a security number), at which point getMeshNode returns NULL and the dereference crashes. Fixes: - handleAdminMessageForModule DO_VERIFY: null-check before setting IS_KEY_MANUALLY_VERIFIED_MASK bitfield - handleReceivedProtobuf (hash2 path): null-check before copying long_name into ClientNotification - handleReceivedProtobuf (hash1 match banner callback): null-check inside lambda before setting bitfield - handleReceivedProtobuf (hash1 match notification): null-check before copying long_name - allocReply: null-check before copying long_name - processSecurityNumber: reuse the remoteNodePtr already null-checked at the top of the function instead of calling getMeshNode again When the node is missing, the long_name copy uses an empty string so the notification still fires but with a blank remote name. --- src/modules/KeyVerificationModule.cpp | 28 +++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/src/modules/KeyVerificationModule.cpp b/src/modules/KeyVerificationModule.cpp index 6d0255d5355..ecc30619aef 100644 --- a/src/modules/KeyVerificationModule.cpp +++ b/src/modules/KeyVerificationModule.cpp @@ -37,7 +37,11 @@ AdminMessageHandleResult KeyVerificationModule::handleAdminMessageForModule(cons } else if (request->key_verification.message_type == meshtastic_KeyVerificationAdmin_MessageType_DO_VERIFY && request->key_verification.nonce == currentNonce) { auto remoteNodePtr = nodeDB->getMeshNode(currentRemoteNode); - remoteNodePtr->bitfield |= NODEINFO_BITFIELD_IS_KEY_MANUALLY_VERIFIED_MASK; + if (remoteNodePtr != nullptr) { + remoteNodePtr->bitfield |= NODEINFO_BITFIELD_IS_KEY_MANUALLY_VERIFIED_MASK; + } else { + LOG_WARN("Key verification DO_VERIFY: remote node 0x%x no longer in NodeDB", currentRemoteNode); + } resetToIdle(); } else if (request->key_verification.message_type == meshtastic_KeyVerificationAdmin_MessageType_DO_NOT_VERIFY) { resetToIdle(); @@ -67,13 +71,14 @@ bool KeyVerificationModule::handleReceivedProtobuf(const meshtastic_MeshPacket & keyVerificationModule->processSecurityNumber(number_picked); });) + auto remoteNodePtr = nodeDB->getMeshNode(currentRemoteNode); meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed(); cn->level = meshtastic_LogRecord_Level_WARNING; sprintf(cn->message, "Enter Security Number for Key Verification"); cn->which_payload_variant = meshtastic_ClientNotification_key_verification_number_request_tag; cn->payload_variant.key_verification_number_request.nonce = currentNonce; - strncpy(cn->payload_variant.key_verification_number_request.remote_longname, // should really check for nulls, etc - nodeDB->getMeshNode(currentRemoteNode)->user.long_name, + strncpy(cn->payload_variant.key_verification_number_request.remote_longname, + remoteNodePtr != nullptr ? remoteNodePtr->user.long_name : "", sizeof(cn->payload_variant.key_verification_number_request.remote_longname)); service->sendClientNotification(cn); LOG_INFO("Received hash2"); @@ -95,17 +100,20 @@ bool KeyVerificationModule::handleReceivedProtobuf(const meshtastic_MeshPacket & [=](int selected) { if (selected == 1) { auto remoteNodePtr = nodeDB->getMeshNode(currentRemoteNode); - remoteNodePtr->bitfield |= NODEINFO_BITFIELD_IS_KEY_MANUALLY_VERIFIED_MASK; + if (remoteNodePtr != nullptr) { + remoteNodePtr->bitfield |= NODEINFO_BITFIELD_IS_KEY_MANUALLY_VERIFIED_MASK; + } } }; screen->showOverlayBanner(options);) + auto remoteNodePtr = nodeDB->getMeshNode(currentRemoteNode); meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed(); cn->level = meshtastic_LogRecord_Level_WARNING; sprintf(cn->message, "Final confirmation for incoming manual key verification %s", message); cn->which_payload_variant = meshtastic_ClientNotification_key_verification_final_tag; cn->payload_variant.key_verification_final.nonce = currentNonce; - strncpy(cn->payload_variant.key_verification_final.remote_longname, // should really check for nulls, etc - nodeDB->getMeshNode(currentRemoteNode)->user.long_name, + strncpy(cn->payload_variant.key_verification_final.remote_longname, + remoteNodePtr != nullptr ? remoteNodePtr->user.long_name : "", sizeof(cn->payload_variant.key_verification_final.remote_longname)); cn->payload_variant.key_verification_final.isSender = false; service->sendClientNotification(cn); @@ -196,14 +204,15 @@ meshtastic_MeshPacket *KeyVerificationModule::allocReply() responsePacket->pki_encrypted = true; IF_SCREEN(snprintf(message, 25, "Security Number \n%03u %03u", currentSecurityNumber / 1000, currentSecurityNumber % 1000); screen->showSimpleBanner(message, 30000); LOG_WARN("%s", message);) + auto remoteNodePtr = nodeDB->getMeshNode(currentRemoteNode); meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed(); cn->level = meshtastic_LogRecord_Level_WARNING; sprintf(cn->message, "Incoming Key Verification.\nSecurity Number\n%03u %03u", currentSecurityNumber / 1000, currentSecurityNumber % 1000); cn->which_payload_variant = meshtastic_ClientNotification_key_verification_number_inform_tag; cn->payload_variant.key_verification_number_inform.nonce = currentNonce; - strncpy(cn->payload_variant.key_verification_number_inform.remote_longname, // should really check for nulls, etc - nodeDB->getMeshNode(currentRemoteNode)->user.long_name, + strncpy(cn->payload_variant.key_verification_number_inform.remote_longname, + remoteNodePtr != nullptr ? remoteNodePtr->user.long_name : "", sizeof(cn->payload_variant.key_verification_number_inform.remote_longname)); cn->payload_variant.key_verification_number_inform.security_number = currentSecurityNumber; service->sendClientNotification(cn); @@ -265,8 +274,7 @@ void KeyVerificationModule::processSecurityNumber(uint32_t incomingNumber) sprintf(cn->message, "Final confirmation for outgoing manual key verification %s", message); cn->which_payload_variant = meshtastic_ClientNotification_key_verification_final_tag; cn->payload_variant.key_verification_final.nonce = currentNonce; - strncpy(cn->payload_variant.key_verification_final.remote_longname, // should really check for nulls, etc - nodeDB->getMeshNode(currentRemoteNode)->user.long_name, + strncpy(cn->payload_variant.key_verification_final.remote_longname, remoteNodePtr->user.long_name, sizeof(cn->payload_variant.key_verification_final.remote_longname)); cn->payload_variant.key_verification_final.isSender = true; service->sendClientNotification(cn); From bfaa1cfed1e905cef18bd421ac9537ff2b0957e8 Mon Sep 17 00:00:00 2001 From: nightjoker7 Date: Thu, 23 Apr 2026 16:10:40 -0500 Subject: [PATCH 2/2] Address Copilot review: NUL-terminate strncpy at 4 sites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback from @Copilot on PR #10258: strncpy(dst, src, sizeof(dst)) does NOT guarantee NUL-termination when the source length reaches or exceeds the buffer size. meshtastic_User.long_name is 40 bytes (39 usable chars + NUL), and remote_longname destinations are similarly sized — a 40-char long_name would fill the buffer and leave it unterminated. Downstream clients would then read garbage past the buffer. Fix: at each of the 4 strncpy call sites, copy sizeof(dst) - 1 bytes and explicitly set the final byte to '\0'. Works whether the source is short (strncpy zero-pads the trailing bytes naturally) or long (we terminate at index sizeof-1). Sites: - handleReceivedProtobuf hash2 path - handleReceivedProtobuf hash1-match banner callback path - allocReply number-inform path - processSecurityNumber final-confirmation path --- src/modules/KeyVerificationModule.cpp | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/modules/KeyVerificationModule.cpp b/src/modules/KeyVerificationModule.cpp index ecc30619aef..01e43d8bb53 100644 --- a/src/modules/KeyVerificationModule.cpp +++ b/src/modules/KeyVerificationModule.cpp @@ -79,7 +79,9 @@ bool KeyVerificationModule::handleReceivedProtobuf(const meshtastic_MeshPacket & cn->payload_variant.key_verification_number_request.nonce = currentNonce; strncpy(cn->payload_variant.key_verification_number_request.remote_longname, remoteNodePtr != nullptr ? remoteNodePtr->user.long_name : "", - sizeof(cn->payload_variant.key_verification_number_request.remote_longname)); + sizeof(cn->payload_variant.key_verification_number_request.remote_longname) - 1); + cn->payload_variant.key_verification_number_request + .remote_longname[sizeof(cn->payload_variant.key_verification_number_request.remote_longname) - 1] = '\0'; service->sendClientNotification(cn); LOG_INFO("Received hash2"); currentState = KEY_VERIFICATION_SENDER_AWAITING_NUMBER; @@ -114,7 +116,9 @@ bool KeyVerificationModule::handleReceivedProtobuf(const meshtastic_MeshPacket & cn->payload_variant.key_verification_final.nonce = currentNonce; strncpy(cn->payload_variant.key_verification_final.remote_longname, remoteNodePtr != nullptr ? remoteNodePtr->user.long_name : "", - sizeof(cn->payload_variant.key_verification_final.remote_longname)); + sizeof(cn->payload_variant.key_verification_final.remote_longname) - 1); + cn->payload_variant.key_verification_final + .remote_longname[sizeof(cn->payload_variant.key_verification_final.remote_longname) - 1] = '\0'; cn->payload_variant.key_verification_final.isSender = false; service->sendClientNotification(cn); @@ -213,7 +217,9 @@ meshtastic_MeshPacket *KeyVerificationModule::allocReply() cn->payload_variant.key_verification_number_inform.nonce = currentNonce; strncpy(cn->payload_variant.key_verification_number_inform.remote_longname, remoteNodePtr != nullptr ? remoteNodePtr->user.long_name : "", - sizeof(cn->payload_variant.key_verification_number_inform.remote_longname)); + sizeof(cn->payload_variant.key_verification_number_inform.remote_longname) - 1); + cn->payload_variant.key_verification_number_inform + .remote_longname[sizeof(cn->payload_variant.key_verification_number_inform.remote_longname) - 1] = '\0'; cn->payload_variant.key_verification_number_inform.security_number = currentSecurityNumber; service->sendClientNotification(cn); LOG_WARN("Security Number %04u, nonce %llu", currentSecurityNumber, currentNonce); @@ -275,7 +281,9 @@ void KeyVerificationModule::processSecurityNumber(uint32_t incomingNumber) cn->which_payload_variant = meshtastic_ClientNotification_key_verification_final_tag; cn->payload_variant.key_verification_final.nonce = currentNonce; strncpy(cn->payload_variant.key_verification_final.remote_longname, remoteNodePtr->user.long_name, - sizeof(cn->payload_variant.key_verification_final.remote_longname)); + sizeof(cn->payload_variant.key_verification_final.remote_longname) - 1); + cn->payload_variant.key_verification_final + .remote_longname[sizeof(cn->payload_variant.key_verification_final.remote_longname) - 1] = '\0'; cn->payload_variant.key_verification_final.isSender = true; service->sendClientNotification(cn); LOG_INFO(message);