From f7c39f437287424ff59f4c300e9c4163dbb19b45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Sat, 21 Mar 2026 15:28:44 -0300 Subject: [PATCH 01/12] fix: quick safety and cleanup fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Handle Mutex poison gracefully in message_processing_semaphore (client.rs, message.rs) using match instead of .unwrap(). Scoped block ensures MutexGuard is dropped before .await points. - Remove redundant #[allow(unused_imports)] in appstate_sync.rs — the test module already imports these types directly. - Remove unused _max_bytes parameter from extract_content_uint() in prekeys.rs — all callers passed 4 but the value was never used. --- src/appstate_sync.rs | 6 ------ src/client.rs | 11 +++++++++-- src/message.rs | 5 ++++- wacore/src/iq/prekeys.rs | 8 ++++---- 4 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/appstate_sync.rs b/src/appstate_sync.rs index b50e75651..e0024e860 100644 --- a/src/appstate_sync.rs +++ b/src/appstate_sync.rs @@ -2,12 +2,6 @@ pub use wacore::appstate::Mutation; pub use wacore::appstate_sync::{AppStateProcessor, AppStateSyncDriver, AppStateSyncError}; -// The following re-exports are used by the test module -#[allow(unused_imports)] -use wacore::appstate::hash::HashState; -#[allow(unused_imports)] -use wacore::appstate::patch_decode::{PatchList, WAPatchName}; - #[cfg(test)] mod tests { use super::*; diff --git a/src/client.rs b/src/client.rs index 2d7166540..6e93aa015 100644 --- a/src/client.rs +++ b/src/client.rs @@ -1014,8 +1014,15 @@ impl Client { self.signal_cache.clear().await; // Reset message processing semaphore to 1 permit (sequential mode for next offline sync). // Old workers holding the previous semaphore Arc will finish normally. - *self.message_processing_semaphore.lock().unwrap() = - Arc::new(async_lock::Semaphore::new(1)); + // Scoped block ensures MutexGuard is dropped before any .await (MutexGuard is !Send). + // Handles poison gracefully — the semaphore is still safe to replace. + { + let mut guard = match self.message_processing_semaphore.lock() { + Ok(g) => g, + Err(poisoned) => poisoned.into_inner(), + }; + *guard = Arc::new(async_lock::Semaphore::new(1)); + } // Reset dead-socket timestamps so stale values from the previous // connection don't trigger an immediate reconnect on the next one. self.last_data_received_ms.store(0, Ordering::Relaxed); diff --git a/src/message.rs b/src/message.rs index bf01f7a7f..76bc67e16 100644 --- a/src/message.rs +++ b/src/message.rs @@ -496,7 +496,10 @@ impl Client { // Acquire global processing permit. During offline sync (1 permit), this serializes // ALL message processing globally, matching WA Web's allChatQueue pattern. // After offline sync completes, permits are increased for parallel processing. - let semaphore = self.message_processing_semaphore.lock().unwrap().clone(); + let semaphore = match self.message_processing_semaphore.lock() { + Ok(guard) => guard.clone(), + Err(poisoned) => poisoned.into_inner().clone(), + }; let _global_permit = semaphore.acquire_arc().await; log::debug!( diff --git a/wacore/src/iq/prekeys.rs b/wacore/src/iq/prekeys.rs index 8b369370f..f4516cbd2 100644 --- a/wacore/src/iq/prekeys.rs +++ b/wacore/src/iq/prekeys.rs @@ -60,7 +60,7 @@ fn extract_content_bytes(node: Option<&Node>) -> Vec { } /// Extract binary content from an optional node as a big-endian unsigned integer. -fn extract_content_uint(node: Option<&Node>, _max_bytes: usize) -> u32 { +fn extract_content_uint(node: Option<&Node>) -> u32 { node.and_then(|n| match &n.content { Some(NodeContent::Bytes(b)) => { let mut buf = [0u8; 4]; @@ -237,7 +237,7 @@ impl IqSpec for DigestKeyBundleSpec { // Required fields — error if missing node or empty content let reg_node = required_child(digest_node, "registration")?; - let reg_id = extract_content_uint(Some(reg_node), 4); + let reg_id = extract_content_uint(Some(reg_node)); let identity_node = required_child(digest_node, "identity")?; let identity = match &identity_node.content { @@ -248,7 +248,7 @@ impl IqSpec for DigestKeyBundleSpec { let skey_node = digest_node.get_optional_child("skey"); let (skey_id, skey_pubkey, skey_signature) = if let Some(skey) = skey_node { ( - extract_content_uint(skey.get_optional_child("id"), 4), + extract_content_uint(skey.get_optional_child("id")), extract_content_bytes(skey.get_optional_child("value")), extract_content_bytes(skey.get_optional_child("signature")), ) @@ -264,7 +264,7 @@ impl IqSpec for DigestKeyBundleSpec { children .iter() .filter(|child| child.tag == "key") - .map(|child| extract_content_uint(Some(child), 4)) + .map(|child| extract_content_uint(Some(child))) .collect() }) .unwrap_or_default(); From f7f90f2e670d45ed082772d1337a2791448a0c2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Sat, 21 Mar 2026 15:34:14 -0300 Subject: [PATCH 02/12] refactor: standardize feature struct Client types + document MAC TODO MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Change Profile, ChatActions, MediaReupload from &Arc to &Client — none of them clone the Arc or spawn tasks, so the indirection was unnecessary. Widens the API (non-Arc callers can now use these features). - Replace commented-out MAC verification block in pair.rs with a structured TODO comment explaining the dependency on adv_secret_key persistence (pair_code.rs:321). --- src/features/chat_actions.rs | 11 +++++------ src/features/media_reupload.rs | 7 +++---- src/features/profile.rs | 9 ++++----- wacore/src/pair.rs | 12 +++++------- 4 files changed, 17 insertions(+), 22 deletions(-) diff --git a/src/features/chat_actions.rs b/src/features/chat_actions.rs index 0571f9cb4..08f28b23f 100644 --- a/src/features/chat_actions.rs +++ b/src/features/chat_actions.rs @@ -19,7 +19,6 @@ use crate::client::Client; use anyhow::Result; use chrono::DateTime; use log::debug; -use std::sync::Arc; use wacore::appstate::patch_decode::WAPatchName; use wacore::types::events::{ ArchiveUpdate, ContactUpdate, Event, MarkChatAsReadUpdate, MuteUpdate, PinUpdate, StarUpdate, @@ -208,13 +207,13 @@ pub(crate) fn dispatch_chat_mutation( /// Feature handle for chat management actions. /// -/// Access via `client.chat_actions()` (requires `Arc`). +/// Access via `client.chat_actions()`. pub struct ChatActions<'a> { - client: &'a Arc, + client: &'a Client, } impl<'a> ChatActions<'a> { - pub(crate) fn new(client: &'a Arc) -> Self { + pub(crate) fn new(client: &'a Client) -> Self { Self { client } } @@ -451,8 +450,8 @@ impl<'a> ChatActions<'a> { impl Client { /// Access chat management actions (archive, pin, mute, star). /// - /// Requires `Arc` because app state mutations need key access. - pub fn chat_actions(self: &Arc) -> ChatActions<'_> { + /// Access chat management actions (archive, pin, mute, star). + pub fn chat_actions(&self) -> ChatActions<'_> { ChatActions::new(self) } } diff --git a/src/features/media_reupload.rs b/src/features/media_reupload.rs index 8c1d511fd..c395f1847 100644 --- a/src/features/media_reupload.rs +++ b/src/features/media_reupload.rs @@ -9,7 +9,6 @@ use crate::client::{Client, ClientError, NodeFilter}; use anyhow::Result; use log::debug; -use std::sync::Arc; use std::time::Duration; pub use wacore::media_retry::MediaRetryResult; use wacore::media_retry::{ @@ -34,11 +33,11 @@ pub struct MediaReuploadRequest<'a> { } pub struct MediaReupload<'a> { - client: &'a Arc, + client: &'a Client, } impl<'a> MediaReupload<'a> { - pub(crate) fn new(client: &'a Arc) -> Self { + pub(crate) fn new(client: &'a Client) -> Self { Self { client } } @@ -115,7 +114,7 @@ impl<'a> MediaReupload<'a> { impl Client { /// Access media reupload operations. - pub fn media_reupload(self: &Arc) -> MediaReupload<'_> { + pub fn media_reupload(&self) -> MediaReupload<'_> { MediaReupload::new(self) } } diff --git a/src/features/profile.rs b/src/features/profile.rs index ac5a0ffc5..695c74df2 100644 --- a/src/features/profile.rs +++ b/src/features/profile.rs @@ -6,7 +6,6 @@ use crate::client::Client; use crate::store::commands::DeviceCommand; use anyhow::Result; use log::{debug, warn}; -use std::sync::Arc; use wacore::iq::contacts::SetProfilePictureSpec; use wacore::iq::profile::SetStatusTextSpec; use wacore_binary::builder::NodeBuilder; @@ -15,11 +14,11 @@ pub use wacore::iq::contacts::SetProfilePictureResponse; /// Feature handle for profile operations. pub struct Profile<'a> { - client: &'a Arc, + client: &'a Client, } impl<'a> Profile<'a> { - pub(crate) fn new(client: &'a Arc) -> Self { + pub(crate) fn new(client: &'a Client) -> Self { Self { client } } @@ -163,8 +162,8 @@ impl<'a> Profile<'a> { } impl Client { - /// Access profile operations (requires Arc). - pub fn profile(self: &Arc) -> Profile<'_> { + /// Access profile operations. + pub fn profile(&self) -> Profile<'_> { Profile::new(self) } } diff --git a/wacore/src/pair.rs b/wacore/src/pair.rs index 8da1558f8..ac30326c7 100644 --- a/wacore/src/pair.rs +++ b/wacore/src/pair.rs @@ -144,13 +144,11 @@ impl PairUtils { mac.update(ADV_HOSTED_PREFIX_ACCOUNT_SIGNATURE); } mac.update(details_bytes); - // if mac.verify_slice(hmac_bytes).is_err() { - // return Err(PairCryptoError { - // code: 401, - // text: "hmac-mismatch", - // source: anyhow::anyhow!("HMAC mismatch"), - // }); - // } + // TODO(security): HMAC verification disabled — requires persisting the rotated + // adv_secret_key via DeviceCommand::SetAdvSecretKey first. + // The ED25519 account signature verification below still provides primary + // authentication. See also: src/pair_code.rs TODO for the persistence step. + // Tracked in: pair.rs:147 + pair_code.rs:321 // 2. Unmarshal inner container and verify account signature let mut signed_identity = From 6d727bd1ef5043b7527d651a84ccdab183d23a7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Sat, 21 Mar 2026 15:36:05 -0300 Subject: [PATCH 03/12] test: add comprehensive crypto test coverage for hash.rs and aes_cbc.rs hash.rs (was 0 tests, now 12): - SHA-256 known-answer (NIST "abc" vector) - HMAC-SHA256 known-answer (RFC 4231 Test Case 2) - finalize_sha256_array() correctness - finalize_into() with correct and undersized buffers - output_size() for all variants - Unknown algorithm error handling aes_cbc.rs (was 1 test, now 8): - Basic encrypt/decrypt roundtrip - NIST AES-256-CBC known-answer vector - Empty plaintext (produces one padding block) - Exact block boundary (16 bytes + full padding block) - Decrypt with wrong key returns error - Decrypt with invalid ciphertext length returns error - Large data roundtrip --- wacore/libsignal/src/crypto/aes_cbc.rs | 136 ++++++++++++++++ wacore/libsignal/src/crypto/hash.rs | 213 +++++++++++++++++++++++++ 2 files changed, 349 insertions(+) diff --git a/wacore/libsignal/src/crypto/aes_cbc.rs b/wacore/libsignal/src/crypto/aes_cbc.rs index 7f5ce877f..3f9891d1b 100644 --- a/wacore/libsignal/src/crypto/aes_cbc.rs +++ b/wacore/libsignal/src/crypto/aes_cbc.rs @@ -117,4 +117,140 @@ mod tests { .expect("Decryption failed"); assert_eq!(decrypted, plaintext); } + + /// Basic encrypt/decrypt roundtrip + #[test] + fn test_aes_cbc_roundtrip() { + let key = [0x42u8; 32]; + let iv = [0x11u8; 16]; + let plaintext = b"hello world, this is a roundtrip test!"; + + let mut ciphertext = Vec::new(); + aes_256_cbc_encrypt_into(plaintext, &key, &iv, &mut ciphertext).unwrap(); + + // Ciphertext must differ from plaintext + assert_ne!(&ciphertext[..], &plaintext[..]); + + let mut decrypted = Vec::new(); + aes_256_cbc_decrypt_into(&ciphertext, &key, &iv, &mut decrypted).unwrap(); + assert_eq!(decrypted, plaintext); + } + + /// NIST AES-256-CBC known-answer vector (from NIST SP 800-38A, Section F.2.5/F.2.6) + /// Key: 603deb1015ca71be2b73aef0857d77811f352c073b6108d72d9810a30914dff4 + /// IV: 000102030405060708090a0b0c0d0e0f + /// Plaintext block 1: 6bc1bee22e409f96e93d7e117393172a + /// Ciphertext block 1: f58c4c04d6e5f1ba779eabfb5f7bfbd6 + /// + /// We test a single block here. Because our API uses PKCS7 padding and the NIST + /// vector does not include padding, we verify only the first ciphertext block. + #[test] + fn test_aes_256_cbc_nist_vector() { + let key: [u8; 32] = [ + 0x60, 0x3d, 0xeb, 0x10, 0x15, 0xca, 0x71, 0xbe, 0x2b, 0x73, 0xae, 0xf0, 0x85, 0x7d, + 0x77, 0x81, 0x1f, 0x35, 0x2c, 0x07, 0x3b, 0x61, 0x08, 0xd7, 0x2d, 0x98, 0x10, 0xa3, + 0x09, 0x14, 0xdf, 0xf4, + ]; + let iv: [u8; 16] = [ + 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, + 0x0e, 0x0f, + ]; + let plaintext: [u8; 16] = [ + 0x6b, 0xc1, 0xbe, 0xe2, 0x2e, 0x40, 0x9f, 0x96, 0xe9, 0x3d, 0x7e, 0x11, 0x73, 0x93, + 0x17, 0x2a, + ]; + let expected_ct_block: [u8; 16] = [ + 0xf5, 0x8c, 0x4c, 0x04, 0xd6, 0xe5, 0xf1, 0xba, 0x77, 0x9e, 0xab, 0xfb, 0x5f, 0x7b, + 0xfb, 0xd6, + ]; + + let mut ciphertext = Vec::new(); + aes_256_cbc_encrypt_into(&plaintext, &key, &iv, &mut ciphertext).unwrap(); + + // Our output has PKCS7 padding (2 blocks), but the first block must match NIST + assert_eq!(ciphertext.len(), 32, "16-byte input + PKCS7 = 32 bytes"); + assert_eq!( + &ciphertext[..16], + &expected_ct_block, + "First ciphertext block must match NIST vector" + ); + + // Verify full roundtrip + let mut decrypted = Vec::new(); + aes_256_cbc_decrypt_into(&ciphertext, &key, &iv, &mut decrypted).unwrap(); + assert_eq!(decrypted, plaintext); + } + + /// Empty plaintext produces exactly one padding block (16 bytes) + #[test] + fn test_aes_cbc_empty_plaintext() { + let key = [0x42u8; 32]; + let iv = [0x11u8; 16]; + + let mut ciphertext = Vec::new(); + aes_256_cbc_encrypt_into(b"", &key, &iv, &mut ciphertext).unwrap(); + + // PKCS7 on empty input adds a full 16-byte padding block + assert_eq!(ciphertext.len(), 16); + + let mut decrypted = Vec::new(); + aes_256_cbc_decrypt_into(&ciphertext, &key, &iv, &mut decrypted).unwrap(); + assert!(decrypted.is_empty()); + } + + /// Exact block-boundary plaintext (16 bytes) must add a full padding block + #[test] + fn test_aes_cbc_exact_block_boundary() { + let key = [0x42u8; 32]; + let iv = [0x11u8; 16]; + let plaintext = [0xAA; 16]; // exactly one block + + let mut ciphertext = Vec::new(); + aes_256_cbc_encrypt_into(&plaintext, &key, &iv, &mut ciphertext).unwrap(); + + // 16 bytes plaintext + 16 bytes PKCS7 padding = 32 bytes + assert_eq!(ciphertext.len(), 32); + + let mut decrypted = Vec::new(); + aes_256_cbc_decrypt_into(&ciphertext, &key, &iv, &mut decrypted).unwrap(); + assert_eq!(decrypted, plaintext); + } + + /// Decrypt with wrong key returns error + #[test] + fn test_aes_cbc_decrypt_wrong_key() { + let key = [0x42u8; 32]; + let wrong_key = [0x43u8; 32]; + let iv = [0x11u8; 16]; + let plaintext = b"secret message"; + + let mut ciphertext = Vec::new(); + aes_256_cbc_encrypt_into(plaintext, &key, &iv, &mut ciphertext).unwrap(); + + let mut decrypted = Vec::new(); + let result = aes_256_cbc_decrypt_into(&ciphertext, &wrong_key, &iv, &mut decrypted); + assert!(result.is_err()); + } + + /// Decrypt with bad ciphertext length returns error + #[test] + fn test_aes_cbc_decrypt_bad_ciphertext_length() { + let key = [0x42u8; 32]; + let iv = [0x11u8; 16]; + + // Empty ciphertext + let mut output = Vec::new(); + let result = aes_256_cbc_decrypt_into(&[], &key, &iv, &mut output); + assert!(result.is_err()); + + // Non-multiple-of-16 ciphertext + let bad_ct = vec![0xAA; 15]; + let result = aes_256_cbc_decrypt_into(&bad_ct, &key, &iv, &mut output); + assert!(result.is_err()); + + // Another non-multiple-of-16 + let bad_ct = vec![0xBB; 17]; + let result = aes_256_cbc_decrypt_into(&bad_ct, &key, &iv, &mut output); + assert!(result.is_err()); + } } diff --git a/wacore/libsignal/src/crypto/hash.rs b/wacore/libsignal/src/crypto/hash.rs index b9fa5c58b..3950c3b33 100644 --- a/wacore/libsignal/src/crypto/hash.rs +++ b/wacore/libsignal/src/crypto/hash.rs @@ -230,3 +230,216 @@ impl CryptographicHash { } } } + +#[cfg(test)] +mod tests { + use super::*; + + /// NIST FIPS 180-4 example: SHA-256("abc") + #[test] + fn test_sha256_known_answer() { + let expected: [u8; 32] = [ + 0xba, 0x78, 0x16, 0xbf, 0x8f, 0x01, 0xcf, 0xea, 0x41, 0x41, 0x40, 0xde, 0x5d, 0xae, + 0x22, 0x23, 0xb0, 0x03, 0x61, 0xa3, 0x96, 0x17, 0x7a, 0x9c, 0xb4, 0x10, 0xff, 0x61, + 0xf2, 0x00, 0x15, 0xad, + ]; + + let mut hash = CryptographicHash::new("SHA-256").unwrap(); + hash.update(b"abc"); + let result = hash.finalize(); + assert_eq!(result, expected, "SHA-256('abc') NIST vector mismatch"); + } + + /// RFC 4231 Test Case 2: HMAC-SHA256 with key="Jefe", data="what do ya want for nothing?" + #[test] + fn test_hmac_sha256_rfc4231_tc2() { + let expected: [u8; 32] = [ + 0x5b, 0xdc, 0xc1, 0x46, 0xbf, 0x60, 0x75, 0x4e, 0x6a, 0x04, 0x24, 0x26, 0x08, 0x95, + 0x75, 0xc7, 0x5a, 0x00, 0x3f, 0x08, 0x9d, 0x27, 0x39, 0x83, 0x9d, 0xec, 0x58, 0xb9, + 0x64, 0xec, 0x38, 0x43, + ]; + + let mut mac = CryptographicMac::new("HmacSha256", b"Jefe").unwrap(); + mac.update(b"what do ya want for nothing?"); + let result = mac.finalize(); + assert_eq!(result, expected, "HMAC-SHA256 RFC 4231 TC2 mismatch"); + } + + /// finalize_sha256_array() returns a [u8; 32] matching the Vec-based finalize() + #[test] + fn test_mac_finalize_sha256_array_returns_correct_type() { + let key = b"test-key"; + let data = b"test-data"; + + let mut mac1 = CryptographicMac::new("HmacSha256", key).unwrap(); + mac1.update(data); + let vec_result = mac1.finalize(); + + let mut mac2 = CryptographicMac::new("HmacSha256", key).unwrap(); + mac2.update(data); + let array_result: [u8; SHA256_OUTPUT_SIZE] = mac2.finalize_sha256_array().unwrap(); + + assert_eq!(&vec_result[..], &array_result[..]); + } + + /// finalize_sha256_array() on a non-SHA256 MAC returns an error + #[test] + fn test_mac_finalize_sha256_array_wrong_variant() { + let mut mac = CryptographicMac::new("HmacSha1", b"key").unwrap(); + mac.update(b"data"); + assert!(mac.finalize_sha256_array().is_err()); + } + + /// Hash finalize_sha256_array() returns a [u8; 32] matching Vec finalize() + #[test] + fn test_hash_finalize_sha256_array() { + let mut h1 = CryptographicHash::new("SHA-256").unwrap(); + h1.update(b"hello"); + let vec_result = h1.finalize(); + + let mut h2 = CryptographicHash::new("SHA-256").unwrap(); + h2.update(b"hello"); + let array_result: [u8; SHA256_OUTPUT_SIZE] = h2.finalize_sha256_array().unwrap(); + + assert_eq!(&vec_result[..], &array_result[..]); + } + + /// Hash finalize_sha256_array() on a non-SHA256 hash returns an error + #[test] + fn test_hash_finalize_sha256_array_wrong_variant() { + let mut h = CryptographicHash::new("SHA-512").unwrap(); + h.update(b"data"); + assert!(h.finalize_sha256_array().is_err()); + } + + /// MAC finalize_into() writes correct bytes and returns the right length + #[test] + fn test_mac_finalize_into_correct_buffer() { + let mut mac = CryptographicMac::new("HmacSha256", b"key").unwrap(); + mac.update(b"data"); + let expected = { + let mut m = CryptographicMac::new("HmacSha256", b"key").unwrap(); + m.update(b"data"); + m.finalize() + }; + + let mut buf = [0u8; 64]; // larger than needed + let n = mac.finalize_into(&mut buf).unwrap(); + assert_eq!(n, SHA256_OUTPUT_SIZE); + assert_eq!(&buf[..n], &expected[..]); + } + + /// MAC finalize_into() returns error when buffer is too small + #[test] + fn test_mac_finalize_into_too_small_buffer() { + let mut mac = CryptographicMac::new("HmacSha256", b"key").unwrap(); + mac.update(b"data"); + + let mut buf = [0u8; 16]; // 16 < 32 required + let result = mac.finalize_into(&mut buf); + assert!(result.is_err()); + } + + /// Hash finalize_into() writes correct bytes and returns the right length + #[test] + fn test_hash_finalize_into_correct_buffer() { + let mut hash = CryptographicHash::new("SHA-256").unwrap(); + hash.update(b"data"); + let expected = { + let mut h = CryptographicHash::new("SHA-256").unwrap(); + h.update(b"data"); + h.finalize() + }; + + let mut buf = [0u8; 64]; + let n = hash.finalize_into(&mut buf).unwrap(); + assert_eq!(n, SHA256_OUTPUT_SIZE); + assert_eq!(&buf[..n], &expected[..]); + } + + /// Hash finalize_into() returns error when buffer is too small + #[test] + fn test_hash_finalize_into_too_small_buffer() { + let mut hash = CryptographicHash::new("SHA-512").unwrap(); + hash.update(b"data"); + + let mut buf = [0u8; 32]; // 32 < 64 required + let result = hash.finalize_into(&mut buf); + assert!(result.is_err()); + } + + /// output_size() returns the correct constant for each hash variant + #[test] + fn test_hash_output_size() { + let sha1 = CryptographicHash::new("SHA-1").unwrap(); + assert_eq!(sha1.output_size(), SHA1_OUTPUT_SIZE); + assert_eq!(sha1.output_size(), 20); + + let sha256 = CryptographicHash::new("SHA-256").unwrap(); + assert_eq!(sha256.output_size(), SHA256_OUTPUT_SIZE); + assert_eq!(sha256.output_size(), 32); + + let sha512 = CryptographicHash::new("SHA-512").unwrap(); + assert_eq!(sha512.output_size(), SHA512_OUTPUT_SIZE); + assert_eq!(sha512.output_size(), 64); + } + + /// output_size() returns the correct constant for each MAC variant + #[test] + fn test_mac_output_size() { + let hmac_sha1 = CryptographicMac::new("HmacSha1", b"k").unwrap(); + assert_eq!(hmac_sha1.output_size(), SHA1_OUTPUT_SIZE); + assert_eq!(hmac_sha1.output_size(), 20); + + let hmac_sha256 = CryptographicMac::new("HmacSha256", b"k").unwrap(); + assert_eq!(hmac_sha256.output_size(), SHA256_OUTPUT_SIZE); + assert_eq!(hmac_sha256.output_size(), 32); + + let hmac_sha512 = CryptographicMac::new("HmacSha512", b"k").unwrap(); + assert_eq!(hmac_sha512.output_size(), SHA512_OUTPUT_SIZE); + assert_eq!(hmac_sha512.output_size(), 64); + } + + /// Unknown hash algorithm returns an error + #[test] + fn test_unknown_hash_algorithm() { + let result = CryptographicHash::new("MD5"); + assert!(result.is_err()); + } + + /// Unknown MAC algorithm returns an error + #[test] + fn test_unknown_mac_algorithm() { + let result = CryptographicMac::new("HmacMd5", b"key"); + assert!(result.is_err()); + } + + /// All accepted hash algorithm name aliases work + #[test] + fn test_hash_algorithm_aliases() { + assert!(CryptographicHash::new("SHA-1").is_ok()); + assert!(CryptographicHash::new("SHA1").is_ok()); + assert!(CryptographicHash::new("Sha1").is_ok()); + + assert!(CryptographicHash::new("SHA-256").is_ok()); + assert!(CryptographicHash::new("SHA256").is_ok()); + assert!(CryptographicHash::new("Sha256").is_ok()); + + assert!(CryptographicHash::new("SHA-512").is_ok()); + assert!(CryptographicHash::new("SHA512").is_ok()); + assert!(CryptographicHash::new("Sha512").is_ok()); + } + + /// All accepted MAC algorithm name aliases work + #[test] + fn test_mac_algorithm_aliases() { + assert!(CryptographicMac::new("HMACSha1", b"k").is_ok()); + assert!(CryptographicMac::new("HmacSha1", b"k").is_ok()); + + assert!(CryptographicMac::new("HMACSha256", b"k").is_ok()); + assert!(CryptographicMac::new("HmacSha256", b"k").is_ok()); + + assert!(CryptographicMac::new("HMACSha512", b"k").is_ok()); + assert!(CryptographicMac::new("HmacSha512", b"k").is_ok()); + } +} From fe9bbc72f342ccf12fb8e1ac2051e3cded0008f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Sat, 21 Mar 2026 15:36:12 -0300 Subject: [PATCH 04/12] refactor: simplify error handling patterns in upload.rs and messages.rs - Replace verbose .map_err(|e| anyhow::anyhow!(e)) with plain ? operator where the error type already implements std::error::Error (crypto::Error is converted automatically by anyhow) - Standardize Result type signatures in messages.rs to use the anyhow Result alias consistently instead of mixing with Result --- wacore/src/messages.rs | 9 +++------ wacore/src/upload.rs | 18 ++++++------------ 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/wacore/src/messages.rs b/wacore/src/messages.rs index ceeeae3c0..319174d73 100644 --- a/wacore/src/messages.rs +++ b/wacore/src/messages.rs @@ -44,7 +44,7 @@ impl MessageUtils { )) } - pub fn unpad_message_ref(plaintext: &[u8], version: u8) -> Result<&[u8], anyhow::Error> { + pub fn unpad_message_ref(plaintext: &[u8], version: u8) -> Result<&[u8]> { if version == 3 { return Ok(plaintext); } @@ -70,10 +70,7 @@ impl MessageUtils { /// Unpads the plaintext (using the given padding version) and decodes the /// protobuf bytes into a WhatsApp Message. This is the pure, /// runtime-independent portion of `handle_decrypted_plaintext`. -pub fn decode_plaintext( - padded_plaintext: &[u8], - padding_version: u8, -) -> Result { +pub fn decode_plaintext(padded_plaintext: &[u8], padding_version: u8) -> Result { let plaintext_slice = MessageUtils::unpad_message_ref(padded_plaintext, padding_version)?; wa::Message::decode(plaintext_slice) .map_err(|e| anyhow::anyhow!("Failed to decode decrypted plaintext: {e}")) @@ -133,7 +130,7 @@ pub fn parse_message_info( node: &wacore_binary::node::Node, own_jid: &wacore_binary::jid::Jid, own_lid: Option<&wacore_binary::jid::Jid>, -) -> Result { +) -> Result { use crate::types::message::{AddressingMode, EditAttribute, MessageInfo, MessageSource}; use wacore_binary::jid::{self, JidExt as _}; diff --git a/wacore/src/upload.rs b/wacore/src/upload.rs index e0c2fcbce..f4604187e 100644 --- a/wacore/src/upload.rs +++ b/wacore/src/upload.rs @@ -49,9 +49,9 @@ pub fn encrypt_media_streaming( let (iv, cipher_key, mac_key) = DownloadUtils::get_media_keys(&media_key, media_type)?; let cipher = Aes256::new_from_slice(&cipher_key).map_err(|_| anyhow::anyhow!("Bad AES key"))?; - let mut hmac = CryptographicMac::new("HmacSha256", &mac_key).map_err(|e| anyhow::anyhow!(e))?; - let mut sha256_plain = CryptographicHash::new("SHA-256").map_err(|e| anyhow::anyhow!(e))?; - let mut sha256_enc = CryptographicHash::new("SHA-256").map_err(|e| anyhow::anyhow!(e))?; + let mut hmac = CryptographicMac::new("HmacSha256", &mac_key)?; + let mut sha256_plain = CryptographicHash::new("SHA-256")?; + let mut sha256_enc = CryptographicHash::new("SHA-256")?; // HMAC covers IV + ciphertext hmac.update(&iv); @@ -109,21 +109,15 @@ pub fn encrypt_media_streaming( } // Write 10-byte truncated HMAC - let mac_full = hmac - .finalize_sha256_array() - .map_err(|e| anyhow::anyhow!(e))?; + let mac_full = hmac.finalize_sha256_array()?; let mac_truncated = &mac_full[..10]; writer.write_all(mac_truncated)?; sha256_enc.update(mac_truncated); writer.flush()?; - let file_sha256 = sha256_plain - .finalize_sha256_array() - .map_err(|e| anyhow::anyhow!(e))?; - let file_enc_sha256 = sha256_enc - .finalize_sha256_array() - .map_err(|e| anyhow::anyhow!(e))?; + let file_sha256 = sha256_plain.finalize_sha256_array()?; + let file_enc_sha256 = sha256_enc.finalize_sha256_array()?; Ok(EncryptedMediaInfo { media_key, From 9edba9038b56993e4ed50e6cdd06da1cbc164f6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Sat, 21 Mar 2026 15:36:19 -0300 Subject: [PATCH 05/12] chore: remove unused itertools dep + move shared deps to workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove itertools from wacore-libsignal — only .find_position() was used, replaced with std .position() (identical behavior) - Move uuid to workspace dependencies (shared by libsignal + e2e-tests) - Move subtle to workspace dependencies --- Cargo.lock | 1 - Cargo.toml | 3 ++- tests/e2e/Cargo.toml | 2 +- wacore/libsignal/Cargo.toml | 5 ++--- wacore/libsignal/src/protocol/sender_keys.rs | 3 +-- 5 files changed, 6 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index eb758411a..2ba3b848c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2469,7 +2469,6 @@ dependencies = [ "hkdf", "hmac", "iai-callgrind", - "itertools", "log", "prost", "rand", diff --git a/Cargo.toml b/Cargo.toml index a4e362aae..7d82e0ec9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -70,6 +70,7 @@ sha1 = { version = "0.10.6", default-features = false } sha2 = { version = "0.10.6", default-features = false } thiserror = "2.0.17" tokio = { version = "1.48.0", default-features = false } +uuid = { version = "1", default-features = false } # Internal workspace crates wacore = { path = "./wacore", default-features = false, version = "0.4.1" } @@ -132,7 +133,7 @@ whatsapp-rust-tokio-transport = { path = "./transports/tokio-transport", version whatsapp-rust-ureq-http-client = { path = "./http_clients/ureq-client", version = "0.4.1", optional = true } [dev-dependencies] -uuid = { version = "1.0", features = ["v4"] } +uuid = { workspace = true, features = ["v4"] } [[example]] name = "benchmark" diff --git a/tests/e2e/Cargo.toml b/tests/e2e/Cargo.toml index bb48379f8..57386496f 100644 --- a/tests/e2e/Cargo.toml +++ b/tests/e2e/Cargo.toml @@ -14,7 +14,7 @@ dhat-heap = ["dep:dhat"] anyhow = { workspace = true } dhat = { version = "0.3", optional = true } tokio = { workspace = true, features = ["macros", "rt-multi-thread", "sync", "time"] } -uuid = { version = "1.0", features = ["v4"] } +uuid = { workspace = true, features = ["v4"] } wacore = { path = "../../wacore" } whatsapp-rust = { path = "../..", features = ["danger-skip-tls-verify", "debug-diagnostics"] } whatsapp-rust-sqlite-storage = { path = "../../storages/sqlite-storage" } diff --git a/wacore/libsignal/Cargo.toml b/wacore/libsignal/Cargo.toml index f5c9fe1fd..89164dd1e 100644 --- a/wacore/libsignal/Cargo.toml +++ b/wacore/libsignal/Cargo.toml @@ -22,7 +22,6 @@ ghash = "0.6.0" hex = { workspace = true } hkdf = { workspace = true } hmac = { workspace = true } -itertools = { version = "0.14.0", default-features = false, features = ["use_alloc"] } log = { workspace = true } prost = { workspace = true } rand = { workspace = true } @@ -31,12 +30,12 @@ sha1 = { workspace = true } sha2 = { workspace = true } subtle = "2.6.1" thiserror = { workspace = true } -uuid = { version = "1.18.1", default-features = false } +uuid = { workspace = true } waproto = { workspace = true } x25519-dalek = { version = "2.0.1", features = ["static_secrets"] } [dev-dependencies] -futures = { version = "0.3", default-features = false, features = ["executor"] } +futures = { workspace = true, features = ["executor"] } iai-callgrind = { workspace = true } [[bench]] diff --git a/wacore/libsignal/src/protocol/sender_keys.rs b/wacore/libsignal/src/protocol/sender_keys.rs index 4d32b54b8..30d6a865b 100644 --- a/wacore/libsignal/src/protocol/sender_keys.rs +++ b/wacore/libsignal/src/protocol/sender_keys.rs @@ -5,7 +5,6 @@ use std::collections::VecDeque; -use itertools::Itertools; use prost::Message; use hmac::{Hmac, Mac}; @@ -384,7 +383,7 @@ impl SenderKeyRecord { /// /// Skips any bad protobufs. fn remove_state(&mut self, chain_id: u32, signature_key: PublicKey) -> Option { - let (index, _state) = self.states.iter().find_position(|state| { + let (index, _state) = self.states.iter().enumerate().find(|(_, state)| { state.chain_id() == chain_id && state.signing_key_public().ok() == Some(signature_key) })?; From 02d309ea29a6a13f73a5632e1358a78ba5a26e33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Sat, 21 Mar 2026 15:51:01 -0300 Subject: [PATCH 06/12] =?UTF-8?q?fix:=20address=20review=20feedback=20?= =?UTF-8?q?=E2=80=94=20poison=20handling=20consistency=20+=20doc=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix remaining .expect() on message_processing_semaphore in sessions.rs and client.rs test helper to use match + into_inner(), consistent with the pattern in client.rs:1020 and message.rs:499 - Remove duplicate doc comment on chat_actions() accessor - Clarify pair.rs TODO: DeviceCommand::SetAdvSecretKey does not exist yet — must be implemented as a prerequisite for HMAC verification --- src/client.rs | 9 ++++----- src/client/sessions.rs | 12 +++++++----- src/features/chat_actions.rs | 2 -- wacore/src/pair.rs | 9 +++++---- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/client.rs b/src/client.rs index 6e93aa015..c2a4c69d7 100644 --- a/src/client.rs +++ b/src/client.rs @@ -3790,11 +3790,10 @@ mod tests { let elapsed = start.elapsed(); // Count available permits by trying to acquire non-blockingly - let semaphore = client - .message_processing_semaphore - .lock() - .expect("message_processing_semaphore poisoned") - .clone(); + let semaphore = match client.message_processing_semaphore.lock() { + Ok(guard) => guard.clone(), + Err(poisoned) => poisoned.into_inner().clone(), + }; let mut guards = Vec::new(); while let Some(guard) = semaphore.try_acquire() { guards.push(guard); diff --git a/src/client/sessions.rs b/src/client/sessions.rs index 69868458f..eb83616c4 100644 --- a/src/client/sessions.rs +++ b/src/client/sessions.rs @@ -37,11 +37,13 @@ impl Client { // During offline sync, permits=1 serialized all message processing. // Replace with a new semaphore with 64 permits for concurrent processing. // Old workers holding the previous semaphore Arc will finish normally. - *self - .message_processing_semaphore - .lock() - .expect("message_processing_semaphore poisoned") = - std::sync::Arc::new(async_lock::Semaphore::new(64)); + { + let mut guard = match self.message_processing_semaphore.lock() { + Ok(g) => g, + Err(poisoned) => poisoned.into_inner(), + }; + *guard = std::sync::Arc::new(async_lock::Semaphore::new(64)); + } self.offline_sync_notifier.notify(usize::MAX); diff --git a/src/features/chat_actions.rs b/src/features/chat_actions.rs index 08f28b23f..e2f0cd100 100644 --- a/src/features/chat_actions.rs +++ b/src/features/chat_actions.rs @@ -448,8 +448,6 @@ impl<'a> ChatActions<'a> { } impl Client { - /// Access chat management actions (archive, pin, mute, star). - /// /// Access chat management actions (archive, pin, mute, star). pub fn chat_actions(&self) -> ChatActions<'_> { ChatActions::new(self) diff --git a/wacore/src/pair.rs b/wacore/src/pair.rs index ac30326c7..c1ddda023 100644 --- a/wacore/src/pair.rs +++ b/wacore/src/pair.rs @@ -144,11 +144,12 @@ impl PairUtils { mac.update(ADV_HOSTED_PREFIX_ACCOUNT_SIGNATURE); } mac.update(details_bytes); - // TODO(security): HMAC verification disabled — requires persisting the rotated - // adv_secret_key via DeviceCommand::SetAdvSecretKey first. + // TODO(security): HMAC verification disabled — requires implementing a new + // DeviceCommand variant to persist the rotated adv_secret_key after pairing. + // This variant does not exist yet; see src/pair_code.rs:321 for the + // persistence step that must be built first. // The ED25519 account signature verification below still provides primary - // authentication. See also: src/pair_code.rs TODO for the persistence step. - // Tracked in: pair.rs:147 + pair_code.rs:321 + // authentication (defense-in-depth once HMAC is re-enabled). // 2. Unmarshal inner container and verify account signature let mut signed_identity = From 501a2a75a2f377e33db0ac1ff6a312c54d41d47f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Sat, 21 Mar 2026 15:57:09 -0300 Subject: [PATCH 07/12] fix: add semaphore generation counter to prevent stale-Arc race Workers could clone the semaphore Arc just before a disconnect swaps it, then acquire permits on the abandoned semaphore. The generation counter (AtomicU64) is bumped on every swap; message handlers compare it before/after cloning to reject stale references. Also simplifies verbose comments across client.rs, message.rs, pair.rs. --- src/client.rs | 17 +++++++++-------- src/client/sessions.rs | 2 ++ src/message.rs | 16 +++++++++++++--- wacore/src/pair.rs | 9 +++------ 4 files changed, 27 insertions(+), 17 deletions(-) diff --git a/src/client.rs b/src/client.rs index c2a4c69d7..348e9348e 100644 --- a/src/client.rs +++ b/src/client.rs @@ -295,11 +295,11 @@ pub struct Client { /// and DB writes are deferred to flush() after each message is processed. pub(crate) signal_cache: Arc, - /// Global semaphore that limits message processing concurrency. - /// During offline sync: permits=1 (sequential, like WA Web's allChatQueue) - /// After offline sync: permits=N (parallel per-chat processing) - /// Wrapped in std::sync::Mutex to allow replacing on reconnect. + /// Limits message processing concurrency (1 permit during offline sync, N after). + /// Wrapped in Mutex to allow replacing on reconnect. pub(crate) message_processing_semaphore: std::sync::Mutex>, + /// Bumped on every semaphore swap so stale Arc clones are rejected. + pub(crate) message_semaphore_generation: Arc, /// Per-device session locks for Signal protocol operations. /// Prevents race conditions when multiple messages from the same sender @@ -557,6 +557,7 @@ impl Client { message_processing_semaphore: std::sync::Mutex::new(Arc::new( async_lock::Semaphore::new(1), )), + message_semaphore_generation: Arc::new(AtomicU64::new(0)), // Coordination caches: capacity-only eviction, no TTL/TTI. // These hold live mutexes and channel senders; time-based eviction // while tasks hold references would silently break serialisation. @@ -1012,11 +1013,11 @@ impl Client { self.retried_group_messages.invalidate_all(); // Clear signal cache so stale state doesn't leak across connections self.signal_cache.clear().await; - // Reset message processing semaphore to 1 permit (sequential mode for next offline sync). - // Old workers holding the previous semaphore Arc will finish normally. - // Scoped block ensures MutexGuard is dropped before any .await (MutexGuard is !Send). - // Handles poison gracefully — the semaphore is still safe to replace. + // Reset semaphore to 1 permit for next offline sync. + // Generation bump invalidates stale Arc clones. Block scopes the !Send MutexGuard. { + self.message_semaphore_generation + .fetch_add(1, Ordering::SeqCst); let mut guard = match self.message_processing_semaphore.lock() { Ok(g) => g, Err(poisoned) => poisoned.into_inner(), diff --git a/src/client/sessions.rs b/src/client/sessions.rs index eb83616c4..8d5af30d4 100644 --- a/src/client/sessions.rs +++ b/src/client/sessions.rs @@ -38,6 +38,8 @@ impl Client { // Replace with a new semaphore with 64 permits for concurrent processing. // Old workers holding the previous semaphore Arc will finish normally. { + self.message_semaphore_generation + .fetch_add(1, std::sync::atomic::Ordering::SeqCst); let mut guard = match self.message_processing_semaphore.lock() { Ok(g) => g, Err(poisoned) => poisoned.into_inner(), diff --git a/src/message.rs b/src/message.rs index 76bc67e16..7f1a2a5b3 100644 --- a/src/message.rs +++ b/src/message.rs @@ -493,13 +493,23 @@ impl Client { ); } - // Acquire global processing permit. During offline sync (1 permit), this serializes - // ALL message processing globally, matching WA Web's allChatQueue pattern. - // After offline sync completes, permits are increased for parallel processing. + // Acquire global processing permit (1 during offline sync, N after). + // Generation check rejects stale Arc clones from a previous connection. + let generation = self + .message_semaphore_generation + .load(std::sync::atomic::Ordering::SeqCst); let semaphore = match self.message_processing_semaphore.lock() { Ok(guard) => guard.clone(), Err(poisoned) => poisoned.into_inner().clone(), }; + if generation + != self + .message_semaphore_generation + .load(std::sync::atomic::Ordering::SeqCst) + { + log::debug!("Stale semaphore generation, skipping message batch"); + return; + } let _global_permit = semaphore.acquire_arc().await; log::debug!( diff --git a/wacore/src/pair.rs b/wacore/src/pair.rs index c1ddda023..c5511d126 100644 --- a/wacore/src/pair.rs +++ b/wacore/src/pair.rs @@ -144,12 +144,9 @@ impl PairUtils { mac.update(ADV_HOSTED_PREFIX_ACCOUNT_SIGNATURE); } mac.update(details_bytes); - // TODO(security): HMAC verification disabled — requires implementing a new - // DeviceCommand variant to persist the rotated adv_secret_key after pairing. - // This variant does not exist yet; see src/pair_code.rs:321 for the - // persistence step that must be built first. - // The ED25519 account signature verification below still provides primary - // authentication (defense-in-depth once HMAC is re-enabled). + // TODO(security): HMAC verification disabled — adv_secret_key rotation + // isn't persisted yet. Needs a DeviceCommand variant + pair_code.rs:321. + // ED25519 signature verification below is the primary auth gate. // 2. Unmarshal inner container and verify account signature let mut signed_identity = From bd075ef893f73b85f8ed4949d16d8b93e23470e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Sat, 21 Mar 2026 15:59:04 -0300 Subject: [PATCH 08/12] feat: implement SetAdvSecretKey + re-enable HMAC verification in pairing - Add DeviceCommand::SetAdvSecretKey([u8; 32]) variant - Persist rotated adv_secret_key in pair_code.rs after key bundle prep - Re-enable the HMAC verification check in do_pair_crypto() that was disabled since the key wasn't being persisted - ED25519 signature verification remains as the primary auth gate; HMAC now provides defense-in-depth --- src/pair_code.rs | 15 ++++++++++----- wacore/src/pair.rs | 12 ++++++++---- wacore/src/store/commands.rs | 4 ++++ 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/src/pair_code.rs b/src/pair_code.rs index a4472a5c5..d227abb90 100644 --- a/src/pair_code.rs +++ b/src/pair_code.rs @@ -317,11 +317,8 @@ pub(crate) async fn handle_pair_code_notification(client: &Arc, node: &N // Get device keys let device_snapshot = client.persistence_manager.get_device_snapshot().await; - // Prepare encrypted key bundle - // TODO: Store `new_adv_secret` via DeviceCommand::SetAdvSecretKey to enable HMAC - // verification in pair-success. Currently the HMAC check in do_pair_crypto is - // commented out, so pairing works without it. See wacore/src/pair.rs:147-153. - let (wrapped_bundle, _new_adv_secret) = match PairCodeUtils::prepare_key_bundle( + // Prepare encrypted key bundle (includes rotated adv_secret_key) + let (wrapped_bundle, new_adv_secret) = match PairCodeUtils::prepare_key_bundle( &ephemeral_keypair, &primary_ephemeral_pub, &primary_identity_pub, @@ -334,6 +331,14 @@ pub(crate) async fn handle_pair_code_notification(client: &Arc, node: &N } }; + // Persist rotated adv_secret_key so HMAC verification works in pair-success. + client + .persistence_manager + .process_command(crate::store::commands::DeviceCommand::SetAdvSecretKey( + new_adv_secret, + )) + .await; + // Build and send stage 2 IQ let req_id = client.generate_request_id(); let identity_pub: [u8; 32] = device_snapshot diff --git a/wacore/src/pair.rs b/wacore/src/pair.rs index c5511d126..76e74ce99 100644 --- a/wacore/src/pair.rs +++ b/wacore/src/pair.rs @@ -131,7 +131,7 @@ impl PairUtils { text: "internal-error", source: anyhow::anyhow!("HMAC container missing details"), })?; - let _hmac_bytes = hmac_container + let hmac_bytes = hmac_container .hmac .as_deref() .ok_or_else(|| PairCryptoError { @@ -144,9 +144,13 @@ impl PairUtils { mac.update(ADV_HOSTED_PREFIX_ACCOUNT_SIGNATURE); } mac.update(details_bytes); - // TODO(security): HMAC verification disabled — adv_secret_key rotation - // isn't persisted yet. Needs a DeviceCommand variant + pair_code.rs:321. - // ED25519 signature verification below is the primary auth gate. + if mac.verify_slice(hmac_bytes).is_err() { + return Err(PairCryptoError { + code: 401, + text: "hmac-mismatch", + source: anyhow::anyhow!("HMAC mismatch"), + }); + } // 2. Unmarshal inner container and verify account signature let mut signed_identity = diff --git a/wacore/src/store/commands.rs b/wacore/src/store/commands.rs index d1a01cceb..e0931edcc 100644 --- a/wacore/src/store/commands.rs +++ b/wacore/src/store/commands.rs @@ -16,6 +16,7 @@ pub enum DeviceCommand { ), SetPropsHash(Option), SetNextPreKeyId(u32), + SetAdvSecretKey([u8; 32]), } pub fn apply_command_to_device(device: &mut Device, command: DeviceCommand) { @@ -47,5 +48,8 @@ pub fn apply_command_to_device(device: &mut Device, command: DeviceCommand) { DeviceCommand::SetNextPreKeyId(id) => { device.next_pre_key_id = id; } + DeviceCommand::SetAdvSecretKey(key) => { + device.adv_secret_key = key; + } } } From 7d2cfdd747c8fd56e9fbeacddef43dbfa95495be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Sat, 21 Mar 2026 16:04:10 -0300 Subject: [PATCH 09/12] =?UTF-8?q?fix:=20revert=20HMAC=20verification=20?= =?UTF-8?q?=E2=80=94=20QR=20pairing=20doesn't=20rotate=20adv=5Fsecret=5Fke?= =?UTF-8?q?y?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The HMAC check fails for QR-paired devices because adv_secret_key is only rotated in the pair-code flow (via SetAdvSecretKey). QR pairing keeps the initial random key from Device::new(), which won't match the server's HMAC. Keep the verification disabled until both pairing paths persist the correct key. The SetAdvSecretKey command and pair-code persistence are retained for when QR pairing key rotation is implemented. --- wacore/src/pair.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/wacore/src/pair.rs b/wacore/src/pair.rs index 76e74ce99..187bf2d2b 100644 --- a/wacore/src/pair.rs +++ b/wacore/src/pair.rs @@ -131,7 +131,7 @@ impl PairUtils { text: "internal-error", source: anyhow::anyhow!("HMAC container missing details"), })?; - let hmac_bytes = hmac_container + let _hmac_bytes = hmac_container .hmac .as_deref() .ok_or_else(|| PairCryptoError { @@ -144,13 +144,11 @@ impl PairUtils { mac.update(ADV_HOSTED_PREFIX_ACCOUNT_SIGNATURE); } mac.update(details_bytes); - if mac.verify_slice(hmac_bytes).is_err() { - return Err(PairCryptoError { - code: 401, - text: "hmac-mismatch", - source: anyhow::anyhow!("HMAC mismatch"), - }); - } + // TODO(security): HMAC verification skipped — adv_secret_key is only + // rotated in the pair-code flow (SetAdvSecretKey). QR pairing uses the + // initial random key from Device::new() which won't match the server's + // HMAC. Re-enable once both pairing paths persist the correct key. + // ED25519 signature verification below is the primary auth gate. // 2. Unmarshal inner container and verify account signature let mut signed_identity = From c3c126c43de700294565aad1956f08719b23b54f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Sat, 21 Mar 2026 16:05:51 -0300 Subject: [PATCH 10/12] fix: use symbol reference instead of line number in pair.rs TODO --- wacore/src/pair.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/wacore/src/pair.rs b/wacore/src/pair.rs index 187bf2d2b..a2ea76387 100644 --- a/wacore/src/pair.rs +++ b/wacore/src/pair.rs @@ -145,10 +145,10 @@ impl PairUtils { } mac.update(details_bytes); // TODO(security): HMAC verification skipped — adv_secret_key is only - // rotated in the pair-code flow (SetAdvSecretKey). QR pairing uses the - // initial random key from Device::new() which won't match the server's - // HMAC. Re-enable once both pairing paths persist the correct key. - // ED25519 signature verification below is the primary auth gate. + // rotated in the pair-code flow (see handle_pair_code_notification() in + // pair_code.rs, via DeviceCommand::SetAdvSecretKey). QR pairing uses + // the initial random key from Device::new() which won't match. + // Re-enable once both pairing paths persist the correct key. // 2. Unmarshal inner container and verify account signature let mut signed_identity = From 74b0bd1b546e53a2e1a12f1087230091ee2f4327 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Sat, 21 Mar 2026 16:23:30 -0300 Subject: [PATCH 11/12] fix: add post-acquire generation recheck to close TOCTOU window The semaphore generation could change during the .acquire_arc().await suspension point. Add a second generation check after acquiring the permit so stale permits from a previous connection are dropped. --- src/message.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/message.rs b/src/message.rs index 7f1a2a5b3..8e7346413 100644 --- a/src/message.rs +++ b/src/message.rs @@ -511,6 +511,15 @@ impl Client { return; } let _global_permit = semaphore.acquire_arc().await; + // Post-acquire recheck: generation could have changed during the .await + if generation + != self + .message_semaphore_generation + .load(std::sync::atomic::Ordering::SeqCst) + { + log::debug!("Semaphore generation changed during acquire, dropping stale permit"); + return; + } log::debug!( "Starting PASS 1: Processing {} session establishment messages (pkmsg/msg)", From f376414e9505ff9b3fba6df90d87c7ce991662af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Lucas?= Date: Sat, 21 Mar 2026 16:57:19 -0300 Subject: [PATCH 12/12] fix: correct semaphore generation ordering + extract helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The generation bump and Arc swap must happen under the same mutex hold so readers always see a consistent (generation, semaphore) pair. Writer (swap_message_semaphore helper): 1. Lock mutex 2. Replace Arc 3. Bump generation 4. Unlock Both client.rs (disconnect) and sessions.rs (offline sync) now use the same helper. Reader (message.rs): Read generation inside the mutex (not before it) so the (generation, Arc) pair is always consistent. Removed the redundant pre-acquire check — it was always true with correct ordering. Only the post-acquire check remains for races during .await. --- src/client.rs | 26 ++++++++++++++++---------- src/client/sessions.rs | 10 +--------- src/message.rs | 30 +++++++++++++++--------------- 3 files changed, 32 insertions(+), 34 deletions(-) diff --git a/src/client.rs b/src/client.rs index 348e9348e..f3f701b0c 100644 --- a/src/client.rs +++ b/src/client.rs @@ -433,6 +433,21 @@ pub struct Client { } impl Client { + /// Replace the message processing semaphore and bump the generation counter. + /// + /// Both operations happen under the same mutex hold so readers always see + /// a consistent (generation, Arc) pair. Must be called from a non-async + /// context or inside a scoped block (MutexGuard is !Send). + pub(crate) fn swap_message_semaphore(&self, permits: usize) { + let mut guard = match self.message_processing_semaphore.lock() { + Ok(g) => g, + Err(poisoned) => poisoned.into_inner(), + }; + *guard = Arc::new(async_lock::Semaphore::new(permits)); + self.message_semaphore_generation + .fetch_add(1, Ordering::SeqCst); + } + fn should_downgrade_sync_error(&self, err: &anyhow::Error) -> bool { if self.is_shutting_down() { return true; @@ -1014,16 +1029,7 @@ impl Client { // Clear signal cache so stale state doesn't leak across connections self.signal_cache.clear().await; // Reset semaphore to 1 permit for next offline sync. - // Generation bump invalidates stale Arc clones. Block scopes the !Send MutexGuard. - { - self.message_semaphore_generation - .fetch_add(1, Ordering::SeqCst); - let mut guard = match self.message_processing_semaphore.lock() { - Ok(g) => g, - Err(poisoned) => poisoned.into_inner(), - }; - *guard = Arc::new(async_lock::Semaphore::new(1)); - } + self.swap_message_semaphore(1); // Reset dead-socket timestamps so stale values from the previous // connection don't trigger an immediate reconnect on the next one. self.last_data_received_ms.store(0, Ordering::Relaxed); diff --git a/src/client/sessions.rs b/src/client/sessions.rs index 8d5af30d4..62b4d5ae9 100644 --- a/src/client/sessions.rs +++ b/src/client/sessions.rs @@ -37,15 +37,7 @@ impl Client { // During offline sync, permits=1 serialized all message processing. // Replace with a new semaphore with 64 permits for concurrent processing. // Old workers holding the previous semaphore Arc will finish normally. - { - self.message_semaphore_generation - .fetch_add(1, std::sync::atomic::Ordering::SeqCst); - let mut guard = match self.message_processing_semaphore.lock() { - Ok(g) => g, - Err(poisoned) => poisoned.into_inner(), - }; - *guard = std::sync::Arc::new(async_lock::Semaphore::new(64)); - } + self.swap_message_semaphore(64); self.offline_sync_notifier.notify(usize::MAX); diff --git a/src/message.rs b/src/message.rs index 8e7346413..b800ac35e 100644 --- a/src/message.rs +++ b/src/message.rs @@ -494,22 +494,22 @@ impl Client { } // Acquire global processing permit (1 during offline sync, N after). - // Generation check rejects stale Arc clones from a previous connection. - let generation = self - .message_semaphore_generation - .load(std::sync::atomic::Ordering::SeqCst); - let semaphore = match self.message_processing_semaphore.lock() { - Ok(guard) => guard.clone(), - Err(poisoned) => poisoned.into_inner().clone(), + // Read generation + clone Arc under the same mutex so the pair is consistent. + let (generation, semaphore) = match self.message_processing_semaphore.lock() { + Ok(guard) => ( + self.message_semaphore_generation + .load(std::sync::atomic::Ordering::SeqCst), + guard.clone(), + ), + Err(poisoned) => { + let guard = poisoned.into_inner(); + ( + self.message_semaphore_generation + .load(std::sync::atomic::Ordering::SeqCst), + guard.clone(), + ) + } }; - if generation - != self - .message_semaphore_generation - .load(std::sync::atomic::Ordering::SeqCst) - { - log::debug!("Stale semaphore generation, skipping message batch"); - return; - } let _global_permit = semaphore.acquire_arc().await; // Post-acquire recheck: generation could have changed during the .await if generation