Limit node database churn from unauthenticated packets - #11102
Merged
Conversation
updateUser created the node entry before checking the signature gate, so a refused identity update had already evicted another node and written the warm tier. The gate now runs against the existing record first. updateFrom admits a node for any decoded packet, and mp.from is unauthenticated header data, so invented node numbers could churn the database at packet rate and push real neighbours out. Admission is now spaced once the database is full. Only the packet-driven path is limited; explicit admission still evicts freely and discovery is untouched while slots remain.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughNodeDB now validates unsigned identity updates before allocation and changes decoded-packet admission to avoid unnecessary churn. When the database is full, new unknown node admission is throttled using a timestamp and interval constant. ChangesNodeDB admission safeguards
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/mesh/NodeDB.cpp (2)
3423-3427: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCondense the explanatory comments.
As per coding guidelines, code comments should be kept minimal (one or two lines maximum) without adding multi-paragraph explanatory blocks. Please condense this comment block.
♻️ Proposed refactor
- // Admitting a node this way costs an eviction once the database is full, and on some platforms a - // warm-tier write. mp.from is unauthenticated header data, so a flood of invented node numbers - // would otherwise churn the database at packet rate and push real neighbours out. Only the - // packet-driven path is limited, and only while the database is full: explicit admission still - // evicts freely, and discovery is untouched while slots remain. + // Throttle new node admission from unauthenticated packets when the database is full + // to prevent rapid eviction of real neighbors by spoofed node numbers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/NodeDB.cpp` around lines 3423 - 3427, Condense the comment immediately preceding the packet-driven node admission logic to one or two lines, retaining only that unauthenticated packet node numbers are rate-limited when the database is full to prevent eviction churn. Remove the extended platform, eviction, and unaffected-path explanation.Source: Coding guidelines
3318-3321: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCondense the explanatory comments.
As per coding guidelines, code comments should be kept minimal (one or two lines maximum) without adding multi-paragraph explanatory blocks. Please condense this comment block.
♻️ Proposed refactor
- // Once a node has proven it signs, only a signed update may change its identity. The public-key guard - // below is no help - an attacker can replay the victim's real (public) key. Our own record is exempt. - // Checked against the existing record before getOrCreateMeshNode, because creating an entry can evict - // another node and write the warm tier; a refused update must not leave those side effects behind. + // Pre-creation check: only signed updates can change a known signer's identity. + // Prevents replay attacks and stops refused updates from causing side-effect evictions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/NodeDB.cpp` around lines 3318 - 3321, Condense the comment above the existing-record check into one or two lines, retaining only that signed updates may change a proven node’s identity and that the local record is exempt. Remove the detailed attack, public-key, eviction, and warm-tier explanations without changing the surrounding logic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/mesh/NodeDB.cpp`:
- Around line 3423-3427: Condense the comment immediately preceding the
packet-driven node admission logic to one or two lines, retaining only that
unauthenticated packet node numbers are rate-limited when the database is full
to prevent eviction churn. Remove the extended platform, eviction, and
unaffected-path explanation.
- Around line 3318-3321: Condense the comment above the existing-record check
into one or two lines, retaining only that signed updates may change a proven
node’s identity and that the local record is exempt. Remove the detailed attack,
public-key, eviction, and warm-tier explanations without changing the
surrounding logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 355e841b-1722-4755-8d4c-669af87f3d0e
📒 Files selected for processing (2)
src/mesh/NodeDB.cppsrc/mesh/NodeDB.h
Member
Author
|
Both comment blocks condensed. No logic change; native-windows still builds clean. |
Contributor
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (30)
Build artifacts expire on 2026-08-19. Updated for |
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
1 participant
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.
Two related problems on the unauthenticated NodeDB path.
Ordering.
updateUser()calledgetOrCreateMeshNode()before the signature gate added in 5ded0ec. Creating an entry can evict another node and write the warm tier, so a refused identity update had already caused both side effects before returning false. The gate now runs against the existing record, and the entry is created only once it passes. Behaviour is otherwise identical: the gate only ever fired for nodes already present and already known to sign.Admission churn.
updateFrom()creates an entry for any decoded packet, andmp.fromis unauthenticated header data. On a channel whose key is known, including the default public channel, invented node numbers churn the database at packet rate. Eviction prefers keyless nodes, so a flood pushes real keyless neighbours out, and each eviction also writes the warm tier.Admission from the packet path is now spaced once the database is full. The limit is deliberately narrow:
An earlier revision put the throttle inside
getOrCreateMeshNode();test_nodedb_blockedcorrectly rejected it, because that also gated eviction paths that must succeed. Narrowing it toupdateFrom()addresses the threat without changing those semantics.The nRF52840 warm store was also examined. Its immediate
persistEntrywrite is by design, buffered through the shared flash page cache withsaveIfDirty()as the durability point, so it is left alone.test_nodedb_blocked, test_packet_signing, test_warm_store and test_optin_migration pass, 75 cases.
Summary by CodeRabbit