From c845f9222c7299a8158637cdf023101417560158 Mon Sep 17 00:00:00 2001 From: James Rich <2199651+jamesarich@users.noreply.github.com> Date: Sat, 14 Feb 2026 07:47:59 -0600 Subject: [PATCH] feat(db): enhance public key conflict handling (#4555) Signed-off-by: James Rich <2199651+jamesarich@users.noreply.github.com> --- AGENTS.md | 1 + core/database/README.md | 20 ++++ .../core/database/dao/NodeInfoDaoTest.kt | 108 +++++++++++++++-- .../core/database/dao/NodeInfoDao.kt | 111 ++++++++++-------- 4 files changed, 180 insertions(+), 60 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 17406e2e8..ee2a47094 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -103,6 +103,7 @@ This file serves as a comprehensive guide for AI agents and developers working o - **Protobuf:** Communication with the device uses Protobufs. The definitions are in `core/proto`. This is a Git submodule, but the build system handles it. - **Legacy:** Some code in `app/` uses the `com.geeksville.mesh` package. Newer code in `core/` and `feature/` uses `org.meshtastic.*`. Respect the existing package structure of the file you are editing. - **Versioning:** Do not manually edit `versionCode` or `versionName`. These are managed by the build system and CI/CD. +- **Database Safety:** When modifying critical database logic (e.g., `NodeInfoDao`), always ensure you have explicit test coverage for security edge cases (like PKC conflicts or key wiping). Refer to `core/database/src/androidTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoTest.kt` for examples. ## 7. Troubleshooting diff --git a/core/database/README.md b/core/database/README.md index de5944b5b..d38c73a12 100644 --- a/core/database/README.md +++ b/core/database/README.md @@ -1,5 +1,25 @@ # `:core:database` +This module provides the local Room database persistence layer for the application. + +## Key Components + +- **`MeshtasticDatabase`**: The main Room database class. +- **DAOs (Data Access Objects)**: + - `NodeInfoDao`: Manages storage and retrieval of node information (`NodeEntity`). Contains critical logic for handling Public Key Conflict (PKC) resolution and preventing identity wiping attacks. + - `PacketDao`: Handles storage of mesh packets. + - `ChatMessageDao`: Manages chat message history. +- **Entities**: + - `NodeEntity`: Represents a node on the mesh. + - `PacketEntity`: Represents a stored packet. + +## Security Considerations + +### Public Key Conflict (PKC) Handling +The `NodeInfoDao` implements specific logic to protect against impersonation and "wipe" attacks: +- **Wipe Protection**: Receiving an `is_licensed=true` packet (which normally clears the public key for compliance) will **not** clear an existing valid public key if one is already known. This prevents attackers from sending fake licensed packets to wipe keys from the DB. +- **Conflict Detection**: If a new key arrives for an existing node ID that conflicts with a known valid key, the key is set to `ERROR_BYTE_STRING` to flag the potential impersonation. + ## Module dependency graph diff --git a/core/database/src/androidTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoTest.kt b/core/database/src/androidTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoTest.kt index 0571e6396..4ca6e26f7 100644 --- a/core/database/src/androidTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoTest.kt +++ b/core/database/src/androidTest/kotlin/org/meshtastic/core/database/dao/NodeInfoDaoTest.kt @@ -38,7 +38,6 @@ import org.meshtastic.core.database.model.Node import org.meshtastic.core.database.model.NodeSortOption import org.meshtastic.core.model.util.onlineTimeThreshold import org.meshtastic.proto.HardwareModel -import org.meshtastic.proto.Position import org.meshtastic.proto.User @RunWith(AndroidJUnit4::class) @@ -256,14 +255,14 @@ class NodeInfoDaoTest { @Test fun testSortByAlpha() = runBlocking { val nodes = getNodes(sort = NodeSortOption.ALPHABETICAL) - val sortedNodes = nodes.sortedBy { it.user.long_name?.uppercase() ?: "" } + val sortedNodes = nodes.sortedBy { it.user.long_name.uppercase() } assertEquals(sortedNodes, nodes) } @Test fun testSortByDistance() = runBlocking { val nodes = getNodes(sort = NodeSortOption.DISTANCE) - fun NodeEntity.toNode() = Node(num = num, user = user, position = position ?: Position()) + fun NodeEntity.toNode() = Node(num = num, user = user, position = position) val sortedNodes = nodes.sortedWith( // nodes with invalid (null) positions at the end compareBy { it.validPosition == null }.thenBy { it.distance(ourNode.toNode()) }, @@ -281,7 +280,7 @@ class NodeInfoDaoTest { @Test fun testSortByViaMqtt() = runBlocking { val nodes = getNodes(sort = NodeSortOption.VIA_MQTT) - val sortedNodes = nodes.sortedBy { it.user.long_name?.contains("(MQTT)") == true } + val sortedNodes = nodes.sortedBy { it.user.long_name.contains("(MQTT)") } assertEquals(sortedNodes, nodes) } @@ -339,8 +338,14 @@ class NodeInfoDaoTest { @Test fun testPkcMismatch() = runBlocking { + val newNodeNum = 9999 // First, ensure the node is in the DB with Key A - val nodeA = testNodes[10].copy(publicKey = ByteArray(32) { 1 }.toByteString()) + val nodeA = + testNodes[0].copy( + num = newNodeNum, + publicKey = ByteArray(32) { 1 }.toByteString(), + user = testNodes[0].user.copy(id = "!uniqueId1", public_key = ByteArray(32) { 1 }.toByteString()), + ) nodeInfoDao.upsert(nodeA) // Now upsert with Key B (mismatch) @@ -358,9 +363,15 @@ class NodeInfoDaoTest { @Test fun testRoutineUpdatePreservesKey() = runBlocking { + val newNodeNum = 9998 // First, ensure the node is in the DB with Key A val keyA = ByteArray(32) { 1 }.toByteString() - val nodeA = testNodes[10].copy(publicKey = keyA, user = testNodes[10].user.copy(public_key = keyA)) + val nodeA = + testNodes[0].copy( + num = newNodeNum, + publicKey = keyA, + user = testNodes[0].user.copy(id = "!uniqueId2", public_key = keyA), + ) nodeInfoDao.upsert(nodeA) // Now upsert with an empty key (common in position/telemetry updates) @@ -374,11 +385,13 @@ class NodeInfoDaoTest { @Test fun testRecoveryFromErrorState() = runBlocking { + val newNodeNum = 9997 // Start in Error state val nodeError = - testNodes[10].copy( + testNodes[0].copy( + num = newNodeNum, publicKey = NodeEntity.ERROR_BYTE_STRING, - user = testNodes[10].user.copy(public_key = NodeEntity.ERROR_BYTE_STRING), + user = testNodes[0].user.copy(id = "!uniqueId3", public_key = NodeEntity.ERROR_BYTE_STRING), ) nodeInfoDao.doUpsert(nodeError) assertTrue(nodeInfoDao.getNodeByNum(nodeError.num)!!.toModel().mismatchKey) @@ -394,13 +407,19 @@ class NodeInfoDaoTest { } @Test - fun testLicensedUserClearsKey() = runBlocking { + fun testLicensedUserDoesNotClearKey() = runBlocking { + val newNodeNum = 9996 // Start with a key val keyA = ByteArray(32) { 1 }.toByteString() - val nodeA = testNodes[10].copy(publicKey = keyA, user = testNodes[10].user.copy(public_key = keyA)) + val nodeA = + testNodes[0].copy( + num = newNodeNum, + publicKey = keyA, + user = testNodes[0].user.copy(id = "!uniqueId4", public_key = keyA), + ) nodeInfoDao.upsert(nodeA) - // Upsert as licensed user + // Upsert as licensed user (without key) val nodeLicensed = nodeA.copy( user = nodeA.user.copy(is_licensed = true, public_key = ByteString.EMPTY), @@ -409,7 +428,72 @@ class NodeInfoDaoTest { nodeInfoDao.upsert(nodeLicensed) val stored = nodeInfoDao.getNodeByNum(nodeA.num)!!.node - assertTrue(stored.publicKey == null || (stored.publicKey?.size ?: 0) == 0) + // Should NOT clear key to prevent PKC wipe attack + assertEquals(keyA, stored.publicKey) assertFalse(stored.toModel().mismatchKey) } + + @Test + fun testValidLicensedUserNoKey() = runBlocking { + val newNodeNum = 9995 + // Start with no key and licensed status + val nodeLicensed = + testNodes[0].copy( + num = newNodeNum, + publicKey = null, + user = testNodes[0].user.copy(id = "!uniqueId5", is_licensed = true, public_key = ByteString.EMPTY), + ) + nodeInfoDao.upsert(nodeLicensed) + + val stored = nodeInfoDao.getNodeByNum(newNodeNum)!!.node + assertEquals(ByteString.EMPTY, stored.publicKey) + assertFalse(stored.toModel().mismatchKey) + } + + @Test + fun testPlaceholderUpdatePreservesIdentity() = runBlocking { + val newNodeNum = 9994 + val keyA = ByteArray(32) { 5 }.toByteString() + val originalName = "Real Name" + // 1. Create a full node with key and name + val fullNode = + testNodes[0].copy( + num = newNodeNum, + longName = originalName, + publicKey = keyA, + user = + testNodes[0] + .user + .copy( + id = "!uniqueId6", + long_name = originalName, + public_key = keyA, + hw_model = HardwareModel.TLORA_V2, // Set a specific HW model + ), + ) + nodeInfoDao.upsert(fullNode) + + // 2. Simulate receiving a placeholder packet (e.g. from a legacy node or partial info) + // HW Model UNSET, Default Name "Meshtastic XXXX" + val placeholderNode = + fullNode.copy( + user = + fullNode.user.copy( + hw_model = HardwareModel.UNSET, + long_name = "Meshtastic 1234", + public_key = ByteString.EMPTY, + ), + longName = "Meshtastic 1234", + publicKey = null, + ) + nodeInfoDao.upsert(placeholderNode) + + // 3. Verify that the identity (Name and Key) is preserved + val stored = nodeInfoDao.getNodeByNum(newNodeNum)!!.node + assertEquals(originalName, stored.longName) + assertEquals(keyA, stored.publicKey) + // Ensure HW model is NOT overwritten by UNSET if we preserve the user + // Note: The logic in handleExistingNodeUpsertValidation copies the *existing* user back. + assertEquals(HardwareModel.TLORA_V2, stored.user.hw_model) + } } diff --git a/core/database/src/main/kotlin/org/meshtastic/core/database/dao/NodeInfoDao.kt b/core/database/src/main/kotlin/org/meshtastic/core/database/dao/NodeInfoDao.kt index a56abe304..999ee8489 100644 --- a/core/database/src/main/kotlin/org/meshtastic/core/database/dao/NodeInfoDao.kt +++ b/core/database/src/main/kotlin/org/meshtastic/core/database/dao/NodeInfoDao.kt @@ -35,6 +35,10 @@ import org.meshtastic.proto.HardwareModel @Dao interface NodeInfoDao { + companion object { + const val KEY_SIZE = 32 + } + /** * Verifies a [NodeEntity] before an upsert operation. It handles populating the publicKey for lazy migration, * checks for public key conflicts with new nodes, and manages updates to existing nodes, particularly in cases of @@ -84,64 +88,75 @@ interface NodeInfoDao { return newNode } + /** + * Resolves the public key for an existing node during an update. + * + * This function implements safety checks to prevent public key conflicts (PKC) and ensure robust handling of key + * updates. + * + * @param existingNode The current state of the node in the database. + * @param incomingNode The new node data being upserted. + * @return The resolved [ByteString] for the public key: + * - [NodeEntity.ERROR_BYTE_STRING]: If there is a mismatch between a valid existing key and a new incoming key. + * - `incomingNode.publicKey`: If the incoming key is new, matches the existing one, or if recovering from an error + * state. + * - `existingNode.publicKey`: If the incoming update has no key, or if the user is licensed but already has a valid + * key (prevents wiping). + * - [ByteString.EMPTY]: If the user is licensed and didn't previously have a key (or if key is explicitly cleared). + */ + private fun resolvePublicKey(existingNode: NodeEntity, incomingNode: NodeEntity): ByteString? { + val existingKey = existingNode.publicKey ?: existingNode.user.public_key + val incomingKey = incomingNode.publicKey + + val incomingHasKey = (incomingKey?.size ?: 0) == KEY_SIZE + val existingHasKey = (existingKey?.size ?: 0) == KEY_SIZE && existingKey != NodeEntity.ERROR_BYTE_STRING + + return when { + incomingHasKey -> { + if (existingHasKey && incomingKey != existingKey) { + // Actual mismatch between two non-empty keys + NodeEntity.ERROR_BYTE_STRING + } else { + // New key, same key, or recovery from Error state + incomingKey + } + } + existingHasKey -> existingKey + incomingNode.user.is_licensed -> ByteString.EMPTY + else -> existingKey + } + } + + /** + * Handles the validation logic when upserting an existing node. + * + * It distinguishes between two scenarios: + * 1. **Preservation**: If the incoming update is a placeholder (unset HW model) with a default name, and the + * existing node has full user info, we preserve the existing identity (user, keys, names, verification) while + * updating telemetry and status fields from the incoming packet. + * 2. **Update**: If it's a normal update, we validate the public key using [resolvePublicKey] to prevent conflicts + * or accidental key wiping, and then update the node. + */ @Suppress("CyclomaticComplexMethod", "MagicNumber") private fun handleExistingNodeUpsertValidation(existingNode: NodeEntity, incomingNode: NodeEntity): NodeEntity { + val resolvedNotes = incomingNode.notes.ifBlank { existingNode.notes } + val isPlaceholder = incomingNode.user.hw_model == HardwareModel.UNSET val hasExistingUser = existingNode.user.hw_model != HardwareModel.UNSET val isDefaultName = incomingNode.user.long_name?.matches(Regex("^Meshtastic [0-9a-fA-F]{4}$")) == true - val shouldPreserve = hasExistingUser && isPlaceholder && isDefaultName - - if (shouldPreserve) { - // Preserve existing name and user info, but update metadata like lastHeard, SNR, and position. - val resolvedNotes = if (incomingNode.notes.isBlank()) existingNode.notes else incomingNode.notes - return existingNode.copy( - lastHeard = incomingNode.lastHeard, - snr = incomingNode.snr, - rssi = incomingNode.rssi, - position = incomingNode.position, - hopsAway = incomingNode.hopsAway, - deviceTelemetry = incomingNode.deviceTelemetry, - environmentTelemetry = incomingNode.environmentTelemetry, - powerTelemetry = incomingNode.powerTelemetry, - paxcounter = incomingNode.paxcounter, - channel = incomingNode.channel, - viaMqtt = incomingNode.viaMqtt, - isFavorite = incomingNode.isFavorite, - isIgnored = incomingNode.isIgnored, - isMuted = incomingNode.isMuted, + if (hasExistingUser && isPlaceholder && isDefaultName) { + return incomingNode.copy( + user = existingNode.user, + publicKey = existingNode.publicKey, + longName = existingNode.longName, + shortName = existingNode.shortName, + manuallyVerified = existingNode.manuallyVerified, notes = resolvedNotes, ) } - val existingKey = existingNode.publicKey ?: existingNode.user.public_key - val incomingKey = incomingNode.publicKey - - val incomingHasKey = (incomingKey?.size ?: 0) == 32 - val existingHasKey = (existingKey?.size ?: 0) == 32 && existingKey != NodeEntity.ERROR_BYTE_STRING - - val resolvedKey = - when { - incomingHasKey -> { - if (existingHasKey && incomingKey != existingKey) { - // Actual mismatch between two non-empty keys - NodeEntity.ERROR_BYTE_STRING - } else { - // New key, same key, or recovery from Error state - incomingKey - } - } - incomingNode.user.is_licensed -> { - // Explicitly clear key for licensed (HAM) users - ByteString.EMPTY - } - else -> { - // Routine update without key: preserve what we have (even if it's currently Error) - existingKey - } - } - - val resolvedNotes = if (incomingNode.notes.isBlank()) existingNode.notes else incomingNode.notes + val resolvedKey = resolvePublicKey(existingNode, incomingNode) return incomingNode.copy( user = incomingNode.user.copy(public_key = resolvedKey ?: ByteString.EMPTY),