matrix-org
The SQLite store serializes user identities and devices using rmp_serde instead of serde_json
User identities and devices are internally using a serde_json::Value, this means that, if the Value isn't empty, rmp_serde will fail to with a cryptic error message.
The interesting part is, as far as my memory serves me, that serialization will succeed, but deserialization will fail. This will lead to UTDs for such devices and incorrect trust chain calculation for the user identities.
Currently this isn't a problem since the Value, which is used to gather unknown additional data, is always empty.
The offending lines can be found here:
https://github.com/matrix-org/matrix-rust-sdk/blob/315a29f5684cea68cef61775e27127fc7387cc2b/crates/matrix-sdk-sqlite/src/crypto_store.rs#L804
https://github.com/matrix-org/matrix-rust-sdk/blob/315a29f5684cea68cef61775e27127fc7387cc2b/crates/matrix-sdk-sqlite/src/crypto_store.rs#L816
This issue is similar to this commit c73aeef2.
While the fix is trivial, the problematic part here will be the data migration.
The interesting part is, as far as my memory serves me, that serialization will succeed, but deserialization will fail.
Is serde_json's RawValue involved in the deserialization path? I don't think this statement is true in isolation.
Hmm, I don't think it is. Only serde_json::Value to handle the custom stuff that might appear in the DeviceKeys so signature verification continues to work.
I modified a test to check if this is working with this patch:
diff --git a/crates/matrix-sdk-crypto/src/store/integration_tests.rs b/crates/matrix-sdk-crypto/src/store/integration_tests.rs
index 9373300ef..6caa3c2cd 100644
--- a/crates/matrix-sdk-crypto/src/store/integration_tests.rs
+++ b/crates/matrix-sdk-crypto/src/store/integration_tests.rs
@@ -18,6 +18,7 @@ macro_rules! cryptostore_integration_tests {
user_id, DeviceId, JsOption, OwnedDeviceId, OwnedUserId, RoomId, TransactionId, UserId
};
use serde_json::value::to_raw_value;
+ use serde_json::json;
use $crate::{
olm::{
Curve25519PublicKey, InboundGroupSession, OlmMessageHash,
@@ -40,9 +41,10 @@ macro_rules! cryptostore_integration_tests {
secret_send::SecretSendContent,
ToDeviceEvent,
},
+ DeviceKeys,
EventEncryptionAlgorithm,
},
- ReadOnlyDevice, SecretInfo, ToDeviceRequest, TrackedUser, GossippedSecret,
+ LocalTrust, ReadOnlyDevice, SecretInfo, ToDeviceRequest, TrackedUser, GossippedSecret,
};
use super::get_store;
@@ -461,9 +463,15 @@ macro_rules! cryptostore_integration_tests {
"SECONDDEVICE".into(),
));
+ let bob_device_1_keys: DeviceKeys = serde_json::from_str(
+ r#"{"algorithms": ["m.olm.v1.curve25519-aes-sha2", "m.megolm.v1.aes-sha2"], "user_id": "@bob:localhost", "device_id": "BOBDEVICE", "extra_property": "somevalue", "keys": {"curve25519:BOBDEVICE": "n0zs7qnaPLLf/OTL+dDLcI5kaPexbUeQ8jLQ2q6sO0E", "ed25519:BOBDEVICE": "RrKiu4+5EHRBWY6Qj6OtQGC0txpmEeanOz2irEZ/IN4"}, "signatures": {"@bob:localhost": {"ed25519:BOBDEVICE": "9NjPewVHfB7Ah32mJ+CBx64mVoiQ8gbh+/2pc9WfAgut/H0Kqd/bbpgJq9Pn518szaXcGqEq0DxDP6CABBX8CQ"}}}"#
+ //r#"{"algorithms": ["m.olm.v1.curve25519-aes-sha2", "m.megolm.v1.aes-sha2"], "user_id": "@bob:localhost", "device_id": "BOBDEVICE", "keys": {"curve25519:BOBDEVICE": "EMSDISiUm0Lik2PTbbm+AutCpYWn2dECImGgyomyiDE", "ed25519:BOBDEVICE": "nOcHxuN38kyvROsdQlp+mwe1oaQHkFLClDvnDpToyps"}, "signatures": {"@bob:localhost": {"ed25519:BOBDEVICE": "TXQcfHkpQzVSzj0ulYF1WNKMgI9mcFq9OpHuDDLOLcmMWg78J9liFg124n99nSpzpNq546KcqMfQpdT7w9eWBg"}}}"#
+ ).unwrap();
+ let bob_device_1 = ReadOnlyDevice::new(bob_device_1_keys, LocalTrust::Unset);
+
let changes = Changes {
devices: DeviceChanges {
- new: vec![alice_device_1.clone(), alice_device_2.clone()],
+ new: vec![alice_device_1.clone(), alice_device_2.clone(), bob_device_1.clone()],
..Default::default()
},
..Default::default()
@@ -493,6 +501,14 @@ macro_rules! cryptostore_integration_tests {
let user_devices = store.get_user_devices(alice_device_1.user_id()).await.unwrap();
assert_eq!(user_devices.len(), 2);
+
+ let bob_device = store
+ .get_device(bob_device_1.user_id(), bob_device_1.device_id())
+ .await
+ .unwrap();
+
+ let bob_device_json = serde_json::to_value(bob_device).unwrap();
+ assert_eq!(bob_device_json["inner"]["extra_property"], json!("somevalue"));
}
#[async_test]
and running cargo test -F crypto-store in crates/matrix-sdk-sqlite.
The device key was created using this Python script:
from canonicaljson import encode_canonical_json
import vodozemac
account = vodozemac.Account()
device_keys = {
"algorithms": [
"m.olm.v1.curve25519-aes-sha2",
"m.megolm.v1.aes-sha2",
],
"user_id": "@bob:localhost",
"device_id": "BOBDEVICE",
"extra_property": "somevalue",
"keys": {
"curve25519:BOBDEVICE": account.curve25519_key,
"ed25519:BOBDEVICE": account.ed25519_key,
},
}
signature = account.sign(encode_canonical_json(device_keys).decode("utf-8"))
device_keys["signatures"] = {
"@bob:localhost": {
"ed25519:BOBDEVICE": signature
}
}
print(device_keys)
Since the test passes, I think that this means that we are OK. Can someone double-check to make sure that the test is checking the right thing? (I did make sure that the test is running, by changing the test so that it failed, and making sure that cargo test ... failed.)
Yup, seems like this is a false alarm. Could you please upstream the test, albeit we could use the json macro to make the device keys a bit more readable, i.e.
diff --git a/crates/matrix-sdk-crypto/src/store/integration_tests.rs b/crates/matrix-sdk-crypto/src/store/integration_tests.rs
index 9373300ef..f8de21307 100644
--- a/crates/matrix-sdk-crypto/src/store/integration_tests.rs
+++ b/crates/matrix-sdk-crypto/src/store/integration_tests.rs
@@ -18,6 +18,7 @@ macro_rules! cryptostore_integration_tests {
user_id, DeviceId, JsOption, OwnedDeviceId, OwnedUserId, RoomId, TransactionId, UserId
};
use serde_json::value::to_raw_value;
+ use serde_json::json;
use $crate::{
olm::{
Curve25519PublicKey, InboundGroupSession, OlmMessageHash,
@@ -40,9 +41,10 @@ macro_rules! cryptostore_integration_tests {
secret_send::SecretSendContent,
ToDeviceEvent,
},
+ DeviceKeys,
EventEncryptionAlgorithm,
},
- ReadOnlyDevice, SecretInfo, ToDeviceRequest, TrackedUser, GossippedSecret,
+ LocalTrust, ReadOnlyDevice, SecretInfo, ToDeviceRequest, TrackedUser, GossippedSecret,
};
use super::get_store;
@@ -461,9 +463,35 @@ macro_rules! cryptostore_integration_tests {
"SECONDDEVICE".into(),
));
+ let json = json!({
+ "algorithms": vec![
+ "m.olm.v1.curve25519-aes-sha2",
+ "m.megolm.v1.aes-sha2"
+ ],
+ "device_id": "BNYQQWUMXO",
+ "user_id": "@example:localhost",
+ "keys": {
+ "curve25519:BNYQQWUMXO": "xfgbLIC5WAl1OIkpOzoxpCe8FsRDT6nch7NQsOb15nc",
+ "ed25519:BNYQQWUMXO": "2/5LWJMow5zhJqakV88SIc7q/1pa8fmkfgAzx72w9G4"
+ },
+ "signatures": {
+ "@example:localhost": {
+ "ed25519:BNYQQWUMXO": "kTwMrbsLJJM/uFGOj/oqlCaRuw7i9p/6eGrTlXjo8UJMCFAetoyWzoMcF35vSe4S6FTx8RJmqX6rM7ep53MHDQ"
+ }
+ },
+ "unsigned": {
+ "device_display_name": "Alice's mobile phone",
+ "other_data": "other_value"
+ },
+ "other_data": "other_value"
+ });
+
+ let bob_device_1_keys: DeviceKeys = serde_json::from_value(json).unwrap();
+ let bob_device_1 = ReadOnlyDevice::new(bob_device_1_keys, LocalTrust::Unset);
+
let changes = Changes {
devices: DeviceChanges {
- new: vec![alice_device_1.clone(), alice_device_2.clone()],
+ new: vec![alice_device_1.clone(), alice_device_2.clone(), bob_device_1.clone()],
..Default::default()
},
..Default::default()
@@ -493,6 +521,14 @@ macro_rules! cryptostore_integration_tests {
let user_devices = store.get_user_devices(alice_device_1.user_id()).await.unwrap();
assert_eq!(user_devices.len(), 2);
+
+ let bob_device = store
+ .get_device(bob_device_1.user_id(), bob_device_1.device_id())
+ .await
+ .unwrap();
+
+ let bob_device_json = serde_json::to_value(bob_device).unwrap();
+ assert_eq!(bob_device_json["inner"]["other_data"], json!("other_value"));
}
#[async_test]
This turned out to be a non-issue in the end, tests confirming this have been added.
Closed by https://github.com/matrix-org/matrix-rust-sdk/issues/3132#issuecomment-2002060930.