Skip to content

Commit 7af0355

Browse files
garthvhgarthvh
authored
Merge pull request #2107 from bruschill/security/ssrf-pki-hardening
Harden inbound public-key handling (first-wins / TOFU)
2 parents d729078 + 55af7ae commit 7af0355

6 files changed

Lines changed: 140 additions & 13 deletions

File tree

‎Meshtastic/Helpers/MeshPackets.swift‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -592,11 +592,9 @@ actor MeshPackets {
592592
modelContext.insert(newUserEntity)
593593
fetchedNode[0].user = newUserEntity
594594
}
595-
// Set the public key for a user if it is empty, don't update
596-
if fetchedNode[0].user?.publicKey == nil && !nodeInfo.user.publicKey.isEmpty {
597-
fetchedNode[0].user?.pkiEncrypted = true
598-
fetchedNode[0].user?.publicKey = nodeInfo.user.publicKey
599-
}
595+
// First-wins on the public key, consistent with the NodeInfo/User paths in UpdateSwiftData
596+
// (previously a `== nil` guard here silently ignored mismatches). See `applyInboundPublicKey`.
597+
fetchedNode[0].user?.applyInboundPublicKey(nodeInfo.user.publicKey, nodeNum: Int64(nodeInfo.num))
600598
fetchedNode[0].user?.userId = nodeInfo.num.toHex()
601599
fetchedNode[0].user?.num = Int64(nodeInfo.num)
602600
fetchedNode[0].user?.numString = String(nodeInfo.num)

‎Meshtastic/Model/UserEntity.swift‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
//
77

88
import Foundation
9+
import OSLog
910
import SwiftData
1011

1112
@Model
@@ -38,3 +39,50 @@ final class UserEntity {
3839

3940
init() {}
4041
}
42+
43+
// MARK: - First-wins public-key handling
44+
45+
extension UserEntity {
46+
47+
/// The result of applying an inbound public key to this user via `applyInboundPublicKey(_:nodeNum:)`.
48+
enum PublicKeyUpdateOutcome: Equatable {
49+
/// The inbound key was empty — nothing to apply.
50+
case ignoredEmpty
51+
/// No key was on file yet, so the inbound key was stored (and `pkiEncrypted` set).
52+
case stored
53+
/// The inbound key matched the stored key — no change.
54+
case matched
55+
/// A *different* key arrived for a contact that already has one. The stored key is kept
56+
/// (first-wins); `keyMatch` is cleared and `newPublicKey` records the rejected key so the UI
57+
/// trust indicators surface the possible key-substitution attempt.
58+
case mismatch
59+
}
60+
61+
/// Applies **first-wins** semantics for an inbound public key.
62+
///
63+
/// Once a non-empty key is stored for a contact it is never silently overwritten by a different
64+
/// inbound key — that would let any mesh/MQTT peer substitute a trusted contact's key and MITM the
65+
/// victim's PKC direct messages. A differing key is refused and surfaced to the UI (`keyMatch =
66+
/// false`, `newPublicKey = inbound`) rather than only logged; the stored key stands.
67+
///
68+
/// - Parameters:
69+
/// - inboundKey: The public key carried by the inbound NodeInfo/User protobuf.
70+
/// - nodeNum: The node number, used only for the security log line on a mismatch.
71+
/// - Returns: The outcome, primarily for tests and callers that want to react to a mismatch.
72+
@discardableResult
73+
func applyInboundPublicKey(_ inboundKey: Data, nodeNum: Int64) -> PublicKeyUpdateOutcome {
74+
guard !inboundKey.isEmpty else { return .ignoredEmpty }
75+
76+
if let storedKey = publicKey, !storedKey.isEmpty {
77+
guard storedKey != inboundKey else { return .matched }
78+
keyMatch = false
79+
newPublicKey = inboundKey
80+
Logger.data.error("🔐 [Security] Ignoring inbound public key change for node \(nodeNum, privacy: .public); a different key is already stored (possible key-substitution attempt).")
81+
return .mismatch
82+
}
83+
84+
pkiEncrypted = true
85+
publicKey = inboundKey
86+
return .stored
87+
}
88+
}

