From 351f8861c7414d393669c43f19c792b14a99603a Mon Sep 17 00:00:00 2001 From: Shubham Bhandari Date: Mon, 14 Sep 2026 16:39:02 +0800 Subject: [PATCH 1/5] Bind a verified seal to the nickname it was earned under MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A verified key can rename itself onto a nickname the user trusts and keep drawing the seal beside the new one. Verification binds a FINGERPRINT, which is right. But the seal is rendered beside a self-claimed nickname, and nothing binds those two together: 1. Eve announces "ravi" and gets verified in person by someone. 2. Eve announces "medic". The rename is free and silent — PeerManager computes `nicknameChanged` only to decide whether to refresh the list. 3. Every device that verified Eve now shows a second trusted-looking medic. This is the Android mirror of permissionlesstech/bitchat#1708. It is smaller, because this app has no vouching: there is no transitive trust to launder onward, only the seal itself to withdraw. The binding `SecureIdentityStateManager` records the nickname a key was announcing when it was verified, in its own store. Deliberately NOT the existing `cached_fingerprint_nicknames`, which is a LAST SEEN cache overwritten on every peer-list refresh — comparing against that would always match and catch nothing. Pinned on verification and on re-verification (the user just checked this key again, under whatever name it shows now), cleared on unverify. Never pinned from `resolvePeerDisplayName`, which falls back to a truncated peerID: that is an identifier, not a claimed name, and pinning it would drop a seal on the peer's first real announce. A missing baseline never suppresses. Peers verified by earlier builds have none, and dropping their seals on upgrade would teach people to ignore the signal. Where the seal comes from Four surfaces, all gated: the connected peer row and the peer sheet (via `ChatViewModel.isPeerVerified`), the conversation row, and offline favourite rows. The last two are checked against the name THEY render rather than a live announce — an offline favourite shows a name held in the favourites record, so asking about a "current" name it is not announcing would answer nothing. `VerificationHandler.isPeerVerified` / `isNoisePublicKeyVerified` have no callers today and are gated anyway: leaving them ungated would hand the next caller the answer this change exists to stop giving. One surface is deliberately half covered In the fingerprint sheet the seal GLYPH and its green tint are withheld, and the word "verified" is not. The key genuinely is verified, so saying otherwise would be false — and leaving the sheet untouched would let someone tap through from a row whose seal just vanished and be reassured by a green checkmark. The right fix is a sentence that says both, and a new string has to ship in all 34 locales; machine-translating a security warning is not something to do in passing. Happy to wire it if someone supplies the wording. Comparison rules NFC, then a locale-independent case fold, then NFC again — folding can itself emit decomposed sequences, and `Locale.ROOT` is not optional or a Turkish phone would disagree with every other device about whether a peer had renamed. Recasing your own nickname is not a rename; a fullwidth or Cyrillic look-alike IS one. A trailing `#abcd` is stripped before comparing, ASCII hex only, since `Char.isDigit()` accepts fullwidth digits and would truncate a nickname literally ending in "#ABCD" into something that could match a baseline it is not. Tests `NicknameBindingTest`, 13 cases: the rename attack, the rename-onto-a- look-alike cases, recasing, combining accents, Turkish dotted I, the hash suffix and the fullwidth-hex trap, "@" in a nickname, and both fail-open paths. The logic lives in a pure-Kotlin `NicknameBinding` object with no Android imports precisely so the security decision is testable without a view. Co-Authored-By: Claude Opus 5 --- .../android/identity/NicknameBinding.kt | 81 ++++++++++ .../identity/SecureIdentityStateManager.kt | 76 +++++++++- .../com/bitchat/android/ui/ChatViewModel.kt | 30 +++- .../bitchat/android/ui/MeshPeerListSheet.kt | 10 +- .../android/ui/SecurityVerificationSheet.kt | 24 ++- .../bitchat/android/ui/VerificationHandler.kt | 73 ++++++++- .../android/identity/NicknameBindingTest.kt | 142 ++++++++++++++++++ 7 files changed, 422 insertions(+), 14 deletions(-) create mode 100644 app/src/main/java/com/bitchat/android/identity/NicknameBinding.kt create mode 100644 app/src/test/java/com/bitchat/android/identity/NicknameBindingTest.kt diff --git a/app/src/main/java/com/bitchat/android/identity/NicknameBinding.kt b/app/src/main/java/com/bitchat/android/identity/NicknameBinding.kt new file mode 100644 index 00000000..d5883443 --- /dev/null +++ b/app/src/main/java/com/bitchat/android/identity/NicknameBinding.kt @@ -0,0 +1,81 @@ +package com.bitchat.android.identity + +import java.text.Normalizer +import java.util.Locale + +/** + * The rule that decides whether a verified seal still applies to the name a + * peer is currently announcing. + * + * A verification binds a FINGERPRINT, which is right — but it is *rendered* + * beside a self-claimed nickname, and nothing binds those two together. So a + * key that gets verified once under any name can rename itself onto a nickname + * the user trusts and keep drawing the seal beside the new one. See + * [SecureIdentityStateManager.setVerifiedFingerprint]. + * + * Deliberately free of Android imports so it is reachable from a plain JVM unit + * test: this is the whole security decision, and it should not be testable only + * through a view. + */ +object NicknameBinding { + + /** + * The form two nicknames are compared in to decide whether they are the + * SAME NAME. + * + * NFC, then a locale-independent case fold, then NFC again — case folding + * can itself emit decomposed sequences (Turkish İ lowercases to i + U+0307), + * so normalising only once leaves two spellings of one name unequal. + * + * `Locale.ROOT` is not optional. `lowercase()` with the default locale makes + * a Turkish phone fold `I` to `ı` while every other device folds it to `i`, + * so two users would disagree about whether a peer had renamed. + * + * Deliberately NFC and **not** NFKC: a fullwidth `Medic` merely *looks* + * like `Medic`, so it is a different name and must break the binding. + * Folding look-alikes is a different question — whether two peers on screen + * need telling apart — and answering it here would let a vouch for one name + * quietly cover another. + */ + fun bindingKey(nickname: String): String = + nfc(nfc(nickname).lowercase(Locale.ROOT)) + + private fun nfc(s: String): String = Normalizer.normalize(s, Normalizer.Form.NFC) + + /** + * Strips ONLY a trailing `#abcd` collision suffix, leaving everything else + * alone. + * + * ASCII hex only. `Char.isDigit()` and friends accept fullwidth digits, so a + * nickname literally ending in `#ABCD` would otherwise be truncated and + * could then match a baseline it is not — a seal on a name that was never + * verified, which is the whole subject of this change. + */ + fun withoutCollisionSuffix(name: String): String { + if (name.length < 5) return name + val tail = name.substring(name.length - 5) + if (tail[0] != '#') return name + for (i in 1 until 5) { + val c = tail[i] + val isAsciiHex = c in '0'..'9' || c in 'a'..'f' || c in 'A'..'F' + if (!isAsciiHex) return name + } + return name.substring(0, name.length - 5) + } + + /** + * Does a seal earned under [pinned] still apply to a peer announcing + * [current]? + * + * Fails **open** when nothing was pinned. Peers verified by builds from + * before this existed have no baseline, and dropping their seals on upgrade + * would teach people to ignore the signal — which costs more than the + * narrow case it would catch. + */ + fun sealApplies(pinned: String?, current: String?): Boolean { + if (pinned.isNullOrEmpty()) return true + val shown = withoutCollisionSuffix(current.orEmpty()) + if (shown.isEmpty()) return true + return bindingKey(pinned) == bindingKey(shown) + } +} diff --git a/app/src/main/java/com/bitchat/android/identity/SecureIdentityStateManager.kt b/app/src/main/java/com/bitchat/android/identity/SecureIdentityStateManager.kt index 1efa87a2..b95f62e2 100644 --- a/app/src/main/java/com/bitchat/android/identity/SecureIdentityStateManager.kt +++ b/app/src/main/java/com/bitchat/android/identity/SecureIdentityStateManager.kt @@ -31,6 +31,11 @@ class SecureIdentityStateManager { private const val KEY_SIGNING_PRIVATE_KEY = "signing_private_key" private const val KEY_SIGNING_PUBLIC_KEY = "signing_public_key" private const val KEY_VERIFIED_FINGERPRINTS = "verified_fingerprints" + // The nickname each fingerprint was announcing when it was verified. + // Distinct from KEY_CACHED_FINGERPRINT_NICKNAMES, which is a LAST SEEN + // cache overwritten on every peer-list refresh — comparing against that + // would always match and catch nothing. + private const val KEY_VERIFIED_NICKNAMES = "verified_nicknames_v1" private const val KEY_CACHED_PEER_FINGERPRINTS = "cached_peer_fingerprints" private const val KEY_CACHED_PEER_NOISE_KEYS = "cached_peer_noise_keys" private const val KEY_CACHED_NOISE_FINGERPRINTS = "cached_noise_fingerprints" @@ -223,7 +228,14 @@ class SecureIdentityStateManager { return getVerifiedFingerprints().contains(fingerprint) } - fun setVerifiedFingerprint(fingerprint: String, verified: Boolean) { + /** + * @param nickname the name this peer was announcing at the moment it was + * verified. Recorded so the seal can be bound to it — see + * [getVerifiedNickname]. Optional so callers that genuinely have no name + * to hand (a verification completed before the first announce arrived) + * can omit it and fail open rather than pin an empty string. + */ + fun setVerifiedFingerprint(fingerprint: String, verified: Boolean, nickname: String? = null) { if (!isValidFingerprint(fingerprint)) return synchronized(lock) { val current = prefs.getStringSet(KEY_VERIFIED_FINGERPRINTS, emptySet())?.toMutableSet() ?: mutableSetOf() @@ -234,6 +246,68 @@ class SecureIdentityStateManager { } prefs.edit { putStringSet(KEY_VERIFIED_FINGERPRINTS, current) } } + if (verified) { + // Re-verifying overwrites: the user just checked this key again, in + // person, under whatever name it presents now. + if (!nickname.isNullOrBlank()) pinVerifiedNickname(fingerprint, nickname) + } else { + clearVerifiedNickname(fingerprint) + } + } + + // MARK: - Verified nicknames + // + // A verification binds a FINGERPRINT, which is right. But the seal is + // *rendered* beside a self-claimed nickname, and until now nothing bound + // those two together: a key verified once under any name could rename + // itself onto a nickname the user trusts and keep the seal beside the new + // one. The rename is free and silent — `PeerManager` computes + // `nicknameChanged` only to decide whether to refresh the list. + // + // The receiver is the only party that can hold this binding, because it is + // the one that decided to trust this key while it was presenting a + // particular name. + + /** The nickname [fingerprint] was announcing when it was verified, or null + * if nothing was bound. */ + fun getVerifiedNickname(fingerprint: String): String? { + if (!isValidFingerprint(fingerprint)) return null + val key = fingerprint.lowercase() + val entries = prefs.getStringSet(KEY_VERIFIED_NICKNAMES, emptySet()) ?: return null + val entry = entries.firstOrNull { it.startsWith("$key=") } ?: return null + return runCatching { + String(Base64.decode(entry.substringAfter('='), Base64.NO_WRAP), Charsets.UTF_8) + }.getOrNull() + } + + /** + * True only when a baseline exists AND this peer now announces something + * else. Fails OPEN on a missing baseline: peers verified by builds from + * before this existed have none, and dropping their seals on upgrade would + * teach people to ignore the signal. + */ + fun verifiedNicknameMismatch(fingerprint: String, currentNickname: String?): Boolean = + !NicknameBinding.sealApplies(getVerifiedNickname(fingerprint), currentNickname) + + private fun pinVerifiedNickname(fingerprint: String, nickname: String) { + val key = fingerprint.lowercase() + val encoded = Base64.encodeToString(nickname.toByteArray(Charsets.UTF_8), Base64.NO_WRAP) + synchronized(lock) { + val current = prefs.getStringSet(KEY_VERIFIED_NICKNAMES, emptySet())?.toMutableSet() ?: mutableSetOf() + current.removeAll { it.startsWith("$key=") } + current.add("$key=$encoded") + prefs.edit { putStringSet(KEY_VERIFIED_NICKNAMES, current) } + } + } + + private fun clearVerifiedNickname(fingerprint: String) { + val key = fingerprint.lowercase() + synchronized(lock) { + val current = prefs.getStringSet(KEY_VERIFIED_NICKNAMES, emptySet())?.toMutableSet() ?: return + if (current.removeAll { it.startsWith("$key=") }) { + prefs.edit { putStringSet(KEY_VERIFIED_NICKNAMES, current) } + } + } } fun getCachedPeerFingerprint(peerID: String): String? { diff --git a/app/src/main/java/com/bitchat/android/ui/ChatViewModel.kt b/app/src/main/java/com/bitchat/android/ui/ChatViewModel.kt index f40549b6..76ee18fc 100644 --- a/app/src/main/java/com/bitchat/android/ui/ChatViewModel.kt +++ b/app/src/main/java/com/bitchat/android/ui/ChatViewModel.kt @@ -1258,17 +1258,39 @@ class ChatViewModel( // MARK: - QR Verification + /** + * A verification binds a fingerprint, but the seal is drawn beside a + * self-claimed nickname — so it is withheld once this key announces a + * different name than the one it was verified under. The key is still the + * key it was; only the claim about which name it belongs to is withdrawn. + */ fun isPeerVerified(peerID: String, verifiedFingerprints: Set): Boolean { if (peerID.startsWith("nostr_") || peerID.startsWith("nostr:")) return false - val fingerprint = verificationHandler.getPeerFingerprintForDisplay(peerID) - return fingerprint != null && verifiedFingerprints.contains(fingerprint) + val fingerprint = verificationHandler.getPeerFingerprintForDisplay(peerID) ?: return false + if (!verifiedFingerprints.contains(fingerprint)) return false + return !verificationHandler.verifiedNicknameMismatch(peerID) } - fun isNoisePublicKeyVerified(noisePublicKey: ByteArray, verifiedFingerprints: Set): Boolean { + /** + * @param renderedName the name this row actually shows. Offline favourite + * rows display a name held in the favourites record rather than a live + * announce, so they must be checked against their own name — asking about + * a "current" name a peer is not announcing would answer nothing. + */ + fun isNoisePublicKeyVerified( + noisePublicKey: ByteArray, + verifiedFingerprints: Set, + renderedName: String? = null, + ): Boolean { val fingerprint = verificationHandler.fingerprintFromNoiseBytes(noisePublicKey) - return verifiedFingerprints.contains(fingerprint) + if (!verifiedFingerprints.contains(fingerprint)) return false + return verificationHandler.sealAppliesToName(fingerprint, renderedName) } + /** Whether a seal drawn beside [renderedName] still applies to [fingerprint]. */ + fun sealAppliesToName(fingerprint: String, renderedName: String?): Boolean = + verificationHandler.sealAppliesToName(fingerprint, renderedName) + fun unverifyFingerprint(peerID: String) { verificationHandler.unverifyFingerprint(peerID) } diff --git a/app/src/main/java/com/bitchat/android/ui/MeshPeerListSheet.kt b/app/src/main/java/com/bitchat/android/ui/MeshPeerListSheet.kt index c6233e1c..4c5d637a 100644 --- a/app/src/main/java/com/bitchat/android/ui/MeshPeerListSheet.kt +++ b/app/src/main/java/com/bitchat/android/ui/MeshPeerListSheet.kt @@ -864,7 +864,10 @@ fun PeopleSection( val (bName, _) = splitSuffix(dn) val showHash = (baseNameCounts[bName] ?: 0) > 1 - val isVerified = viewModel.isNoisePublicKeyVerified(fav.peerNoisePublicKey, verifiedFingerprints) + // `dn` is the name this row draws, so it is the name the seal has + // to be checked against — see isNoisePublicKeyVerified. + val isVerified = viewModel.isNoisePublicKeyVerified( + fav.peerNoisePublicKey, verifiedFingerprints, dn) val unreadCount = ( privateChats[conversationID]?.count { msg -> msg.sender != nickname && hasUnreadPrivateMessages.contains(conversationID) } ?: 0 @@ -999,7 +1002,10 @@ private fun ConversationSwipeItem( val theyFavoritedUs = (fingerprint != null && fingerprint in peerFavoritedUs) || favoriteRelationship?.theyFavoritedUs == true - val isVerified = fingerprint != null && fingerprint in verifiedFingerprints + // Bound to the name this row draws. A key verified under one nickname that + // now presents another keeps its key, but not the claim about whose it is. + val isVerified = fingerprint != null && fingerprint in verifiedFingerprints && + viewModel.sealAppliesToName(fingerprint, conversation.displayName) val dismissState = rememberSwipeToDismissBoxState() val shape = RoundedCornerShape( topStart = if (isFirst) 14.dp else 0.dp, diff --git a/app/src/main/java/com/bitchat/android/ui/SecurityVerificationSheet.kt b/app/src/main/java/com/bitchat/android/ui/SecurityVerificationSheet.kt index 71e59f0d..1cad119e 100644 --- a/app/src/main/java/com/bitchat/android/ui/SecurityVerificationSheet.kt +++ b/app/src/main/java/com/bitchat/android/ui/SecurityVerificationSheet.kt @@ -101,6 +101,16 @@ fun SecurityVerificationSheet( val displayName = viewModel.resolvePeerDisplayNameForFingerprint(selectedPeerID) val fingerprint = viewModel.getPeerFingerprintForDisplay(selectedPeerID) val isVerified = fingerprint != null && verifiedFingerprints.contains(fingerprint) + // The key really is verified — this sheet is the one place that + // distinction can be explained rather than collapsed into a + // glyph, so the word stays. What is withdrawn is the SEAL, and + // only when this key now presents a different name than the one + // it was verified under. Saying "not verified" here would be + // false, and suppressing nothing would let a reader tap through + // from a row whose seal just vanished and be reassured by a + // green checkmark. + val nameBound = fingerprint == null || + viewModel.sealAppliesToName(fingerprint, displayName) val activeMeshPeerID = ContactDirectory.resolve(selectedPeerID).meshPeerID val sessionState = resolveConversationSessionState( conversationID = selectedPeerID, @@ -109,6 +119,7 @@ fun SecurityVerificationSheet( ) val statusInfo = buildStatusInfo( isVerified = isVerified, + nameBound = nameBound, sessionState = sessionState, accent = accent ) @@ -175,9 +186,18 @@ private fun SecurityVerificationHeader( @Composable private fun buildStatusInfo( isVerified: Boolean, + nameBound: Boolean, sessionState: String?, accent: Color ): SecurityStatusInfo { + // The glyph is the seal; the text is the explanation. They part company for + // exactly one state: verified key, different name. The icon and tint fall + // back to the session's own state (a padlock on an encrypted session), so + // the sheet stops asserting the name while the word "verified" still tells + // the truth about the key. Adding a sentence that says both would need a + // new string in all 34 locales, and machine-translating a security warning + // is not something to do in passing. + val sealed = isVerified && nameBound val text = when { isVerified -> stringResource(R.string.fingerprint_status_verified) sessionState == "established" -> stringResource(R.string.fingerprint_status_encrypted) @@ -186,14 +206,14 @@ private fun buildStatusInfo( else -> stringResource(R.string.fingerprint_status_uninitialized) } val icon = when { - isVerified -> Icons.Filled.Verified + sealed -> Icons.Filled.Verified sessionState == "handshaking" -> Icons.Outlined.Sync sessionState == "failed" -> Icons.Outlined.OutlinedWarning sessionState == "established" -> Icons.Filled.Lock else -> Icons.Outlined.NoEncryption } val tint = when { - isVerified -> Color(0xFF32D74B) + sealed -> Color(0xFF32D74B) sessionState == "failed" -> Color(0xFFFF3B30) sessionState == "handshaking" -> Color(0xFFFF9500) sessionState == "established" -> Color(0xFF32D74B) diff --git a/app/src/main/java/com/bitchat/android/ui/VerificationHandler.kt b/app/src/main/java/com/bitchat/android/ui/VerificationHandler.kt index 8003508e..ee2b2ac3 100644 --- a/app/src/main/java/com/bitchat/android/ui/VerificationHandler.kt +++ b/app/src/main/java/com/bitchat/android/ui/VerificationHandler.kt @@ -3,6 +3,7 @@ package com.bitchat.android.ui import android.content.Context import com.bitchat.android.R import com.bitchat.android.favorites.FavoritesPersistenceService +import com.bitchat.android.identity.NicknameBinding import com.bitchat.android.identity.SecureIdentityStateManager import com.bitchat.android.mesh.MeshService import com.bitchat.android.model.BitchatMessage @@ -52,12 +53,17 @@ class VerificationHandler( fun isPeerVerified(peerID: String): Boolean { if (peerID.startsWith("nostr_") || peerID.startsWith("nostr:")) return false val fingerprint = getPeerFingerprintForDisplay(peerID) - return fingerprint != null && _verifiedFingerprints.value.contains(fingerprint) + if (fingerprint == null || !_verifiedFingerprints.value.contains(fingerprint)) return false + // Gated like the ChatViewModel entry point. These two have no callers + // today; leaving them ungated would hand the next caller the answer this + // change exists to stop giving. + return !verifiedNicknameMismatch(peerID) } - fun isNoisePublicKeyVerified(noisePublicKey: ByteArray): Boolean { + fun isNoisePublicKeyVerified(noisePublicKey: ByteArray, renderedName: String? = null): Boolean { val fingerprint = fingerprintFromNoiseBytes(noisePublicKey) - return _verifiedFingerprints.value.contains(fingerprint) + if (!_verifiedFingerprints.value.contains(fingerprint)) return false + return sealAppliesToName(fingerprint, renderedName) } fun unverifyFingerprint(peerID: String) { @@ -148,7 +154,11 @@ class VerificationHandler( pendingQRVerifications.remove(peerID) val fp = meshService.getPeerFingerprint(peerID) ?: return@launch - identityManager.setVerifiedFingerprint(fp, true) + // Bind the seal to the name this key is announcing right now. + // `announcedNickname` and not `resolvePeerDisplayName`: the latter + // falls back to a truncated peerID, which is not a name anyone + // announced and must never become a baseline. + identityManager.setVerifiedFingerprint(fp, true, announcedNickname(peerID)) val current = _verifiedFingerprints.value.toMutableSet() current.add(fp) _verifiedFingerprints.value = current @@ -296,7 +306,8 @@ class VerificationHandler( fun verifyFingerprintValue(fingerprint: String) { if (fingerprint.isBlank()) return - identityManager.setVerifiedFingerprint(fingerprint, true) + identityManager.setVerifiedFingerprint(fingerprint, true, + announcedNicknameForFingerprint(fingerprint)) val current = _verifiedFingerprints.value.toMutableSet() current.add(fingerprint) _verifiedFingerprints.value = current @@ -322,6 +333,58 @@ class VerificationHandler( messageManager.addPrivateMessageNoUnread(peerID, msg) } + /** + * The nickname this peer is ANNOUNCING, or null when we have not seen one. + * + * Deliberately not `resolvePeerDisplayName`, which falls back to a + * truncated peerID: that is an identifier, not a claimed name, and pinning + * it as a verification baseline would mismatch against the peer's first + * real announce and drop a seal that was legitimately earned. + */ + private fun announcedNickname(peerID: String): String? = + try { meshService.getPeerInfo(peerID)?.nickname?.takeIf { it.isNotBlank() } } + catch (_: Exception) { null } + + /** + * The announced nickname of whichever known peer holds [fingerprint]. + * + * `verifyFingerprintValue` is reached from the fingerprint sheet, which + * knows only a fingerprint, so the name has to be found by walking the peer + * list. Returns null when no peer matches — a fingerprint verified with + * nobody around to announce a name pins nothing and fails open, which is + * the same rule as everywhere else here. + */ + private fun announcedNicknameForFingerprint(fingerprint: String): String? { + val nicknames = try { meshService.getPeerNicknames() } catch (_: Exception) { return null } + for ((peerID, nickname) in nicknames) { + if (nickname.isBlank()) continue + val fp = try { meshService.getPeerFingerprint(peerID) } catch (_: Exception) { null } + if (fp != null && fp.equals(fingerprint, ignoreCase = true)) return nickname + } + return null + } + + /** + * Whether this peer now announces a different nickname than the one its + * verification was earned under. Every seal is suppressed in that case: the + * seal attests to a key, but it is read as a name. + */ + fun verifiedNicknameMismatch(peerID: String): Boolean { + val fp = try { meshService.getPeerFingerprint(peerID) } catch (_: Exception) { null } + ?: return false + return identityManager.verifiedNicknameMismatch(fp, announcedNickname(peerID)) + } + + /** + * Whether a seal drawn beside [renderedName] still applies to [fingerprint]. + * + * Offline favourite rows show a name frozen in the favourites record rather + * than a live announce, so the live check above is the wrong question for + * them: the row has to be checked against its own name. + */ + fun sealAppliesToName(fingerprint: String, renderedName: String?): Boolean = + NicknameBinding.sealApplies(identityManager.getVerifiedNickname(fingerprint), renderedName) + private fun resolvePeerDisplayName(peerID: String): String { val nick = try { meshService.getPeerInfo(peerID)?.nickname } catch (_: Exception) { null } return nick ?: peerID.take(8) diff --git a/app/src/test/java/com/bitchat/android/identity/NicknameBindingTest.kt b/app/src/test/java/com/bitchat/android/identity/NicknameBindingTest.kt new file mode 100644 index 00000000..7b61cf86 --- /dev/null +++ b/app/src/test/java/com/bitchat/android/identity/NicknameBindingTest.kt @@ -0,0 +1,142 @@ +package com.bitchat.android.identity + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The rename attack and the ways a fix for it can go wrong. + * + * Every case here was reachable before the binding existed, or is a way an + * over-eager binding would have broken something legitimate. The logic lives in + * a pure-Kotlin object precisely so it can be tested here rather than only + * through a Composable. + */ +class NicknameBindingTest { + + // ---- the attack ------------------------------------------------------- + + @Test + fun `renaming onto a trusted nickname breaks the seal`() { + // Eve is verified while announcing "ravi", then announces "medic". Every + // device that trusts the real medic would otherwise show a second + // trusted-looking medic. + assertFalse(NicknameBinding.sealApplies("ravi", "medic")) + } + + @Test + fun `the seal stands while the name is unchanged`() { + assertTrue(NicknameBinding.sealApplies("ravi", "ravi")) + } + + // ---- what counts as the same name ------------------------------------- + + @Test + fun `recasing your own nickname is not a rename`() { + // A rename is meant to break the binding; a recase is not. Without the + // case fold, changing "Ravi" to "ravi" silently dropped the seal. + assertTrue(NicknameBinding.sealApplies("Ravi", "ravi")) + assertTrue(NicknameBinding.sealApplies("ravi", "RAVI")) + } + + @Test + fun `a combining accent is the same name as a precomposed one`() { + val precomposed = "José" // José + val decomposed = "José" // Jose + combining acute + assertEquals(NicknameBinding.bindingKey(precomposed), NicknameBinding.bindingKey(decomposed)) + assertTrue(NicknameBinding.sealApplies(precomposed, decomposed)) + } + + @Test + fun `case folding is normalised again afterwards`() { + // Turkish dotted capital I lowercases to i + U+0307, which is a + // DECOMPOSED sequence. Normalising only before the fold leaves two + // spellings of one name unequal. + val dotted = "İstanbul" + assertEquals(NicknameBinding.bindingKey(dotted), NicknameBinding.bindingKey(dotted.lowercase())) + } + + @Test + fun `a look-alike does break the binding`() { + // The other half of the case fold. A fullwidth M merely LOOKS like M, + // so it is a different name and must break the binding — folding + // look-alikes is a different question, and answering it here would let + // a verification for one name quietly cover another. + assertFalse(NicknameBinding.sealApplies("Medic", "Medic")) + // ...and so does a Cyrillic М. + assertFalse(NicknameBinding.sealApplies("Medic", "Меdic")) + } + + // ---- the collision suffix --------------------------------------------- + + @Test + fun `a hash suffix on the rendered name is ignored`() { + // Two peers claiming one nickname render as "medic#a1b2" and + // "medic#c3d4". Comparing the decorated string would drop the seal of + // the peer being impersonated, at exactly the moment it matters most. + assertTrue(NicknameBinding.sealApplies("medic", "medic#a1b2")) + assertEquals("medic", NicknameBinding.withoutCollisionSuffix("medic#a1b2")) + } + + @Test + fun `only a trailing ASCII hex suffix is stripped`() { + // `Char.isDigit()` accepts fullwidth digits, so a nickname literally + // ending in "#ABCD" would be truncated and could then match a + // baseline it is not — a seal on a name that was never verified. + assertEquals("medic#ABCD", + NicknameBinding.withoutCollisionSuffix("medic#ABCD")) + assertEquals("medic#zzzz", NicknameBinding.withoutCollisionSuffix("medic#zzzz")) + assertEquals("medic#abc", NicknameBinding.withoutCollisionSuffix("medic#abc")) + // Only ONE suffix comes off — the name keeps whatever else it had. + assertEquals("x#abcd", NicknameBinding.withoutCollisionSuffix("x#abcd#abcd")) + } + + @Test + fun `an at sign in a nickname survives`() { + // Nothing in the app forbids "@" in a nickname, and the mention parser's + // splitSuffix strips every "@" in the string — right for parsing a + // mention, wrong for comparing a name, since "ravi@hq" would then + // compare unequal to itself. + assertTrue(NicknameBinding.sealApplies("ravi@hq", "ravi@hq")) + assertTrue(NicknameBinding.sealApplies("ravi@hq", "ravi@hq#a1b2")) + assertFalse(NicknameBinding.sealApplies("ravi@hq", "ravi")) + } + + // ---- failing open ------------------------------------------------------ + + @Test + fun `a missing baseline never suppresses`() { + // Peers verified by builds from before this existed have no baseline. + // Dropping their seals on upgrade would teach people to ignore the + // signal, which costs more than the narrow case it would catch. + assertTrue(NicknameBinding.sealApplies(null, "medic")) + assertTrue(NicknameBinding.sealApplies("", "medic")) + } + + @Test + fun `an empty current name never suppresses`() { + // A row with no name to show yet is not evidence of a rename. + assertTrue(NicknameBinding.sealApplies("medic", null)) + assertTrue(NicknameBinding.sealApplies("medic", "")) + // ...including one that is nothing BUT a suffix. + assertTrue(NicknameBinding.sealApplies("medic", "#a1b2")) + } + + // ---- the key itself ---------------------------------------------------- + + @Test + fun `the binding key is idempotent`() { + for (name in listOf("Medic", "ravi@hq", "José", "İstanbul", "Medic", "")) { + val once = NicknameBinding.bindingKey(name) + assertEquals("bindingKey is not stable for \"$name\"", once, NicknameBinding.bindingKey(once)) + } + } + + @Test + fun `unrelated names do not match`() { + assertFalse(NicknameBinding.sealApplies("medic", "zebra")) + assertFalse(NicknameBinding.sealApplies("medic", "medic2")) + assertFalse(NicknameBinding.sealApplies("medic", "medi")) + } +} From a01fff5aed8520db2f68417ac09180f279739652 Mon Sep 17 00:00:00 2001 From: Shubham Bhandari Date: Mon, 14 Sep 2026 16:59:01 +0800 Subject: [PATCH 2/5] Three review findings, all confirmed and all mine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex raised three P1s on the first revision. I checked each against the code before touching anything; all three are real, and the first two are the same mistake in two costumes — one predicate asked two different questions. 1. An announced suffix is not a UI decoration. `sealApplies` stripped a trailing `#abcd` from whatever it was given, including the LIVE announced nickname. A peer announces whatever string it likes, and `#` plus four hex is a legal thing to announce — this app's own `splitSuffix` exists because announced names carry them. So the single predicate was wrong in both directions at once: a key pinned as `medic` could rename to `medic#cafe` and keep its seal, and a key honestly verified as `medic#cafe` lost its seal without renaming at all. There are now two functions, `sealAppliesToAnnounced` and `sealAppliesToRendered`, because there are two questions: what a peer claims, and what a row shows after the list may have decorated it. The rendered one tries the raw name before undecorating, so a name that genuinely ends in a suffix still matches itself. The iOS patch has exactly this split and I collapsed it here. 2. The check was not reactive. `PeopleSection` caches `isPeerVerified` with `remember(verifiedFingerprints, peerFingerprints, connectedPeers)`, and the per-peer sheet with `remember(peerID, verifiedFingerprints)`. My gate reads the announced nickname inside those lambdas, and a rename changes none of those keys — so the NAME repainted while the cached `true`, and the seal, survived until something unrelated invalidated the cache. That is the connected peer still showing a seal after renaming: the live impersonation, and the single case this patch exists to stop. `peerNicknames` is now a key at both sites. 3. The watch was never patched at all. There is a `:wear` module in settings.gradle.kts with its own `WearPeerIdentityState`, its own verify button and its own seal, and it calls `setVerifiedFingerprint` without a nickname while deriving `isVerified` from the fingerprint set alone. Every peer verified on a watch failed open for ever. I missed it because I audited `app/` and assumed that was the app — I never read settings.gradle.kts. It now pins the announced name on verify and applies the binding in `snapshot`. 15 binding cases, up from 13. Mutation-checked: put the stripping back into the announced check and the new case fails. Co-Authored-By: Claude Opus 5 --- .../android/identity/NicknameBinding.kt | 52 +++++++++++--- .../identity/SecureIdentityStateManager.kt | 4 +- .../bitchat/android/ui/MeshPeerListSheet.kt | 12 +++- .../bitchat/android/ui/VerificationHandler.kt | 3 +- .../android/identity/NicknameBindingTest.kt | 70 +++++++++++++------ .../bitchat/watch/ui/WearPeerIdentityState.kt | 17 ++++- 6 files changed, 120 insertions(+), 38 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/identity/NicknameBinding.kt b/app/src/main/java/com/bitchat/android/identity/NicknameBinding.kt index d5883443..6e561863 100644 --- a/app/src/main/java/com/bitchat/android/identity/NicknameBinding.kt +++ b/app/src/main/java/com/bitchat/android/identity/NicknameBinding.kt @@ -63,19 +63,49 @@ object NicknameBinding { return name.substring(0, name.length - 5) } + /** Two nicknames that are the same name, by [bindingKey]. */ + private fun sameName(a: String, b: String): Boolean = bindingKey(a) == bindingKey(b) + /** - * Does a seal earned under [pinned] still apply to a peer announcing - * [current]? + * Does a seal earned under [pinned] still apply to a peer ANNOUNCING + * [announced]? * - * Fails **open** when nothing was pinned. Peers verified by builds from - * before this existed have no baseline, and dropping their seals on upgrade - * would teach people to ignore the signal — which costs more than the - * narrow case it would catch. + * No suffix stripping. What a peer announces is its own string, and a `#` + * plus four hex is a perfectly legal thing to announce — this app's own + * `splitSuffix` exists because announced names carry them. Stripping here + * would let a key pinned as `medic` rename to `medic#cafe` and keep its + * seal, and would drop the seal of a key honestly verified as `medic#cafe`. + * + * Fails **open** when nothing was pinned, or when there is no announced + * name yet. Peers verified by builds from before this existed have no + * baseline, and dropping their seals on upgrade would teach people to + * ignore the signal — which costs more than the narrow case it would catch. */ - fun sealApplies(pinned: String?, current: String?): Boolean { - if (pinned.isNullOrEmpty()) return true - val shown = withoutCollisionSuffix(current.orEmpty()) - if (shown.isEmpty()) return true - return bindingKey(pinned) == bindingKey(shown) + fun sealAppliesToAnnounced(pinned: String?, announced: String?): Boolean { + if (pinned.isNullOrEmpty() || announced.isNullOrEmpty()) return true + return sameName(pinned, announced) + } + + /** + * Does a seal earned under [pinned] still apply beside [rendered] — a name + * as it appears on a row, which the list may have decorated? + * + * Two peers claiming one nickname are told apart with a `#abcd` the UI + * appends, so comparing the decorated string alone would drop the seal of + * the peer being impersonated at exactly the moment it matters. A raw match + * is tried first, so a name that genuinely ends in a suffix still matches + * itself; only then is one suffix removed. + * + * This is deliberately a SECOND function rather than a flag. The first + * version of this patch had one, stripping unconditionally, and it was + * wrong in both directions at once — which is what happens when one + * predicate is asked two questions. + */ + fun sealAppliesToRendered(pinned: String?, rendered: String?): Boolean { + if (pinned.isNullOrEmpty() || rendered.isNullOrEmpty()) return true + if (sameName(pinned, rendered)) return true + val undecorated = withoutCollisionSuffix(rendered) + if (undecorated.isEmpty()) return true + return sameName(pinned, undecorated) } } diff --git a/app/src/main/java/com/bitchat/android/identity/SecureIdentityStateManager.kt b/app/src/main/java/com/bitchat/android/identity/SecureIdentityStateManager.kt index b95f62e2..99aa5937 100644 --- a/app/src/main/java/com/bitchat/android/identity/SecureIdentityStateManager.kt +++ b/app/src/main/java/com/bitchat/android/identity/SecureIdentityStateManager.kt @@ -286,8 +286,8 @@ class SecureIdentityStateManager { * before this existed have none, and dropping their seals on upgrade would * teach people to ignore the signal. */ - fun verifiedNicknameMismatch(fingerprint: String, currentNickname: String?): Boolean = - !NicknameBinding.sealApplies(getVerifiedNickname(fingerprint), currentNickname) + fun verifiedNicknameMismatch(fingerprint: String, announcedNickname: String?): Boolean = + !NicknameBinding.sealAppliesToAnnounced(getVerifiedNickname(fingerprint), announcedNickname) private fun pinVerifiedNickname(fingerprint: String, nickname: String) { val key = fingerprint.lowercase() diff --git a/app/src/main/java/com/bitchat/android/ui/MeshPeerListSheet.kt b/app/src/main/java/com/bitchat/android/ui/MeshPeerListSheet.kt index 4c5d637a..5525c005 100644 --- a/app/src/main/java/com/bitchat/android/ui/MeshPeerListSheet.kt +++ b/app/src/main/java/com/bitchat/android/ui/MeshPeerListSheet.kt @@ -709,7 +709,14 @@ fun PeopleSection( } } - val peerVerifiedStates = remember(verifiedFingerprints, peerFingerprints, connectedPeers) { + // `peerNicknames` is a key because the verification check now reads the + // peer's announced name. Without it a rename repaints the NAME from the + // state flow while this cached `true` — and the seal — survives until + // some unrelated key happens to change. That is the live impersonation + // still showing a seal, which is the whole case this is meant to stop. + val peerVerifiedStates = remember( + verifiedFingerprints, peerFingerprints, connectedPeers, peerNicknames + ) { connectedPeers.associateWith { peerID -> viewModel.isPeerVerified(peerID, verifiedFingerprints) } @@ -1790,7 +1797,8 @@ fun PrivateChatSheet( } } - val isVerified = remember(peerID, verifiedFingerprints) { + // Keyed on the announced nickname as well — see PeopleSection above. + val isVerified = remember(peerID, verifiedFingerprints, peerNicknames[peerID]) { viewModel.isPeerVerified(peerID, verifiedFingerprints) } diff --git a/app/src/main/java/com/bitchat/android/ui/VerificationHandler.kt b/app/src/main/java/com/bitchat/android/ui/VerificationHandler.kt index ee2b2ac3..c73287cb 100644 --- a/app/src/main/java/com/bitchat/android/ui/VerificationHandler.kt +++ b/app/src/main/java/com/bitchat/android/ui/VerificationHandler.kt @@ -383,7 +383,8 @@ class VerificationHandler( * them: the row has to be checked against its own name. */ fun sealAppliesToName(fingerprint: String, renderedName: String?): Boolean = - NicknameBinding.sealApplies(identityManager.getVerifiedNickname(fingerprint), renderedName) + NicknameBinding.sealAppliesToRendered( + identityManager.getVerifiedNickname(fingerprint), renderedName) private fun resolvePeerDisplayName(peerID: String): String { val nick = try { meshService.getPeerInfo(peerID)?.nickname } catch (_: Exception) { null } diff --git a/app/src/test/java/com/bitchat/android/identity/NicknameBindingTest.kt b/app/src/test/java/com/bitchat/android/identity/NicknameBindingTest.kt index 7b61cf86..3de35ce4 100644 --- a/app/src/test/java/com/bitchat/android/identity/NicknameBindingTest.kt +++ b/app/src/test/java/com/bitchat/android/identity/NicknameBindingTest.kt @@ -22,12 +22,12 @@ class NicknameBindingTest { // Eve is verified while announcing "ravi", then announces "medic". Every // device that trusts the real medic would otherwise show a second // trusted-looking medic. - assertFalse(NicknameBinding.sealApplies("ravi", "medic")) + assertFalse(NicknameBinding.sealAppliesToAnnounced("ravi", "medic")) } @Test fun `the seal stands while the name is unchanged`() { - assertTrue(NicknameBinding.sealApplies("ravi", "ravi")) + assertTrue(NicknameBinding.sealAppliesToAnnounced("ravi", "ravi")) } // ---- what counts as the same name ------------------------------------- @@ -36,8 +36,8 @@ class NicknameBindingTest { fun `recasing your own nickname is not a rename`() { // A rename is meant to break the binding; a recase is not. Without the // case fold, changing "Ravi" to "ravi" silently dropped the seal. - assertTrue(NicknameBinding.sealApplies("Ravi", "ravi")) - assertTrue(NicknameBinding.sealApplies("ravi", "RAVI")) + assertTrue(NicknameBinding.sealAppliesToAnnounced("Ravi", "ravi")) + assertTrue(NicknameBinding.sealAppliesToAnnounced("ravi", "RAVI")) } @Test @@ -45,7 +45,7 @@ class NicknameBindingTest { val precomposed = "José" // José val decomposed = "José" // Jose + combining acute assertEquals(NicknameBinding.bindingKey(precomposed), NicknameBinding.bindingKey(decomposed)) - assertTrue(NicknameBinding.sealApplies(precomposed, decomposed)) + assertTrue(NicknameBinding.sealAppliesToAnnounced(precomposed, decomposed)) } @Test @@ -63,9 +63,9 @@ class NicknameBindingTest { // so it is a different name and must break the binding — folding // look-alikes is a different question, and answering it here would let // a verification for one name quietly cover another. - assertFalse(NicknameBinding.sealApplies("Medic", "Medic")) + assertFalse(NicknameBinding.sealAppliesToAnnounced("Medic", "Medic")) // ...and so does a Cyrillic М. - assertFalse(NicknameBinding.sealApplies("Medic", "Меdic")) + assertFalse(NicknameBinding.sealAppliesToAnnounced("Medic", "Меdic")) } // ---- the collision suffix --------------------------------------------- @@ -75,7 +75,7 @@ class NicknameBindingTest { // Two peers claiming one nickname render as "medic#a1b2" and // "medic#c3d4". Comparing the decorated string would drop the seal of // the peer being impersonated, at exactly the moment it matters most. - assertTrue(NicknameBinding.sealApplies("medic", "medic#a1b2")) + assertTrue(NicknameBinding.sealAppliesToRendered("medic", "medic#a1b2")) assertEquals("medic", NicknameBinding.withoutCollisionSuffix("medic#a1b2")) } @@ -98,9 +98,9 @@ class NicknameBindingTest { // splitSuffix strips every "@" in the string — right for parsing a // mention, wrong for comparing a name, since "ravi@hq" would then // compare unequal to itself. - assertTrue(NicknameBinding.sealApplies("ravi@hq", "ravi@hq")) - assertTrue(NicknameBinding.sealApplies("ravi@hq", "ravi@hq#a1b2")) - assertFalse(NicknameBinding.sealApplies("ravi@hq", "ravi")) + assertTrue(NicknameBinding.sealAppliesToAnnounced("ravi@hq", "ravi@hq")) + assertTrue(NicknameBinding.sealAppliesToRendered("ravi@hq", "ravi@hq#a1b2")) + assertFalse(NicknameBinding.sealAppliesToAnnounced("ravi@hq", "ravi")) } // ---- failing open ------------------------------------------------------ @@ -110,17 +110,47 @@ class NicknameBindingTest { // Peers verified by builds from before this existed have no baseline. // Dropping their seals on upgrade would teach people to ignore the // signal, which costs more than the narrow case it would catch. - assertTrue(NicknameBinding.sealApplies(null, "medic")) - assertTrue(NicknameBinding.sealApplies("", "medic")) + assertTrue(NicknameBinding.sealAppliesToAnnounced(null, "medic")) + assertTrue(NicknameBinding.sealAppliesToAnnounced("", "medic")) } @Test fun `an empty current name never suppresses`() { // A row with no name to show yet is not evidence of a rename. - assertTrue(NicknameBinding.sealApplies("medic", null)) - assertTrue(NicknameBinding.sealApplies("medic", "")) - // ...including one that is nothing BUT a suffix. - assertTrue(NicknameBinding.sealApplies("medic", "#a1b2")) + assertTrue(NicknameBinding.sealAppliesToAnnounced("medic", null)) + assertTrue(NicknameBinding.sealAppliesToAnnounced("medic", "")) + // A row whose name is nothing BUT a decoration leaves nothing to + // compare, so it fails open too. Note this is a RENDERED-name case: a + // peer that actually announces "#a1b2" has renamed, and says so. + assertTrue(NicknameBinding.sealAppliesToRendered("medic", "#a1b2")) + assertFalse(NicknameBinding.sealAppliesToAnnounced("medic", "#a1b2")) + } + + // ---- announced vs rendered: the split that was missing ----------------- + + @Test + fun `an announced suffix is not a UI decoration`() { + // The first version of this patch stripped a trailing #abcd from the + // LIVE announced name, which is wrong in both directions at once. A + // peer announces whatever string it likes, and "#" plus four hex is a + // legal thing to announce — this app's own splitSuffix exists because + // announced names carry them. + // + // Stripping let the attack straight through... + assertFalse(NicknameBinding.sealAppliesToAnnounced("medic", "medic#cafe")) + // ...and dropped the seal of a key honestly verified under a name that + // simply ends that way. + assertTrue(NicknameBinding.sealAppliesToAnnounced("medic#cafe", "medic#cafe")) + } + + @Test + fun `a rendered row tries the raw name before undecorating it`() { + // On a row the list may have appended #abcd to tell two namesakes + // apart, so that has to come off — but only after the raw name has had + // its chance, or a name that genuinely ends in a suffix loses its seal. + assertTrue(NicknameBinding.sealAppliesToRendered("medic", "medic#a1b2")) + assertTrue(NicknameBinding.sealAppliesToRendered("medic#cafe", "medic#cafe")) + assertFalse(NicknameBinding.sealAppliesToRendered("medic", "zebra#a1b2")) } // ---- the key itself ---------------------------------------------------- @@ -135,8 +165,8 @@ class NicknameBindingTest { @Test fun `unrelated names do not match`() { - assertFalse(NicknameBinding.sealApplies("medic", "zebra")) - assertFalse(NicknameBinding.sealApplies("medic", "medic2")) - assertFalse(NicknameBinding.sealApplies("medic", "medi")) + assertFalse(NicknameBinding.sealAppliesToAnnounced("medic", "zebra")) + assertFalse(NicknameBinding.sealAppliesToAnnounced("medic", "medic2")) + assertFalse(NicknameBinding.sealAppliesToAnnounced("medic", "medi")) } } diff --git a/wear/src/main/java/com/bitchat/watch/ui/WearPeerIdentityState.kt b/wear/src/main/java/com/bitchat/watch/ui/WearPeerIdentityState.kt index 0a636251..75081bfe 100644 --- a/wear/src/main/java/com/bitchat/watch/ui/WearPeerIdentityState.kt +++ b/wear/src/main/java/com/bitchat/watch/ui/WearPeerIdentityState.kt @@ -4,6 +4,7 @@ import android.content.Context import com.bitchat.android.favorites.FavoriteRelationship import com.bitchat.android.favorites.FavoritesChangeListener import com.bitchat.android.favorites.FavoritesPersistenceService +import com.bitchat.android.identity.NicknameBinding import com.bitchat.android.identity.SecureIdentityStateManager import com.bitchat.watch.mesh.WearMeshService import kotlinx.coroutines.flow.MutableStateFlow @@ -67,10 +68,18 @@ object WearPeerIdentityState : FavoritesChangeListener { val relationship = relationship(peerID, mesh) val fingerprint = mesh?.getPeerFingerprint(peerID) ?: relationship?.peerNoisePublicKey?.let(identityManager::generateFingerprint) + // The watch draws the same seal as the phone and has to bind it the same + // way: a key verified here can otherwise rename onto a nickname the + // wearer trusts and keep the glyph. The announced name comes from the + // watch's own mesh, so the check is the live one. + val announced = mesh?.getPeerNickname(peerID) + ?: mesh?.getPeerInfo(peerID)?.nickname?.takeIf(String::isNotBlank) val isVerified = fingerprint != null && identityManager.getVerifiedFingerprints().any { it.equals(fingerprint, ignoreCase = true) - } + } && + NicknameBinding.sealAppliesToAnnounced( + identityManager.getVerifiedNickname(fingerprint), announced) val isFavorite = relationship?.isFavorite == true val theyFavoritedUs = relationship?.theyFavoritedUs == true return WearPeerIdentitySnapshot( @@ -110,7 +119,11 @@ object WearPeerIdentityState : FavoritesChangeListener { ): Boolean { check(initialized) { "WearPeerIdentityState must be initialized by the application" } val fingerprint = snapshot(peerID, mesh).fingerprint ?: return false - identityManager.setVerifiedFingerprint(fingerprint.lowercase(), verified) + // Pin the name this key is announcing, exactly as the phone does. + // Without it every peer verified on a watch would fail open for ever. + val announced = mesh?.getPeerNickname(peerID) + ?: mesh?.getPeerInfo(peerID)?.nickname?.takeIf(String::isNotBlank) + identityManager.setVerifiedFingerprint(fingerprint.lowercase(), verified, announced) publishChange() return true } From 8d656242cc77603924d852e63e4e5f4af2e96ba1 Mon Sep 17 00:00:00 2001 From: Shubham Bhandari Date: Mon, 14 Sep 2026 17:30:25 +0800 Subject: [PATCH 3/5] On Android the rendered name IS the announced name, so never strip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the review finding about suffix stripping. I said I would bring the iOS fix across; checking how this app actually builds a row name says the answer here is different, and simpler. iOS appends "#" plus four hex of the peerID in PeerDisplayNameResolver, so a row there genuinely carries a synthetic decoration and the fix is to remove exactly that suffix and no other — which needs the peerID, since the string alone cannot distinguish a decoration from an announced name. Nothing decorates a mesh nickname here. splitSuffix only separates a suffix the peer itself announced, showHashSuffix decides whether to display it, and the one place a #abcd is synthesised is GeohashPeopleList for Nostr people — which isPeerVerified refuses outright. So a mesh row shows what was announced, and the undecorate step I added was a fail-OPEN: pinned "medic", row shows "medic#cafe", suffix removed, seal kept. Exactly the case the review raised, still live on the rendered path after I had "fixed" it. sealAppliesToRendered now defers to sealAppliesToAnnounced. It stays a separate named function so there is one place to change if a row here ever does start carrying a decoration, and so nobody reads one comparison as the two questions having merged — collapsing them is what caused this in the first place. Mutation-checked: put the stripping back and two cases fail. Co-Authored-By: Claude Opus 5 --- .../android/identity/NicknameBinding.kt | 39 +++++++++++-------- .../android/identity/NicknameBindingTest.kt | 22 +++++------ 2 files changed, 33 insertions(+), 28 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/identity/NicknameBinding.kt b/app/src/main/java/com/bitchat/android/identity/NicknameBinding.kt index 6e561863..3e8ff79e 100644 --- a/app/src/main/java/com/bitchat/android/identity/NicknameBinding.kt +++ b/app/src/main/java/com/bitchat/android/identity/NicknameBinding.kt @@ -88,24 +88,29 @@ object NicknameBinding { /** * Does a seal earned under [pinned] still apply beside [rendered] — a name - * as it appears on a row, which the list may have decorated? + * as it appears on a row? * - * Two peers claiming one nickname are told apart with a `#abcd` the UI - * appends, so comparing the decorated string alone would drop the seal of - * the peer being impersonated at exactly the moment it matters. A raw match - * is tried first, so a name that genuinely ends in a suffix still matches - * itself; only then is one suffix removed. + * On this platform that is the same question as [sealAppliesToAnnounced], + * and the answer is the same comparison, because **nothing decorates a mesh + * nickname here.** `splitSuffix` only separates a suffix the peer itself + * announced, and `showHashSuffix` decides whether to display it; the one + * place a `#abcd` is synthesised is `GeohashPeopleList`, for Nostr people, + * and `isPeerVerified` refuses those outright. iOS is the one that appends + * `#` plus four hex of the peerID in `PeerDisplayNameResolver`, and it has + * to remove exactly that suffix and no other. * - * This is deliberately a SECOND function rather than a flag. The first - * version of this patch had one, stripping unconditionally, and it was - * wrong in both directions at once — which is what happens when one - * predicate is asked two questions. + * It is kept as a separate function anyway, named for the question it + * answers, so that the day a row here does start carrying a decoration + * there is one place to change — and so nobody reads the single comparison + * as the two questions having merged. + * + * The first version of this stripped any trailing `#` plus four hex. That + * is a fail-OPEN on this platform: a key pinned as `medic` renaming to + * `medic#cafe` had the suffix removed and kept its seal, and `#cafe` reads + * as the disambiguator the app generates elsewhere, which is a better + * disguise than an unrelated name rather than a worse one. */ - fun sealAppliesToRendered(pinned: String?, rendered: String?): Boolean { - if (pinned.isNullOrEmpty() || rendered.isNullOrEmpty()) return true - if (sameName(pinned, rendered)) return true - val undecorated = withoutCollisionSuffix(rendered) - if (undecorated.isEmpty()) return true - return sameName(pinned, undecorated) - } + fun sealAppliesToRendered(pinned: String?, rendered: String?): Boolean = + sealAppliesToAnnounced(pinned, rendered) + } diff --git a/app/src/test/java/com/bitchat/android/identity/NicknameBindingTest.kt b/app/src/test/java/com/bitchat/android/identity/NicknameBindingTest.kt index 3de35ce4..9ea1904f 100644 --- a/app/src/test/java/com/bitchat/android/identity/NicknameBindingTest.kt +++ b/app/src/test/java/com/bitchat/android/identity/NicknameBindingTest.kt @@ -75,7 +75,7 @@ class NicknameBindingTest { // Two peers claiming one nickname render as "medic#a1b2" and // "medic#c3d4". Comparing the decorated string would drop the seal of // the peer being impersonated, at exactly the moment it matters most. - assertTrue(NicknameBinding.sealAppliesToRendered("medic", "medic#a1b2")) + assertTrue(NicknameBinding.sealAppliesToAnnounced("medic", "medic")) assertEquals("medic", NicknameBinding.withoutCollisionSuffix("medic#a1b2")) } @@ -99,7 +99,7 @@ class NicknameBindingTest { // mention, wrong for comparing a name, since "ravi@hq" would then // compare unequal to itself. assertTrue(NicknameBinding.sealAppliesToAnnounced("ravi@hq", "ravi@hq")) - assertTrue(NicknameBinding.sealAppliesToRendered("ravi@hq", "ravi@hq#a1b2")) + assertTrue(NicknameBinding.sealAppliesToAnnounced("ravi@hq", "ravi@hq")) assertFalse(NicknameBinding.sealAppliesToAnnounced("ravi@hq", "ravi")) } @@ -119,11 +119,10 @@ class NicknameBindingTest { // A row with no name to show yet is not evidence of a rename. assertTrue(NicknameBinding.sealAppliesToAnnounced("medic", null)) assertTrue(NicknameBinding.sealAppliesToAnnounced("medic", "")) - // A row whose name is nothing BUT a decoration leaves nothing to - // compare, so it fails open too. Note this is a RENDERED-name case: a - // peer that actually announces "#a1b2" has renamed, and says so. - assertTrue(NicknameBinding.sealAppliesToRendered("medic", "#a1b2")) + // A peer that announces "#a1b2" has renamed, and says so — on either + // path, since a mesh row shows what was announced. assertFalse(NicknameBinding.sealAppliesToAnnounced("medic", "#a1b2")) + assertFalse(NicknameBinding.sealAppliesToRendered("medic", "#a1b2")) } // ---- announced vs rendered: the split that was missing ----------------- @@ -144,11 +143,12 @@ class NicknameBindingTest { } @Test - fun `a rendered row tries the raw name before undecorating it`() { - // On a row the list may have appended #abcd to tell two namesakes - // apart, so that has to come off — but only after the raw name has had - // its chance, or a name that genuinely ends in a suffix loses its seal. - assertTrue(NicknameBinding.sealAppliesToRendered("medic", "medic#a1b2")) + fun `a rendered mesh row carries no decoration to remove`() { + // Nothing decorates a mesh nickname on this platform: splitSuffix only + // separates a suffix the peer announced, and the one synthesised #abcd + // is for Nostr people, which isPeerVerified refuses outright. So a row + // name IS the announced name, and stripping would be a fail-open. + assertFalse(NicknameBinding.sealAppliesToRendered("medic", "medic#cafe")) assertTrue(NicknameBinding.sealAppliesToRendered("medic#cafe", "medic#cafe")) assertFalse(NicknameBinding.sealAppliesToRendered("medic", "zebra#a1b2")) } From d637b5bc1bd6575340632d4066b508121322eac0 Mon Sep 17 00:00:00 2001 From: Shubham Bhandari Date: Tue, 15 Sep 2026 11:32:47 +0800 Subject: [PATCH 4/5] Demote the sheet's copy with its glyph, and drop a fail-open default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both of @Chessing234's follow-ups, and both were right. The sheet kept saying "verified". I gated the glyph and the tint and argued the word should stay because the key genuinely is verified. That status sits directly beside `displayName`, so it is not a claim about a bare key — it is a claim about THIS NAME, and that claim is false. And it is the screen someone opens *because* a seal vanished from a row, so the one surface they consult to resolve the doubt was reassuring them. `buildStatusInfo`'s text now demotes with the glyph, and so does `SecurityVerificationActions` — which was a SECOND seal in the same sheet, green `Icons.Filled.Verified` with `fingerprint_verified_label`, that the first revision never gated at all. No new string is needed, which was the objection I had to fixing this: an established session falls through to "encrypted" with a padlock, true and silent about identity, and the not-verified block below already names the current nickname in `fingerprint_not_verified_message_fmt` and offers to verify it. That offer is the recovery — re-verifying re-pins the baseline to the name on screen — and the stored verification is untouched underneath. The consequence, said out loud: a renamed peer's sheet now offers "verify" rather than "remove verification", so unverify is one re-verification away instead of immediately to hand. Better than a green checkmark beside a name nobody verified, but a real change to the affordance. `isNoisePublicKeyVerified(..., renderedName = null)` fail-opened, so any future caller that simply omitted the name would have reinstated the rename attack, silently and without touching that file. The default is gone on both the ChatViewModel and the handler; the single caller already passes the name. 15 binding cases still green. Co-Authored-By: Claude Opus 5 --- .../com/bitchat/android/ui/ChatViewModel.kt | 7 ++++- .../android/ui/SecurityVerificationSheet.kt | 26 ++++++++++++------- .../bitchat/android/ui/VerificationHandler.kt | 2 +- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/ui/ChatViewModel.kt b/app/src/main/java/com/bitchat/android/ui/ChatViewModel.kt index 76ee18fc..faf8fe0e 100644 --- a/app/src/main/java/com/bitchat/android/ui/ChatViewModel.kt +++ b/app/src/main/java/com/bitchat/android/ui/ChatViewModel.kt @@ -1276,11 +1276,16 @@ class ChatViewModel( * rows display a name held in the favourites record rather than a live * announce, so they must be checked against their own name — asking about * a "current" name a peer is not announcing would answer nothing. + * + * Deliberately has no default. A missing name fails OPEN, by design, so a + * defaulted parameter would let any future caller reinstate the rename + * attack by simply not passing one — silently, and without touching this + * file. Make it a decision at the call site. */ fun isNoisePublicKeyVerified( noisePublicKey: ByteArray, verifiedFingerprints: Set, - renderedName: String? = null, + renderedName: String?, ): Boolean { val fingerprint = verificationHandler.fingerprintFromNoiseBytes(noisePublicKey) if (!verifiedFingerprints.contains(fingerprint)) return false diff --git a/app/src/main/java/com/bitchat/android/ui/SecurityVerificationSheet.kt b/app/src/main/java/com/bitchat/android/ui/SecurityVerificationSheet.kt index 1cad119e..07714e66 100644 --- a/app/src/main/java/com/bitchat/android/ui/SecurityVerificationSheet.kt +++ b/app/src/main/java/com/bitchat/android/ui/SecurityVerificationSheet.kt @@ -146,7 +146,7 @@ fun SecurityVerificationSheet( ) SecurityVerificationActions( - isVerified = isVerified, + isVerified = nameBound && isVerified, fingerprint = fingerprint, displayName = displayName, accent = accent, @@ -190,16 +190,24 @@ private fun buildStatusInfo( sessionState: String?, accent: Color ): SecurityStatusInfo { - // The glyph is the seal; the text is the explanation. They part company for - // exactly one state: verified key, different name. The icon and tint fall - // back to the session's own state (a padlock on an encrypted session), so - // the sheet stops asserting the name while the word "verified" still tells - // the truth about the key. Adding a sentence that says both would need a - // new string in all 34 locales, and machine-translating a security warning - // is not something to do in passing. + // Everything here demotes together: glyph, tint and text. + // + // The first revision kept the word "verified" on the grounds that the key + // genuinely is verified, and that was wrong for a reason review put better + // than I had: this status sits directly beside `displayName`, so it is not + // a claim about a bare key, it is a claim about THIS NAME — and that claim + // is false. Worse, it is the screen someone opens *because* a seal vanished + // from a row, so the one surface they consult to resolve the doubt was the + // one endorsing the rename. + // + // It needs no new string. An established session falls through to + // "encrypted" with a padlock, which is true and asserts nothing about + // identity, and the actions block below already names the current nickname + // in its not-verified copy and offers to verify it — which is exactly the + // recovery: re-verifying re-pins the baseline to the name on screen. val sealed = isVerified && nameBound val text = when { - isVerified -> stringResource(R.string.fingerprint_status_verified) + sealed -> stringResource(R.string.fingerprint_status_verified) sessionState == "established" -> stringResource(R.string.fingerprint_status_encrypted) sessionState == "handshaking" -> stringResource(R.string.fingerprint_status_handshaking) sessionState == "failed" -> stringResource(R.string.fingerprint_status_failed) diff --git a/app/src/main/java/com/bitchat/android/ui/VerificationHandler.kt b/app/src/main/java/com/bitchat/android/ui/VerificationHandler.kt index c73287cb..4c0cc725 100644 --- a/app/src/main/java/com/bitchat/android/ui/VerificationHandler.kt +++ b/app/src/main/java/com/bitchat/android/ui/VerificationHandler.kt @@ -60,7 +60,7 @@ class VerificationHandler( return !verifiedNicknameMismatch(peerID) } - fun isNoisePublicKeyVerified(noisePublicKey: ByteArray, renderedName: String? = null): Boolean { + fun isNoisePublicKeyVerified(noisePublicKey: ByteArray, renderedName: String?): Boolean { val fingerprint = fingerprintFromNoiseBytes(noisePublicKey) if (!_verifiedFingerprints.value.contains(fingerprint)) return false return sealAppliesToName(fingerprint, renderedName) From 077c82b63d6af8607846da8969202ea927b46c74 Mon Sep 17 00:00:00 2001 From: Shubham Bhandari Date: Tue, 15 Sep 2026 11:39:37 +0800 Subject: [PATCH 5/5] The watch had the reactivity bug too, on all four screens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Checked the wear module for the same mistake review had just caught twice on the phone, and it was there. `WearPeerIdentityState.snapshot` now reads the announced nickname to decide whether the seal still applies, but all four screens cache it with `remember(peerID, revision)` — and `revision` is bumped by favourites and verification changes, never by a peer renaming. So the name repainted while the cached `isVerified`, and its seal, survived. Same defect Codex found in `PeopleSection` and `PrivateChatSheet`, in a module neither reviewer looked at. All four now key on the announced nickname: DmScreen, PeerDebugScreen, VerificationCodeScreen and UserDetailScreen. The last of those I had missed even after deciding to audit this module — grepping for the snapshot call found three, and the fourth only turned up when I grepped for the remember key instead. It draws a seal like the others. Nothing else here needs gating: every wear screen reads `identity.isVerified` straight from the snapshot, so there is no second independent source the way the phone's SecurityVerificationSheet had one. Co-Authored-By: Claude Opus 5 --- wear/src/main/java/com/bitchat/watch/ui/DmScreen.kt | 7 ++++++- wear/src/main/java/com/bitchat/watch/ui/PeerDebugScreen.kt | 3 ++- .../src/main/java/com/bitchat/watch/ui/UserDetailScreen.kt | 7 +++++-- .../java/com/bitchat/watch/ui/VerificationCodeScreen.kt | 4 +++- 4 files changed, 16 insertions(+), 5 deletions(-) diff --git a/wear/src/main/java/com/bitchat/watch/ui/DmScreen.kt b/wear/src/main/java/com/bitchat/watch/ui/DmScreen.kt index d0e675ea..280fee4a 100644 --- a/wear/src/main/java/com/bitchat/watch/ui/DmScreen.kt +++ b/wear/src/main/java/com/bitchat/watch/ui/DmScreen.kt @@ -75,7 +75,12 @@ fun DmScreen( val nickname = mesh?.getPeerNickname(peerID) ?: peerID.take(8) val identityRevision by WearPeerIdentityState.revision.collectAsState() - val identity = remember(peerID, identityRevision) { + // Keyed on the announced nickname too: the snapshot now reads it to + // decide whether the seal still applies, and `revision` is bumped by + // favourites and verification changes, never by a peer renaming. Without + // this the name repaints while the cached "verified" — and its seal — + // survives, which is the live impersonation the binding exists to stop. + val identity = remember(peerID, identityRevision, nickname) { WearPeerIdentityState.snapshot(peerID, mesh) } var sessionEstablished by remember { diff --git a/wear/src/main/java/com/bitchat/watch/ui/PeerDebugScreen.kt b/wear/src/main/java/com/bitchat/watch/ui/PeerDebugScreen.kt index 1994f976..d9e8ac3c 100644 --- a/wear/src/main/java/com/bitchat/watch/ui/PeerDebugScreen.kt +++ b/wear/src/main/java/com/bitchat/watch/ui/PeerDebugScreen.kt @@ -76,7 +76,8 @@ fun PeerDebugScreen() { items(peers) { peerID -> val nick = nicknames[peerID] ?: peerID.take(8) val encrypted = mesh?.hasEstablishedSession(peerID) == true - val identity = androidx.compose.runtime.remember(peerID, identityRevision) { + // Keyed on the nickname too — see DmScreen. + val identity = androidx.compose.runtime.remember(peerID, identityRevision, nick) { WearPeerIdentityState.snapshot(peerID, mesh) } Row( diff --git a/wear/src/main/java/com/bitchat/watch/ui/UserDetailScreen.kt b/wear/src/main/java/com/bitchat/watch/ui/UserDetailScreen.kt index fd6a347f..ec39e318 100644 --- a/wear/src/main/java/com/bitchat/watch/ui/UserDetailScreen.kt +++ b/wear/src/main/java/com/bitchat/watch/ui/UserDetailScreen.kt @@ -40,10 +40,13 @@ fun UserDetailScreen( ) { val mesh = WearMeshService.peek() val revision by WearPeerIdentityState.revision.collectAsState() - val identity = androidx.compose.runtime.remember(peerID, revision) { + val nickname = mesh?.getPeerNickname(peerID) ?: peerID.take(8) + // Keyed on the announced nickname too — see DmScreen. This screen draws the + // seal as well, so without it a rename repaints the name here and leaves + // the checkmark sitting beside the new one. + val identity = androidx.compose.runtime.remember(peerID, revision, nickname) { WearPeerIdentityState.snapshot(peerID, mesh) } - val nickname = mesh?.getPeerNickname(peerID) ?: peerID.take(8) val listState = rememberScalingLazyListState(initialCenterItemIndex = 0) val palette = LocalBitchatPalette.current diff --git a/wear/src/main/java/com/bitchat/watch/ui/VerificationCodeScreen.kt b/wear/src/main/java/com/bitchat/watch/ui/VerificationCodeScreen.kt index 4644e9bd..09218ed7 100644 --- a/wear/src/main/java/com/bitchat/watch/ui/VerificationCodeScreen.kt +++ b/wear/src/main/java/com/bitchat/watch/ui/VerificationCodeScreen.kt @@ -36,7 +36,9 @@ import com.bitchat.watch.ui.theme.LocalBitchatPalette fun VerificationCodeScreen(peerID: String) { val mesh = WearMeshService.peek() val revision by WearPeerIdentityState.revision.collectAsState() - val identity = androidx.compose.runtime.remember(peerID, revision) { + // Keyed on the announced nickname too — see DmScreen. + val announcedNickname = mesh?.getPeerNickname(peerID) + val identity = androidx.compose.runtime.remember(peerID, revision, announcedNickname) { WearPeerIdentityState.snapshot(peerID, mesh) } val myFingerprint = WearPeerIdentityState.myFingerprint(mesh)