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 19 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
39 changes: 39 additions & 0 deletions crates/matrix-sdk-crypto/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,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 @@ -115,6 +141,19 @@ 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")]
SenderCrossSigningUntrusted,

/// The sender's cross-signing identity has changed, and the change hasn't
/// been acknowledged
#[error("message quarantined because sender's cross-signing identity has changed")]
SenderCrossSigningIdentityChanged,

/// The sender's cross-signing identity is unknown
#[error("message quarantined because sender is unknown")]
SenderCrossSigningIdentityUnknown,
}

/// Error that occurs when decrypting an event that is malformed.
Expand Down
128 changes: 127 additions & 1 deletion crates/matrix-sdk-crypto/src/identities/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,7 @@ impl IdentityManager {
*changed_private_identity = self.check_private_identity(&identity).await;
Ok(identity.into())
} else {
// First time seen, create the identity. The current MSK will be pinned.
let identity = OtherUserIdentityData::new(master_key, self_signing)?;
Ok(identity.into())
}
Expand Down Expand Up @@ -1338,7 +1339,7 @@ pub(crate) mod tests {
use std::ops::Deref;

use futures_util::pin_mut;
use matrix_sdk_test::{async_test, response_from_file};
use matrix_sdk_test::{async_test, response_from_file, test_json};
use ruma::{
api::{client::keys::get_keys::v3::Response as KeysQueryResponse, IncomingResponse},
device_id, user_id, TransactionId,
Expand Down Expand Up @@ -1897,4 +1898,129 @@ pub(crate) mod tests {

manager.store.get_device_data(other_user, device_id!("OBEBOSKTBE")).await.unwrap().unwrap();
}

#[async_test]
async fn test_manager_identity_updates() {
use test_json::keys_query_sets::IdentityChangeDataSet as DataSet;

let manager = manager_test_helper(user_id(), device_id()).await;
let other_user = DataSet::user_id();
let devices = manager.store.get_user_devices(other_user).await.unwrap();
assert_eq!(devices.devices().count(), 0);

let identity = manager.store.get_user_identity(other_user).await.unwrap();
assert!(identity.is_none());

manager
.receive_keys_query_response(
&TransactionId::new(),
&DataSet::key_query_with_identity_a(),
)
.await
.unwrap();

let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap();
let other_identity = identity.other().unwrap();

// There should be now an identity and no pin violation (pinned msk is the
// current one)
assert!(!other_identity.has_pin_violation());
let first_device = manager
.store
.get_device_data(other_user, DataSet::first_device_id())
.await
.unwrap()
.unwrap();
assert!(first_device.is_cross_signed_by_owner(&identity));

// We receive a new keys update for that user, with a new identity
manager
.receive_keys_query_response(
&TransactionId::new(),
&DataSet::key_query_with_identity_b(),
)
.await
.unwrap();

let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap();
let other_identity = identity.other().unwrap();

// The previous known identity has been replaced, there should be a pin
// violation
assert!(other_identity.has_pin_violation());

let second_device = manager
.store
.get_device_data(other_user, DataSet::second_device_id())
.await
.unwrap()
.unwrap();

// There is a new device signed by the new identity
assert!(second_device.is_cross_signed_by_owner(&identity));

// The first device should not be signed by the new identity
let first_device = manager
.store
.get_device_data(other_user, DataSet::first_device_id())
.await
.unwrap()
.unwrap();
assert!(!first_device.is_cross_signed_by_owner(&identity));

let remember_previous_identity = other_identity.clone();
// We receive updated keys for that user, with no identity anymore.
// Notice that there is no server API to delete identity, but we want to test
// here that a home server cannot clear the identity and serve a new one
// after that would get automatically approved.
manager
.receive_keys_query_response(
&TransactionId::new(),
&DataSet::key_query_with_identity_no_identity(),
)
.await
.unwrap();

let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap();
let other_identity = identity.other().unwrap();

assert_eq!(other_identity, &remember_previous_identity);
assert!(other_identity.has_pin_violation());
}

#[async_test]
async fn test_manager_resolve_identity_mismatch() {
use test_json::keys_query_sets::IdentityChangeDataSet as DataSet;

let manager = manager_test_helper(user_id(), device_id()).await;
let other_user = DataSet::user_id();

manager
.receive_keys_query_response(
&TransactionId::new(),
&DataSet::key_query_with_identity_a(),
)
.await
.unwrap();

// We receive a new keys update for that user, with a new identity
manager
.receive_keys_query_response(
&TransactionId::new(),
&DataSet::key_query_with_identity_b(),
)
.await
.unwrap();

let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap();
let other_identity = identity.other().unwrap();

// We have a new identity now, so there should be a pin violation
assert!(other_identity.has_pin_violation());

// Resolve the misatch by pinning the new identity
other_identity.pin();

assert!(!other_identity.has_pin_violation());
}
}
Loading
Loading