‎Meshtastic/Persistence/NodeBackupManager+Import.swift‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,11 @@ extension NodeBackupManager {
4040
let backupUsers = try backupContext.fetch(FetchDescriptor<UserEntity>())
4141
var usersByNum: [Int64: UserEntity] = [:]
4242
for src in backupUsers {
43+
// Each `dst` is a freshly-created entity that we insert into the live context — this copy never
44+
// overwrites the public key of an already-populated *live* record, so the inbound-mesh
45+
// first-wins key protection (UpdateSwiftData / MeshPackets) doesn't apply here. This is a
46+
// user-initiated restore of the user's own trusted backup, carrying `keyMatch`/`newPublicKey`
47+
// forward verbatim.
4348
let dst = UserEntity()
4449
dst.hwDisplayName = src.hwDisplayName
4550
dst.hwModel = src.hwModel

‎Meshtastic/Persistence/UpdateSwiftData.swift‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -511,10 +511,8 @@ extension MeshPackets {
511511
fetchedNode[0].user?.unmessagable = false
512512
}
513513
}
514-
if !nodeInfoMessage.user.publicKey.isEmpty {
515-
fetchedNode[0].user?.pkiEncrypted = true
516-
fetchedNode[0].user?.publicKey = nodeInfoMessage.user.publicKey
517-
}
514+
// Security (finding H1): first-wins on the public key. See `applyInboundPublicKey`.
515+
fetchedNode[0].user?.applyInboundPublicKey(nodeInfoMessage.user.publicKey, nodeNum: Int64(nodeInfoMessage.num))
518516
if let user = fetchedNode.first?.user {
519517
let fetchHwModel2 = Int64(user.hwModelId)
520518
let hwDescriptor2 = FetchDescriptor<DeviceHardwareEntity>(
@@ -546,10 +544,8 @@ extension MeshPackets {
546544
let containsRole = roles.contains(Int(fetchedNode[0].user?.role ?? -1))
547545
fetchedNode[0].user?.unmessagable = containsRole
548546
}
549-
if !userMessage.publicKey.isEmpty {
550-
fetchedNode[0].user?.pkiEncrypted = true
551-
fetchedNode[0].user?.publicKey = userMessage.publicKey
552-
}
547+
// Security (finding H1): first-wins on the public key. See `applyInboundPublicKey`.
548+
fetchedNode[0].user?.applyInboundPublicKey(userMessage.publicKey, nodeNum: Int64(packet.from))
553549
if packet.hopStart != 0 && packet.hopLimit <= packet.hopStart {
554550
fetchedNode[0].hopsAway = Int32(packet.hopStart - packet.hopLimit)
555551
}

‎Meshtastic/Views/Settings/Config/SecurityConfig.swift‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,10 @@ struct SecurityConfig: View {
281281
// Should show a saved successfully alert once I know that to be true
282282
// for now just disable the button after a successful save
283283
if keyUpdated {
284+
// This is the *local* node's own keypair being changed deliberately by the user in the
285+
// Security config screen (gated behind `keyUpdated` + an explicit save), not an inbound
286+
// mesh key — so the first-wins protection doesn't apply. `keyMatch`/`newPublicKey` track
287+
// *remote* contacts' keys, so they are intentionally left untouched here.
284288
node?.user?.publicKey = Data(base64Encoded: publicKey) ?? Data()
285289
do {
286290
try context.save()

‎MeshtasticTests/UserEntityPublicKeyTests.swift‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
// UserEntityPublicKeyTests.swift
2+
// MeshtasticTests
3+
4+
import Testing
5+
import Foundation
6+
@testable import Meshtastic
7+
8+
/// Covers `UserEntity.applyInboundPublicKey(_:nodeNum:)` — the first-wins public-key handling shared
9+
/// by the three inbound ingestion paths (UpdateSwiftData NodeInfo + User, MeshPackets NodeInfo).
10+
@Suite("UserEntity.applyInboundPublicKey")
11+
@MainActor
12+
struct UserEntityApplyInboundPublicKeyTests {
13+
14+
private let keyA = Data([0x01, 0x02, 0x03, 0x04])
15+
private let keyB = Data([0xAA, 0xBB, 0xCC, 0xDD])
16+
17+
@Test("stores the first non-empty key when none is on file")
18+
func storesFirstKey() {
19+
let user = UserEntity()
20+
let outcome = user.applyInboundPublicKey(keyA, nodeNum: 42)
21+
22+
#expect(outcome == .stored)
23+
#expect(user.publicKey == keyA)
24+
#expect(user.pkiEncrypted == true)
25+
#expect(user.keyMatch == true) // untouched — no mismatch
26+
#expect(user.newPublicKey == nil)
27+
}
28+
29+
@Test("treats a non-nil but empty stored key as no key and stores")
30+
func emptyStoredKeyIsTreatedAsNoKey() {
31+
let user = UserEntity()
32+
user.publicKey = Data() // non-nil but empty — must NOT count as a stored key
33+
let outcome = user.applyInboundPublicKey(keyA, nodeNum: 42)
34+
35+
#expect(outcome == .stored)
36+
#expect(user.publicKey == keyA)
37+
}
38+
39+
@Test("ignores an empty inbound key")
40+
func ignoresEmptyInboundKey() {
41+
let user = UserEntity()
42+
user.publicKey = keyA
43+
user.pkiEncrypted = true
44+
let outcome = user.applyInboundPublicKey(Data(), nodeNum: 42)
45+
46+
#expect(outcome == .ignoredEmpty)
47+
#expect(user.publicKey == keyA) // unchanged
48+
#expect(user.keyMatch == true)
49+
}
50+
51+
@Test("a matching inbound key is a no-op")
52+
func matchingKeyIsNoOp() {
53+
let user = UserEntity()
54+
user.publicKey = keyA
55+
user.pkiEncrypted = true
56+
let outcome = user.applyInboundPublicKey(keyA, nodeNum: 42)
57+
58+
#expect(outcome == .matched)
59+
#expect(user.publicKey == keyA)
60+
#expect(user.keyMatch == true)
61+
#expect(user.newPublicKey == nil)
62+
}
63+
64+
@Test("a different inbound key is refused and surfaced to the UI (key-substitution attempt)")
65+
func mismatchKeepsStoredKeyAndFlagsUI() {
66+
let user = UserEntity()
67+
user.publicKey = keyA
68+
user.pkiEncrypted = true
69+
let outcome = user.applyInboundPublicKey(keyB, nodeNum: 42)
70+
71+
#expect(outcome == .mismatch)
72+
#expect(user.publicKey == keyA) // first-wins: stored key stands
73+
#expect(user.keyMatch == false) // drives the red key.slash indicator
74+
#expect(user.newPublicKey == keyB) // records the rejected key
75+
}
76+
}

0 commit comments

Comments
 (0)