refactor: Improve node public key handling and security (#2395)

Signed-off-by: James Rich <2199651+jamesarich@users.noreply.github.com>
This commit is contained in:
James Rich 2025-07-09 14:43:10 +00:00 committed by GitHub
parent 93dc691625
commit 9259e21aed
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 800 additions and 35 deletions

View file

@ -23,6 +23,7 @@ import com.geeksville.mesh.MeshProtos
import com.geeksville.mesh.PaxcountProtos
import com.geeksville.mesh.TelemetryProtos
import com.geeksville.mesh.android.Logging
import com.google.protobuf.ByteString
import com.google.protobuf.InvalidProtocolBufferException
import kotlinx.serialization.json.Json
@ -145,4 +146,14 @@ class Converters : Logging {
}
return Json.encodeToString(list)
}
@TypeConverter
fun bytesToByteString(bytes: ByteArray?): ByteString? {
return if (bytes == null) null else ByteString.copyFrom(bytes)
}
@TypeConverter
fun byteStringToBytes(value: ByteString?): ByteArray? {
return value?.toByteArray()
}
}

View file

@ -71,8 +71,9 @@ import com.geeksville.mesh.database.entity.ReactionEntity
AutoMigration(from = 15, to = 16),
AutoMigration(from = 16, to = 17),
AutoMigration(from = 17, to = 18),
AutoMigration(from = 18, to = 19),
],
version = 18,
version = 19,
exportSchema = true,
)
@TypeConverters(Converters::class)

View file

