Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configurable sender trust checking on decrypting #3701

Closed
wants to merge 41 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
bb7383d
feature(crypto): Add support for master key local pinning
BillCarsonFr Jul 2, 2024
e08dfab
initial decryption implementation
uhoreg Jul 11, 2024
9415308
add test for trust requirement
uhoreg Jul 15, 2024
67af31a
feature(crypto): Add support for master key local pinning
BillCarsonFr Jul 2, 2024
8253618
Review: Do not use msk abbreviation
BillCarsonFr Jul 17, 2024
b7241c3
Review: Use TryFrom instead of From for serializer helper struct
BillCarsonFr Jul 17, 2024
4d66828
Review: Fix typo
BillCarsonFr Jul 17, 2024
c045d26
Review: Improve doc
BillCarsonFr Jul 17, 2024
bfb8818
Review: Fix data set comments
BillCarsonFr Jul 17, 2024
fb39a9e
Review: Improve comments
BillCarsonFr Jul 17, 2024
1d4cefb
Clippy | unneeded clone
BillCarsonFr Jul 22, 2024
de60e6e
refactor: Rename serializer helper to match new OtherUserIdentityData
BillCarsonFr Jul 22, 2024
3731190
implement logic for changing cross-signing keys, and add test
uhoreg Jul 22, 2024
7803617
crypto: refactor extract detection of removed device from outbound
BillCarsonFr Jul 3, 2024
9b00de3
refactor: Move user loop inside strategy specific branch
BillCarsonFr Jul 3, 2024
e309219
feat(crypto): Key distribution errors for Pin violations
BillCarsonFr Jul 8, 2024
945f844
Merge remote-tracking branch 'origin/valere/invisible_crypto/identity…
uhoreg Jul 23, 2024
f0e9a18
remove debugging statements
uhoreg Jul 23, 2024
7254e7e
add some comments
uhoreg Jul 23, 2024
3f7b840
cargo fmt
uhoreg Jul 24, 2024
ee9edf9
make other crates compile
uhoreg Jul 31, 2024
62531c8
address some review comments
uhoreg Aug 2, 2024
b8b8c8b
fix test failure
uhoreg Aug 7, 2024
a09ede9
add ChangedIdentity to VerificationLevel and use that for sender iden…
uhoreg Aug 7, 2024
73300aa
factor out test utilities
uhoreg Aug 8, 2024
0a8acce
Merge branch 'main' into invisible_crypto/decrypt
uhoreg Aug 9, 2024
b0994a5
revert change to Cargo.lock
uhoreg Aug 9, 2024
c53674b
remove VerifiedUserIdentity requirement -- not used for now
uhoreg Aug 9, 2024
773c8fe
move decryption settings types into new file
uhoreg Aug 9, 2024
4cbfd6b
add doc and fix compilation of other crates
uhoreg Aug 9, 2024
65ba067
lint
uhoreg Aug 9, 2024
e85bf3b
fix bindings and more linting
uhoreg Aug 9, 2024
14d1e07
fix type
uhoreg Aug 9, 2024
e5c0c06
more lint
uhoreg Aug 9, 2024
24a3ebb
only error on identity change if sender was previously verified
uhoreg Aug 14, 2024
a119d79
fix logic and fix test
uhoreg Aug 14, 2024
fa61076
Merge branch 'main' into invisible_crypto/decrypt
uhoreg Aug 15, 2024
368699a
fix clippy error
uhoreg Aug 15, 2024
0a1f80b
use the right punctuation
uhoreg Aug 15, 2024
04c81e3
fmt
uhoreg Aug 15, 2024
0e9b581
add/fix comments and add changelog
uhoreg Aug 15, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion bindings/matrix-sdk-crypto-ffi/src/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use matrix_sdk_crypto::{
decrypt_room_key_export, encrypt_room_key_export,
olm::ExportedRoomKey,
store::{BackupDecryptionKey, Changes},
types::decryption::{DecryptionSettings, TrustRequirement},
LocalTrust, OlmMachine as InnerMachine, ToDeviceRequest, UserIdentities,
};
use ruma::{
Expand Down Expand Up @@ -884,7 +885,13 @@ impl OlmMachine {
let event: Raw<_> = serde_json::from_str(&event)?;
let room_id = RoomId::parse(room_id)?;

let decrypted = self.runtime.block_on(self.inner.decrypt_room_event(&event, &room_id))?;
let decryption_settings =
DecryptionSettings { trust_requirement: TrustRequirement::Untrusted };
let decrypted = self.runtime.block_on(self.inner.decrypt_room_event(
&event,
&room_id,
&decryption_settings,
))?;

if handle_verification_events {
if let Ok(AnyTimelineEvent::MessageLike(e)) = decrypted.event.deserialize() {
Expand Down
10 changes: 7 additions & 3 deletions crates/matrix-sdk-base/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,10 @@ use futures_util::Stream;
use matrix_sdk_common::instant::Instant;
#[cfg(feature = "e2e-encryption")]
use matrix_sdk_crypto::{
store::DynCryptoStore, CollectStrategy, EncryptionSettings, EncryptionSyncChanges, OlmError,
OlmMachine, ToDeviceRequest,
store::DynCryptoStore,
types::decryption::{DecryptionSettings, TrustRequirement},
CollectStrategy, EncryptionSettings, EncryptionSyncChanges, OlmError, OlmMachine,
ToDeviceRequest,
};
#[cfg(feature = "e2e-encryption")]
use ruma::events::{
Expand Down Expand Up @@ -298,8 +300,10 @@ impl BaseClient {
let olm = self.olm_machine().await;
let Some(olm) = olm.as_ref() else { return Ok(None) };

let decryption_settings =
DecryptionSettings { trust_requirement: TrustRequirement::Untrusted };
let event: SyncTimelineEvent =
olm.decrypt_room_event(event.cast_ref(), room_id).await?.into();
olm.decrypt_room_event(event.cast_ref(), room_id, &decryption_settings).await?.into();

if let Ok(AnySyncTimelineEvent::MessageLike(e)) = event.event.deserialize() {
match &e {
Expand Down
36 changes: 27 additions & 9 deletions crates/matrix-sdk-common/src/deserialized_responses.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use ruma::{
DeviceKeyAlgorithm, OwnedDeviceId, OwnedEventId, OwnedUserId,
};
use serde::{Deserialize, Serialize};
use thiserror::Error;

use crate::debug::{DebugRawEvent, DebugStructExt};

Expand All @@ -30,6 +31,7 @@ const UNVERIFIED_IDENTITY: &str = "Encrypted by an unverified user.";
const UNSIGNED_DEVICE: &str = "Encrypted by a device not verified by its owner.";
const UNKNOWN_DEVICE: &str = "Encrypted by an unknown or deleted device.";
pub const SENT_IN_CLEAR: &str = "Not encrypted.";
const PREVIOUSLY_VERIFIED: &str = "Encrypted by a previously-verified user.";

/// Represents the state of verification for a decrypted message sent by a
/// device.
Expand Down Expand Up @@ -88,12 +90,12 @@ impl VerificationState {
match self {
VerificationState::Verified => ShieldState::None,
VerificationState::Unverified(level) => match level {
VerificationLevel::UnverifiedIdentity | VerificationLevel::UnsignedDevice => {
ShieldState::Red {
code: ShieldStateCode::UnverifiedIdentity,
message: UNVERIFIED_IDENTITY,
}
}
VerificationLevel::UnverifiedIdentity
| VerificationLevel::PreviouslyVerified
| VerificationLevel::UnsignedDevice => ShieldState::Red {
code: ShieldStateCode::UnverifiedIdentity,
message: UNVERIFIED_IDENTITY,
},
VerificationLevel::None(link) => match link {
DeviceLinkProblem::MissingDevice => ShieldState::Red {
code: ShieldStateCode::UnknownDevice,
Expand Down Expand Up @@ -122,10 +124,16 @@ impl VerificationState {
VerificationLevel::UnverifiedIdentity => {
// If you didn't show interest in verifying that user we don't
// nag you with an error message.
// TODO: We should detect identity rotation of a previously trusted identity and
// then warn see https://github.com/matrix-org/matrix-rust-sdk/issues/1129
ShieldState::None
}
VerificationLevel::PreviouslyVerified => {
// This is a high warning. The sender was previously
// verified, but changed their identity.
ShieldState::Red {
code: ShieldStateCode::PreviouslyVerified,
message: PREVIOUSLY_VERIFIED,
}
}
VerificationLevel::UnsignedDevice => {
// This is a high warning. The sender hasn't verified his own device.
ShieldState::Red {
Expand Down Expand Up @@ -159,20 +167,28 @@ impl VerificationState {

/// The sub-enum containing detailed information on why a message is considered
/// to be unverified.
#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq)]
#[derive(Clone, Debug, Error, Deserialize, Serialize, PartialEq, Eq)]
pub enum VerificationLevel {
/// The message was sent by a user identity we have not verified.
#[error("The sender's identity was not verified")]
UnverifiedIdentity,

/// The message was sent by a user who was previously verified, but changed
/// their identity, and we have not yet verified the new identity.
#[error("The sender's identity was previously verified but has changed")]
PreviouslyVerified,

/// The message was sent by a device not linked to (signed by) any user
/// identity.
#[error("The sending device was not signed by the user's identity")]
UnsignedDevice,

/// We weren't able to link the message back to any device. This might be
/// because the message claims to have been sent by a device which we have
/// not been able to obtain (for example, because the device was since
/// deleted) or because the key to decrypt the message was obtained from
/// an insecure source.
#[error("The sending device is not known")]
None(DeviceLinkProblem),
}

Expand Down Expand Up @@ -227,6 +243,8 @@ pub enum ShieldStateCode {
UnverifiedIdentity,
/// An unencrypted event in an encrypted room.
SentInClear,
/// The sender was previously verified but changed their identity.
PreviouslyVerified,
}

/// The algorithm specific information of a decrypted event.
Expand Down
3 changes: 3 additions & 0 deletions crates/matrix-sdk-crypto/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ Changes:

Breaking changes:

- `OlmMachine::decrypt_room_event` now takes an `EncryptionSettings` argument.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- `OlmMachine::decrypt_room_event` now takes an `EncryptionSettings` argument.
- `OlmMachine::decrypt_room_event` now takes a `DecryptionSettings` argument.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably also document some of the changes to publicly-visible types (MegolmError, VerificationLevel, etc)

([#3701](https://github.com/matrix-org/matrix-rust-sdk/pull/3701))

- Add a new `error_on_verified_user_problem` property to
`CollectStrategy::DeviceBasedStrategy`, which, when set, causes
`OlmMachine::share_room_key` to fail with an error if any verified users on
Expand Down
5 changes: 5 additions & 0 deletions crates/matrix-sdk-crypto/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

use std::collections::BTreeMap;

use matrix_sdk_common::deserialized_responses::VerificationLevel;
use ruma::{CanonicalJsonError, IdParseError, OwnedDeviceId, OwnedRoomId, OwnedUserId};
use serde::{ser::SerializeMap, Serializer};
use serde_json::Error as SerdeError;
Expand Down Expand Up @@ -115,6 +116,10 @@ pub enum MegolmError {
/// The storage layer returned an error.
#[error(transparent)]
Store(#[from] CryptoStoreError),

uhoreg marked this conversation as resolved.
Show resolved Hide resolved
/// The sender's cross-signing identity isn't trusted
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The sender's cross-signing identity isn't trusted
/// An encrypted message wasn't decrypted, because the sender's cross-signing identity isn't trusted.

(This should maybe also link to DecryptionSettings and say that it only happens under some conditions)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, isn't it more "because the message could not be linked to a verified device" than specific to the sender's identity?

#[error("message quarantined because sender's cross-signing identity is not trusted: {0}")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does "quarantined" mean here? Is it stored somewhere within the store?

SenderIdentity(#[from] VerificationLevel),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SenderIdentity feels like it doesn't quite describe the problem here.

Suggested change
SenderIdentity(#[from] VerificationLevel),
SenderIdentityNotTrusted(#[from] VerificationLevel),

}

/// Decryption failed because of a mismatch between the identity keys of the
Expand Down
101 changes: 90 additions & 11 deletions crates/matrix-sdk-crypto/src/machine/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ use crate::{
StoreCache, StoreTransaction,
},
types::{
decryption::{DecryptionSettings, TrustRequirement},
events::{
olm_v1::{AnyDecryptedOlmEvent, DecryptedRoomKeyEvent},
room::encrypted::{
Expand Down Expand Up @@ -934,8 +935,6 @@ impl OlmMachine {
&self,
room_id: &RoomId,
) -> OlmResult<()> {
use crate::olm::SenderData;

let (_, session) = self
.inner
.group_session_manager
Expand All @@ -957,8 +956,6 @@ impl OlmMachine {
&self,
room_id: &RoomId,
) -> OlmResult<InboundGroupSession> {
use crate::olm::SenderData;

let (_, session) = self
.inner
.group_session_manager
Expand Down Expand Up @@ -1570,11 +1567,69 @@ impl OlmMachine {
self.get_encryption_info(&session, &event.sender).await
}

/// Check whether the sender of a Megolm session is trusted, based on the
/// verification state.
///
/// This is used by `check_sender_trust_requirement`, and ensures that the
/// sending device is cross-signed.
fn check_sender_trusted(&self, encryption_info: &EncryptionInfo) -> MegolmResult<()> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per a discussion today in #matrix-rust-sdk-dev, helper functions should probably come after the functions they help.

match &encryption_info.verification_state {
// Device is cross-signed, and identity is verified
VerificationState::Verified => Ok(()),
// Device is cross-signed, but identity is not verified
VerificationState::Unverified(VerificationLevel::UnverifiedIdentity) => Ok(()),
// Device is not cross-signed
VerificationState::Unverified(verification_level) => {
Err(MegolmError::SenderIdentity(verification_level.clone()))
}
}
}

/// Check that the sender of a Megolm session satisfies the trust
/// requirement from the decryption settings.
fn check_sender_trust_requirement(
&self,
session: &InboundGroupSession,
encryption_info: &EncryptionInfo,
decryption_settings: &DecryptionSettings,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this just checks the trust requirement, maybe it should take a &TrustRequirement rather than the whole &DecryptionSettings ?

) -> MegolmResult<()> {
match decryption_settings.trust_requirement {
TrustRequirement::Untrusted => Ok(()),

TrustRequirement::CrossSignedOrLegacy => match &session.sender_data {
// Reject if the sender was previously verified, but changed
// their identity and is not verified any more.
SenderData::SenderKnown {
master_key_verified: false,
previously_verified: true,
..
} => Err(MegolmError::SenderIdentity(VerificationLevel::PreviouslyVerified)),
SenderData::SenderKnown { .. } => Ok(()),
SenderData::DeviceInfo { legacy_session: true, .. } => Ok(()),
SenderData::UnknownDevice { legacy_session: true, .. } => Ok(()),
_ => self.check_sender_trusted(encryption_info),
},

TrustRequirement::CrossSigned => match &session.sender_data {
// Reject if the sender was previously verified, but changed
// their identity and is not verified any more.
SenderData::SenderKnown {
master_key_verified: false,
previously_verified: true,
..
} => Err(MegolmError::SenderIdentity(VerificationLevel::PreviouslyVerified)),
SenderData::SenderKnown { .. } => Ok(()),
_ => self.check_sender_trusted(encryption_info),
},
Comment on lines +1599 to +1623
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm finding all this rather hard to follow, and I think it could do with some more comments, at the very least.

As far as I can tell, the general gist is that (with the exception of PreviouslyVerified), we allow all SenderData::SenderKnown sessions, but for other types of sender data (DeviceInfo and UnknownDevice), we have to check_sender_trusted. But, check_sender_trusted looks at the verification level of the sender, and by definition, we haven't verified the cross-signatures on such sender data, so that seems contradictory.

I'm probably being stupid, but can you add some comments to explain this more clearly?

(also I wonder if we could reduce the duplication between the CrossSignedOrLegacy and CrossSigned arms, by having check_sender_trusted take an allow_legacy_session parameter, or something)

}
}

async fn decrypt_megolm_events(
&self,
room_id: &RoomId,
event: &EncryptedEvent,
content: &SupportedEventEncryptionSchemes<'_>,
decryption_settings: &DecryptionSettings,
) -> MegolmResult<(JsonObject, EncryptionInfo)> {
let session =
self.get_inbound_group_session_or_error(room_id, content.session_id()).await?;
Expand All @@ -1590,6 +1645,13 @@ impl OlmMachine {
match result {
Ok((decrypted_event, _)) => {
let encryption_info = self.get_encryption_info(&session, &event.sender).await?;

self.check_sender_trust_requirement(
&session,
&encryption_info,
decryption_settings,
)?;

Ok((decrypted_event, encryption_info))
}
Err(error) => Err(
Expand Down Expand Up @@ -1648,8 +1710,9 @@ impl OlmMachine {
&self,
event: &Raw<EncryptedEvent>,
room_id: &RoomId,
decryption_settings: &DecryptionSettings,
) -> MegolmResult<TimelineEvent> {
self.decrypt_room_event_inner(event, room_id, true).await
self.decrypt_room_event_inner(event, room_id, true, decryption_settings).await
}

#[instrument(name = "decrypt_room_event", skip_all, fields(?room_id, event_id, origin_server_ts, sender, algorithm, session_id, sender_key))]
Expand All @@ -1658,6 +1721,7 @@ impl OlmMachine {
event: &Raw<EncryptedEvent>,
room_id: &RoomId,
decrypt_unsigned: bool,
decryption_settings: &DecryptionSettings,
) -> MegolmResult<TimelineEvent> {
let event = event.deserialize()?;

Expand Down Expand Up @@ -1685,7 +1749,8 @@ impl OlmMachine {
};

Span::current().record("session_id", content.session_id());
let result = self.decrypt_megolm_events(room_id, &event, &content).await;
let result =
self.decrypt_megolm_events(room_id, &event, &content, decryption_settings).await;

if let Err(e) = &result {
#[cfg(feature = "automatic-room-key-forwarding")]
Expand All @@ -1710,8 +1775,9 @@ impl OlmMachine {
let mut unsigned_encryption_info = None;
if decrypt_unsigned {
// Try to decrypt encrypted unsigned events.
unsigned_encryption_info =
self.decrypt_unsigned_events(&mut decrypted_event, room_id).await;
unsigned_encryption_info = self
.decrypt_unsigned_events(&mut decrypted_event, room_id, decryption_settings)
.await;
}

let event = serde_json::from_value::<Raw<AnyTimelineEvent>>(decrypted_event.into())?;
Expand All @@ -1737,6 +1803,7 @@ impl OlmMachine {
&self,
main_event: &mut JsonObject,
room_id: &RoomId,
decryption_settings: &DecryptionSettings,
) -> Option<BTreeMap<UnsignedEventLocation, UnsignedDecryptionResult>> {
let unsigned = main_event.get_mut("unsigned")?.as_object_mut()?;
let mut unsigned_encryption_info: Option<
Expand All @@ -1746,7 +1813,9 @@ impl OlmMachine {
// Search for an encrypted event in `m.replace`, an edit.
let location = UnsignedEventLocation::RelationsReplace;
let replace = location.find_mut(unsigned);
if let Some(decryption_result) = self.decrypt_unsigned_event(replace, room_id).await {
if let Some(decryption_result) =
self.decrypt_unsigned_event(replace, room_id, decryption_settings).await
{
unsigned_encryption_info
.get_or_insert_with(Default::default)
.insert(location, decryption_result);
Expand All @@ -1757,7 +1826,7 @@ impl OlmMachine {
let location = UnsignedEventLocation::RelationsThreadLatestEvent;
let thread_latest_event = location.find_mut(unsigned);
if let Some(decryption_result) =
self.decrypt_unsigned_event(thread_latest_event, room_id).await
self.decrypt_unsigned_event(thread_latest_event, room_id, decryption_settings).await
{
unsigned_encryption_info
.get_or_insert_with(Default::default)
Expand All @@ -1778,6 +1847,7 @@ impl OlmMachine {
&'a self,
event: Option<&'a mut Value>,
room_id: &'a RoomId,
decryption_settings: &'a DecryptionSettings,
) -> BoxFuture<'a, Option<UnsignedDecryptionResult>> {
Box::pin(async move {
let event = event?;
Expand All @@ -1791,7 +1861,10 @@ impl OlmMachine {
}

let raw_event = serde_json::from_value(event.clone()).ok()?;
match self.decrypt_room_event_inner(&raw_event, room_id, false).await {
match self
.decrypt_room_event_inner(&raw_event, room_id, false, decryption_settings)
.await
{
Ok(decrypted_event) => {
// Replace the encrypted event.
*event = serde_json::to_value(decrypted_event.event).ok()?;
Expand Down Expand Up @@ -2359,6 +2432,12 @@ fn sender_data_to_verification_state(
VerificationState::Unverified(VerificationLevel::UnsignedDevice),
Some(device_keys.device_id),
),
SenderData::SenderKnown {
master_key_verified: false,
previously_verified: true,
device_id,
..
} => (VerificationState::Unverified(VerificationLevel::PreviouslyVerified), device_id),
Comment on lines +2435 to +2440
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be opt-in, according to the DecryptionSettings? Or, if not, shouldn't it be in a separate PR?

SenderData::SenderKnown { master_key_verified: false, device_id, .. } => {
(VerificationState::Unverified(VerificationLevel::UnverifiedIdentity), device_id)
}
Expand Down
Loading
Loading