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 30 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: 6 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,9 @@ use futures_util::Stream;
use matrix_sdk_common::instant::Instant;
#[cfg(feature = "e2e-encryption")]
use matrix_sdk_crypto::{
store::DynCryptoStore, EncryptionSettings, EncryptionSyncChanges, OlmError, OlmMachine,
ToDeviceRequest,
types::decryption::{DecryptionSettings, TrustRequirement},
store::DynCryptoStore,
EncryptionSettings, EncryptionSyncChanges, OlmError, OlmMachine, ToDeviceRequest,
};
#[cfg(feature = "e2e-encryption")]
use ruma::events::{
Expand Down Expand Up @@ -298,8 +299,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
17 changes: 15 additions & 2 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 Down Expand Up @@ -88,7 +89,7 @@ impl VerificationState {
match self {
VerificationState::Verified => ShieldState::None,
VerificationState::Unverified(level) => match level {
VerificationLevel::UnverifiedIdentity | VerificationLevel::UnsignedDevice => {
VerificationLevel::UnverifiedIdentity | VerificationLevel::ChangedIdentity | VerificationLevel::UnsignedDevice => {
ShieldState::Red {
code: ShieldStateCode::UnverifiedIdentity,
message: UNVERIFIED_IDENTITY,
Expand Down Expand Up @@ -126,6 +127,11 @@ impl VerificationState {
// then warn see https://github.com/matrix-org/matrix-rust-sdk/issues/1129
ShieldState::None
}
VerificationLevel::ChangedIdentity => {
// As above, if you didn't show interest in verifying that
// user we don't nag you
ShieldState::None
}
VerificationLevel::UnsignedDevice => {
// This is a high warning. The sender hasn't verified his own device.
ShieldState::Red {
Expand Down Expand Up @@ -159,20 +165,27 @@ 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 whose identity has changed and the change has not yet been confirmed
#[error("The sender's identity has changed and the change has not yet been confirmed")]
ChangedIdentity,

/// 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
31 changes: 31 additions & 0 deletions crates/matrix-sdk-crypto/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

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 @@ -70,8 +71,34 @@ pub enum OlmError {
have a valid Olm session with us"
)]
MissingSession,
#[error(transparent)]
/// The room key that should be shared was not due to an error.
KeyDistributionError(RoomKeyDistributionError),
}

/// Depending on the sharing strategy for room keys, the distribution of the
/// room key could fail.
#[derive(Error, Debug)]
pub enum RoomKeyDistributionError {
/// When encrypting using the IdentityBased strategy.
/// Will be thrown when sharing room keys when there is a new identity for a
/// user that has not been confirmed by the user.
/// Application should display identity changes to the user as soon as
/// possible to avoid hitting this case. If it happens the app might
/// just retry automatically after the identity change has been
/// notified, or offer option to cancel.
#[error("Encryption failed because there are key pinning violation, please re-pin or verify the problematic users")]
KeyPinningViolation(Vec<OwnedUserId>),

/// Cross-signing is required for encryption with invisible crypto
#[error("Encryption failed: Setup cross-signing on your account")]
CrossSigningNotSetup,
/// The current device needs to be verified when encrypting using the
/// IdentityBased strategy. Apps should prevent sending in the UI to
/// avoid hitting this case.
#[error("Encryption failed: Verify your device to send encrypted messages")]
SendingFromUnverifiedDevice,
}
/// Error representing a failure during a group encryption operation.
#[derive(Error, Debug)]
pub enum MegolmError {
Expand Down Expand Up @@ -107,6 +134,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
93 changes: 82 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,61 @@ impl OlmMachine {
self.get_encryption_info(&session, &event.sender).await
}

/// Check whether the sender of a Megolm session is trusted.
///
/// Checks that the device is cross-signed, that the sender's identity is
/// cross-signed, and that the sender's identity is pinned. If
/// `require_verified` is `true`, then also checks if we have verified the
/// sender's identity
fn check_sender_trusted(
&self,
encryption_info: &EncryptionInfo,
require_verified: bool,
) -> MegolmResult<()> {
match dbg!(&encryption_info.verification_state) {
VerificationState::Verified => Ok(()),
VerificationState::Unverified(VerificationLevel::UnverifiedIdentity) => {
if require_verified {
Err(MegolmError::SenderIdentity(VerificationLevel::UnverifiedIdentity))
} else {
Ok(())
}
}
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 {
SenderData::SenderKnown { .. } => Ok(()),
SenderData::DeviceInfo { legacy_session: true, .. } => Ok(()),
SenderData::UnknownDevice { legacy_session: true, .. } => Ok(()),
_ => self.check_sender_trusted(encryption_info, false),
},
TrustRequirement::CrossSigned => match session.sender_data {
SenderData::SenderKnown { .. } => Ok(()),
_ => self.check_sender_trusted(encryption_info, false),
},
}
}

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 +1637,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 +1702,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 +1713,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 +1741,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 +1767,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 +1795,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 +1805,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 +1818,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 +1839,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 +1853,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 +2424,12 @@ fn sender_data_to_verification_state(
VerificationState::Unverified(VerificationLevel::UnsignedDevice),
Some(device_keys.device_id),
),
SenderData::SenderKnown {
master_key_verified: false,
identity_needs_user_approval: true,
device_id,
..
} => (VerificationState::Unverified(VerificationLevel::ChangedIdentity), device_id),
SenderData::SenderKnown { master_key_verified: false, device_id, .. } => {
(VerificationState::Unverified(VerificationLevel::UnverifiedIdentity), device_id)
}
Expand Down
Loading
Loading