Closed poljar closed 3 months ago
jplatte
commented
6 months ago 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.
poljar
commented
6 months ago 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.
uhoreg
commented
6 months ago 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.)
poljar
commented
6 months ago 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.
User identities and devices are internally using a
serde_json::Value, this means that, if theValueisn't empty,rmp_serdewill 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.