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..3e8ff79e --- /dev/null +++ b/app/src/main/java/com/bitchat/android/identity/NicknameBinding.kt @@ -0,0 +1,116 @@ +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 `Ｍedic` 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 `#ＡＢＣＤ` 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) + } + + /** 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 + * [announced]? + * + * 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 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? + * + * 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. + * + * 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 = + sealAppliesToAnnounced(pinned, rendered) + +} 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..99aa5937 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, announcedNickname: String?): Boolean = + !NicknameBinding.sealAppliesToAnnounced(getVerifiedNickname(fingerprint), announcedNickname) + + 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..faf8fe0e 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,44 @@ 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. + * + * 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?, + ): 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..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) } @@ -864,7 +871,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 +1009,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, @@ -1784,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/SecurityVerificationSheet.kt b/app/src/main/java/com/bitchat/android/ui/SecurityVerificationSheet.kt index 71e59f0d..07714e66 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 ) @@ -135,7 +146,7 @@ fun SecurityVerificationSheet( ) SecurityVerificationActions( - isVerified = isVerified, + isVerified = nameBound && isVerified, fingerprint = fingerprint, displayName = displayName, accent = accent, @@ -175,25 +186,42 @@ private fun SecurityVerificationHeader( @Composable private fun buildStatusInfo( isVerified: Boolean, + nameBound: Boolean, sessionState: String?, accent: Color ): SecurityStatusInfo { + // 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) 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..4c0cc725 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?): 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,59 @@ 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.sealAppliesToRendered( + 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..9ea1904f --- /dev/null +++ b/app/src/test/java/com/bitchat/android/identity/NicknameBindingTest.kt @@ -0,0 +1,172 @@ +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.sealAppliesToAnnounced("ravi", "medic")) + } + + @Test + fun `the seal stands while the name is unchanged`() { + assertTrue(NicknameBinding.sealAppliesToAnnounced("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.sealAppliesToAnnounced("Ravi", "ravi")) + assertTrue(NicknameBinding.sealAppliesToAnnounced("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.sealAppliesToAnnounced(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 Ｍ 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.sealAppliesToAnnounced("Medic", "Ｍedic")) + // ...and so does a Cyrillic М. + assertFalse(NicknameBinding.sealAppliesToAnnounced("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.sealAppliesToAnnounced("medic", "medic")) + 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 "#ＡＢＣＤ" would be truncated and could then match a + // baseline it is not — a seal on a name that was never verified. + assertEquals("medic#ＡＢＣＤ", + NicknameBinding.withoutCollisionSuffix("medic#ＡＢＣＤ")) + 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.sealAppliesToAnnounced("ravi@hq", "ravi@hq")) + assertTrue(NicknameBinding.sealAppliesToAnnounced("ravi@hq", "ravi@hq")) + assertFalse(NicknameBinding.sealAppliesToAnnounced("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.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.sealAppliesToAnnounced("medic", null)) + assertTrue(NicknameBinding.sealAppliesToAnnounced("medic", "")) + // 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 ----------------- + + @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 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")) + } + + // ---- the key itself ---------------------------------------------------- + + @Test + fun `the binding key is idempotent`() { + for (name in listOf("Medic", "ravi@hq", "José", "İstanbul", "Ｍedic", "")) { + 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.sealAppliesToAnnounced("medic", "zebra")) + assertFalse(NicknameBinding.sealAppliesToAnnounced("medic", "medic2")) + assertFalse(NicknameBinding.sealAppliesToAnnounced("medic", "medi")) + } +} 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) 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 }