From f0dc51082141625940efee9d7f3d4a8dbe085e5e Mon Sep 17 00:00:00 2001 From: Mathieu Amiot Date: Tue, 15 Nov 2022 18:14:20 +0100 Subject: [PATCH] fix: Broken Proteus implementation --- .github/dependabot.yml | 2 +- Cargo.toml | 2 +- crypto-ffi/src/generic.rs | 24 ++-- crypto-ffi/src/wasm.rs | 4 +- crypto/Cargo.toml | 7 +- crypto/benches/mls_proteus.rs | 9 +- crypto/src/proteus.rs | 139 +++++++++++++++-------- interop/Cargo.toml | 3 +- interop/src/clients/corecrypto/native.rs | 2 +- interop/src/main.rs | 4 +- keystore/Cargo.toml | 5 +- keystore/src/entities/proteus.rs | 7 ++ 12 files changed, 138 insertions(+), 70 deletions(-) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 41fa3145e4..4678a5ec09 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -7,4 +7,4 @@ updates: - package-ecosystem: "github-actions" directory: "/" schedule: - interval: "weekly" \ No newline at end of file + interval: "weekly" diff --git a/Cargo.toml b/Cargo.toml index 5ab6e77aae..b61e8c4113 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,7 +17,7 @@ branch = "otak/fix-1.0.3" [patch.crates-io.proteus-traits] package = "proteus-traits" git = "https://github.com/wireapp/proteus" -branch = "otak/oxidize-wasm-v2" +branch = "otak/2.0" [patch.crates-io.openmls] git = "https://github.com/wireapp/openmls" diff --git a/crypto-ffi/src/generic.rs b/crypto-ffi/src/generic.rs index fc1f1aa69a..a6beace3cd 100644 --- a/crypto-ffi/src/generic.rs +++ b/crypto-ffi/src/generic.rs @@ -869,10 +869,14 @@ impl CoreCrypto<'_> { /// See [core_crypto::proteus::ProteusCentral::encrypt] pub fn proteus_encrypt(&self, session_id: &str, plaintext: &[u8]) -> CryptoResult> { proteus_impl! {{ - self.central - .lock() - .map_err(|_| CryptoError::LockPoisonError)? - .proteus_encrypt(session_id, plaintext) + future::block_on( + self.executor.lock().map_err(|_| CryptoError::LockPoisonError)?.run( + self.central + .lock() + .map_err(|_| CryptoError::LockPoisonError)? + .proteus_encrypt(session_id, plaintext) + ), + ) }} } @@ -883,10 +887,14 @@ impl CoreCrypto<'_> { plaintext: &[u8], ) -> CryptoResult>> { proteus_impl! {{ - self.central - .lock() - .map_err(|_| CryptoError::LockPoisonError)? - .proteus_encrypt_batched(sessions.as_slice(), plaintext) + future::block_on( + self.executor.lock().map_err(|_| CryptoError::LockPoisonError)?.run( + self.central + .lock() + .map_err(|_| CryptoError::LockPoisonError)? + .proteus_encrypt_batched(sessions.as_slice(), plaintext) + ) + ) }} } diff --git a/crypto-ffi/src/wasm.rs b/crypto-ffi/src/wasm.rs index d9f24d25f6..aee17ed534 100644 --- a/crypto-ffi/src/wasm.rs +++ b/crypto-ffi/src/wasm.rs @@ -1439,7 +1439,7 @@ impl CoreCrypto { #[cfg_attr(not(feature = "proteus"), allow(unused_variables))] pub async fn proteus_encrypt(&self, session_id: String, plaintext: Box<[u8]>) -> WasmCryptoResult { proteus_impl!({ - let encrypted = self.0.write().await.proteus_encrypt(&session_id, &plaintext)?; + let encrypted = self.0.write().await.proteus_encrypt(&session_id, &plaintext).await?; WasmCryptoResult::Ok(Uint8Array::from(encrypted.as_slice()).into()) } or throw WasmCryptoResult<_>) } @@ -1455,7 +1455,7 @@ impl CoreCrypto { ) -> WasmCryptoResult { proteus_impl!({ let session_ids: Vec = sessions.into_iter().map(String::from).collect(); - let batch = self.0.write().await.proteus_encrypt_batched(session_ids.as_slice(), &plaintext)?; + let batch = self.0.write().await.proteus_encrypt_batched(session_ids.as_slice(), &plaintext).await?; let js_obj = js_sys::Map::new(); for (key, payload) in batch.into_iter() { js_obj.set(&js_sys::JsString::from(key).into(), &Uint8Array::from(payload.as_slice())); diff --git a/crypto/Cargo.toml b/crypto/Cargo.toml index 26f8c4798a..495f35ed57 100644 --- a/crypto/Cargo.toml +++ b/crypto/Cargo.toml @@ -46,11 +46,10 @@ serde_json = "1.0" [dependencies.proteus-wasm] version = "2.0" -package = "proteus" features = ["hazmat"] optional = true git = "https://github.com/wireapp/proteus" -branch = "otak/oxidize-wasm-v2" +branch = "otak/2.0" [target.'cfg(not(target_family = "wasm"))'.dependencies] async-fs = { version = "1.6", optional = true } @@ -87,8 +86,8 @@ proteus-traits = "2.0" async-trait = "0.1" [target.'cfg(not(target_family = "wasm"))'.dev-dependencies] -cryptobox = { git = "https://github.com/wireapp/cryptobox" } -proteus = { git = "https://github.com/wireapp//proteus", branch = "otak/fix-1.0.3" } +cryptobox = { git = "https://github.com/wireapp/cryptobox", tag = "v1.0.3" } +proteus = { git = "https://github.com/wireapp//proteus", branch = "otak/fix-1.0.3", features = [] } [target.'cfg(not(target_family = "wasm"))'.dev-dependencies.criterion] version = "0.3" diff --git a/crypto/benches/mls_proteus.rs b/crypto/benches/mls_proteus.rs index c7a5b47a9e..92d43556e6 100644 --- a/crypto/benches/mls_proteus.rs +++ b/crypto/benches/mls_proteus.rs @@ -73,9 +73,14 @@ fn encrypt_message_bench(c: &mut Criterion) { let text = Alphanumeric.sample_string(&mut rand::thread_rng(), MSG_MAX); (central, keystore, session_material, text) }, - |(mut central, keystore, session_material, text)| async move { + |(mut central, mut keystore, session_material, text)| async move { for (session_id, _) in session_material { - black_box(central.encrypt(&session_id, text.as_bytes()).unwrap()); + black_box( + central + .encrypt(&mut keystore, &session_id, text.as_bytes()) + .await + .unwrap(), + ); black_box(central.session_save(&keystore, &session_id).await.unwrap()); } }, diff --git a/crypto/src/proteus.rs b/crypto/src/proteus.rs index 5a23744f80..e0c4e2f9e5 100644 --- a/crypto/src/proteus.rs +++ b/crypto/src/proteus.rs @@ -95,7 +95,10 @@ impl CoreCrypto { prekey: &[u8], ) -> CryptoResult<&mut ProteusConversationSession> { let proteus = self.proteus.as_mut().ok_or(CryptoError::ProteusNotInitialized)?; - proteus.session_from_prekey(session_id, prekey).await + let session = proteus.session_from_prekey(session_id, prekey).await?; + ProteusCentral::session_save_by_ref(self.mls.mls_backend.borrow_keystore_mut(), session).await?; + + Ok(session) } /// Creates a proteus session from a Proteus message envelope @@ -108,7 +111,10 @@ impl CoreCrypto { ) -> CryptoResult<(&mut ProteusConversationSession, Vec)> { let proteus = self.proteus.as_mut().ok_or(CryptoError::ProteusNotInitialized)?; let keystore = self.mls.mls_backend.borrow_keystore_mut(); - proteus.session_from_message(keystore, session_id, envelope).await + let (session, message) = proteus.session_from_message(keystore, session_id, envelope).await?; + ProteusCentral::session_save_by_ref(keystore, session).await?; + + Ok((session, message)) } /// Saves a proteus session in the keystore @@ -157,22 +163,24 @@ impl CoreCrypto { /// Encrypts proteus message for a given session ID /// /// Warning: The Proteus client **MUST** be initialized with [CoreCrypto::proteus_init] first or an error will be returned - pub fn proteus_encrypt(&mut self, session_id: &str, plaintext: &[u8]) -> CryptoResult> { + pub async fn proteus_encrypt(&mut self, session_id: &str, plaintext: &[u8]) -> CryptoResult> { let proteus = self.proteus.as_mut().ok_or(CryptoError::ProteusNotInitialized)?; - proteus.encrypt(session_id, plaintext) + let keystore = self.mls.mls_backend.borrow_keystore_mut(); + proteus.encrypt(keystore, session_id, plaintext).await } /// Encrypts a proteus message for several sessions ID. This is more efficient than other methods as the calls are batched. /// This also reduces the rountrips when crossing over the FFI /// /// Warning: The Proteus client **MUST** be initialized with [CoreCrypto::proteus_init] first or an error will be returned - pub fn proteus_encrypt_batched( + pub async fn proteus_encrypt_batched( &mut self, sessions: &[impl AsRef], plaintext: &[u8], ) -> CryptoResult>> { let proteus = self.proteus.as_mut().ok_or(CryptoError::ProteusNotInitialized)?; - proteus.encrypt_batched(sessions, plaintext) + let keystore = self.mls.mls_backend.borrow_keystore_mut(); + proteus.encrypt_batched(keystore, sessions, plaintext).await } /// Creates a new Proteus prekey and returns the CBOR-serialized version of the prekey bundle @@ -276,7 +284,7 @@ impl ProteusCentral { let pk = hex::decode(pk_fingerprint)?; let ks_identity = ProteusIdentity { - sk: kp.secret_key.to_bytes_extended().into(), + sk: kp.secret_key.as_slice().into(), pk, }; keystore.save(ks_identity).await?; @@ -356,16 +364,21 @@ impl ProteusCentral { /// Persists a session in store pub async fn session_save(&self, keystore: &CryptoKeystore, session_id: &str) -> CryptoResult<()> { if let Some(session) = self.proteus_sessions.get(session_id) { - let db_session = ProteusSession { - id: session_id.into(), - session: session.session.serialise().map_err(ProteusError::from)?, - }; - keystore.save(db_session).await?; + Self::session_save_by_ref(keystore, session).await?; } Ok(()) } + async fn session_save_by_ref(keystore: &CryptoKeystore, session: &ProteusConversationSession) -> CryptoResult<()> { + let db_session = ProteusSession { + id: session.identifier().to_string(), + session: session.session.serialise().map_err(ProteusError::from)?, + }; + keystore.save(db_session).await?; + Ok(()) + } + /// Deletes a session in the store pub async fn session_delete(&mut self, keystore: &CryptoKeystore, session_id: &str) -> CryptoResult<()> { if keystore.remove::(session_id).await.is_ok() { @@ -398,16 +411,27 @@ impl ProteusCentral { ciphertext: &[u8], ) -> CryptoResult> { if let Some(session) = self.proteus_sessions.get_mut(session_id) { - session.decrypt(keystore, ciphertext).await + let plaintext = session.decrypt(keystore, ciphertext).await?; + ProteusCentral::session_save_by_ref(keystore, session).await?; + + Ok(plaintext) } else { Err(CryptoError::ConversationNotFound(session_id.as_bytes().into())) } } /// Encrypt a message for a session - pub fn encrypt(&mut self, session_id: &str, plaintext: &[u8]) -> CryptoResult> { + pub async fn encrypt( + &mut self, + keystore: &mut CryptoKeystore, + session_id: &str, + plaintext: &[u8], + ) -> CryptoResult> { if let Some(session) = self.session_mut(session_id) { - session.encrypt(plaintext) + let ciphertext = session.encrypt(plaintext)?; + ProteusCentral::session_save_by_ref(keystore, session).await?; + + Ok(ciphertext) } else { Err(CryptoError::ConversationNotFound(session_id.as_bytes().into())) } @@ -415,8 +439,9 @@ impl ProteusCentral { /// Encrypts a message for a list of sessions /// This is mainly used for conversations with multiple clients, this allows to minimize FFI roundtrips - pub fn encrypt_batched( + pub async fn encrypt_batched( &mut self, + keystore: &mut CryptoKeystore, sessions: &[impl AsRef], plaintext: &[u8], ) -> CryptoResult>> { @@ -424,6 +449,7 @@ impl ProteusCentral { for session_id in sessions { if let Some(session) = self.session_mut(session_id.as_ref()) { acc.insert(session.identifier.clone(), session.encrypt(plaintext)?); + ProteusCentral::session_save_by_ref(keystore, session).await?; } } Ok(acc) @@ -445,6 +471,14 @@ impl ProteusCentral { Ok(bundle) } + /// Generates a new Proteus Prekey, with an automatically auto-incremented ID. + /// + /// See [ProteusCentral::new_prekey] + pub async fn new_prekey_auto(&self, keystore: &CryptoKeystore) -> CryptoResult> { + let id = core_crypto_keystore::entities::ProteusPrekey::get_free_id(keystore).await?; + self.new_prekey(id, keystore).await + } + /// Proteus identity keypair pub fn identity(&self) -> &IdentityKeyPair { self.proteus_identity.as_ref() @@ -536,15 +570,16 @@ impl ProteusCentral { }; if let Some((kp, delete)) = identity_check { - let pk_fingerprint = kp.public_key.public_key.fingerprint(); - let pk = hex::decode(pk_fingerprint)?; + let pk = kp.public_key.public_key.as_slice().into(); let ks_identity = ProteusIdentity { - sk: kp.secret_key.to_bytes_extended().into(), + sk: kp.secret_key.as_slice().into(), pk, }; + keystore.save(ks_identity).await?; - if delete { + + if delete && legacy_identity.exists() { async_fs::remove_file(legacy_identity).await?; } @@ -577,14 +612,17 @@ impl ProteusCentral { } let raw_session = async_fs::read(session_file.path()).await?; - if Session::deserialise(&identity, &raw_session).is_ok() { - let keystore_session = ProteusSession { - id: proteus_session_id, - session: raw_session, - }; + // Session integrity check + let Ok(_) = Session::deserialise(&identity, &raw_session) else { + continue; + }; - keystore.save(keystore_session).await?; - } + let keystore_session = ProteusSession { + id: proteus_session_id, + session: raw_session, + }; + + keystore.save(keystore_session).await?; } // Prekey migration @@ -683,7 +721,7 @@ impl ProteusCentral { let pk = hex::decode(pk_fingerprint)?; let ks_identity = ProteusIdentity { - sk: kp.secret_key.to_bytes_extended().into(), + sk: kp.secret_key.as_slice().into(), pk, }; keystore.save(ks_identity).await?; @@ -875,7 +913,7 @@ mod tests { let message = b"Hello world"; - let encrypted = alice.encrypt(&session_id, message).unwrap(); + let encrypted = alice.encrypt(&mut keystore, &session_id, message).await.unwrap(); let decrypted = bob.decrypt(&session_id, &encrypted).await; assert_eq!(decrypted, message); @@ -913,8 +951,9 @@ mod tests { assert_eq!(message, decrypted.as_slice()); - let encrypted = alice.encrypt(&session_id, message).unwrap(); + let encrypted = alice.encrypt(&mut keystore, &session_id, message).await.unwrap(); let decrypted = bob.decrypt(&session_id, &encrypted).await; + assert_eq!(message, decrypted.as_slice()); keystore.wipe().await.unwrap(); @@ -941,12 +980,19 @@ mod tests { .unwrap(); let message = b"Hello world!"; + let alice_msg_envelope = alice_session.encrypt(message).unwrap(); let decrypted = bob.decrypt(&session_id, &alice_msg_envelope).await; assert_eq!(decrypted, message); alice.session_save(&mut alice_session).unwrap(); + let encrypted = bob.encrypt(&session_id, &message[..]); + let decrypted = alice_session.decrypt(&encrypted).unwrap(); + assert_eq!(decrypted, message); + + alice.session_save(&mut alice_session).unwrap(); + drop(alice); let keystore_dir = tempfile::tempdir().unwrap(); @@ -967,13 +1013,13 @@ mod tests { assert_eq!(proteus_central.fingerprint(), alice_fingerprint); // Session integrity check - let session = proteus_central.session_mut(&session_id).unwrap(); + let alice_new_session = proteus_central.session_mut(&session_id).unwrap(); assert_eq!( - session.session.local_identity().fingerprint(), + alice_new_session.session.local_identity().fingerprint(), alice_session.fingerprint_local() ); assert_eq!( - session.session.remote_identity().fingerprint(), + alice_new_session.session.remote_identity().fingerprint(), alice_session.fingerprint_remote() ); @@ -987,18 +1033,23 @@ mod tests { ); // Make sure ProteusCentral can still keep communicating with bob - let encrypted = proteus_central.encrypt(&session_id, &message[..]).unwrap(); + let encrypted = proteus_central + .encrypt(&mut keystore, &session_id, &message[..]) + .await + .unwrap(); let decrypted = bob.decrypt(&session_id, &encrypted).await; assert_eq!(&decrypted, &message[..]); - // FIXME: Known bug, see CL-110 - // let encrypted = bob.encrypt(&session_id, &message[..]); - // let decrypted = proteus_central - // .decrypt(&mut keystore, &session_id, &encrypted) - // .await - // .unwrap(); - // assert_eq!(&decrypted, &message[..]); + // CL-110 assertion + let encrypted = bob.encrypt(&session_id, &message[..]); + let decrypted = proteus_central + .decrypt(&mut keystore, &session_id, &encrypted) + .await + .unwrap(); + assert_eq!(&decrypted, &message[..]); + + proteus_central.session_save(&keystore, &session_id).await.unwrap(); keystore.wipe().await.unwrap(); } @@ -1156,14 +1207,12 @@ mod tests { // Make sure ProteusCentral can still keep communicating with bob - let encrypted = proteus_central.encrypt(&session_id, &message[..]).unwrap(); + let encrypted = proteus_central.encrypt(&mut keystore, &session_id, &message[..]).await.unwrap(); let decrypted = bob.decrypt(&session_id, &encrypted).await; assert_eq!(&decrypted, &message[..]); - // FIXME: Known bug, see CL-110 - // This is passing for now because the keys / prekeys are generated using proteus 2.0, - // which seems to not trigger the bug + // CL-110 assertion let encrypted = bob.encrypt(&session_id, &message[..]); let decrypted = proteus_central .decrypt(&mut keystore, &session_id, &encrypted) diff --git a/interop/Cargo.toml b/interop/Cargo.toml index 18ad3a9a36..1406b7d91c 100644 --- a/interop/Cargo.toml +++ b/interop/Cargo.toml @@ -49,11 +49,10 @@ fantoccini = "0.19" [dependencies.proteus-wasm] version = "2.0" -package = "proteus" features = ["hazmat"] optional = true git = "https://github.com/wireapp/proteus" -branch = "otak/oxidize-wasm-v2" +branch = "otak/2.0" diff --git a/interop/src/clients/corecrypto/native.rs b/interop/src/clients/corecrypto/native.rs index bb67859396..273d0ebd2b 100644 --- a/interop/src/clients/corecrypto/native.rs +++ b/interop/src/clients/corecrypto/native.rs @@ -149,7 +149,7 @@ impl crate::clients::EmulatedProteusClient for CoreCryptoNativeClient { } async fn encrypt(&mut self, session_id: &str, plaintext: &[u8]) -> Result> { - Ok(self.cc.proteus_encrypt(session_id, plaintext)?) + Ok(self.cc.proteus_encrypt(session_id, plaintext).await?) } async fn decrypt(&mut self, session_id: &str, ciphertext: &[u8]) -> Result> { diff --git a/interop/src/main.rs b/interop/src/main.rs index 60106440c7..42214bf489 100644 --- a/interop/src/main.rs +++ b/interop/src/main.rs @@ -317,7 +317,9 @@ async fn run_proteus_test(chrome_driver_addr: &std::net::SocketAddr) -> Result<( prng.fill_bytes(&mut message); - let mut messages_to_decrypt = master_client.proteus_encrypt_batched(&master_sessions, &message)?; + let mut messages_to_decrypt = master_client + .proteus_encrypt_batched(&master_sessions, &message) + .await?; for c in clients.iter_mut() { let fingerprint = c.fingerprint().await?; diff --git a/keystore/Cargo.toml b/keystore/Cargo.toml index 50bb6c2443..b6c8008414 100644 --- a/keystore/Cargo.toml +++ b/keystore/Cargo.toml @@ -47,7 +47,7 @@ openmls_traits = { version = "0.1", optional = true, features = ["single-threade optional = true package = "proteus-traits" git = "https://github.com/wireapp/proteus" -branch = "otak/oxidize-wasm-v2" +branch = "otak/2.0" [target.'cfg(not(target_family = "wasm"))'.dependencies] async-fs = "1.5" @@ -100,9 +100,8 @@ futures-lite = "1.12" [dev-dependencies.proteus-wasm] version = "2.0" -package = "proteus" git = "https://github.com/wireapp/proteus" -branch = "otak/oxidize-wasm-v2" +branch = "otak/2.0" [target.'cfg(not(target_family = "wasm"))'.dev-dependencies.criterion] version = "0.3" diff --git a/keystore/src/entities/proteus.rs b/keystore/src/entities/proteus.rs index 05d46d663e..5c97645f5f 100644 --- a/keystore/src/entities/proteus.rs +++ b/keystore/src/entities/proteus.rs @@ -30,12 +30,14 @@ impl ProteusIdentity { pub fn sk_raw(&self) -> zeroize::Zeroizing<[u8; Self::SK_KEY_SIZE]> { let mut slice = zeroize::Zeroizing::new([0u8; Self::SK_KEY_SIZE]); + debug_assert_eq!(self.sk.len(), Self::SK_KEY_SIZE); slice.copy_from_slice(&self.sk[..Self::SK_KEY_SIZE]); slice } pub fn pk_raw(&self) -> zeroize::Zeroizing<[u8; Self::PK_KEY_SIZE]> { let mut slice = zeroize::Zeroizing::new([0u8; Self::PK_KEY_SIZE]); + debug_assert_eq!(self.pk.len(), Self::PK_KEY_SIZE); slice.copy_from_slice(&self.pk[..Self::PK_KEY_SIZE]); slice } @@ -78,6 +80,11 @@ impl ProteusPrekey { self.id = id; self.id_bytes = self.id.to_le_bytes().into(); } + + pub async fn get_free_id(conn: &crate::Connection) -> crate::CryptoKeystoreResult { + let count = conn.count::().await?; + Ok((count % u16::MAX as usize) as u16) + } } #[derive(Debug, Clone, Zeroize, PartialEq, Eq)]