KeyVerificationModule: null-check getMeshNode() before dereference#10258
Open
nightjoker7 wants to merge 5 commits into
Open
KeyVerificationModule: null-check getMeshNode() before dereference#10258nightjoker7 wants to merge 5 commits into
nightjoker7 wants to merge 5 commits into
Conversation
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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens KeyVerificationModule against NodeDB eviction races by null-checking nodeDB->getMeshNode(currentRemoteNode) before dereferencing node fields during key-verification state transitions, preventing firmware crashes and allowing verification to cancel/continue gracefully.
Changes:
- Add null checks around
getMeshNode(currentRemoteNode)dereferences in multiple key-verification flows (admin verify, hash2 request, hash1 accept, notifications). - Reuse an already-null-checked
remoteNodePtrinprocessSecurityNumber()instead of re-queryingNodeDB. - When the node is missing, fall back to an empty remote long name and (in one path) log a warning.
Review feedback from @Copilot on PR meshtastic#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
Contributor
Author
|
Addressed in bfaa1cfed: at each of the 4 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Six sites in
KeyVerificationModule.cppdereferencednodeDB->getMeshNode(currentRemoteNode)->user(or->bitfield) without a null check. Four of these carry a literal// should really check for nulls, etccomment from the original commit, acknowledging the bug was known at merge time.getMeshNode()returnsNULLwhen the node number isn't in the NodeDB — which can happen between key-verification state transitions if the DB evicts the remote node (eg. DB fills up while we wait for the user to punch in a Security Number on the banner overlay). When that happens, the firmware crashes instead of cancelling verification cleanly.Sites fixed
handleAdminMessageForModule—DO_VERIFYpath: null-check before settingNODEINFO_BITFIELD_IS_KEY_MANUALLY_VERIFIED_MASK.handleReceivedProtobuf— hash2-received path: null-check before copyinguser.long_nameinto the Security Number Request ClientNotification.handleReceivedProtobuf— hash1-match banner callback: null-check inside the lambda before setting the verified bitfield.handleReceivedProtobuf— hash1-match notification: null-check before copyinguser.long_nameinto the Final ClientNotification.allocReply— null-check before copyinguser.long_nameinto the Number Inform ClientNotification.processSecurityNumber— reuse theremoteNodePtralready null-checked at the top of the function, instead of callinggetMeshNode()a second time.When the node is missing, the long_name copy falls back to an empty string so the notification still fires — blank remote name is better than a crash.
Test plan
nodeDB->getMeshNode(currentRemoteNode)->dereferences without a preceding null check in this fileprocessSecurityNumberalready guards site some minor cleanups #6'sremoteNodePtrreuseThis is part of an ongoing null-deref sweep across the codebase; related PRs in flight: #10226 (Router PKI decode), #10229 (MeshService has_user check).