Skip to content

Commit 7c88cbb

Browse files
committed
fix(libsignal): keep skipped message-key seeds projectable
A session that skipped a message key could not be exported to the v1 model: v1 expresses a message key as its 32-byte seed, the native format persisted only the derived cipher/mac/iv, and that derivation has no inverse. The round trip was lossy from the first cycle, so importing a v1 record and exporting it back already failed with ChainNotRepresentable. The seed now rides along with the keys it derives, in a new local field on SessionStructure.Chain.MessageKey. It is written unconditionally: the persisted format must not vary with build flags, or a binary without legacy-session-interop would erase what one with it wrote. The derived triple stays, so a build that ignores the seed still loads the record and decrypts; only the export path reads it. The field is declared in build.rs and spliced into the descriptor rather than added to whatsapp.proto, which is regenerated wholesale from whatspec and would lose it on the next sync. Field number 100, because upstream appends low numbers without warning - kyberPreKeyId = 4 and kyberCiphertext = 5 landed on SessionStructure.PendingPreKey that way - and a collision would silently reinterpret the seed in every record already on disk. The splice now fails the build on such a collision instead of resolving it. SessionMessageKeyMaterial carries fixed-width arrays instead of Vec<u8>. The lengths were already invariants enforced at every conversion; in the type they cost no allocation and no runtime check. Writing the four persisted fields out of one buffer keeps the skipped-key path at a single allocation, down from three before this change.
1 parent 786c635 commit 7c88cbb

8 files changed

Lines changed: 809 additions & 133 deletions

File tree

‎.github/workflows/main.yml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,10 @@ jobs:
157157
run: |
158158
cargo nextest run --profile ci -p wacore --features voip --lib
159159
cargo nextest run --profile ci -p whatsapp-rust --features "voip tokio-native tokio-transport" --lib
160+
# legacy-session-interop is off by default, so every other test job
161+
# compiles its module away and never runs a single one of its tests.
162+
- name: Test (legacy-session-interop)
163+
run: cargo nextest run --profile ci -p wacore-libsignal --features legacy-session-interop
160164

161165
rustdoc:
162166
name: Rustdoc

‎AGENTS.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ Things that look correct and are not:
3232
- **Locks.** `session_locks` serializes Signal encrypt/decrypt per protocol address; `chat_lanes` (`ChatLane::enqueue_lock` in `src/client.rs`) serializes *incoming* processing per chat. Outgoing sends are deliberately not per-chat locked — WA Web doesn't lock them either.
3333
- **Wire-tagged enums.** Every protocol enum derives `WireEnum`, and its `#[wire = ...]` attribute is the single source of truth for the wire value. Do not also derive `serde::Serialize`/`Deserialize` or add `#[serde(rename_all)]` — the derive owns both. In tagged mode it generates a sibling `<Name>Tag`; parsers must dispatch on `<Name>Tag::try_from(node.tag.as_ref())` rather than string literals, so renaming a tag stays a one-attribute change. Modes and attributes: `agent_docs/protocol_architecture.md`.
3434
- **Event payloads are a frozen API.** Sealed with `#[non_exhaustive]` + `#[derive(bon::Builder)]` and constructed via `Type::builder()…build()`; a maybe-absent field is `Option<T>`, never an empty-string or zero sentinel. The full stability policy is the `Event` doc comment in `wacore/src/types/events.rs`.
35+
- **`whatsapp.proto` is not the whole persisted schema.** It comes from whatspec and is regenerated wholesale, so fields we persist but upstream does not declare live in `LOCAL_FIELDS` in `waproto/build.rs`, spliced into the descriptor at build time. Never hand-edit the `.proto` or `.desc` to add one — the next sync would drop it.
3536
- **Blocking work** — `ureq`, heavy CPU — belongs in `tokio::task::spawn_blocking`; it shares a runtime with the read loop.
3637
- **let-chains**, never nested `if let`. Clippy's `collapsible_if` is denied in CI.
3738
- **No real PII in tests**, including vectors derived from production captures. Regenerate them from fictitious JIDs and numbers.

