mirror of
https://github.com/permissionlesstech/bitchat.git
synced 2026-07-27 13:05:19 +00:00
Close the review findings on the rotation spec
**P1 — the binding proof was replayable onto another session.** The §4.5 verifier checklist omitted the check that the proof's noiseStaticPublicKey equals the remote static key the Noise session actually established. The proof is a self-contained signed blob with nothing tying it to the session it arrives on, so a peer M that had seen A's proof could replay it verbatim inside M's own session with B; B would verify A's signature, see a well-formed binding, and on first contact TOFU-pin A's signing key against M's fingerprint. Added as the first item in the checklist, with the attack written out, because "signed" and "bound to this conversation" are different properties and the difference is easy to lose in a bullet list. **announceV2 is 0x2C, not 0x05.** 0x05 only looks free. It has been recycled twice — announce, then bulkTransferResponse, then fragmentStart until #446 — so an old peer could still map it to a fragment header and misparse presence as a partial message. Values above voiceFrame = 0x29 have only ever been allocated forward, and 0x2A/0x2B belong to the courier spray-ack work, leaving 0x2C. Confirmed never used anywhere in this repository's history. **Outbound priority is now stated, not inherited.** announceV2 fell through to `default: .high`. High is the right answer — presence is small, time-bounded to its epoch and useless once stale — but for a type nothing emits yet, a fall-through means the choice gets made without anyone seeing it. **Reverted unexplained pbxproj churn.** Xcode had rewritten resource-phase ordering and dropped a share-extension entitlements membership exception; none of it belongs in this PR. The file now matches main byte for byte. **O7 said "payloads" where the arithmetic is over encoded frames.** The 241-256 / 497-768 / 1009-1792 ranges are what `pad` receives, which is the whole encoded packet, not the payload alone. **Added O9: a seized device recomputes every past peer ID.** K_rot is long-lived, so peerID_e is computable for any epoch by whoever holds it — someone who seizes a phone, or pulls the static key from a backup, can go back over historical radio captures and identify which were this device. Rotation defends against the passive observer, not against later key compromise. A hash ratchet would give forward secrecy for the ID stream at the cost of state that must survive restarts, tolerate clock jumps, and resynchronise after a gap — a real trade rather than an obvious win, so it is written down as a question rather than silently adopted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Generated
+8
-5
@@ -94,6 +94,7 @@
|
||||
isa = PBXFileSystemSynchronizedBuildFileExceptionSet;
|
||||
membershipExceptions = (
|
||||
Info.plist,
|
||||
bitchatShareExtension.entitlements,
|
||||
);
|
||||
target = 57CA17A36A2532A6CFF367BB /* bitchatShareExtension */;
|
||||
};
|
||||
@@ -378,11 +379,6 @@
|
||||
E0A1B2C3D4E5F6012345678D /* relays/online_relays_gps.csv in Resources */,
|
||||
);
|
||||
};
|
||||
7E9B64F63F93443FB7BA12DF /* Resources */ = {
|
||||
isa = PBXResourcesBuildPhase;
|
||||
files = (
|
||||
);
|
||||
};
|
||||
C5E027A42ECCDFD700BD6012 /* Resources */ = {
|
||||
isa = PBXResourcesBuildPhase;
|
||||
files = (
|
||||
@@ -399,6 +395,13 @@
|
||||
E0A1B2C3D4E5F6012345678E /* relays/online_relays_gps.csv in Resources */,
|
||||
);
|
||||
};
|
||||
7E9B64F63F93443FB7BA12DF /* Resources */ = {
|
||||
isa = PBXResourcesBuildPhase;
|
||||
buildActionMask = 2147483647;
|
||||
files = (
|
||||
);
|
||||
runOnlyForDeploymentPostprocessing = 0;
|
||||
};
|
||||
/* End PBXResourcesBuildPhase section */
|
||||
|
||||
/* Begin PBXSourcesBuildPhase section */
|
||||
|
||||
@@ -35,6 +35,13 @@ enum BLEOutboundPacketPolicy {
|
||||
return .fragment(totalFragments: fragmentTotalCount(from: packet.payload))
|
||||
case .fileTransfer:
|
||||
return .fileTransfer
|
||||
case .announceV2:
|
||||
// Stated rather than inherited from `default`. Presence is small,
|
||||
// time-bounded to its epoch, and useless once stale, so it belongs
|
||||
// with the other control traffic at high priority — but that should
|
||||
// be a decision on the record, not a fall-through, since this type
|
||||
// is not emitted yet and nobody would notice the choice being made.
|
||||
return .high
|
||||
default:
|
||||
return .high
|
||||
}
|
||||
|
||||
@@ -9,7 +9,7 @@
|
||||
| Piece | File |
|
||||
|---|---|
|
||||
| Epochs, ID derivation, recognition tags, tag block, binding message | `localPackages/BitFoundation/Sources/BitFoundation/PeerIDRotation.swift` |
|
||||
| `announceV2 = 0x05` wire format | `localPackages/BitFoundation/Sources/BitFoundation/AnnounceV2Packet.swift` |
|
||||
| `announceV2 = 0x2C` wire format | `localPackages/BitFoundation/Sources/BitFoundation/AnnounceV2Packet.swift` |
|
||||
| Executable test vectors | `localPackages/BitFoundation/Tests/BitFoundationTests/PeerIDRotationTests.swift` |
|
||||
| Wire-format tests | `localPackages/BitFoundation/Tests/BitFoundationTests/AnnounceV2PacketTests.swift` |
|
||||
|
||||
@@ -175,12 +175,15 @@ proof = Ed25519-Sign(signingPrivateKey,
|
||||
|
||||
Sent as a new TLV in `AuthenticatedPeerStatePacket`, whose existing structure already carries a version byte, a canonicality-checked capability TLV, and the 32-byte signing key. The receiver verifies:
|
||||
|
||||
- **that the `noiseStaticPublicKey` inside the proof is byte-equal to the remote static key the Noise session actually established** — see below, this one is load-bearing, and
|
||||
- the signature against the signing key in the same packet, **and**
|
||||
- that the signing key matches whatever it has already pinned for this fingerprint, using the existing trust ladder (authenticated key, then TOFU pin), and
|
||||
- that `peerID_e` equals the ID the session was actually conducted under, and
|
||||
- that `epoch` is within the ±1 window.
|
||||
|
||||
This replaces `authenticatedRemoteKey`'s derivation check with an explicit signed statement. It is strictly stronger than today's self-signed announce, because the signing key is checked against a pin rather than taken from the same message.
|
||||
An earlier draft of this list omitted the first check, which left a hole worth spelling out because it is the kind that survives review. The proof is a self-contained signed blob: nothing in the signature ties it to *the session it arrives on*. So a peer M who has observed A's proof — it travels inside a session, but M can be a peer A legitimately talked to — could replay A's proof verbatim inside M's own session with B. Without the static-key check, B verifies A's signature successfully, sees a well-formed binding, and on **first contact** TOFU-pins A's signing key against M's fingerprint. From then on B attributes M's identity to A's key. Comparing the proof's static key against the key the handshake actually produced closes it: M cannot substitute A's key without also being A.
|
||||
|
||||
This replaces `authenticatedRemoteKey`'s derivation check with an explicit signed statement. With the static-key check present it is strictly stronger than today's self-signed announce, because the signing key is checked against a pin rather than taken from the same message. Without it, it is weaker — a reminder that "signed" and "bound to this conversation" are different properties.
|
||||
|
||||
Note the canonical-bytes helper for this already half-exists: `NoiseEncryptionService.buildAnnounceSignature` / `verifyAnnounceSignature` / `canonicalAnnounceBytes`, with context `"bitchat-announce-v1"`, are present but unreferenced in production (only tests call them). They sign `context‖peerID(8)‖noiseKey(32)‖ed25519Key(32)‖nickname‖timestampMs`. The binding above is deliberately a **different context string** and a different field set, so the two can never be confused; the dead code should be deleted or repurposed explicitly rather than silently reused.
|
||||
|
||||
@@ -199,7 +202,9 @@ Note the canonical-bytes helper for this already half-exists: `NoiseEncryptionSe
|
||||
|
||||
`0x01`, `0x02`, `0x03` are **required** by the decoder (`Packets.swift:147`).
|
||||
|
||||
**Proposed v2 announce.** Because the existing decoder hard-requires the three identity TLVs, a v2 announce cannot simply omit them — that is a parse failure, not a graceful degrade. It therefore needs a distinct message type. Assigned values today are `0x01`–`0x04`, `0x10`–`0x11`, and `0x20`–`0x29`, so **`announceV2 = 0x05`** is proposed: free, and adjacent to `announce = 0x01` so the grouping stays readable (see O3).
|
||||
**Proposed v2 announce.** Because the existing decoder hard-requires the three identity TLVs, a v2 announce cannot simply omit them — that is a parse failure, not a graceful degrade. It therefore needs a distinct message type: **`announceV2 = 0x2C`**.
|
||||
|
||||
An earlier draft proposed `0x05` on the grounds that it is unassigned today and sits next to `announce = 0x01`. That was wrong. `0x05` has already been recycled twice — `announce`, then `bulkTransferResponse`, then `fragmentStart` until #446 — so a sufficiently old peer may still map it to a fragment header and misparse presence as a partial message. Values above `voiceFrame = 0x29` have only ever been allocated forward, which is the safe direction; `0x2A`/`0x2B` are spoken for by the courier spray-ack work, leaving `0x2C`. Verified never used anywhere in this repository's history (see O3).
|
||||
|
||||
TLVs, all cleartext but none identifying:
|
||||
|
||||
@@ -323,7 +328,9 @@ Still to be written jointly: a full `announceV2` packet as a hex blob, and the
|
||||
- **O4 — Unsigned v2 announces.** Binding tags to the announced peer ID removes impersonation-as-any-ID, but a recorded announce can still be rebroadcast verbatim within the epoch window, so a peer can be made to look present when absent. Is "presence is a hint; nothing consequential until a handshake whose static key matches the favourite that produced the match" acceptable? The alternatives are an ephemeral per-epoch signing key with a proof-of-continuity, or a freshness nonce echoed by the recipient — both more machinery and more bytes.
|
||||
- **O5 — Rotation while a session is live.** Defer rotation until sessions are idle, or rotate and migrate? Deferring is simpler and safer, but a long-lived session pins the ID for its lifetime, which weakens G1 for exactly the people who talk most.
|
||||
- **O6 — Nickname timing.** Moving the nickname into the session means a stranger's name appears only after a handshake. Is that acceptable UX on both platforms, or does the peer list need a "someone nearby" placeholder state?
|
||||
- **O7 — Padding is a coordinated change, not a local one.** This started as a question about decoder tolerance and turned into something firmer. `BitchatPacket.toBinaryDataForSigning()` encodes with padding enabled, so **the padding bytes are inside the signed material for every signed packet**. Changing the padding algorithm therefore changes the signed byte stream, and signatures stop verifying against any peer that has not made the identical change. Both outstanding padding fixes are affected: extending coverage beyond `noiseEncrypted`/`noiseHandshake`, and closing the gap where a frame needing more than 255 bytes of padding is emitted unpadded (payloads of 241–256, 497–768 and 1009–1792 bytes ship at exact length today). Two things to settle: whether Android's decoder also tolerates trailing bytes the way iOS's does (`guard offset <= buf.count`, plus an unpad retry), and whether padding changes ride this protocol revision or get their own capability-gated one.
|
||||
- **O7 — Padding is a coordinated change, not a local one.** This started as a question about decoder tolerance and turned into something firmer. `BitchatPacket.toBinaryDataForSigning()` encodes with padding enabled, so **the padding bytes are inside the signed material for every signed packet**. Changing the padding algorithm therefore changes the signed byte stream, and signatures stop verifying against any peer that has not made the identical change. Both outstanding padding fixes are affected: extending coverage beyond `noiseEncrypted`/`noiseHandshake`, and closing the gap where a frame needing more than 255 bytes of padding is emitted unpadded (encoded *frames* of 241–256, 497–768 and 1009–1792 bytes ship at exact length today — the arithmetic is over the whole encoded packet that `pad` receives, not the payload alone). Two things to settle: whether Android's decoder also tolerates trailing bytes the way iOS's does (`guard offset <= buf.count`, plus an unpad retry), and whether padding changes ride this protocol revision or get their own capability-gated one.
|
||||
|
||||
- **O9 — A seized device recomputes every past peer ID.** `K_rot` is a long-lived secret, so `peerID_e = HMAC(K_rot, epoch)` is computable for *any* epoch by whoever holds it. Someone who seizes a phone, or extracts the Noise static key from a backup, can therefore take historical radio captures and identify which of them were this device — retroactively defeating the unlinkability for every past epoch. Rotation protects against the passive observer, not against later key compromise. A hash ratchet (`K_{e+1} = HKDF(K_e)`, discarding `K_e`) would give forward secrecy for the ID stream, at the cost of state that must survive restarts, tolerate clock jumps, and resynchronise after a gap — none of which is free, and all of which interacts with the ±1 window. Worth deciding deliberately rather than inheriting.
|
||||
|
||||
## 9. Relationship to other work
|
||||
|
||||
|
||||
@@ -15,14 +15,6 @@ public enum MessageType: UInt8 {
|
||||
case message = 0x02 // Public chat message
|
||||
case leave = 0x03 // "I'm leaving"
|
||||
case courierEnvelope = 0x04 // Store-and-forward envelope carried by a trusted peer
|
||||
/// Identity-free presence for rotating peer IDs. Carries an epoch, a fixed
|
||||
/// block of pairwise recognition tags, and capabilities — no nickname, no
|
||||
/// public keys, no neighbour list. A separate type rather than a version of
|
||||
/// `announce` because that decoder hard-requires the identity TLVs, so
|
||||
/// omitting them is a parse failure rather than a graceful degrade.
|
||||
/// Not emitted or consumed by the shipping mesh yet; see
|
||||
/// `docs/PEER-ID-ROTATION.md`.
|
||||
case announceV2 = 0x05
|
||||
case requestSync = 0x21 // GCS filter-based sync request (local-only)
|
||||
|
||||
// Noise encryption
|
||||
@@ -48,6 +40,23 @@ public enum MessageType: UInt8 {
|
||||
// never gossip-synced). Private bursts ride noiseEncrypted instead.
|
||||
case voiceFrame = 0x29
|
||||
|
||||
/// Identity-free presence for rotating peer IDs. Carries an epoch, a fixed
|
||||
/// block of pairwise recognition tags, and capabilities — no nickname, no
|
||||
/// public keys, no neighbour list. A separate type rather than a version of
|
||||
/// `announce` because that decoder hard-requires the identity TLVs, so
|
||||
/// omitting them is a parse failure rather than a graceful degrade.
|
||||
///
|
||||
/// `0x2C`, not the seemingly-free `0x05`: that value has been recycled
|
||||
/// twice already (`announce`, then `bulkTransferResponse`, then
|
||||
/// `fragmentStart` until #446), and a very old peer that still maps it to a
|
||||
/// fragment header would misparse presence as a partial message. Values
|
||||
/// after `voiceFrame` have only ever been allocated forward. `0x2A`/`0x2B`
|
||||
/// are spoken for by the courier spray-ack work, hence `0x2C`.
|
||||
///
|
||||
/// Not emitted or consumed by the shipping mesh yet; see
|
||||
/// `docs/PEER-ID-ROTATION.md`.
|
||||
case announceV2 = 0x2C
|
||||
|
||||
public var description: String {
|
||||
switch self {
|
||||
case .announce: return "announce"
|
||||
|
||||
@@ -11,10 +11,16 @@ struct AnnounceV2PacketTests {
|
||||
}
|
||||
|
||||
@Test func typeValueIsStable() {
|
||||
// Changing this breaks every deployed decoder. 0x05 was free; 0x01-0x04,
|
||||
// 0x10-0x11 and 0x20-0x29 were already taken.
|
||||
#expect(MessageType.announceV2.rawValue == 0x05)
|
||||
#expect(MessageType(rawValue: 0x05) == .announceV2)
|
||||
// Changing this breaks every deployed decoder.
|
||||
//
|
||||
// Deliberately NOT 0x05, which merely looks free: it has been recycled
|
||||
// twice already (announce, then bulkTransferResponse, then fragmentStart
|
||||
// until #446), so an old peer could still map it to a fragment header
|
||||
// and misparse presence as a partial message. Values above
|
||||
// voiceFrame = 0x29 have only ever been allocated forward; 0x2A/0x2B
|
||||
// belong to the courier spray-ack work.
|
||||
#expect(MessageType.announceV2.rawValue == 0x2C)
|
||||
#expect(MessageType(rawValue: 0x2C) == .announceV2)
|
||||
#expect(MessageType.announceV2.description == "announceV2")
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user