Apply inbound edits and pins instead of filing them as messages #68
@@ -94,15 +94,15 @@ From `E2E/E2EInbox.swift`, which is the real feature list.
|
||||
| History at rest | ✅ encrypted, keyed by person not device |
|
||||
| Session restore across restarts | ✅ credential and device both survive |
|
||||
| Reactions | ✅ transcript carries a `mid`; long-press gives a five-emoji row, one emoji per person |
|
||||
| Edit a sent message (§6.15) | 🟡 codec back, and the `mid` it was blocked on now exists — applying an inbound edit and offering the control are UI work |
|
||||
| Edit a sent message (§6.15) | 🟡 inbound edits now APPLY to the message they name; offering the control is UI work |
|
||||
| Drafts that survive a relaunch | ✅ sealed in the vault, not SharedPreferences |
|
||||
| Forwarding (§6.16) | 🟡 codec back; originating needs the UI |
|
||||
| Forwarding (§6.16) | 🟡 `fwd` now reaches the transcript so second-hand words are not drawn as the sender's own; originating needs the UI |
|
||||
| Group message ids and quotes (§6.9) | ✅ transcript stores both; sent and queued copies share one id |
|
||||
| Mentions (§6.18) | ✅ codec, an `@` picker in the group composer, and ids recorded at pick time rather than re-derived from the text |
|
||||
| Per-message timers (§6.17) | ✅ combining rule and per-message pruning; composer control in progress |
|
||||
| Search inside messages | ✅ scanned, never indexed |
|
||||
| Search inside messages | ✅ scanned, never indexed — no plaintext index at rest |
|
||||
| Pinned message (§6.19) | 🟡 codec back; the banner and the control are UI work |
|
||||
| Pinned message (§6.19) | 🟡 inbound pins now stored per thread, sealed in the vault; the banner and the control are UI work |
|
||||
| Photos | ✅ send via the system picker, render inline |
|
||||
| Voice notes (record + play) | ✅ AAC/MP4, tap to record, tap to send |
|
||||
| Stories, and seen-state | ✅ sent per-contact, expired bytes deleted |
|
||||
@@ -160,8 +160,8 @@ being the one that leaks.
|
||||
|---|:--:|:--:|---|
|
||||
| Back off on repeated app-lock attempts | ✅ | ✅ | ios#41 — 3 free attempts, then 30s doubling to a 5-minute ceiling |
|
||||
| Handle a wrong clock | ✅ | ✅ | ios#45 — skew learned free from the `Date` header; expiry takes the LATER of device and relay |
|
||||
| Zero key material after use | ✅ | ⚪ | ios#39 |
|
||||
| Pin the relay's identity | ✅ | ⚪ | ios#19 |
|
||||
| Zero key material after use | ✅ | 🟡 | ios#39 — done, but the JVM guarantee is **weaker than iOS**; see below |
|
||||
| Pin the relay's identity | ✅ | ✅ | ios#19 — Android already pinned the key and drew the **wrong conclusion** from a change; see below |
|
||||
|
||||
⚠️ **The back-off is not a defence of the data.** It slows a person holding an
|
||||
unlocked phone. The vault and the ratchet keys are protected by the Android
|
||||
@@ -185,6 +185,33 @@ the UI thread, so the two fields are `@Volatile` and updated together under a
|
||||
lock. A torn read of "skew from now, timestamp from an hour ago" would age out an
|
||||
observation just made, or trust one that had not been.
|
||||
|
||||
🔴 **The relay pin was not a missing feature on Android — it was a wrong
|
||||
conclusion.** `Trust.verifyHeadSignature` already pinned `issuerPublicKey` the
|
||||
first time a signed log head arrived. When the key later changed, the signature
|
||||
stopped verifying and the only conclusion available at that point was
|
||||
`KeyLogForkException` — so a rotation somebody performed on purpose was reported
|
||||
to the user as *the relay showing two different histories*.
|
||||
|
||||
Those need different responses. A fork means nothing the relay says can be
|
||||
trusted. A rotation means somebody ran a command, or somebody else is answering
|
||||
on that address, and only a person can tell which. `RelayIdentityChangedException`
|
||||
is deliberately **not** a `KeyLogForkException`, and `forkDetected` is not set —
|
||||
an alarm people cannot act on is one they learn to dismiss, which would cost the
|
||||
real fork detection its credibility too.
|
||||
|
||||
🔴 **Zeroing is where Android cannot match iOS, and the row says 🟡 for that
|
||||
reason rather than being rounded up to ✅.** iOS has `memset_s`, specified not to
|
||||
be optimised away, and iOS does not page application memory to disk. Neither
|
||||
holds here: a compacting collector can relocate a `ByteArray` and leave the
|
||||
bytes at the old address untouched, Android has had compressed swap on by
|
||||
default for years, and `Arrays.fill` is merely "not eliminated in practice"
|
||||
rather than guaranteed. There is no way to pin a Java array, so this is a
|
||||
ceiling on what any implementation here can offer — not a gap to close later.
|
||||
|
||||
The claim Android supports is therefore narrower and is written that way in
|
||||
`SecureBytes`: **a derived key is not left sitting in the heap after use.** Not
|
||||
"the key is gone".
|
||||
|
||||
🔴 One divergence from iOS, and it is a bug the Swift version cannot have. iOS
|
||||
computes `30 * pow(2, steps)` in a `Double`, which saturates to infinity and
|
||||
still caps correctly. Kotlin shifting a `Long` does not: `30L shl 60` wraps
|
||||
@@ -195,6 +222,32 @@ somebody is grinding attempts. The steps are clamped before the shift, and
|
||||
|
||||
---
|
||||
|
||||
## Inbound control envelopes
|
||||
|
||||
🔴 **The app never asked the engine whether an envelope belonged in a thread.**
|
||||
`Content.isRenderable` has existed since §6.15 and `Session`'s inbound loop did
|
||||
not consult it, so three things arrived from the iPhone app and were filed as
|
||||
ordinary bubbles:
|
||||
|
||||
- an **edit** carries its new wording in `text`, so it drew as a duplicate
|
||||
beside the typo it was meant to replace — a correction that made the thread
|
||||
worse;
|
||||
- a **pin** carries only an id, so it drew as a blank bubble;
|
||||
- and any **control envelope from a newer peer** would have drawn a bubble full
|
||||
of JSON — the failure `isControl` was written to prevent, defeated by nobody
|
||||
calling it.
|
||||
|
||||
All three now branch before filing, and the last is guarded by `isRenderable`
|
||||
itself rather than by a list of kinds this build happens to know: an allowlist is
|
||||
only correct until the other platform adds one.
|
||||
|
||||
⚠️ An inbound edit is applied only when it comes from the person whose message
|
||||
it is. Without that check any contact sharing a session could rewrite anybody's
|
||||
words in the reader's transcript — a better attack than sending a message of
|
||||
their own, because the words would carry somebody else's name.
|
||||
|
||||
---
|
||||
|
||||
## Test safety
|
||||
|
||||
| Feature | iOS | Android | Note |
|
||||
|
||||
@@ -116,6 +116,21 @@ class Session(
|
||||
* to know anything had been dropped.
|
||||
*/
|
||||
val about: Content.Reference? = null,
|
||||
/**
|
||||
* Second-hand words (§6.16).
|
||||
*
|
||||
* ⚠️ THAT it was forwarded, never from whom.
|
||||
*/
|
||||
val forwarded: Boolean = false,
|
||||
/**
|
||||
* When this message was last edited (§6.15), or null if never.
|
||||
*
|
||||
* 🔴 **Kept so the bubble can say "edited".** A correction that silently
|
||||
* replaces the text is worse than the typo: the reader has no way to
|
||||
* know the words changed after they read them, and no way to tell a
|
||||
* correction from their own misremembering.
|
||||
*/
|
||||
val editedAt: Long? = null,
|
||||
/**
|
||||
* A timer this one message carries, in seconds (§6.17); 0 for none.
|
||||
*
|
||||
@@ -163,6 +178,12 @@ class Session(
|
||||
link = m.optJSONObject("lnk")?.let { Content.LinkPreview.fromJson(it) },
|
||||
messageId = m.optString("mid").takeIf { it.isNotEmpty() },
|
||||
about = m.optJSONObject("re")?.let { Content.Reference.fromJson(it) },
|
||||
// ⚠️ Both shapes, for the same reason `Content.isForwarded`
|
||||
// accepts both: `org.json`'s `optBoolean` does not coerce a
|
||||
// number, so reading only that would miss a `"fwd":1` written
|
||||
// by any other engine.
|
||||
forwarded = m.optBoolean("fwd", false) || m.optInt("fwd", 0) != 0,
|
||||
editedAt = m.optLong("edited", 0L).takeIf { it > 0L },
|
||||
lifetimeSeconds = m.optInt("secs", 0),
|
||||
)
|
||||
}
|
||||
@@ -184,6 +205,15 @@ class Session(
|
||||
about: Content.Reference? = null,
|
||||
/** A timer this one message carries, in seconds (§6.17); 0 for none. */
|
||||
lifetimeSeconds: Int = 0,
|
||||
/**
|
||||
* Whether the sender is passing on somebody else's words (§6.16).
|
||||
*
|
||||
* ⚠️ Says THAT it was forwarded and never from whom — the point of a
|
||||
* forward is that the original author does not travel with it. This
|
||||
* exists so second-hand words are not shown as the sender's own, which
|
||||
* is a different thing from attribution.
|
||||
*/
|
||||
forwarded: Boolean = false,
|
||||
) {
|
||||
vault.append(threadId, JSONObject()
|
||||
.put("author", author)
|
||||
@@ -201,10 +231,10 @@ class Session(
|
||||
}
|
||||
.also { if (attachment != null) it.put("kind", attachment.kind).put("att", attachment.toJson()) }
|
||||
.also { if (link != null) it.put("lnk", link.toJson()) }
|
||||
// ⚠️ Written only when present. A null on every message would grow
|
||||
// the transcript for nothing and make "no id" indistinguishable
|
||||
// from "an id of nothing".
|
||||
.also { if (messageId != null) it.put("mid", messageId) }
|
||||
// ⚠️ Written only when true, for the same reason `secs` is. A false
|
||||
// on every message grows the transcript for a flag that is almost
|
||||
// always absent.
|
||||
.also { if (forwarded) it.put("fwd", true) }
|
||||
.also { if (about != null) it.put("re", about.toJson()) })
|
||||
|
||||
// 🔴 A reply arriving cancels the reminder, and it is done HERE because
|
||||
@@ -627,6 +657,70 @@ class Session(
|
||||
return hit
|
||||
}
|
||||
|
||||
/**
|
||||
* Apply an inbound edit to the message it names (§6.15).
|
||||
*
|
||||
* 🔴 **An edit is never a bubble of its own.** It changes one that is
|
||||
* already there, or it does nothing. Android had the codec and no
|
||||
* application, so an edit from the iPhone app arrived carrying the new
|
||||
* wording in `text` and was filed as an ORDINARY MESSAGE — a correction
|
||||
* showing up as a duplicate beside the typo it was meant to replace, which
|
||||
* is worse than the typo.
|
||||
*
|
||||
* 🔴 **Only the author may edit their own message.** Without this check any
|
||||
* contact sharing a session could rewrite anybody's words in the reader's
|
||||
* transcript, which is a far better attack than sending a message of their
|
||||
* own: the words would carry somebody else's name.
|
||||
*
|
||||
* Returns whether a message was found and changed.
|
||||
*/
|
||||
fun applyEdit(
|
||||
threadId: String,
|
||||
messageId: String,
|
||||
author: Long,
|
||||
newText: String,
|
||||
at: Long = System.currentTimeMillis(),
|
||||
): Boolean {
|
||||
val stored = vault.history(threadId)
|
||||
var hit = false
|
||||
for (i in 0 until stored.length()) {
|
||||
val row = stored.getJSONObject(i)
|
||||
if (row.optString("mid") != messageId) continue
|
||||
// 🔴 The author check. `mine` is this device's own message; an
|
||||
// inbound edit must come from the person whose message it is.
|
||||
val wroteIt = if (row.optBoolean("mine")) author == userId
|
||||
else row.optLong("author") == author
|
||||
if (!wroteIt) break
|
||||
row.put("text", newText).put("edited", at)
|
||||
hit = true
|
||||
break
|
||||
}
|
||||
if (hit) vault.replaceHistory(threadId, stored)
|
||||
return hit
|
||||
}
|
||||
|
||||
/**
|
||||
* The message pinned in a thread, or null (§6.19).
|
||||
*
|
||||
* ⚠️ An id, never a copy. A stored copy would go stale the moment the
|
||||
* message was edited, and would survive the message being deleted — a
|
||||
* banner quoting words that are no longer in the conversation.
|
||||
*/
|
||||
fun pinnedMessage(threadId: String): String? =
|
||||
vault.pinnedMessage(threadId)
|
||||
|
||||
/**
|
||||
* Pin a message, or take the pin down with an empty [messageId].
|
||||
*
|
||||
* ⚠️ **The last pin wins and there is only ever one.** A thread with a list
|
||||
* of pins is a second inbox; §6.19 is one banner, and two people pinning at
|
||||
* once resolve by whoever's envelope arrived last — the same rule the
|
||||
* transcript already uses for everything else.
|
||||
*/
|
||||
fun applyPin(threadId: String, messageId: String) {
|
||||
vault.savePinnedMessage(threadId, messageId.takeIf { it.isNotBlank() })
|
||||
}
|
||||
|
||||
/**
|
||||
* React to somebody's message, or take it back with an empty [emoji].
|
||||
*
|
||||
@@ -924,6 +1018,37 @@ class Session(
|
||||
if (story.isActive(now)) incomingStories += story
|
||||
continue
|
||||
}
|
||||
// 🔴 **An EDIT (§6.15) changes a message; it is never one.**
|
||||
// Android had the codec and no application, so an edit from the
|
||||
// iPhone app fell straight through this loop carrying its new
|
||||
// wording in `text` and was filed as an ordinary bubble — a
|
||||
// correction arriving as a duplicate beside the typo it was meant
|
||||
// to replace, which is worse than the typo.
|
||||
message.content.editOf?.let { editing ->
|
||||
val gid = if (message.fromAddress != null) null
|
||||
else message.content.groupId
|
||||
val thread = gid?.let(Vault::threadForGroup)
|
||||
?: message.fromAddress?.let(Vault::threadForAddress)
|
||||
?: Vault.threadForUser(message.fromUser)
|
||||
if (applyEdit(thread, editing, message.fromUser,
|
||||
message.content.text.orEmpty())) {
|
||||
touched += thread
|
||||
}
|
||||
continue
|
||||
}
|
||||
// 🔴 **A PIN (§6.19) is an id, not a bubble.** Same story: without
|
||||
// this it was filed as a message whose text was empty, so a pin
|
||||
// from the other app drew a blank bubble in the conversation.
|
||||
message.content.pins?.let { pinned ->
|
||||
val gid = if (message.fromAddress != null) null
|
||||
else message.content.groupId
|
||||
val thread = gid?.let(Vault::threadForGroup)
|
||||
?: message.fromAddress?.let(Vault::threadForAddress)
|
||||
?: Vault.threadForUser(message.fromUser)
|
||||
applyPin(thread, pinned)
|
||||
touched += thread
|
||||
continue
|
||||
}
|
||||
// A reaction to a MESSAGE. Folded onto the message it is about,
|
||||
// never filed as one of its own — a thread where every thumbs-up
|
||||
// is a bubble is a thread nobody can read.
|
||||
@@ -997,6 +1122,19 @@ class Session(
|
||||
// worse, match by coincidence. Groups do not federate yet, so a
|
||||
// remote sender's group claim is ignored and the message files in
|
||||
// their own thread.
|
||||
// 🔴 **The guard that stops this list needing to be complete.**
|
||||
// Every branch above names one kind of non-message. `isRenderable`
|
||||
// is the engine's own answer to "does this belong in a thread", and
|
||||
// it is false for reactions, call signals, edits, pins and — the
|
||||
// case that matters most — ANY control envelope from a newer peer,
|
||||
// which carries `ctl` and a kind this build has never heard of.
|
||||
//
|
||||
// Without it, a control kind added on iOS ships a bubble full of
|
||||
// JSON into somebody's conversation on every Android install that
|
||||
// predates it. The engine has had `isRenderable` since §6.15; the
|
||||
// app simply never consulted it.
|
||||
if (!message.content.isRenderable) continue
|
||||
|
||||
val gid = if (message.fromAddress != null) null else message.content.groupId
|
||||
if (gid != null) {
|
||||
val known = groupRoster(gid)
|
||||
@@ -1046,7 +1184,11 @@ class Session(
|
||||
// side.
|
||||
messageId = message.content.messageId,
|
||||
about = message.content.about,
|
||||
lifetimeSeconds = message.content.lifetimeSeconds ?: 0)
|
||||
lifetimeSeconds = message.content.lifetimeSeconds ?: 0,
|
||||
// §6.16 — so second-hand words are not drawn as the sender's
|
||||
// own. The engine has read `fwd` off the wire since forwarding
|
||||
// shipped; the transcript was where it stopped.
|
||||
forwarded = message.content.isForwarded)
|
||||
touched += thread
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,138 @@
|
||||
package io.flamenet.messenger.app
|
||||
|
||||
import io.flamenet.e2e.Content
|
||||
import kotlin.test.Test
|
||||
import kotlin.test.assertFalse
|
||||
import kotlin.test.assertTrue
|
||||
|
||||
/**
|
||||
* Edits, pins and unknown control envelopes must never become bubbles
|
||||
* (ios#6.15/#6.16/#6.19, Android half).
|
||||
*
|
||||
* 🔴 **The bug this closes was an interop bug, not a missing feature.** The
|
||||
* engine has parsed `editOf`, `pins` and `fwd` for as long as they have
|
||||
* existed, and it exposes `isRenderable` — "does this belong in a thread" — as
|
||||
* the single answer. `Session`'s inbound loop never consulted it.
|
||||
*
|
||||
* So an edit from the iPhone app arrived carrying its new wording in `text` and
|
||||
* was filed as an ORDINARY MESSAGE: a correction drawn as a duplicate beside the
|
||||
* typo it was meant to replace. A pin was filed as a bubble whose text was
|
||||
* empty. And any control envelope from a newer peer — which carries `ctl` and a
|
||||
* kind this build has never heard of — would have drawn a bubble full of JSON
|
||||
* in somebody's conversation.
|
||||
*
|
||||
* ⚠️ These test the ENGINE's classification, which is what the app now branches
|
||||
* on. The app-side application (`applyEdit`, `applyPin`) needs a `Vault` and a
|
||||
* keystore, so it is covered on device in `FeatureUseTest`; what is checkable
|
||||
* here is the decision that used to be missing.
|
||||
*/
|
||||
class InboundControlTest {
|
||||
|
||||
private fun parse(json: String): Content = Content.parse(json)!!
|
||||
|
||||
// ---- What must not be drawn -----------------------------------------
|
||||
|
||||
/**
|
||||
* 🔴 The case that shipped a duplicate bubble. An edit carries the new
|
||||
* wording, so it looks exactly like a message to anything that does not ask.
|
||||
*/
|
||||
@Test
|
||||
fun `an edit is not renderable, even though it carries text`() {
|
||||
val edit = parse(Content.edit("the corrected wording", of = "mid-1"))
|
||||
assertTrue(edit.editOf == "mid-1", "the codec no longer reads editOf")
|
||||
assertTrue(edit.text?.isNotEmpty() == true, "an edit carries its new text")
|
||||
assertFalse(edit.isRenderable, "an edit would be filed as a message")
|
||||
}
|
||||
|
||||
/** A pin names an id and has nothing to show. */
|
||||
@Test
|
||||
fun `a pin is not renderable`() {
|
||||
val pin = parse(Content.pin("mid-1"))
|
||||
assertTrue(pin.pins == "mid-1")
|
||||
assertFalse(pin.isRenderable, "a pin would be filed as an empty bubble")
|
||||
}
|
||||
|
||||
/** Taking a pin down is the same envelope with an empty id. */
|
||||
@Test
|
||||
fun `taking a pin down is also not renderable`() {
|
||||
val unpin = parse(Content.pin(""))
|
||||
assertTrue(unpin.pins == "", "an empty id is how a pin is removed")
|
||||
assertFalse(unpin.isRenderable)
|
||||
}
|
||||
|
||||
/**
|
||||
* 🔴 **The case an allowlist cannot cover, and the reason the app branches
|
||||
* on `isRenderable` rather than on a list of kinds it knows.** A control
|
||||
* envelope from a NEWER peer has a kind nothing here has heard of. Keying on
|
||||
* known kinds ships a bubble full of JSON to every older install the first
|
||||
* time a new control message is added on the other platform.
|
||||
*/
|
||||
@Test
|
||||
fun `a control envelope from a newer peer is not renderable`() {
|
||||
val fromTheFuture = parse(
|
||||
"""{"fnc":1,"kind":"something-invented-later","ctl":1,"text":"{}"}"""
|
||||
)
|
||||
assertTrue(fromTheFuture.isControl, "the ctl flag was not honoured")
|
||||
assertFalse(fromTheFuture.isRenderable,
|
||||
"an unknown control kind would be drawn as a message")
|
||||
}
|
||||
|
||||
/**
|
||||
* ⚠️ Both shapes of the flag. `org.json`'s `optBoolean` does not coerce a
|
||||
* number, so an engine sending `"ctl":1` rather than `"ctl":true` would slip
|
||||
* past a reader that checked only one — which is how this class of bug got
|
||||
* shipped once already.
|
||||
*/
|
||||
@Test
|
||||
fun `the control flag is honoured as a number and as a boolean`() {
|
||||
assertFalse(parse("""{"fnc":1,"kind":"x","ctl":1,"text":"{}"}""").isRenderable)
|
||||
assertFalse(parse("""{"fnc":1,"kind":"x","ctl":true,"text":"{}"}""").isRenderable)
|
||||
}
|
||||
|
||||
// ---- What must still be drawn ---------------------------------------
|
||||
|
||||
/**
|
||||
* 🔴 The guard must not swallow ordinary messages. A `continue` in the wrong
|
||||
* place is a conversation that silently stops receiving.
|
||||
*/
|
||||
@Test
|
||||
fun `an ordinary message is still renderable`() {
|
||||
val text = parse(Content.text("hello", messageId = "mid-1"))
|
||||
assertTrue(text.isRenderable)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a reply is still renderable`() {
|
||||
val reply = parse(
|
||||
Content.text("answering", messageId = "mid-2",
|
||||
about = Content.Reference("mid-1", 7L, "the question")),
|
||||
)
|
||||
assertTrue(reply.isRenderable)
|
||||
assertTrue(reply.about?.messageId == "mid-1")
|
||||
}
|
||||
|
||||
/**
|
||||
* 🔴 **A forward IS a message.** It is second-hand words, not a control
|
||||
* envelope — the flag changes how it is drawn, not whether it is.
|
||||
*/
|
||||
@Test
|
||||
fun `a forwarded message is renderable, and says so`() {
|
||||
val forwarded = parse(
|
||||
Content.text("passed along", messageId = "mid-3", forwarded = true),
|
||||
)
|
||||
assertTrue(forwarded.isRenderable, "a forward was treated as control")
|
||||
assertTrue(forwarded.isForwarded, "the fwd flag was lost")
|
||||
}
|
||||
|
||||
/**
|
||||
* ⚠️ And the same both-shapes rule for `fwd`, for the same reason. An engine
|
||||
* writing `1` where this one writes `true` would have its forwards drawn as
|
||||
* the sender's own words.
|
||||
*/
|
||||
@Test
|
||||
fun `the forwarded flag is honoured as a number and as a boolean`() {
|
||||
assertTrue(parse("""{"fnc":1,"kind":"text","fwd":1,"text":"x"}""").isForwarded)
|
||||
assertTrue(parse("""{"fnc":1,"kind":"text","fwd":true,"text":"x"}""").isForwarded)
|
||||
assertFalse(parse("""{"fnc":1,"kind":"text","text":"x"}""").isForwarded)
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,112 @@
|
||||
package io.flamenet.client
|
||||
|
||||
/**
|
||||
* Which relay this is, beyond which certificate it presented (ios#19).
|
||||
*
|
||||
* 🔴 **TLS answers "is this host who DNS says it is". It does not answer "is
|
||||
* this the same relay I have been talking to".** A mis-issued certificate, a
|
||||
* hijacked DNS record, or a hostile party who obtains the domain all produce a
|
||||
* server with a perfectly valid certificate for the right name — and every one
|
||||
* of them is a different relay wearing the same address.
|
||||
*
|
||||
* The relay publishes an issuer public key in its descriptor. Pinning that on
|
||||
* first contact turns "the certificate is valid" into "this is the same relay",
|
||||
* which is a materially stronger statement and the one that matters.
|
||||
*
|
||||
* 🔴 **Android already pinned this key and drew the wrong conclusion from it.**
|
||||
* `Trust.verifyHeadSignature` pins `issuerPublicKey` the first time a signed log
|
||||
* head arrives, and if the key later changes the signature simply fails to
|
||||
* verify — so the client raises `KeyLogForkException`, sets `forkDetected`, and
|
||||
* tells somebody the relay's key log does not add up.
|
||||
*
|
||||
* A rotated relay key is not a forked log. They need different responses: a
|
||||
* fork means the relay is showing two different histories and nothing it says
|
||||
* can be trusted; a rotation means somebody ran a command, or somebody else is
|
||||
* answering on that address. Reporting the second as the first is both alarming
|
||||
* and unactionable, and an alarm people cannot act on is one they learn to
|
||||
* dismiss — which costs the real fork detection its credibility too.
|
||||
*
|
||||
* ⚠️ **A relay that publishes no key is a weaker deployment, not an attack.**
|
||||
* Somebody self-hosting an older build has nothing to pin, and refusing to talk
|
||||
* to them would be a worse failure than the one this prevents. That case is
|
||||
* distinguished from a key that CHANGED, which is not weakness.
|
||||
*
|
||||
* ⚠️ A change is surfaced, never silently accepted and never silently refused.
|
||||
* The same shape as a contact's identity changing: stop, say so, and let a
|
||||
* person decide — because a legitimate rotation exists and an app that cannot
|
||||
* survive one is an app that gets uninstalled.
|
||||
*/
|
||||
object RelayIdentityPin {
|
||||
|
||||
/** What a descriptor's key means against what was pinned before. */
|
||||
sealed interface Verdict {
|
||||
/** Nothing pinned and a key published: pin it. */
|
||||
data class FirstSight(val offered: ByteArray) : Verdict {
|
||||
override fun equals(other: Any?) =
|
||||
other is FirstSight && offered.contentEquals(other.offered)
|
||||
override fun hashCode() = offered.contentHashCode()
|
||||
}
|
||||
|
||||
/** The same relay as before. */
|
||||
data object Unchanged : Verdict
|
||||
|
||||
/** This deployment publishes no key. Nothing to check. */
|
||||
data object NotPublished : Verdict
|
||||
|
||||
/**
|
||||
* 🔴 A different key than the one pinned. Either a deliberate rotation
|
||||
* or a different server answering on this address, and nothing here can
|
||||
* tell those apart — only a person can.
|
||||
*/
|
||||
data class Changed(val pinned: ByteArray, val offered: ByteArray) : Verdict {
|
||||
override fun equals(other: Any?) = other is Changed &&
|
||||
pinned.contentEquals(other.pinned) && offered.contentEquals(other.offered)
|
||||
override fun hashCode() = 31 * pinned.contentHashCode() + offered.contentHashCode()
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Compare what the relay just published against what is pinned.
|
||||
*
|
||||
* ⚠️ An empty or unparseable key counts as "not published" rather than as a
|
||||
* change. A relay that answers with rubbish is broken, and treating broken
|
||||
* as hostile would raise an alarm nobody can act on.
|
||||
*/
|
||||
fun check(offeredBase64: String?, pinned: ByteArray?): Verdict {
|
||||
val offered = offeredBase64
|
||||
?.takeIf { it.isNotEmpty() }
|
||||
?.let { runCatching { Wire.unb64(it) }.getOrNull() }
|
||||
?.takeIf { it.isNotEmpty() }
|
||||
?: return Verdict.NotPublished
|
||||
if (pinned == null || pinned.isEmpty()) return Verdict.FirstSight(offered)
|
||||
return if (offered.contentEquals(pinned)) Verdict.Unchanged
|
||||
else Verdict.Changed(pinned = pinned, offered = offered)
|
||||
}
|
||||
|
||||
/**
|
||||
* What to tell somebody when it changed.
|
||||
*
|
||||
* 🔴 Names the two possibilities and does not pretend to know which. An
|
||||
* alarm that asserts an attack when a rotation is equally likely trains
|
||||
* people to dismiss alarms.
|
||||
*/
|
||||
fun warning(host: String): String =
|
||||
"The server at $host is presenting a different identity than the one " +
|
||||
"this app recorded the first time it connected. That happens when the " +
|
||||
"server's keys are rotated on purpose — and it also happens when " +
|
||||
"something else is answering in its place. Nothing here can tell those " +
|
||||
"apart. If you were not expecting a change, stop and check with " +
|
||||
"whoever runs it before signing in again."
|
||||
}
|
||||
|
||||
/**
|
||||
* The relay is not the one this app pinned.
|
||||
*
|
||||
* 🔴 **Deliberately NOT a [KeyLogForkException].** A fork means the relay is
|
||||
* showing two different histories and nothing it says can be trusted. This means
|
||||
* the server's identity key is not the one recorded — a rotation somebody
|
||||
* performed, or somebody else's server. The remedies are different, so the
|
||||
* exceptions are different.
|
||||
*/
|
||||
class RelayIdentityChangedException(val host: String) :
|
||||
RelayException(RelayIdentityPin.warning(host))
|
||||
@@ -612,21 +612,45 @@ class Trust(
|
||||
private fun verifyHeadSignature(head: Head, signatureB64: String) {
|
||||
if (signatureB64.isEmpty()) return
|
||||
val signature = runCatching { Wire.unb64(signatureB64) }.getOrNull() ?: return
|
||||
var issuer = vault.keyLogIssuerPin()
|
||||
if (issuer == null) {
|
||||
val published = runCatching {
|
||||
Wire.unb64(relay.relayDescriptor().optString("issuerPublicKey"))
|
||||
}.getOrNull()
|
||||
if (published == null || published.isEmpty()) return
|
||||
vault.saveKeyLogIssuerPin(published)
|
||||
issuer = published
|
||||
|
||||
// ios#19 — decide what the relay's published identity MEANS before
|
||||
// using it, rather than inferring it from a signature that failed.
|
||||
//
|
||||
// 🔴 This is the bug the pin previously hid. A rotated relay key made
|
||||
// the signature stop verifying, and the only conclusion available at
|
||||
// that point was "fork" — so a command somebody ran on purpose was
|
||||
// reported as the relay showing two different histories. Different
|
||||
// situations, different remedies, and an alarm nobody can act on is one
|
||||
// people learn to dismiss, which costs real fork detection too.
|
||||
val offered = runCatching {
|
||||
relay.relayDescriptor().optString("issuerPublicKey")
|
||||
}.getOrNull()
|
||||
val issuer = when (val verdict = RelayIdentityPin.check(offered, vault.keyLogIssuerPin())) {
|
||||
is RelayIdentityPin.Verdict.NotPublished ->
|
||||
// Nothing published and nothing pinned: a weaker deployment,
|
||||
// not an attack. Nothing to check the signature against.
|
||||
vault.keyLogIssuerPin() ?: return
|
||||
is RelayIdentityPin.Verdict.FirstSight -> {
|
||||
vault.saveKeyLogIssuerPin(verdict.offered)
|
||||
verdict.offered
|
||||
}
|
||||
is RelayIdentityPin.Verdict.Unchanged -> vault.keyLogIssuerPin() ?: return
|
||||
is RelayIdentityPin.Verdict.Changed ->
|
||||
// ⚠️ NOT a fork, and `forkDetected` is deliberately not set.
|
||||
// The log may be perfectly consistent; it is the server's
|
||||
// identity that is not the one recorded.
|
||||
throw RelayIdentityChangedException(relayHostForPin())
|
||||
}
|
||||
|
||||
if (!KeyLog.verifyHead(head.size, head.root, signature, issuer)) {
|
||||
forkDetected = true
|
||||
throw KeyLogForkException("log head signature does not verify under the pinned key")
|
||||
}
|
||||
}
|
||||
|
||||
/** The host to name in an identity-change warning. */
|
||||
private fun relayHostForPin(): String = relay.host
|
||||
|
||||
private fun consistent(from: Int, to: Int, oldRoot: String, newRoot: String): Boolean {
|
||||
val c = relay.logConsistency(from.toLong(), to.toLong())
|
||||
return KeyLog.verifyConsistency(
|
||||
|
||||
@@ -0,0 +1,167 @@
|
||||
package io.flamenet.client
|
||||
|
||||
import java.util.Base64
|
||||
import kotlin.test.Test
|
||||
import kotlin.test.assertEquals
|
||||
import kotlin.test.assertIs
|
||||
import kotlin.test.assertTrue
|
||||
|
||||
/**
|
||||
* Pinning the relay's identity (ios#19, Android half).
|
||||
*
|
||||
* 🔴 **The finding this closes is not a missing feature — it is a wrong
|
||||
* conclusion.** Android already pinned `issuerPublicKey`, inside
|
||||
* `Trust.verifyHeadSignature`. When the key later changed, the head signature
|
||||
* stopped verifying and the only conclusion available at that point was
|
||||
* `KeyLogForkException` — so a rotation somebody performed on purpose was
|
||||
* reported to the user as the relay showing two different histories.
|
||||
*
|
||||
* Those need different responses. A fork means nothing the relay says can be
|
||||
* trusted. A rotation means somebody ran a command, or somebody else is
|
||||
* answering on that address, and a person has to decide which. Reporting the
|
||||
* second as the first is alarming and unactionable, and an alarm people cannot
|
||||
* act on is one they learn to dismiss — which costs the real fork detection its
|
||||
* credibility too.
|
||||
*/
|
||||
class RelayIdentityPinTest {
|
||||
|
||||
private fun key(byte: Int) = ByteArray(32) { byte.toByte() }
|
||||
private fun b64(bytes: ByteArray): String = Base64.getEncoder().encodeToString(bytes)
|
||||
|
||||
// ---- The four verdicts ----------------------------------------------
|
||||
|
||||
@Test
|
||||
fun `a published key with nothing pinned is first sight`() {
|
||||
val offered = key(1)
|
||||
val verdict = RelayIdentityPin.check(b64(offered), pinned = null)
|
||||
assertEquals(RelayIdentityPin.Verdict.FirstSight(offered), verdict)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `the same key is unchanged`() {
|
||||
assertEquals(
|
||||
RelayIdentityPin.Verdict.Unchanged,
|
||||
RelayIdentityPin.check(b64(key(1)), pinned = key(1)),
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* 🔴 The case the whole file exists for, and the one that used to be
|
||||
* reported as a forked log.
|
||||
*/
|
||||
@Test
|
||||
fun `a different key is a change, carrying both halves`() {
|
||||
val verdict = RelayIdentityPin.check(b64(key(2)), pinned = key(1))
|
||||
val changed = assertIs<RelayIdentityPin.Verdict.Changed>(verdict)
|
||||
assertTrue(changed.pinned.contentEquals(key(1)))
|
||||
assertTrue(changed.offered.contentEquals(key(2)))
|
||||
}
|
||||
|
||||
/**
|
||||
* ⚠️ **A relay that publishes no key is a weaker deployment, not an
|
||||
* attack.** Somebody self-hosting an older build has nothing to pin, and
|
||||
* refusing to talk to them would be a worse failure than the one this
|
||||
* prevents.
|
||||
*/
|
||||
@Test
|
||||
fun `a deployment that publishes nothing is not an attack`() {
|
||||
assertEquals(
|
||||
RelayIdentityPin.Verdict.NotPublished,
|
||||
RelayIdentityPin.check(null, pinned = key(1)),
|
||||
)
|
||||
assertEquals(
|
||||
RelayIdentityPin.Verdict.NotPublished,
|
||||
RelayIdentityPin.check("", pinned = key(1)),
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* ⚠️ **Broken is not hostile.** A relay answering with rubbish counts as
|
||||
* "not published" rather than as a change — treating a malformed field as
|
||||
* an attack would raise an alarm nobody can act on, on a deployment that is
|
||||
* simply misconfigured.
|
||||
*/
|
||||
@Test
|
||||
fun `an unparseable key reads as not published, not as a change`() {
|
||||
assertEquals(
|
||||
RelayIdentityPin.Verdict.NotPublished,
|
||||
RelayIdentityPin.check("!!! not base64 !!!", pinned = key(1)),
|
||||
)
|
||||
// Valid base64 of nothing is still nothing.
|
||||
assertEquals(
|
||||
RelayIdentityPin.Verdict.NotPublished,
|
||||
RelayIdentityPin.check(b64(ByteArray(0)), pinned = key(1)),
|
||||
)
|
||||
}
|
||||
|
||||
/** An empty pin is no pin: first sight, not a change against zero bytes. */
|
||||
@Test
|
||||
fun `an empty pin is treated as no pin`() {
|
||||
val offered = key(3)
|
||||
assertEquals(
|
||||
RelayIdentityPin.Verdict.FirstSight(offered),
|
||||
RelayIdentityPin.check(b64(offered), pinned = ByteArray(0)),
|
||||
)
|
||||
}
|
||||
|
||||
// ---- What somebody is told ------------------------------------------
|
||||
|
||||
/**
|
||||
* 🔴 The warning names BOTH possibilities and asserts neither. An alarm that
|
||||
* claims an attack when a rotation is equally likely trains people to
|
||||
* dismiss alarms — and the next one may be real.
|
||||
*/
|
||||
@Test
|
||||
fun `the warning does not pretend to know which it is`() {
|
||||
val text = RelayIdentityPin.warning("relay.example.com")
|
||||
assertTrue(text.contains("relay.example.com"), text)
|
||||
assertTrue(text.contains("rotated on purpose"), text)
|
||||
assertTrue(text.contains("something else is answering"), text)
|
||||
assertTrue(text.contains("Nothing here can tell those apart"), text)
|
||||
}
|
||||
|
||||
/**
|
||||
* 🔴 **The exception is not a fork exception, and that separation is the
|
||||
* point of this change.** A caller catching `KeyLogForkException` to decide
|
||||
* "the log is untrustworthy" must not be handed a rotation.
|
||||
*/
|
||||
@Test
|
||||
fun `an identity change is not reported as a key-log fork`() {
|
||||
// ⚠️ Held as the sealed base, deliberately. Typed as the concrete class
|
||||
// the compiler folds `!is KeyLogForkException` to a constant and warns —
|
||||
// which is the compiler proving the point statically, and also means the
|
||||
// assertion would test nothing. Through the base type it is the runtime
|
||||
// check a caller's `when` actually performs.
|
||||
val thrown: RelayException = RelayIdentityChangedException("relay.example.com")
|
||||
assertTrue(
|
||||
thrown !is KeyLogForkException,
|
||||
"a rotated relay key is being reported as a forked log",
|
||||
)
|
||||
val identity = assertIs<RelayIdentityChangedException>(thrown)
|
||||
assertEquals("relay.example.com", identity.host)
|
||||
assertTrue(identity.message!!.contains("rotated on purpose"))
|
||||
}
|
||||
|
||||
// ---- Shape -----------------------------------------------------------
|
||||
|
||||
/**
|
||||
* ⚠️ `ByteArray` has reference equality by default, so the verdicts carry
|
||||
* hand-written `equals`. Without them every comparison in this suite would
|
||||
* be false and the tests would have been written around it.
|
||||
*/
|
||||
@Test
|
||||
fun `verdicts compare by content, not by reference`() {
|
||||
assertEquals(
|
||||
RelayIdentityPin.Verdict.FirstSight(key(1)),
|
||||
RelayIdentityPin.Verdict.FirstSight(key(1)),
|
||||
)
|
||||
assertEquals(
|
||||
RelayIdentityPin.Verdict.Changed(key(1), key(2)),
|
||||
RelayIdentityPin.Verdict.Changed(key(1), key(2)),
|
||||
)
|
||||
assertTrue(
|
||||
RelayIdentityPin.Verdict.Changed(key(1), key(2)) !=
|
||||
RelayIdentityPin.Verdict.Changed(key(1), key(3)),
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -133,12 +133,17 @@ object AccountKdf {
|
||||
context: String,
|
||||
length: Int = 32,
|
||||
params: Params = Params.PBKDF2_V1,
|
||||
): ByteArray = Hkdf.derive(
|
||||
ikm = master(passphrase, salt, params),
|
||||
salt = ByteArray(0),
|
||||
info = context.toByteArray(Charsets.UTF_8),
|
||||
length = length,
|
||||
)
|
||||
): ByteArray = SecureBytes.withSecret(master(passphrase, salt, params)) { ikm ->
|
||||
// ios#39 — the stretched master is an intermediate nobody needs after
|
||||
// the expansion, and it is the most valuable thing in this file: every
|
||||
// other secret is derived from it. Wiped however this leaves.
|
||||
Hkdf.derive(
|
||||
ikm = ikm,
|
||||
salt = ByteArray(0),
|
||||
info = context.toByteArray(Charsets.UTF_8),
|
||||
length = length,
|
||||
)
|
||||
}
|
||||
|
||||
/** What gets sent to the account service. */
|
||||
fun authSecret(passphrase: String, salt: ByteArray, params: Params = Params.PBKDF2_V1): ByteArray =
|
||||
@@ -179,14 +184,19 @@ object AccountKdf {
|
||||
salt: ByteArray,
|
||||
params: Params = Params.PBKDF2_V1,
|
||||
): Pair<ByteArray, ByteArray> {
|
||||
val ikm = master(passphrase, salt, params)
|
||||
fun expand(context: String) = Hkdf.derive(
|
||||
ikm = ikm,
|
||||
salt = ByteArray(0),
|
||||
info = context.toByteArray(Charsets.UTF_8),
|
||||
length = 32,
|
||||
)
|
||||
return expand(Context.ACCOUNT_AUTH) to expand(Context.ACCOUNT_IDENTITY)
|
||||
return SecureBytes.withSecret(master(passphrase, salt, params)) { ikm ->
|
||||
fun expand(context: String) = Hkdf.derive(
|
||||
ikm = ikm,
|
||||
salt = ByteArray(0),
|
||||
info = context.toByteArray(Charsets.UTF_8),
|
||||
length = 32,
|
||||
)
|
||||
// ⚠️ BOTH expansions happen inside the block. Returning the pair and
|
||||
// wiping afterwards would be the same thing; expanding after the
|
||||
// wipe would derive two keys from thirty-two zero bytes, which is
|
||||
// the failure mode this shape makes impossible to write.
|
||||
expand(Context.ACCOUNT_AUTH) to expand(Context.ACCOUNT_IDENTITY)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -0,0 +1,78 @@
|
||||
package io.flamenet.e2e
|
||||
|
||||
import java.util.Arrays
|
||||
|
||||
/**
|
||||
* Zeroing key material once it has been used (ios#39).
|
||||
*
|
||||
* 🔴 **This narrows a window. It does not remove the material, and claiming
|
||||
* otherwise would spend the whole value of the feature on a lie.** What it buys
|
||||
* is that a derived key does not sit in the process for the lifetime of the app
|
||||
* after the one moment it was needed — which matters for a heap dump, a crash
|
||||
* report, or a debugger attached later. It buys nothing against somebody
|
||||
* reading memory *while* the secret is in use.
|
||||
*
|
||||
* 🔴 **The JVM guarantee is weaker than the iOS one, and the difference is not
|
||||
* a detail.** iOS has `memset_s`, which is specified not to be optimised away,
|
||||
* and iOS does not page application memory to disk. Neither holds here:
|
||||
*
|
||||
* - **A compacting garbage collector moves objects.** A `ByteArray` can be
|
||||
* relocated between allocation and wiping, and the bytes at the old address
|
||||
* are not cleared by the move. Zeroing reaches the array you are holding; it
|
||||
* cannot reach a copy the runtime made on its own. There is no way to pin a
|
||||
* Java array, so this is a ceiling on what any implementation here can offer.
|
||||
* - **Android has compressed swap.** zram has been on by default for years, so
|
||||
* a page holding key material can reach a backing store — unlike iOS, where
|
||||
* the platform does not page application memory out at all.
|
||||
* - **There is no `memset_s`.** `Arrays.fill` on an array that escapes the
|
||||
* method is not eliminated in practice, because the JIT cannot prove the
|
||||
* write is dead. "Not in practice" is a weaker statement than the C standard
|
||||
* makes, and it is the honest one.
|
||||
*
|
||||
* ⚠️ **A Kotlin `String` cannot be zeroed**, same as Swift and for the same
|
||||
* reasons: immutable, freely copied, and possibly interned. A passphrase that
|
||||
* arrived as a `String` is beyond reach. The right response is to turn it into
|
||||
* bytes early, wipe those, and not pretend about the original — which is why
|
||||
* `AccountKdf.master` already calls `PBEKeySpec.clearPassword()` rather than
|
||||
* trying to do anything about the `String` it was handed.
|
||||
*
|
||||
* So the claim this type supports, precisely: **a derived key is not left
|
||||
* sitting in the heap after use.** Not "the key is gone".
|
||||
*/
|
||||
object SecureBytes {
|
||||
|
||||
/**
|
||||
* Zero this array in place.
|
||||
*
|
||||
* ⚠️ Reaches THIS array only. Anything that copied it beforehand keeps an
|
||||
* untouched copy, and the runtime may have made copies nobody asked for.
|
||||
*/
|
||||
fun wipe(bytes: ByteArray?) {
|
||||
if (bytes == null || bytes.isEmpty()) return
|
||||
Arrays.fill(bytes, 0)
|
||||
}
|
||||
|
||||
/**
|
||||
* Run [body] with key material and wipe it however [body] leaves.
|
||||
*
|
||||
* ⚠️ Wipes on the way out of a throw as well. A derivation that failed
|
||||
* half-way is exactly the case where the material is most likely to be
|
||||
* forgotten — which is why this is `try/finally` rather than a wipe at the
|
||||
* end of a happy path.
|
||||
*/
|
||||
inline fun <T> withSecret(secret: ByteArray, body: (ByteArray) -> T): T =
|
||||
try {
|
||||
body(secret)
|
||||
} finally {
|
||||
wipe(secret)
|
||||
}
|
||||
|
||||
/**
|
||||
* A copy of [source], for callers that genuinely need one.
|
||||
*
|
||||
* 🔴 The copy is ordinary memory and this object stops being responsible
|
||||
* for it. Every use of this is a decision to leave a second copy behind,
|
||||
* and it is spelled out rather than implicit for that reason.
|
||||
*/
|
||||
fun copyOf(source: ByteArray): ByteArray = source.copyOf()
|
||||
}
|
||||
@@ -1658,7 +1658,36 @@ class Vault(private val storage: Storage, private val key: SecretKey) {
|
||||
* make the next message from that person undecryptable — a "clear chat"
|
||||
* that quietly breaks the chat.
|
||||
*/
|
||||
fun forgetHistory(threadId: String) = storage.delete(historyName(threadId))
|
||||
fun forgetHistory(threadId: String) {
|
||||
storage.delete(historyName(threadId))
|
||||
// ⚠️ The pin goes with the transcript. A banner pointing at a message
|
||||
// that was just cleared is a reference to nothing, and it would survive
|
||||
// "clear this conversation" — which reads as the clear not having
|
||||
// worked.
|
||||
storage.delete(pinnedName(threadId))
|
||||
}
|
||||
|
||||
// -- Pinned message (§6.19) ----------------------------------------------
|
||||
|
||||
private fun pinnedName(threadId: String) = "pinned-$threadId"
|
||||
|
||||
/**
|
||||
* The message pinned in a thread, or null.
|
||||
*
|
||||
* 🔴 **Encrypted at rest like the transcript, not a preference.** Which
|
||||
* message somebody pinned in which conversation is exactly the shape of
|
||||
* metadata the vault exists to keep off a stolen phone — and it is stored
|
||||
* as an ID, never a copy, so it cannot go stale against an edit or outlive
|
||||
* a deletion.
|
||||
*/
|
||||
fun pinnedMessage(threadId: String): String? =
|
||||
readJson(pinnedName(threadId))?.optString("mid")?.takeIf { it.isNotEmpty() }
|
||||
|
||||
/** Pin a message, or clear the pin with null. */
|
||||
fun savePinnedMessage(threadId: String, messageId: String?) {
|
||||
if (messageId.isNullOrEmpty()) storage.delete(pinnedName(threadId))
|
||||
else writeJson(pinnedName(threadId), JSONObject().put("mid", messageId))
|
||||
}
|
||||
|
||||
/**
|
||||
* Delete one line from a thread's history (delete for me).
|
||||
|
||||
@@ -0,0 +1,140 @@
|
||||
package io.flamenet.e2e
|
||||
|
||||
import kotlin.test.Test
|
||||
import kotlin.test.assertContentEquals
|
||||
import kotlin.test.assertEquals
|
||||
import kotlin.test.assertFailsWith
|
||||
import kotlin.test.assertFalse
|
||||
import kotlin.test.assertTrue
|
||||
|
||||
/**
|
||||
* Zeroing key material once it has been used (ios#39, Android half).
|
||||
*
|
||||
* 🔴 **The claim being tested is narrow, and the tests are shaped to keep it
|
||||
* narrow.** What this buys is that a derived key is not left sitting in the
|
||||
* heap after the one moment it was needed. It is not "the key is gone" — a
|
||||
* compacting collector can have moved the array, Android has compressed swap,
|
||||
* and there is no `memset_s` on the JVM. `SecureBytes` says all of that in
|
||||
* prose; these assert only what is actually true.
|
||||
*
|
||||
* 🔴 **The most valuable test here is that the derived keys did not change.**
|
||||
* Wiping an intermediate one line too early derives every account secret from
|
||||
* thirty-two zero bytes — which produces a perfectly consistent, perfectly
|
||||
* wrong identity, silently, on every device. That is a far worse failure than
|
||||
* the leak this feature exists to narrow, so it is checked first.
|
||||
*/
|
||||
class SecureBytesTest {
|
||||
|
||||
private val passphrase = "a-long-enough-passphrase"
|
||||
private val salt = ByteArray(16) { it.toByte() }
|
||||
|
||||
// ---- The failure that would matter most -----------------------------
|
||||
|
||||
/**
|
||||
* 🔴 The pinned vectors already cover `authSecret` and `accountKeySeed`.
|
||||
* This covers the shape wiping could break without moving a vector: that
|
||||
* the two secrets from one stretch still match the two derived separately.
|
||||
* If the wipe landed between the expansions, the second would be derived
|
||||
* from zeroes and this diverges.
|
||||
*/
|
||||
@Test
|
||||
fun `wiping the intermediate did not change what is derived`() {
|
||||
val (auth, account) = AccountKdf.signInSecrets(passphrase, salt)
|
||||
assertContentEquals(AccountKdf.authSecret(passphrase, salt), auth)
|
||||
assertContentEquals(AccountKdf.accountKeySeed(passphrase, salt), account)
|
||||
}
|
||||
|
||||
/**
|
||||
* ⚠️ And neither is zeroes, which is what "derived from a wiped buffer"
|
||||
* would look like. A test that only compared the two paths would pass if
|
||||
* BOTH were wrong in the same way.
|
||||
*/
|
||||
@Test
|
||||
fun `no derived secret is a run of zeroes`() {
|
||||
val (auth, account) = AccountKdf.signInSecrets(passphrase, salt)
|
||||
assertFalse(auth.all { it == 0.toByte() }, "the auth secret is all zeroes")
|
||||
assertFalse(account.all { it == 0.toByte() }, "the account seed is all zeroes")
|
||||
assertFalse(
|
||||
AccountKdf.identityKey(passphrase, salt).all { it == 0.toByte() },
|
||||
"the identity key is all zeroes",
|
||||
)
|
||||
}
|
||||
|
||||
/** Derivation stays deterministic across calls, wiping or not. */
|
||||
@Test
|
||||
fun `derivation is still deterministic`() {
|
||||
assertContentEquals(
|
||||
AccountKdf.identityKey(passphrase, salt),
|
||||
AccountKdf.identityKey(passphrase, salt),
|
||||
)
|
||||
}
|
||||
|
||||
/** Different contexts still separate, so the wipe did not collapse them. */
|
||||
@Test
|
||||
fun `the contexts still produce different secrets`() {
|
||||
val auth = AccountKdf.authSecret(passphrase, salt)
|
||||
val identity = AccountKdf.identityKey(passphrase, salt)
|
||||
val account = AccountKdf.accountKeySeed(passphrase, salt)
|
||||
assertFalse(auth.contentEquals(identity))
|
||||
assertFalse(auth.contentEquals(account))
|
||||
assertFalse(identity.contentEquals(account))
|
||||
}
|
||||
|
||||
// ---- The wiping itself ----------------------------------------------
|
||||
|
||||
@Test
|
||||
fun `wipe zeroes the array it is given`() {
|
||||
val secret = ByteArray(32) { 0x41 }
|
||||
SecureBytes.wipe(secret)
|
||||
assertTrue(secret.all { it == 0.toByte() })
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `wiping nothing is not an error`() {
|
||||
SecureBytes.wipe(null)
|
||||
SecureBytes.wipe(ByteArray(0))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `withSecret wipes on the way out`() {
|
||||
val secret = ByteArray(32) { 0x41 }
|
||||
val seen = SecureBytes.withSecret(secret) { it.copyOf() }
|
||||
assertTrue(seen.all { it == 0x41.toByte() }, "the body saw a wiped buffer")
|
||||
assertTrue(secret.all { it == 0.toByte() }, "the buffer was not wiped")
|
||||
}
|
||||
|
||||
/**
|
||||
* 🔴 **The case the `finally` exists for.** A derivation that failed
|
||||
* half-way is exactly where the material gets forgotten, so the wipe has to
|
||||
* survive a throw — not sit at the end of a happy path.
|
||||
*/
|
||||
@Test
|
||||
fun `withSecret wipes when the body throws`() {
|
||||
val secret = ByteArray(32) { 0x41 }
|
||||
assertFailsWith<IllegalStateException> {
|
||||
SecureBytes.withSecret(secret) { error("derivation failed half-way") }
|
||||
}
|
||||
assertTrue(secret.all { it == 0.toByte() }, "a throw left the material behind")
|
||||
}
|
||||
|
||||
/**
|
||||
* ⚠️ **The honest limit, asserted rather than left to the prose.** A copy
|
||||
* taken before the wipe is untouched by it. This is not a bug to fix — a
|
||||
* `ByteArray` cannot reach copies of itself — and stating it as a passing
|
||||
* test stops anybody reading `wipe` as stronger than it is.
|
||||
*/
|
||||
@Test
|
||||
fun `a copy taken beforehand survives the wipe`() {
|
||||
val secret = ByteArray(32) { 0x41 }
|
||||
val copy = SecureBytes.copyOf(secret)
|
||||
SecureBytes.wipe(secret)
|
||||
assertTrue(secret.all { it == 0.toByte() })
|
||||
assertTrue(copy.all { it == 0x41.toByte() },
|
||||
"the copy was wiped too, which would mean these share a buffer")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `withSecret returns what the body returned`() {
|
||||
assertEquals(7, SecureBytes.withSecret(ByteArray(4)) { 7 })
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user