From 5d716f969d795823d463e40da5fee95eec1ea03c Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Wed, 3 Jul 2024 09:42:45 +0100 Subject: [PATCH] Stores: Remove `StoreCipher::{en,de}crypt_value_[base64_]typed` (#3638) * Use `IndexeddbSerializer` more widely in test code reuse `IndexeddbSerializer::maybe_encrypt_value` instead of re-inventing it. * Rewrite `StoreCipher::decrypt_value_base64_typed` Instead of un-base64-ing and calling `decrypt_value_typed` (which deserializes the result`), call `decrypt_value_base64_data` (which un-base64s before decrypting but does not deserialize the result), then deserialize. This makes it more symmetrical with `encrypt_value_base64_typed`, and helps me get rid of `decrypt_value_typed` (which is barely used.) * Fix docs on `StoreCipher::encrypt_value_base64_typed` looks like they got C&Ped from `encrypt_value_typed`. * Inline `StoreCipher::{encrypt,decrypt}_value_typed` Each of these are quite simple, are only used in two places, and their existence melts my brain. * Rewrite `IndexeddbSerializer::maybe_{encrypt,decrypt}_value` ... to use `en/decrypt_value_base64_data` instead of `en/decrypt_value_base64_typed`. We have to have the de/serialization code for the unencrypted case anyway, so using the higher-level method isn't helping us much. * Remove unused `StoreCipher::{en,de}crypt_value_base64_typed` Outside of tests, these things are totally unused. --- .../src/crypto_store/indexeddb_serializer.rs | 37 ++-- .../src/crypto_store/migrations/mod.rs | 14 +- .../src/state_store/mod.rs | 13 +- crates/matrix-sdk-store-encryption/src/lib.rs | 164 +----------------- 4 files changed, 48 insertions(+), 180 deletions(-) diff --git a/crates/matrix-sdk-indexeddb/src/crypto_store/indexeddb_serializer.rs b/crates/matrix-sdk-indexeddb/src/crypto_store/indexeddb_serializer.rs index 46ac7a51b..39fd84f84 100644 --- a/crates/matrix-sdk-indexeddb/src/crypto_store/indexeddb_serializer.rs +++ b/crates/matrix-sdk-indexeddb/src/crypto_store/indexeddb_serializer.rs @@ -25,6 +25,7 @@ use matrix_sdk_store_encryption::{EncryptedValueBase64, StoreCipher}; use serde::{de::DeserializeOwned, Deserialize, Serialize}; use wasm_bindgen::JsValue; use web_sys::IdbKeyRange; +use zeroize::Zeroizing; use crate::{safe_encode::SafeEncode, IndexeddbCryptoStoreError}; @@ -145,18 +146,29 @@ impl IndexeddbSerializer { } } - /// Encode an InboundGroupSession for storage as a value in indexeddb. + /// Encode an object for storage as a value in indexeddb. + /// + /// First serializes the object as JSON bytes. + /// + /// Then, if a cipher is set, encrypts the JSON with a nonce into binary + /// blobs, and base64-encodes the blobs. + /// + /// If no cipher is set, just base64-encodes the JSON bytes. + /// + /// Finally, returns an object encapsulating the result. pub fn maybe_encrypt_value( &self, value: PickledInboundGroupSession, ) -> Result { + // First serialize the object as JSON. + let serialized = serde_json::to_vec(&value).map_err(CryptoStoreError::backend)?; + + // Then either encrypt the JSON, or just base64-encode it. Ok(match &self.store_cipher { Some(cipher) => MaybeEncrypted::Encrypted( - cipher.encrypt_value_base64_typed(&value).map_err(CryptoStoreError::backend)?, - ), - None => MaybeEncrypted::Unencrypted( - BASE64.encode(serde_json::to_vec(&value).map_err(CryptoStoreError::backend)?), + cipher.encrypt_value_base64_data(serialized).map_err(CryptoStoreError::backend)?, ), + None => MaybeEncrypted::Unencrypted(BASE64.encode(serialized)), }) } @@ -198,16 +210,19 @@ impl IndexeddbSerializer { &self, value: MaybeEncrypted, ) -> Result { - match (&self.store_cipher, value) { + // First extract the plaintext JSON, either by decrypting or un-base64-ing. + let plaintext = Zeroizing::new(match (&self.store_cipher, value) { (Some(cipher), MaybeEncrypted::Encrypted(enc)) => { - cipher.decrypt_value_base64_typed(enc).map_err(CryptoStoreError::backend) + cipher.decrypt_value_base64_data(enc).map_err(CryptoStoreError::backend)? } (None, MaybeEncrypted::Unencrypted(unc)) => { - Ok(serde_json::from_slice(&BASE64.decode(unc).map_err(CryptoStoreError::backend)?) - .map_err(CryptoStoreError::backend)?) + BASE64.decode(unc).map_err(CryptoStoreError::backend)? } - _ => Err(CryptoStoreError::UnpicklingError), - } + _ => return Err(CryptoStoreError::UnpicklingError), + }); + + // Then deserialize the JSON. + Ok(serde_json::from_slice(&plaintext)?) } } diff --git a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs index 285c37c50..091ebc7a0 100644 --- a/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs +++ b/crates/matrix-sdk-indexeddb/src/crypto_store/migrations/mod.rs @@ -235,8 +235,8 @@ mod tests { /// Make lots of sessions and see how long it takes to count them in v10 #[async_test] async fn count_lots_of_sessions_v10() { - let cipher = Arc::new(StoreCipher::new().unwrap()); - let serializer = IndexeddbSerializer::new(Some(cipher.clone())); + let serializer = IndexeddbSerializer::new(Some(Arc::new(StoreCipher::new().unwrap()))); + // Session keys are slow to create, so make one upfront and use it for every // session let session_key = create_session_key(); @@ -244,9 +244,7 @@ mod tests { // Create lots of InboundGroupSessionIndexedDbObject objects let mut objects = Vec::with_capacity(NUM_RECORDS_FOR_PERF); for i in 0..NUM_RECORDS_FOR_PERF { - objects.push( - create_inbound_group_sessions3_record(i, &session_key, &cipher, &serializer).await, - ); + objects.push(create_inbound_group_sessions3_record(i, &session_key, &serializer).await); } // Create a DB with an inbound_group_sessions3 store @@ -334,15 +332,13 @@ mod tests { async fn create_inbound_group_sessions3_record( i: usize, session_key: &SessionKey, - cipher: &Arc, serializer: &IndexeddbSerializer, ) -> (JsValue, JsValue) { let session = create_inbound_group_session(i, session_key); let pickled_session = session.pickle().await; + let session_dbo = InboundGroupSessionIndexedDbObject { - pickled_session: MaybeEncrypted::Encrypted( - cipher.encrypt_value_base64_typed(&pickled_session).unwrap(), - ), + pickled_session: serializer.maybe_encrypt_value(pickled_session).unwrap(), needs_backup: false, backed_up_to: -1, }; diff --git a/crates/matrix-sdk-indexeddb/src/state_store/mod.rs b/crates/matrix-sdk-indexeddb/src/state_store/mod.rs index 8a0a6a45a..a9a7b3fa6 100644 --- a/crates/matrix-sdk-indexeddb/src/state_store/mod.rs +++ b/crates/matrix-sdk-indexeddb/src/state_store/mod.rs @@ -150,7 +150,10 @@ pub use keys::ALL_STORES; /// Encrypt (if needs be) then JSON-serialize a value. fn serialize_value(store_cipher: Option<&StoreCipher>, event: &impl Serialize) -> Result { Ok(match store_cipher { - Some(cipher) => JsValue::from_serde(&cipher.encrypt_value_typed(event)?)?, + Some(cipher) => { + let data = serde_json::to_vec(event)?; + JsValue::from_serde(&cipher.encrypt_value_data(data)?)? + } None => JsValue::from_serde(event)?, }) } @@ -161,7 +164,13 @@ fn deserialize_value( event: &JsValue, ) -> Result { match store_cipher { - Some(cipher) => Ok(cipher.decrypt_value_typed(event.into_serde()?)?), + Some(cipher) => { + use zeroize::Zeroize; + let mut plaintext = cipher.decrypt_value_data(event.into_serde()?)?; + let ret = serde_json::from_slice(&plaintext); + plaintext.zeroize(); + Ok(ret?) + } None => Ok(event.into_serde()?), } } diff --git a/crates/matrix-sdk-store-encryption/src/lib.rs b/crates/matrix-sdk-store-encryption/src/lib.rs index 55792a885..78947d96a 100644 --- a/crates/matrix-sdk-store-encryption/src/lib.rs +++ b/crates/matrix-sdk-store-encryption/src/lib.rs @@ -418,45 +418,8 @@ impl StoreCipher { /// # anyhow::Ok(()) }; /// ``` pub fn encrypt_value(&self, value: &impl Serialize) -> Result, Error> { - Ok(serde_json::to_vec(&self.encrypt_value_typed(value)?)?) - } - - /// Encrypt a value before it is inserted into the key/value store. - /// - /// A value can be decrypted using the - /// [`StoreCipher::decrypt_value_typed()`] method. This is the lower - /// level function to `encrypt_value`, but returns the - /// full `EncryptdValue`-type - /// - /// # Arguments - /// - /// * `value` - A value that should be encrypted, any value that implements - /// `Serialize` can be given to this method. The value will be serialized - /// as json before it is encrypted. - /// - /// - /// # Examples - /// - /// ```no_run - /// # let example = || { - /// use matrix_sdk_store_encryption::StoreCipher; - /// use serde_json::{json, value::Value}; - /// - /// let store_cipher = StoreCipher::new()?; - /// - /// let value = json!({ - /// "some": "data", - /// }); - /// - /// let encrypted = store_cipher.encrypt_value_typed(&value)?; - /// let decrypted: Value = store_cipher.decrypt_value_typed(encrypted)?; - /// - /// assert_eq!(value, decrypted); - /// # anyhow::Ok(()) }; - /// ``` - pub fn encrypt_value_typed(&self, value: &impl Serialize) -> Result { let data = serde_json::to_vec(value)?; - self.encrypt_value_data(data) + Ok(serde_json::to_vec(&self.encrypt_value_data(data)?)?) } /// Encrypt some data before it is inserted into the key/value store. @@ -497,47 +460,6 @@ impl StoreCipher { Ok(EncryptedValue { version: VERSION, ciphertext, nonce }) } - /// Encrypt a value before it is inserted into the key/value store. - /// - /// A value can be decrypted using the - /// [`StoreCipher::decrypt_value_typed()`] method. This is the lower - /// level function to `encrypt_value`, but returns the - /// full `EncryptdValue`-type - /// - /// # Arguments - /// - /// * `value` - A value that should be encrypted, any value that implements - /// `Serialize` can be given to this method. The value will be serialized - /// as json before it is encrypted. - /// - /// - /// # Examples - /// - /// ```no_run - /// # let example = || { - /// use matrix_sdk_store_encryption::StoreCipher; - /// use serde_json::{json, value::Value}; - /// - /// let store_cipher = StoreCipher::new()?; - /// - /// let value = json!({ - /// "some": "data", - /// }); - /// - /// let encrypted = store_cipher.encrypt_value_typed(&value)?; - /// let decrypted: Value = store_cipher.decrypt_value_typed(encrypted)?; - /// - /// assert_eq!(value, decrypted); - /// # anyhow::Ok(()) }; - /// ``` - pub fn encrypt_value_base64_typed( - &self, - value: &impl Serialize, - ) -> Result { - let data = serde_json::to_vec(value)?; - self.encrypt_value_base64_data(data) - } - /// Encrypt some data before it is inserted into the key/value store, /// using base64 for arrays of integers. /// @@ -603,89 +525,12 @@ impl StoreCipher { /// ``` pub fn decrypt_value(&self, value: &[u8]) -> Result { let value: EncryptedValue = serde_json::from_slice(value)?; - self.decrypt_value_typed(value) - } - - /// Decrypt a value after it was fetched from the key/value store. - /// - /// A value can be encrypted using the - /// [`StoreCipher::encrypt_value_typed()`] method. Lower level method to - /// [`StoreCipher::decrypt_value_typed()`] - /// - /// # Arguments - /// - /// * `value` - The EncryptedValue of a value that should be decrypted. - /// - /// The method will deserialize the decrypted value into the expected type. - /// - /// # Examples - /// - /// ``` - /// # let example = || { - /// use matrix_sdk_store_encryption::StoreCipher; - /// use serde_json::{json, value::Value}; - /// - /// let store_cipher = StoreCipher::new()?; - /// - /// let value = json!({ - /// "some": "data", - /// }); - /// - /// let encrypted = store_cipher.encrypt_value_typed(&value)?; - /// let decrypted: Value = store_cipher.decrypt_value_typed(encrypted)?; - /// - /// assert_eq!(value, decrypted); - /// # anyhow::Ok(()) }; - /// ``` - pub fn decrypt_value_typed( - &self, - value: EncryptedValue, - ) -> Result { let mut plaintext = self.decrypt_value_data(value)?; let ret = serde_json::from_slice(&plaintext); plaintext.zeroize(); Ok(ret?) } - /// Decrypt a base64-encoded value after it was fetched from the key/value - /// store. - /// - /// A value can be encrypted using the - /// [`StoreCipher::encrypt_value_base64_typed()`] method. - /// - /// # Arguments - /// - /// * `value` - The EncryptedValueBase64 of a value that should be - /// decrypted. - /// - /// The method will deserialize the decrypted value into the expected type. - /// - /// # Examples - /// - /// ``` - /// # let example = || { - /// use matrix_sdk_store_encryption::StoreCipher; - /// use serde_json::{json, value::Value}; - /// - /// let store_cipher = StoreCipher::new()?; - /// - /// let value = json!({ - /// "some": "data", - /// }); - /// - /// let encrypted = store_cipher.encrypt_value_base64_typed(&value)?; - /// let decrypted: Value = store_cipher.decrypt_value_base64_typed(encrypted)?; - /// - /// assert_eq!(value, decrypted); - /// # anyhow::Ok(()) }; - /// ``` - pub fn decrypt_value_base64_typed( - &self, - value: EncryptedValueBase64, - ) -> Result { - self.decrypt_value_typed(value.try_into()?) - } - /// Decrypt a base64-encoded value after it was fetched from the key/value /// store. /// @@ -1100,8 +945,11 @@ mod tests { let store_cipher = StoreCipher::new()?; - let encrypted = store_cipher.encrypt_value_base64_typed(&event)?; - let decrypted: Value = store_cipher.decrypt_value_base64_typed(encrypted)?; + let data = serde_json::to_vec(&event)?; + let encrypted = store_cipher.encrypt_value_base64_data(data)?; + + let plaintext = store_cipher.decrypt_value_base64_data(encrypted)?; + let decrypted: Value = serde_json::from_slice(&plaintext)?; assert_eq!(event, decrypted);