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", "Ｍedic")) + assertFalse(NicknameBinding.sealAppliesToAnnounced("Medic", "Ｍedic")) // ...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 }