‎wacore/libsignal/src/protocol/legacy_session.rs‎

Lines changed: 93 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -569,8 +569,10 @@ impl SessionRecord {
569569
/// is an inert placeholder. Local identity and registration values remain
570570
/// external because v1 does not persist them.
571571
///
572-
/// Derived skipped-message keys have no inverse to their v1 seed, so their
573-
/// presence returns [`LegacySessionInteropError::NotRepresentable`].
572+
/// Skipped message keys carry the seed v1 expects. A record persisted
573+
/// before that seed was retained has only the derived keys, which have no
574+
/// inverse, so it returns
575+
/// [`LegacySessionInteropError::ChainNotRepresentable`].
574576
pub fn into_legacy_session_v1_operational(
575577
self,
576578
) -> Result<LegacySessionRecordV1, LegacySessionInteropError> {
@@ -961,7 +963,10 @@ fn chain_into_components(
961963
.into_iter()
962964
.map(|key| SessionMessageKeyComponents {
963965
index: key.index,
964-
material: SessionMessageKeyMaterial::Seed(key.seed.into()),
966+
material: SessionMessageKeyMaterial::Seed(
967+
<[u8; LEGACY_KEY_MATERIAL_LEN]>::try_from(key.seed.as_ref())
968+
.expect("validated skipped message-key seed"),
969+
),
965970
})
966971
.collect(),
967972
}
@@ -1208,28 +1213,24 @@ fn project_chain_parts(
12081213
chain: chain_index,
12091214
field: LegacySessionFieldV1::ChainKeyIndex,
12101215
})?;
1211-
if message_keys
1212-
.iter()
1213-
.any(|key| matches!(key.material, SessionMessageKeyMaterial::Derived { .. }))
1214-
{
1215-
return Err(LegacySessionInteropError::ChainNotRepresentable {
1216-
session,
1217-
chain: chain_index,
1218-
field: LegacySessionUnrepresentableFieldV1::DerivedMessageKey,
1219-
});
1220-
}
1216+
// Exhaustive so a new material variant has to state its v1 form here
1217+
// instead of compiling into a silent projection.
12211218
let message_keys = message_keys
12221219
.into_iter()
1223-
.map(|key| {
1224-
let SessionMessageKeyMaterial::Seed(seed) = key.material else {
1225-
unreachable!("derived message-key material was rejected before allocation")
1226-
};
1227-
LegacySessionMessageKeyV1 {
1220+
.map(|key| match key.material {
1221+
SessionMessageKeyMaterial::Seed(seed) => Ok(LegacySessionMessageKeyV1 {
12281222
index: key.index,
1229-
seed: seed.into(),
1223+
seed: Bytes::copy_from_slice(&seed),
1224+
}),
1225+
SessionMessageKeyMaterial::Derived { .. } => {
1226+
Err(LegacySessionInteropError::ChainNotRepresentable {
1227+
session,
1228+
chain: chain_index,
1229+
field: LegacySessionUnrepresentableFieldV1::DerivedMessageKey,
1230+
})
12301231
}
12311232
})
1232-
.collect();
1233+
.collect::<Result<Vec<_>, _>>()?;
12331234

12341235
Ok(LegacySessionChainV1 {
12351236
ratchet_key,
@@ -1659,27 +1660,58 @@ mod tests {
16591660
seed: Bytes::copy_from_slice(&seed),
16601661
}];
16611662

1662-
let components = record(vec![session])
1663+
let native = record(vec![session])
16631664
.into_session_record(local_context())
1664-
.expect("import")
1665+
.expect("import");
1666+
let persisted = crate::protocol::stores::SessionStructure::from(
1667+
native.session_state().expect("current state"),
1668+
);
1669+
let stored = &persisted.receiver_chains[0].message_keys[0];
1670+
let expected = MessageKeyGenerator::new_from_seed(&seed, 0).generate_keys();
1671+
assert_eq!(
1672+
stored.cipher_key.as_deref(),
1673+
Some(&expected.cipher_key()[..])
1674+
);
1675+
assert_eq!(stored.mac_key.as_deref(), Some(&expected.mac_key()[..]));
1676+
assert_eq!(stored.iv.as_deref(), Some(&expected.iv()[..]));
1677+
assert_eq!(stored.seed.as_deref(), Some(&seed[..]));
1678+
1679+
let material = &native
16651680
.into_components()
1666-
.expect("components");
1667-
let material = &components.current_session.expect("current").receiver_chains[0]
1681+
.expect("components")
1682+
.current_session
1683+
.expect("current")
1684+
.receiver_chains[0]
16681685
.message_keys[0]
16691686
.material;
1670-
let expected = MessageKeyGenerator::new_from_seed(&seed, 0).generate_keys();
1671-
match material {
1672-
SessionMessageKeyMaterial::Derived {
1673-
cipher_key,
1674-
mac_key,
1675-
iv,
1676-
} => {
1677-
assert_eq!(cipher_key, expected.cipher_key());
1678-
assert_eq!(mac_key, expected.mac_key());
1679-
assert_eq!(iv, expected.iv());
1680-
}
1681-
SessionMessageKeyMaterial::Seed(_) => panic!("seed must be derived on import"),
1682-
}
1687+
assert_eq!(material, &SessionMessageKeyMaterial::Seed(seed));
1688+
}
1689+
1690+
/// Regression: a session holding a skipped key used to be unprojectable
1691+
/// from the first cycle, because import kept only the derived keys.
1692+
#[test]
1693+
fn a_retained_skipped_key_projects_back_to_its_seed() {
1694+
let seed = vec![0x77; 32];
1695+
let mut session = reference_session(63, LegacySessionDispositionV1::Current);
1696+
session.chains[1].message_keys = vec![LegacySessionMessageKeyV1 {
1697+
index: 0,
1698+
seed: seed.clone().into(),
1699+
}];
1700+
1701+
let projected = record(vec![session])
1702+
.into_session_record(local_context())
1703+
.expect("import")
1704+
.into_legacy_session_v1_operational()
1705+
.expect("skipped key stays projectable");
1706+
1707+
let chains = &projected.sessions[0].session.chains;
1708+
let receiving = chains
1709+
.iter()
1710+
.find(|chain| chain.role == LegacySessionChainRoleV1::Receiving)
1711+
.expect("receiving chain");
1712+
assert_eq!(receiving.message_keys.len(), 1);
1713+
assert_eq!(receiving.message_keys[0].index, 0);
1714+
assert_eq!(receiving.message_keys[0].seed, seed);
16831715
}
16841716

16851717
#[test]
@@ -1813,9 +1845,31 @@ mod tests {
18131845
index: 0,
18141846
seed: vec![0x44; 32].into(),
18151847
}];
1816-
let native = record(vec![session])
1848+
// Skipped keys persisted before the seed was retained come back as
1849+
// `Derived`; strip the seed to reproduce one of those records.
1850+
let mut components = record(vec![session])
18171851
.into_session_record(local_context())
1818-
.expect("import");
1852+
.expect("import")
1853+
.into_components()
1854+
.expect("components");
1855+
let key = &mut components
1856+
.current_session
1857+
.as_mut()
1858+
.expect("current")
1859+
.receiver_chains[0]
1860+
.message_keys[0];
1861+
let SessionMessageKeyMaterial::Seed(seed) = &key.material else {
1862+
panic!("imported skipped key must retain its seed")
1863+
};
1864+
let seed = <[u8; 32]>::try_from(seed.as_slice()).expect("32-byte seed");
1865+
let keys = MessageKeyGenerator::new_from_seed(&seed, key.index).generate_keys();
1866+
key.material = SessionMessageKeyMaterial::Derived {
1867+
cipher_key: *keys.cipher_key(),
1868+
mac_key: *keys.mac_key(),
1869+
iv: *keys.iv(),
1870+
};
1871+
1872+
let native = SessionRecord::from_components(components).expect("seedless record");
18191873
assert!(matches!(
18201874
native.into_legacy_session_v1_operational(),
18211875
Err(LegacySessionInteropError::ChainNotRepresentable {

‎wacore/libsignal/src/protocol/ratchet/keys.rs‎

Lines changed: 98 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,6 @@ use crate::protocol::{PrivateKey, PublicKey, Result, crypto, stores::session_str
1818
/// 2. **Zero-cost round-trip**: Keys loaded from protobuf are kept in serialized form and
1919
/// returned as-is when saving, avoiding unnecessary deserialization and re-serialization
2020
pub enum MessageKeyGenerator {
21-
/// Native computed keys - from encryption operations
22-
Keys(MessageKeys),
2321
/// Seed for lazy derivation - keys derived on demand
2422
Seed(([u8; 32], u32)),
2523
/// Original protobuf - zero-cost pass-through on save
@@ -37,7 +35,6 @@ impl MessageKeyGenerator {
3735
pub fn generate_keys(self) -> MessageKeys {
3836
match self {
3937
Self::Seed((seed, counter)) => MessageKeys::derive_keys(&seed, None, counter),
40-
Self::Keys(k) => k,
4138
Self::Serialized(pb) => {
4239
// Parse on demand - only when keys are actually needed.
4340
// Note: from_pb() validates field lengths before creating Serialized,
@@ -71,19 +68,39 @@ impl MessageKeyGenerator {
7168

7269
/// Convert to protobuf format for storage.
7370
/// Zero-cost for Serialized variant (pass-through), allocates for others.
71+
///
72+
/// The seed is persisted next to the keys it derives: the derivation is
73+
/// one-way, so a record that kept only the derived keys could never be
74+
/// projected back into a seed-based external format.
7475
pub fn into_pb(self) -> session_structure::chain::MessageKey {
7576
match self {
7677
// Zero-cost pass-through: return original protobuf unchanged
7778
Self::Serialized(pb) => pb,
7879
// Need to serialize: derive keys and convert
79-
Self::Seed(_) | Self::Keys(_) => {
80-
use bytes::Bytes;
81-
let keys = self.generate_keys();
80+
Self::Seed((seed, counter)) => {
81+
use bytes::BytesMut;
82+
let keys = MessageKeys::derive_keys(&seed, None, counter);
83+
// The four fields are written, stored and dropped together, so
84+
// they share one buffer: `split_to` hands out refcounted views
85+
// instead of copying each field into its own allocation.
86+
let mut material = BytesMut::with_capacity(
87+
keys.cipher_key().len() + keys.mac_key().len() + keys.iv().len() + seed.len(),
88+
);
89+
material.extend_from_slice(keys.cipher_key());
90+
material.extend_from_slice(keys.mac_key());
91+
material.extend_from_slice(keys.iv());
92+
material.extend_from_slice(&seed);
93+
94+
let mut material = material.freeze();
95+
let cipher_key = material.split_to(keys.cipher_key().len());
96+
let mac_key = material.split_to(keys.mac_key().len());
97+
let iv = material.split_to(keys.iv().len());
8298
session_structure::chain::MessageKey {
83-
cipher_key: Some(Bytes::copy_from_slice(keys.cipher_key())),
84-
mac_key: Some(Bytes::copy_from_slice(keys.mac_key())),
85-
iv: Some(Bytes::copy_from_slice(keys.iv())),
99+
cipher_key: Some(cipher_key),
100+
mac_key: Some(mac_key),
101+
iv: Some(iv),
86102
index: Some(keys.counter()),
103+
seed: Some(material),
87104
}
88105
}
89106
}
@@ -109,7 +126,6 @@ impl MessageKeyGenerator {
109126
#[inline]
110127
pub fn counter(&self) -> u32 {
111128
match self {
112-
Self::Keys(k) => k.counter(),
113129
Self::Seed((_, counter)) => *counter,
114130
Self::Serialized(pb) => pb.index.unwrap_or(0),
115131
}
@@ -463,6 +479,78 @@ mod tests {
463479
assert_eq!(keys.cipher_key(), keys2.cipher_key());
464480
}
465481

482+
/// The seed is one-way, so persisting it alongside the keys it derives is
483+
/// the only thing that keeps a skipped key exportable.
484+
#[test]
485+
fn into_pb_persists_the_seed_next_to_the_derived_keys() {
486+
let seed = [0x3Cu8; 32];
487+
let pb = MessageKeyGenerator::new_from_seed(&seed, 11).into_pb();
488+
let expected = MessageKeys::derive_keys(&seed, None, 11);
489+
490+
assert_eq!(pb.index, Some(11));
491+
assert_eq!(pb.seed.as_deref(), Some(&seed[..]));
492+
assert_eq!(pb.cipher_key.as_deref(), Some(&expected.cipher_key()[..]));
493+
assert_eq!(pb.mac_key.as_deref(), Some(&expected.mac_key()[..]));
494+
assert_eq!(pb.iv.as_deref(), Some(&expected.iv()[..]));
495+
}
496+
497+
/// Reloading a persisted key must keep using the stored derived material,
498+
/// not re-derive from the seed: a decrypt that changed keys here would
499+
/// silently fail the MAC. Only material the seed does *not* produce can
500+
/// tell the two apart, so the fixture stores a deliberately unrelated
501+
/// triple next to it.
502+
#[test]
503+
fn reloaded_keys_come_from_the_persisted_derived_material() {
504+
use bytes::Bytes;
505+
506+
let seed = [0x9Eu8; 32];
507+
let mut pb = MessageKeyGenerator::new_from_seed(&seed, 4).into_pb();
508+
pb.cipher_key = Some(Bytes::from_static(&[0x11; 32]));
509+
pb.mac_key = Some(Bytes::from_static(&[0x22; 32]));
510+
pb.iv = Some(Bytes::from_static(&[0x33; 16]));
511+
let from_seed = MessageKeys::derive_keys(&seed, None, 4);
512+
513+
let reloaded = MessageKeyGenerator::from_pb(pb)
514+
.expect("key stays loadable")
515+
.generate_keys();
516+
517+
assert_eq!(reloaded.cipher_key(), &[0x11; 32]);
518+
assert_eq!(reloaded.mac_key(), &[0x22; 32]);
519+
assert_eq!(reloaded.iv(), &[0x33; 16]);
520+
assert_eq!(reloaded.counter(), 4);
521+
assert_ne!(reloaded.cipher_key(), from_seed.cipher_key());
522+
}
523+
524+
/// Keys persisted before the seed was retained must still load and produce
525+
/// exactly what was stored.
526+
#[test]
527+
fn seedless_persisted_keys_still_load() {
528+
let seed = [0x9Eu8; 32];
529+
let mut pb = MessageKeyGenerator::new_from_seed(&seed, 4).into_pb();
530+
let expected = MessageKeys::derive_keys(&seed, None, 4);
531+
pb.seed = None;
532+
533+
let reloaded = MessageKeyGenerator::from_pb(pb)
534+
.expect("seedless key stays loadable")
535+
.generate_keys();
536+
537+
assert_eq!(reloaded.cipher_key(), expected.cipher_key());
538+
assert_eq!(reloaded.mac_key(), expected.mac_key());
539+
assert_eq!(reloaded.iv(), expected.iv());
540+
assert_eq!(reloaded.counter(), 4);
541+
}
542+
543+
/// A seed alone is not a loadable key: `from_pb` still requires the three
544+
/// derived fields, so a downgrade that drops the seed cannot fail the
545+
/// whole record.
546+
#[test]
547+
fn from_pb_still_rejects_a_key_without_derived_material() {
548+
let mut pb = MessageKeyGenerator::new_from_seed(&[0x2Bu8; 32], 0).into_pb();
549+
pb.cipher_key = None;
550+
551+
assert!(MessageKeyGenerator::from_pb(pb).is_err());
552+
}
553+
466554
/// Test MessageKeys derive_keys with known inputs
467555
#[test]
468556
fn test_message_keys_derive_with_salt() {

0 commit comments

Comments
 (0)