From 3dcdc7be6901ea38831c84ade376c387030933be Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Jan 2026 12:32:56 +0000 Subject: [PATCH 1/3] fix(signal): critical fixes for identity key detection - batch 1 High Priority Fixes based on Copilot/Codex code review: 1. Propagate errorCode to shouldRecreateSession (CRITICAL) - Extract error attribute from retry node - Pass errorCode to shouldRecreateSession in both sendRetryRequest and sendMessagesAgain - Without this fix, MAC error detection was NOT working - Now immediate session recreation works for MAC errors 2. Use .unref() on metrics interval - Prevents blocking process exit in short-lived scripts/tests - Interval no longer keeps event loop alive unnecessarily 3. Reset circuit breaker regardless of state - Previously only reset when isOpen() - Now resets in any state (open, half-open, or closed with accumulated failures) - Ensures clean slate after identity change detection https://claude.ai/code/session_01SWAcNuGZQmEKyBPYkBhVHg --- src/Signal/libsignal.ts | 9 +++++---- src/Socket/messages-recv.ts | 19 +++++++++++++++---- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/Signal/libsignal.ts b/src/Signal/libsignal.ts index 2755086b..0aef6d22 100644 --- a/src/Signal/libsignal.ts +++ b/src/Signal/libsignal.ts @@ -255,9 +255,9 @@ export function makeLibSignalRepository( metrics.signalIdentityKeyCacheSize?.set(identityKeyCache.size) }, 60000) // Every minute - // Cleanup interval on process exit - if (typeof process !== 'undefined') { - process.on('beforeExit', () => clearInterval(cacheMetricsInterval)) + // Allow process to exit even with interval active (prevents blocking in short-lived scripts/tests) + if (typeof cacheMetricsInterval.unref === 'function') { + cacheMetricsInterval.unref() } const storage = signalStorage(auth, lidMapping, identityKeyCache, ev, preKeyCircuitBreaker, logger) @@ -334,7 +334,8 @@ export function makeLibSignalRepository( ) // Reset prekey circuit breaker since we identified the cause - if (preKeyCircuitBreaker?.isOpen()) { + // Reset regardless of state (could be open, half-open, or closed with accumulated failures) + if (preKeyCircuitBreaker) { preKeyCircuitBreaker.reset() logger.debug({ jid }, 'Reset prekey circuit breaker after identity key change detection') } diff --git a/src/Socket/messages-recv.ts b/src/Socket/messages-recv.ts index c0b30181..ccb66a53 100644 --- a/src/Socket/messages-recv.ts +++ b/src/Socket/messages-recv.ts @@ -452,12 +452,18 @@ export const makeMessagesRecvSocket = (config: SocketConfig) => { // Check if we have a session with this JID const sessionId = signalRepository.jidToSignalProtocolAddress(fromJid) const hasSession = await signalRepository.validateSession(fromJid) - const result = messageRetryManager.shouldRecreateSession(fromJid, hasSession.exists) + + // Extract error code from retry node if present (for MAC error detection) + const retryNode = getBinaryNodeChild(node, 'retry') + const errorAttr = retryNode?.attrs?.error as string | undefined + const errorCode = messageRetryManager.parseRetryErrorCode(errorAttr) + + const result = messageRetryManager.shouldRecreateSession(fromJid, hasSession.exists, errorCode) shouldRecreateSession = result.recreate recreateReason = result.reason if (shouldRecreateSession) { - logger.debug({ fromJid, retryCount, reason: recreateReason }, 'recreating session for retry') + logger.debug({ fromJid, retryCount, reason: recreateReason, errorCode }, 'recreating session for retry') // Delete existing session to force recreation await authState.keys.set({ session: { [sessionId]: null } }) forceIncludeKeys = true @@ -1006,12 +1012,17 @@ export const makeMessagesRecvSocket = (config: SocketConfig) => { const sessionId = signalRepository.jidToSignalProtocolAddress(participant) const hasSession = await signalRepository.validateSession(participant) - const result = messageRetryManager.shouldRecreateSession(participant, hasSession.exists) + + // Extract error code from retry node if present (for MAC error detection) + const errorAttr = retryNode?.attrs?.error as string | undefined + const errorCode = messageRetryManager.parseRetryErrorCode(errorAttr) + + const result = messageRetryManager.shouldRecreateSession(participant, hasSession.exists, errorCode) shouldRecreateSession = result.recreate recreateReason = result.reason if (shouldRecreateSession) { - logger.debug({ participant, retryCount, reason: recreateReason }, 'recreating session for outgoing retry') + logger.debug({ participant, retryCount, reason: recreateReason, errorCode }, 'recreating session for outgoing retry') await authState.keys.set({ session: { [sessionId]: null } }) } } catch (error) { From 81cf601250b6a24d7475ab44171fd2b63bcc5cd2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Jan 2026 12:33:33 +0000 Subject: [PATCH 2/3] fix(signal): improved security and robustness - batch 2 Medium Priority Fixes based on Copilot code review: 1. Use full SHA-256 fingerprint (64 characters) - Previously truncated to 32 characters (128 bits) - Now uses full 256-bit hash for cryptographic consistency - Aligns with standard security practices 2. Add bounds checking in protobuf parsing - Added bounds check after offset += length for length-delimited fields - Added bounds check for 64-bit and 32-bit fixed fields - Prevents reading beyond buffer bounds with malformed messages - Defense in depth against malformed/malicious PreKeyWhisperMessages https://claude.ai/code/session_01SWAcNuGZQmEKyBPYkBhVHg --- src/Signal/libsignal.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/Signal/libsignal.ts b/src/Signal/libsignal.ts index 0aef6d22..fb35c713 100644 --- a/src/Signal/libsignal.ts +++ b/src/Signal/libsignal.ts @@ -78,12 +78,13 @@ export interface LibSignalRepositoryOptions { /** * Generate a SHA-256 fingerprint of an identity key * This is used to display to users for verification + * Returns full 64-character hex string (256 bits) for cryptographic consistency * * @param key - The identity key bytes - * @returns Hex string fingerprint + * @returns Full SHA-256 hex string fingerprint (64 characters) */ function generateKeyFingerprint(key: Uint8Array): string { - return createHash('sha256').update(key).digest('hex').substring(0, 32) + return createHash('sha256').update(key).digest('hex') } /** @@ -175,6 +176,11 @@ function extractIdentityFromPkmsg(ciphertext: Uint8Array, logger?: ILogger): Uin } offset += length + // Bounds check after skipping field + if (offset > ciphertext.length) { + logger?.debug({ offset, length: ciphertext.length }, 'Offset exceeds ciphertext bounds after length-delimited field') + break + } } else if (wireType === 0) { // Varint const varintResult = readVarint(ciphertext, offset) @@ -183,9 +189,11 @@ function extractIdentityFromPkmsg(ciphertext: Uint8Array, logger?: ILogger): Uin } else if (wireType === 1) { // 64-bit fixed offset += 8 + if (offset > ciphertext.length) break } else if (wireType === 5) { // 32-bit fixed offset += 4 + if (offset > ciphertext.length) break } else { // Unknown wire type, cannot continue logger?.debug({ wireType }, 'Unknown wire type in protobuf') From 9ef56cc59dab4a79a32d1df991e494e3cf698cd2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 30 Jan 2026 12:34:14 +0000 Subject: [PATCH 3/3] docs(signal): improved documentation and comments - batch 3 Low Priority Fixes based on Copilot code review: 1. Clarified IdentitySaveResult.changed comment - Explicitly documents that false means "new OR unchanged" - Explains to use isNew to distinguish between cases - Documents that previousFingerprint only present when changed 2. Improved fingerprint documentation - Documents that fingerprint is 64 hex characters (full SHA-256) 3. Added comments to varint parsing - Documents that varint too long could indicate malformed/malicious message - Added comment for incomplete varint case https://claude.ai/code/session_01SWAcNuGZQmEKyBPYkBhVHg --- src/Signal/libsignal.ts | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/Signal/libsignal.ts b/src/Signal/libsignal.ts index fb35c713..9b75ecf6 100644 --- a/src/Signal/libsignal.ts +++ b/src/Signal/libsignal.ts @@ -51,13 +51,18 @@ const PREKEY_MSG_VERSION = 3 * Result of identity key save operation */ export interface IdentitySaveResult { - /** Whether the identity key changed (true) or is new/unchanged (false for unchanged) */ + /** + * Whether the identity key changed from a previous known value. + * - true: Key changed (contact reinstalled WhatsApp or switched devices) + * - false: Key is new (first contact) OR unchanged (same key as before) + * Use `isNew` to distinguish between new and unchanged cases. + */ changed: boolean /** Whether this is a new contact (first time seeing their key) */ isNew: boolean - /** Fingerprint of the previous key (if changed) */ + /** Fingerprint of the previous key (only present if changed === true) */ previousFingerprint?: string - /** Fingerprint of the current/new key */ + /** SHA-256 fingerprint of the current/new key (64 hex characters) */ currentFingerprint: string } @@ -234,10 +239,13 @@ function readVarint(buffer: Uint8Array, offset: number): { value: number; nextOf shift += 7 if (shift > 35) { // Varint too long (max 5 bytes for 32-bit) + // This could indicate a malformed or malicious message + // Caller should log this condition at debug level return undefined } } + // Incomplete varint - buffer ended before termination byte return undefined }