From 4bdcd5769d753ae996316a4f92f573615d7494a3 Mon Sep 17 00:00:00 2001 From: Renato Alcara Date: Mon, 27 Apr 2026 18:27:35 -0300 Subject: [PATCH] =?UTF-8?q?fix(signal):=20lock=20on=20wire=20address=20(no?= =?UTF-8?q?t=20raw=20JID)=20=E2=80=94=20addresses=20bot=20review?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bots (Codex P1, Copilot, CodeRabbit) flagged a real defect in the previous commit (79844a9d29): locking on the raw `jid` doesn't actually serialize concurrent ops on the same session, because `signalStorage.loadSession`/ `storeSession` internally call `resolveLIDSignalAddress(id)` which canonicalizes PN→LID before hitting `keys.get/set('session', ...)`. So: - Op A arrives as PN → lock(PN_addr) → storage rewrites to LID_addr - Op B arrives as LID → lock(LID_addr) → storage stays at LID_addr - Two locks, SAME session record → ratchet interleaving → Bad MAC. This was the same drift the original `canonicalJid` was trying to prevent — just expressed at a different layer. Pedro's snapshot doesn't trip it because in Feb/6 there were no PN→LID mappings populated yet (WhatsApp hadn't fully rolled out LID-addressed DSM), so storage canonicalization was a no-op and lock key == storage key by accident. Fix: extract `resolveLIDSignalAddress` to a top-level helper (parameterized by `lidMapping`) and use it in BOTH places — 1. `signalStorage.loadSession`/`storeSession` (unchanged behavior, just no longer a closure). 2. `decryptMessage`/`encryptMessage` to compute the *wire address* and lock on it. Lock key now matches the storage key exactly. This eliminates both directions of the PN/LID drift. PN/LID mapping latency on the hot path is unchanged from the previous commit (one `getLIDForPN` lookup per decrypt — same as before — but now correctly placed). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Signal/libsignal.ts | 101 ++++++++++++++++++++++++---------------- 1 file changed, 62 insertions(+), 39 deletions(-) diff --git a/src/Signal/libsignal.ts b/src/Signal/libsignal.ts index 3420cd00..6c93819f 100644 --- a/src/Signal/libsignal.ts +++ b/src/Signal/libsignal.ts @@ -341,6 +341,14 @@ export function makeLibSignalRepository( }, async decryptMessage({ jid, type, ciphertext }) { const addr = jidToSignalProtocolAddress(jid) + // Wire address = the EXACT key that signalStorage's loadSession/storeSession + // uses internally (it canonicalizes PN→LID via resolveLIDSignalAddress). + // We MUST lock on this same key — locking on the raw `jid` lets two + // concurrent ops for the same logical contact (one arriving as PN, one as + // LID) acquire DIFFERENT mutex slots and interleave on the same session + // record, corrupting the ratchet → perpetual `Bad MAC` on own DSM after + // the WhatsApp PN→LID DSM rollout. + const wireAddr = await resolveLIDSignalAddress(addr.toString(), lidMapping) const session = new libsignal.SessionCipher(storage, addr) async function doDecrypt() { @@ -389,29 +397,23 @@ export function makeLibSignalRepository( return result } - // Lock on the raw JID — same string libsignal uses to derive the - // SessionCipher address (line above). Locking on a *resolved* canonical - // JID would mean the lock key and the session storage key drift apart, - // letting concurrent ops on the same session interleave and corrupt the - // ratchet (observed as a perpetual Bad MAC loop on own DSM after the - // PN→LID rollout). PN/LID race protection lives upstream in - // migrateSession + storeLIDPNMappings, not here. return parsedKeys.transaction(async () => { return await doDecrypt() - }, jid) + }, wireAddr) }, async encryptMessage({ jid, data }) { const addr = jidToSignalProtocolAddress(jid) + // See decryptMessage above for the rationale: lock on the wire address, + // not the raw JID, so concurrent PN/LID ops on the same session serialize. + const wireAddr = await resolveLIDSignalAddress(addr.toString(), lidMapping) const cipher = new libsignal.SessionCipher(storage, addr) - // Lock on raw JID — same string used to derive `addr` above. See comment - // in decryptMessage for the rationale (lock key must match session storage key). return parsedKeys.transaction(async () => { const { type: sigType, body } = await cipher.encrypt(data) const type = sigType === 3 ? 'pkmsg' : 'msg' return { type, ciphertext: Buffer.from(body, 'binary') } - }, jid) + }, wireAddr) }, async encryptGroupMessage({ group, meId, data }) { @@ -719,6 +721,48 @@ const jidToSignalSenderKeyName = (group: string, user: string): SenderKeyName => return new SenderKeyName(group, jidToSignalProtocolAddress(user)) } +/** + * Resolve a Signal protocol address (string form `userDomain.device`) to its + * canonical *wire* address — the exact key that `loadSession`/`storeSession` + * uses when reading/writing `keys.get('session', [...])`. + * + * This is the SAME key the storage layer uses internally, so callers that need + * a transaction lock around session mutations MUST lock on this resolved + * address (not the raw JID), otherwise the lock key and storage key drift + * apart and concurrent ops on the same session interleave → ratchet + * corruption → perpetual `Bad MAC` errors. + * + * Behavior: if the input already targets a LID domain, returns it unchanged. + * Otherwise looks up the PN→LID mapping; if found, returns the LID-form + * Signal address. If no mapping exists, returns the input unchanged. + * + * Top-level (not inside `signalStorage`) so `makeLibSignalRepository` can + * compute the same wire address for transaction locking. + */ +const resolveLIDSignalAddress = async (id: string, lidMapping: LIDMappingStore): Promise => { + if (id.includes('.')) { + const [deviceId, device] = id.split('.') + if (!deviceId) { + throw new Error('Missing device ID') + } + + const [user, domainType_] = deviceId.split('_') + const domainType = parseInt(domainType_ || '0') + + if (domainType === WAJIDDomains.LID || domainType === WAJIDDomains.HOSTED_LID) return id + + const pnJid = `${user!}${device !== '0' ? `:${device}` : ''}@${domainType === WAJIDDomains.HOSTED ? 'hosted' : 's.whatsapp.net'}` + + const lidForPN = await lidMapping.getLIDForPN(pnJid) + if (lidForPN) { + const lidAddr = jidToSignalProtocolAddress(lidForPN) + return lidAddr.toString() + } + } + + return id +} + /** * Extended SignalStorage with identity key management * This type adds identity key operations to the standard Signal storage @@ -750,30 +794,9 @@ function signalStorage( ev?: BaileysEventEmitter, logger?: ILogger ): ExtendedSignalStorage { - // Shared function to resolve PN signal address to LID if mapping exists - const resolveLIDSignalAddress = async (id: string): Promise => { - if (id.includes('.')) { - const [deviceId, device] = id.split('.') - if (!deviceId) { - throw new Error('Missing device ID') - } - - const [user, domainType_] = deviceId.split('_') - const domainType = parseInt(domainType_ || '0') - - if (domainType === WAJIDDomains.LID || domainType === WAJIDDomains.HOSTED_LID) return id - - const pnJid = `${user!}${device !== '0' ? `:${device}` : ''}@${domainType === WAJIDDomains.HOSTED ? 'hosted' : 's.whatsapp.net'}` - - const lidForPN = await lidMapping.getLIDForPN(pnJid) - if (lidForPN) { - const lidAddr = jidToSignalProtocolAddress(lidForPN) - return lidAddr.toString() - } - } - - return id - } + // Bind the top-level `resolveLIDSignalAddress` to this instance's lidMapping + // so call sites below stay readable. + const resolveAddr = (id: string) => resolveLIDSignalAddress(id, lidMapping) // Delayed PreKey deletion: grace period to handle race conditions // where two pkmsg with the same preKeyId arrive nearly simultaneously. @@ -785,7 +808,7 @@ function signalStorage( return { loadSession: async (id: string) => { try { - const wireJid = await resolveLIDSignalAddress(id) + const wireJid = await resolveAddr(id) const { [wireJid]: sess } = await keys.get('session', [wireJid]) if (sess) { @@ -798,7 +821,7 @@ function signalStorage( return null }, storeSession: async (id: string, session: libsignal.SessionRecord) => { - const wireJid = await resolveLIDSignalAddress(id) + const wireJid = await resolveAddr(id) await keys.set({ session: { [wireJid]: session.serialize() } }) }, isTrustedIdentity: () => { @@ -875,7 +898,7 @@ function signalStorage( const timer = metrics.signalIdentityKeyOperations?.startTimer({ operation: 'load' }) try { - const wireJid = await resolveLIDSignalAddress(id) + const wireJid = await resolveAddr(id) // Check cache first const cached = identityKeyCache.get(wireJid) @@ -909,7 +932,7 @@ function signalStorage( const timer = metrics.signalIdentityKeyOperations?.startTimer({ operation: 'save' }) try { - const wireJid = await resolveLIDSignalAddress(id) + const wireJid = await resolveAddr(id) const currentFingerprint = generateKeyFingerprint(identityKey) // Load existing key (from cache or storage)