@ -25,17 +25,62 @@ import androidx.room.Query
import androidx.room.Transaction
import androidx.room.Upsert
import com.geeksville.mesh.android.BuildUtils.warn
import com.geeksville.mesh.copy
import com.geeksville.mesh.database.entity.MetadataEntity
import com.geeksville.mesh.database.entity.MyNodeEntity
import com.geeksville.mesh.database.entity.NodeEntity
import com.geeksville.mesh.database.entity.NodeWithRelations
import com.google.protobuf.ByteString
import kotlinx.coroutines.flow.Flow
@Suppress("TooManyFunctions")
@Dao
interface NodeInfoDao {
// Helper function to contain all validation logic
private fun getVerifiedNodeForUpsert(node: NodeEntity): NodeEntity? {
// Populate the new publicKey field for lazy migration
node.publicKey = node.user.publicKey
val existingNode = getNodeByNum(node.num)?.node
return if (existingNode == null) {
// This is a new node. We must check if its public key is already claimed by another node.
if (node.publicKey != null && node.publicKey?.isEmpty == false) {
val nodeWithSamePK = findNodeByPublicKey(node.publicKey)
if (nodeWithSamePK != null && nodeWithSamePK.num != node.num) {
// This is the impersonation attempt we want to block.
@Suppress("MaxLineLength")
warn("NodeInfoDao: Blocking new node #${node.num} because its public key is already used by #${nodeWithSamePK.num}.")
return null // ABORT
}
}
// If we're here, the new node is safe to add.
node
} else {
// This is an update to an existing node.
val keyMatch =
existingNode.user.publicKey == node.user.publicKey || existingNode.user.publicKey.isEmpty
if (keyMatch) {
// Keys match, trust the incoming node completely.
// This allows for legit nodeId changes etc.
node
} else {
// Keys do NOT match. This is a potential attack.
// Log it, and create a NEW entity based on the EXISTING trusted one,
// only updating dynamic data and setting the public key to EMPTY to signal a conflict.
@Suppress("MaxLineLength")
warn("NodeInfoDao: Received packet for #${node.num} with non-matching public key. Identity data ignored, key set to EMPTY.")
existingNode.copy(
lastHeard = node.lastHeard,
snr = node.snr,
position = node.position,
user = existingNode.user.toBuilder().setPublicKey(ByteString.EMPTY).build(),
publicKey = ByteString.EMPTY
)
}
}
}
@Query("SELECT * FROM my_node")
fun getMyNodeInfo(): Flow<MyNodeEntity?>
@ -113,40 +158,18 @@ interface NodeInfoDao {
lastHeardMin: Int,
): Flow<List<NodeWithRelations>>
@Upsert
@Transaction
fun upsert(node: NodeEntity) {
val found = getNodeByNum(node.num)?.node
found?.let {
val keyMatch = !it.hasPKC || it.user.publicKey == node.user.publicKey
it.user = if (keyMatch) {
node.user
} else {
node.user.copy {
warn("Public key mismatch from $longName ($shortName)")
publicKey = NodeEntity.ERROR_BYTE_STRING
}
}
}
doUpsert(node)
getVerifiedNodeForUpsert(node)?.let { doUpsert(it) }
}
@Insert(onConflict = OnConflictStrategy.REPLACE)
@Suppress("NestedBlockDepth")
@Transaction
fun putAll(nodes: List<NodeEntity>) {
nodes.forEach { node ->
val found = getNodeByNum(node.num)?.node
found?.let {
val keyMatch = !it.hasPKC || it.user.publicKey == node.user.publicKey
it.user = if (keyMatch) {
node.user
} else {
node.user.copy {
warn("Public key mismatch from $longName ($shortName)")
publicKey = NodeEntity.ERROR_BYTE_STRING
}
}
}
val safeNodes = nodes.mapNotNull { getVerifiedNodeForUpsert(it) }
if (safeNodes.isNotEmpty()) {
doPutAll(safeNodes)
}
doPutAll(nodes)
}
@Query("DELETE FROM nodes")
@ -165,6 +188,9 @@ interface NodeInfoDao {
@Transaction
fun getNodeByNum(num: Int): NodeWithRelations?
@Query("SELECT * FROM nodes WHERE public_key = :publicKey LIMIT 1")
fun findNodeByPublicKey(publicKey: ByteString?): NodeEntity?
@Upsert
fun doUpsert(node: NodeEntity)

View file

@ -35,6 +35,7 @@ import com.geeksville.mesh.copy
import com.geeksville.mesh.model.Node
import com.geeksville.mesh.util.onlineTimeThreshold
import com.google.protobuf.ByteString
import com.google.protobuf.kotlin.isNotEmpty
data class NodeWithRelations(
@Embedded val node: NodeEntity,
@ -143,6 +144,9 @@ data class NodeEntity(
@ColumnInfo(typeAffinity = ColumnInfo.BLOB)
var paxcounter: PaxcountProtos.Paxcount = PaxcountProtos.Paxcount.getDefaultInstance(),
@ColumnInfo(name = "public_key")
var publicKey: ByteString? = null,
) {
val deviceMetrics: TelemetryProtos.DeviceMetrics
get() = deviceTelemetry.deviceMetrics
@ -151,8 +155,7 @@ data class NodeEntity(
get() = environmentTelemetry.environmentMetrics
val isUnknownUser get() = user.hwModel == MeshProtos.HardwareModel.UNSET
val hasPKC get() = !user.publicKey.isEmpty
val errorByteString: ByteString get() = ERROR_BYTE_STRING
val hasPKC get() = (publicKey ?: user.publicKey).isNotEmpty()
fun setPosition(p: MeshProtos.Position, defaultTime: Int = currentTime()) {
position = p.copy { time = if (p.time != 0) p.time else defaultTime }

View file

@ -29,6 +29,8 @@ import com.geeksville.mesh.database.entity.NodeEntity
import com.geeksville.mesh.util.GPSFormat
import com.geeksville.mesh.util.latLongToMeter
import com.geeksville.mesh.util.toDistanceString
import com.google.protobuf.ByteString
import com.google.protobuf.kotlin.isNotEmpty
@Suppress("MagicNumber")
data class Node(
@ -48,6 +50,7 @@ data class Node(
val environmentMetrics: EnvironmentMetrics = EnvironmentMetrics.getDefaultInstance(),
val powerMetrics: PowerMetrics = PowerMetrics.getDefaultInstance(),
val paxcounter: PaxcountProtos.Paxcount = PaxcountProtos.Paxcount.getDefaultInstance(),
val publicKey: ByteString? = null,
) {
val colors: Pair<Int, Int>
get() { // returns foreground and background @ColorInt for each 'num'
@ -59,8 +62,8 @@ data class Node(
}
val isUnknownUser get() = user.hwModel == MeshProtos.HardwareModel.UNSET
val hasPKC get() = !user.publicKey.isEmpty
val mismatchKey get() = user.publicKey == NodeEntity.ERROR_BYTE_STRING
val hasPKC get() = (publicKey ?: user.publicKey).isNotEmpty()
val mismatchKey get() = (publicKey ?: user.publicKey) == NodeEntity.ERROR_BYTE_STRING
val hasEnvironmentMetrics: Boolean
get() = environmentMetrics != EnvironmentMetrics.getDefaultInstance()