Fix: a contact with no address was pinned as the string "null"
The same platform difference as the vault index, in shipped code, with a worse consequence. 🔴 `saveTrust` wrote `"fnid": null` explicitly when a contact's global address was not yet known. Android's bundled `org.json` reads a JSON null back as the four-character string `"null"` — a non-null value that is not an address. What that caused on a phone: - `Trust.pin` compares a newly offered fnid against the pinned one and treats a difference as a different person. `"null" != "fnid:…"`, so the FIRST TIME a real address arrived the contact was marked **CHANGED** — the "security keys changed" warning, which is supposed to mean an attack, firing on an ordinary first sighting. The vault's own comment about minting a fresh device warns about exactly this shape of false alarm. - `rememberIdentity` returns early once `fnid != null`, so the real address was never resolved and the record stayed wrong permanently. - `pinnedFnid` fed `"null"` into key-log verification, which recomputes a leaf against the address a contact was accepted under — every check against the wrong subject. Two halves, because records written the old way are already on phones: the writer now omits the key entirely (an absent key reads as `""` on both platforms, which is what the reader always expected), and the reader rejects the literal `"null"` so existing records repair themselves. ⚠️ The test that matters is the one feeding the reader the literal string. On the JVM a real JSON null reads back as `""` whether the key is omitted or not, so the plain round-trip passes with the bug intact — which is why this survived every suite. Confirmed by restoring the old reader and watching it go red. Found by the sweep flagged in android!54. `JSONObject.NULL` is written in exactly two places in this app, and both were wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
436ce240c7
commit
2d05bbe7c4
@@ -461,15 +461,42 @@ class Vault(private val storage: Storage, private val key: SecretKey) {
|
|||||||
Trust(
|
Trust(
|
||||||
identities = (0 until a.length()).map { a.getString(it) },
|
identities = (0 until a.length()).map { a.getString(it) },
|
||||||
verified = o.optBoolean("verified", false),
|
verified = o.optBoolean("verified", false),
|
||||||
fnid = o.optString("fnid").ifEmpty { null },
|
// 🔴 **`"null"` is rejected, and that is not paranoia — records
|
||||||
|
// written that way are already on phones.** `saveTrust` used to
|
||||||
|
// write an explicit JSON null here, and Android's `org.json`
|
||||||
|
// coerces that to the four-character string on the way back out.
|
||||||
|
// See the note on [saveTrust]; this is the half that repairs what
|
||||||
|
// has already been stored.
|
||||||
|
fnid = o.optString("fnid").takeIf { it.isNotEmpty() && it != "null" },
|
||||||
changed = o.optBoolean("changed", false),
|
changed = o.optBoolean("changed", false),
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* 🔴 **The key is OMITTED when there is no address, never written as an
|
||||||
|
* explicit JSON null.** Android's bundled `org.json` reads a JSON null back
|
||||||
|
* as the four-character string `"null"`, so a contact pinned before their
|
||||||
|
* fnid was known reloaded with `fnid = "null"` — a non-null value that is
|
||||||
|
* not an address. What that caused:
|
||||||
|
*
|
||||||
|
* - `Trust.pin` compares a newly offered fnid against the pinned one and
|
||||||
|
* treats a difference as a different person. `"null" != "fnid:…"`, so the
|
||||||
|
* first time a real address arrived the contact was marked **CHANGED** —
|
||||||
|
* the "security keys changed" warning, which is supposed to mean an
|
||||||
|
* attack, fired on an ordinary first sighting.
|
||||||
|
* - `rememberIdentity` returns early once `fnid != null`, so the real
|
||||||
|
* address was never resolved and the record stayed wrong for good.
|
||||||
|
* - `pinnedFnid` fed `"null"` into key-log verification, which recomputes a
|
||||||
|
* leaf against the address a contact was accepted under. Every check
|
||||||
|
* would have been against the wrong subject.
|
||||||
|
*
|
||||||
|
* An absent key reads as `""` on both platforms, which is what the reader
|
||||||
|
* has always expected.
|
||||||
|
*/
|
||||||
fun saveTrust(userId: Long, trust: Trust) = writeJson(trustName(userId), JSONObject()
|
fun saveTrust(userId: Long, trust: Trust) = writeJson(trustName(userId), JSONObject()
|
||||||
.put("identities", org.json.JSONArray(trust.identities))
|
.put("identities", org.json.JSONArray(trust.identities))
|
||||||
.put("verified", trust.verified)
|
.put("verified", trust.verified)
|
||||||
.put("fnid", trust.fnid ?: JSONObject.NULL)
|
.also { if (trust.fnid != null) it.put("fnid", trust.fnid) }
|
||||||
.put("changed", trust.changed))
|
.put("changed", trust.changed))
|
||||||
|
|
||||||
fun forgetTrust(userId: Long) = storage.delete(trustName(userId))
|
fun forgetTrust(userId: Long) = storage.delete(trustName(userId))
|
||||||
|
|||||||
@@ -315,4 +315,66 @@ class VaultTest {
|
|||||||
Vault(storage, secret).saveIdentity(identity)
|
Vault(storage, secret).saveIdentity(identity)
|
||||||
assertNotEquals(Bytes.b64(first), Bytes.b64(storage.records.getValue("identity")))
|
assertNotEquals(Bytes.b64(first), Bytes.b64(storage.records.getValue("identity")))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ---- Trust records and the platform's JSON ---------------------------
|
||||||
|
|
||||||
|
/**
|
||||||
|
* 🔴 **A contact with no known address must come back with NO address.**
|
||||||
|
*
|
||||||
|
* `saveTrust` used to write `"fnid": null` explicitly, and Android's
|
||||||
|
* bundled `org.json` reads a JSON null back as the four-character string
|
||||||
|
* `"null"` — a non-null value that is not an address. `Trust.pin` compares a
|
||||||
|
* newly offered fnid against the pinned one and treats a difference as a
|
||||||
|
* different person, so the first time a real address arrived the contact was
|
||||||
|
* marked **CHANGED**: the "security keys changed" warning, which is meant to
|
||||||
|
* mean an attack, fired on an ordinary first sighting.
|
||||||
|
*
|
||||||
|
* ⚠️ The JVM's `org.json` returns `""` there, so this passed on a laptop and
|
||||||
|
* failed on a phone. The two tests below cover both halves — what is
|
||||||
|
* written now, and what has to be tolerated because it is already on
|
||||||
|
* devices in the field.
|
||||||
|
*/
|
||||||
|
@Test
|
||||||
|
fun `a trust record with no address writes no address at all`() {
|
||||||
|
val storage = MapStorage()
|
||||||
|
val secret = key()
|
||||||
|
Vault(storage, secret).saveTrust(7, Vault.Trust(listOf("aaa"), verified = false, fnid = null))
|
||||||
|
|
||||||
|
assertNull(Vault(storage, secret).loadTrust(7)?.fnid,
|
||||||
|
"an absent address came back as a value")
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* ⚠️ **This is the test that reproduces the device**, and the one above is
|
||||||
|
* not. On the JVM a real JSON null reads back as `""` whether the writer
|
||||||
|
* omits the key or not, so the round trip above passes with the bug intact.
|
||||||
|
* Handing the reader the literal string is what Android actually does, so
|
||||||
|
* writing it here is what makes a laptop run tell the truth.
|
||||||
|
*/
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `a record already holding the literal string null reads as no address`() {
|
||||||
|
// 🔴 Records written the old way exist on phones already. The reader has
|
||||||
|
// to repair them, or those contacts stay permanently mis-addressed —
|
||||||
|
// `rememberIdentity` returns early once `fnid != null`, so nothing would
|
||||||
|
// ever ask again.
|
||||||
|
val storage = MapStorage()
|
||||||
|
val secret = key()
|
||||||
|
val vault = Vault(storage, secret)
|
||||||
|
vault.saveTrust(7, Vault.Trust(listOf("aaa"), verified = false, fnid = "null"))
|
||||||
|
|
||||||
|
assertNull(Vault(storage, secret).loadTrust(7)?.fnid,
|
||||||
|
"the literal string \"null\" was accepted as somebody's global address")
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `a real address still round-trips`() {
|
||||||
|
val storage = MapStorage()
|
||||||
|
val secret = key()
|
||||||
|
Vault(storage, secret).saveTrust(
|
||||||
|
7, Vault.Trust(listOf("aaa"), verified = true, fnid = "fnid:abc123"))
|
||||||
|
val back = Vault(storage, secret).loadTrust(7)
|
||||||
|
assertEquals("fnid:abc123", back?.fnid)
|
||||||
|
assertTrue(back!!.verified)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user