crypto: Disallow re-sharing room keys with devices with changed curve keys

This commit is contained in:
Damir Jelić 2021-08-11 14:12:31 +02:00
parent f9de77a75d
commit e4a9cf0bba
7 changed files with 207 additions and 128 deletions

View file

@ -41,7 +41,7 @@ use ruma::{
};
use tracing::{debug, info, trace, warn};
use super::{GossipRequest, KeyforwardDecision, RequestEvent, RequestInfo, SecretInfo, WaitQueue};
use super::{GossipRequest, KeyForwardDecision, RequestEvent, RequestInfo, SecretInfo, WaitQueue};
use crate::{
error::{OlmError, OlmResult},
olm::{InboundGroupSession, Session, ShareState},
@ -370,12 +370,21 @@ impl GossipMachine {
if let Some(device) = device {
match self.should_share_key(&device, &session).await {
Err(e) => {
debug!(
user_id = device.user_id().as_str(),
device_id = device.device_id().as_str(),
reason =? e,
"Received a key request that we won't serve",
);
if let KeyForwardDecision::ChangedSenderKey = e {
warn!(
user_id = device.user_id().as_str(),
device_id = device.device_id().as_str(),
"Received a key request from a device that changed \
their curve25519 sender key"
);
} else {
debug!(
user_id = device.user_id().as_str(),
device_id = device.device_id().as_str(),
reason =? e,
"Received a key request that we won't serve",
);
}
Ok(None)
}
@ -482,7 +491,7 @@ impl GossipMachine {
&self,
device: &Device,
session: &InboundGroupSession,
) -> Result<Option<u32>, KeyforwardDecision> {
) -> Result<Option<u32>, KeyForwardDecision> {
let outbound_session = self
.outbound_group_sessions
.get_with_id(session.room_id(), session.session_id())
@ -494,7 +503,7 @@ impl GossipMachine {
if device.verified() {
Ok(None)
} else {
Err(KeyforwardDecision::UntrustedDevice)
Err(KeyForwardDecision::UntrustedDevice)
}
};
@ -502,14 +511,11 @@ impl GossipMachine {
// users/devices that received the session, if it wasn't shared check if
// it's our own device and if it's trusted.
if let Some(outbound) = outbound_session {
if let ShareState::Shared(message_index) =
outbound.is_shared_with(device.user_id(), device.device_id())
{
Ok(Some(message_index))
} else if device.user_id() == self.user_id() {
own_device_check()
} else {
Err(KeyforwardDecision::OutboundSessionNotShared)
match outbound.is_shared_with(device) {
ShareState::Shared(message_index) => Ok(Some(message_index)),
_ if device.user_id() == self.user_id() => own_device_check(),
ShareState::SharedButChangedSenderKey => Err(KeyForwardDecision::ChangedSenderKey),
ShareState::NotShared => Err(KeyForwardDecision::OutboundSessionNotShared),
}
// Else just check if it's one of our own devices that requested the key
// and check if the device is trusted.
@ -518,7 +524,7 @@ impl GossipMachine {
// Otherwise, there's not enough info to decide if we can safely share
// the session.
} else {
Err(KeyforwardDecision::MissingOutboundSession)
Err(KeyForwardDecision::MissingOutboundSession)
}
}
@ -902,10 +908,10 @@ mod test {
},
room_id,
to_device::DeviceIdOrAllDevices,
user_id, DeviceIdBox, RoomId, UserId,
user_id, DeviceIdBox, DeviceKeyAlgorithm, RoomId, UserId,
};
use super::{GossipMachine, KeyforwardDecision};
use super::{GossipMachine, KeyForwardDecision};
use crate::{
identities::{LocalTrust, ReadOnlyDevice},
olm::{Account, PrivateCrossSigningIdentity, ReadOnlyAccount},
@ -1182,12 +1188,9 @@ mod test {
account.create_group_session_pair_with_defaults(&room_id()).await.unwrap();
// We don't share keys with untrusted devices.
assert_eq!(
machine
.should_share_key(&own_device, &inbound)
.await
.expect_err("Should not share with untrusted"),
KeyforwardDecision::UntrustedDevice
assert_matches!(
machine.should_share_key(&own_device, &inbound).await,
Err(KeyForwardDecision::UntrustedDevice)
);
own_device.set_trust_state(LocalTrust::Verified);
// Now we do want to share the keys.
@ -1201,12 +1204,9 @@ mod test {
// We don't share sessions with other user's devices if no outbound
// session was provided.
assert_eq!(
machine
.should_share_key(&bob_device, &inbound)
.await
.expect_err("Should not share with other."),
KeyforwardDecision::MissingOutboundSession
assert_matches!(
machine.should_share_key(&bob_device, &inbound).await,
Err(KeyForwardDecision::MissingOutboundSession)
);
let mut changes = Changes::default();
@ -1218,28 +1218,26 @@ mod test {
// We don't share sessions with other user's devices if the session
// wasn't shared in the first place.
assert_eq!(
machine
.should_share_key(&bob_device, &inbound)
.await
.expect_err("Should not share with other unless shared."),
KeyforwardDecision::OutboundSessionNotShared
assert_matches!(
machine.should_share_key(&bob_device, &inbound).await,
Err(KeyForwardDecision::OutboundSessionNotShared)
);
bob_device.set_trust_state(LocalTrust::Verified);
// We don't share sessions with other user's devices if the session
// wasn't shared in the first place even if the device is trusted.
assert_eq!(
machine
.should_share_key(&bob_device, &inbound)
.await
.expect_err("Should not share with other unless shared."),
KeyforwardDecision::OutboundSessionNotShared
assert_matches!(
machine.should_share_key(&bob_device, &inbound).await,
Err(KeyForwardDecision::OutboundSessionNotShared)
);
// We now share the session, since it was shared before.
outbound.mark_shared_with(bob_device.user_id(), bob_device.device_id());
outbound.mark_shared_with(
bob_device.user_id(),
bob_device.device_id(),
bob_device.get_key(DeviceKeyAlgorithm::Curve25519).unwrap(),
);
assert!(machine.should_share_key(&bob_device, &inbound).await.is_ok());
// But we don't share some other session that doesn't match our outbound
@ -1247,12 +1245,21 @@ mod test {
let (_, other_inbound) =
account.create_group_session_pair_with_defaults(&room_id()).await.unwrap();
assert_eq!(
machine
.should_share_key(&bob_device, &other_inbound)
.await
.expect_err("Should not share with other unless shared."),
KeyforwardDecision::MissingOutboundSession
assert_matches!(
machine.should_share_key(&bob_device, &other_inbound).await,
Err(KeyForwardDecision::MissingOutboundSession)
);
// And we don't share the session with a device that rotated its
// curve25519 key.
let bob_device = ReadOnlyDevice::from_account(&bob_account()).await;
machine.store.save_devices(&[bob_device]).await.unwrap();
let bob_device =
machine.store.get_device(&bob_id(), &bob_device_id()).await.unwrap().unwrap();
assert_matches!(
machine.should_share_key(&bob_device, &inbound).await,
Err(KeyForwardDecision::ChangedSenderKey)
);
}
@ -1282,7 +1289,7 @@ mod test {
alice_machine.store.save_sessions(&[alice_session]).await.unwrap();
alice_machine.store.save_devices(&[bob_device]).await.unwrap();
bob_machine.store.save_sessions(&[bob_session]).await.unwrap();
bob_machine.store.save_devices(&[alice_device]).await.unwrap();
bob_machine.store.save_devices(&[alice_device.clone()]).await.unwrap();
let (group_session, inbound_group_session) =
bob_account.create_group_session_pair_with_defaults(&room_id()).await.unwrap();
@ -1298,7 +1305,11 @@ mod test {
)
.await
.unwrap();
group_session.mark_shared_with(&alice_id(), &alice_device_id());
group_session.mark_shared_with(
&alice_device.user_id(),
&alice_device.device_id(),
alice_device.get_key(DeviceKeyAlgorithm::Curve25519).unwrap(),
);
// Put the outbound session into bobs store.
bob_machine.outbound_group_sessions.insert(group_session.clone());
@ -1484,7 +1495,7 @@ mod test {
// Populate our stores with Olm sessions and a Megolm session.
alice_machine.store.save_devices(&[bob_device]).await.unwrap();
bob_machine.store.save_devices(&[alice_device]).await.unwrap();
bob_machine.store.save_devices(&[alice_device.clone()]).await.unwrap();
let (group_session, inbound_group_session) =
bob_account.create_group_session_pair_with_defaults(&room_id()).await.unwrap();
@ -1500,7 +1511,11 @@ mod test {
)
.await
.unwrap();
group_session.mark_shared_with(&alice_id(), &alice_device_id());
group_session.mark_shared_with(
alice_device.user_id(),
alice_device.device_id(),
alice_device.get_key(DeviceKeyAlgorithm::Curve25519).unwrap(),
);
// Put the outbound session into bobs store.
bob_machine.outbound_group_sessions.insert(group_session.clone());

View file

@ -41,7 +41,7 @@ use crate::{
/// An error describing why a key share request won't be honored.
#[derive(Debug, Clone, Error, PartialEq)]
pub enum KeyforwardDecision {
pub enum KeyForwardDecision {
/// The key request is from a device that we don't own, we're only sharing
/// sessions that we know the requesting device already was supposed to get.
#[error("can't find an active outbound group session")]
@ -53,6 +53,10 @@ pub enum KeyforwardDecision {
/// The key request is from a device we own, yet we don't trust it.
#[error("requesting device isn't trusted")]
UntrustedDevice,
/// The outbound session was shared with the device, but the device either
/// accidentally or maliciously changed their curve25519 sender key.
#[error("the device has changed their curve25519 sender key")]
ChangedSenderKey,
}
/// A struct describing an outgoing key request.

View file

@ -28,7 +28,7 @@ mod outbound;
pub use inbound::{InboundGroupSession, InboundGroupSessionPickle, PickledInboundGroupSession};
pub use outbound::{
EncryptionSettings, OutboundGroupSession, PickledOutboundGroupSession, ShareState,
EncryptionSettings, OutboundGroupSession, PickledOutboundGroupSession, ShareInfo, ShareState,
};
/// The private session key of a group session.

View file

@ -43,8 +43,7 @@ use ruma::{
room_key::RoomKeyToDeviceEventContent,
AnyMessageEventContent, AnyToDeviceEventContent, EventContent,
},
to_device::DeviceIdOrAllDevices,
DeviceId, DeviceIdBox, EventEncryptionAlgorithm, RoomId, UserId,
DeviceId, DeviceIdBox, DeviceKeyAlgorithm, EventEncryptionAlgorithm, RoomId, UserId,
};
use serde::{Deserialize, Serialize};
use serde_json::json;
@ -54,13 +53,15 @@ use super::{
super::{deserialize_instant, serialize_instant},
GroupSessionKey,
};
use crate::ToDeviceRequest;
use crate::{Device, ToDeviceRequest};
const ROTATION_PERIOD: Duration = Duration::from_millis(604800000);
const ROTATION_MESSAGES: u64 = 100;
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum ShareState {
NotShared,
SharedButChangedSenderKey,
Shared(u32),
}
@ -125,8 +126,23 @@ pub struct OutboundGroupSession {
shared: Arc<AtomicBool>,
invalidated: Arc<AtomicBool>,
settings: Arc<EncryptionSettings>,
pub(crate) shared_with_set: Arc<DashMap<UserId, DashMap<DeviceIdBox, u32>>>,
to_share_with_set: Arc<DashMap<Uuid, (Arc<ToDeviceRequest>, u32)>>,
pub(crate) shared_with_set: Arc<DashMap<UserId, DashMap<DeviceIdBox, ShareInfo>>>,
to_share_with_set: Arc<DashMap<Uuid, (Arc<ToDeviceRequest>, ShareInfoSet)>>,
}
/// A a map of userid/device it to a `ShareInfo`.
///
/// Holds the `ShareInfo` for all the user/device pairs that will receive the
/// room key.
pub type ShareInfoSet = BTreeMap<UserId, BTreeMap<DeviceIdBox, ShareInfo>>;
/// Struct holding info about the share state of a outbound group session.
#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct ShareInfo {
/// The sender key of the device that was used to encrypt the room key.
pub sender_key: String,
/// The message index that the device received.
pub message_index: u32,
}
impl OutboundGroupSession {
@ -174,9 +190,9 @@ impl OutboundGroupSession {
&self,
request_id: Uuid,
request: Arc<ToDeviceRequest>,
message_index: u32,
share_infos: ShareInfoSet,
) {
self.to_share_with_set.insert(request_id, (request, message_index));
self.to_share_with_set.insert(request_id, (request, share_infos));
}
/// This should be called if an the user wishes to rotate this session.
@ -194,28 +210,15 @@ impl OutboundGroupSession {
/// This removes the request from the queue and marks the set of
/// users/devices that received the session.
pub fn mark_request_as_sent(&self, request_id: &Uuid) {
if let Some((_, r)) = self.to_share_with_set.remove(request_id) {
if let Some((_, (_, r))) = self.to_share_with_set.remove(request_id) {
trace!(
request_id = request_id.to_string().as_str(),
"Marking to-device request carrying a room key as sent"
);
let user_pairs = r.0.messages.iter().map(|(u, v)| {
(
u.clone(),
v.iter().filter_map(|d| {
if let DeviceIdOrAllDevices::DeviceId(d) = d.0 {
Some((d.clone(), r.1))
} else {
None
}
}),
)
});
user_pairs.for_each(|(u, d)| {
self.shared_with_set.entry(u).or_insert_with(DashMap::new).extend(d);
});
for (user_id, info) in r.into_iter() {
self.shared_with_set.entry(user_id).or_insert_with(DashMap::new).extend(info)
}
if self.to_share_with_set.is_empty() {
debug!(
@ -368,36 +371,39 @@ impl OutboundGroupSession {
}
/// Has or will the session be shared with the given user/device pair.
pub(crate) fn is_shared_with(&self, user_id: &UserId, device_id: &DeviceId) -> ShareState {
pub(crate) fn is_shared_with(&self, device: &Device) -> ShareState {
// Check if we shared the session.
let shared_state = self
.shared_with_set
.get(user_id)
.and_then(|d| d.get(device_id).map(|m| ShareState::Shared(*m.value())));
let shared_state = self.shared_with_set.get(device.user_id()).and_then(|d| {
d.get(device.device_id()).map(|s| {
if Some(&s.sender_key) == device.get_key(DeviceKeyAlgorithm::Curve25519) {
ShareState::Shared(s.message_index)
} else {
ShareState::SharedButChangedSenderKey
}
})
});
if let Some(state) = shared_state {
state
} else {
// If we haven't shared the session, check if we're going to share
// the session.
let device_id = DeviceIdOrAllDevices::DeviceId(device_id.into());
// Find the first request that contains the given user id and
// device id.
let shared = self.to_share_with_set.iter().find_map(|item| {
let request = &item.value().0;
let message_index = item.value().1;
let share_info = &item.value().1;
if request
.messages
.get(user_id)
.map(|e| e.contains_key(&device_id))
.unwrap_or(false)
{
Some(ShareState::Shared(message_index))
} else {
None
}
share_info.get(device.user_id()).and_then(|d| {
d.get(device.device_id()).map(|info| {
if Some(&info.sender_key) == device.get_key(DeviceKeyAlgorithm::Curve25519)
{
ShareState::Shared(info.message_index)
} else {
ShareState::SharedButChangedSenderKey
}
})
})
});
shared.unwrap_or(ShareState::NotShared)
@ -406,11 +412,11 @@ impl OutboundGroupSession {
/// Mark that the session was shared with the given user/device pair.
#[cfg(test)]
pub fn mark_shared_with(&self, user_id: &UserId, device_id: &DeviceId) {
self.shared_with_set
.entry(user_id.to_owned())
.or_insert_with(DashMap::new)
.insert(device_id.to_owned(), 0);
pub fn mark_shared_with(&self, user_id: &UserId, device_id: &DeviceId, sender_key: &str) {
self.shared_with_set.entry(user_id.to_owned()).or_insert_with(DashMap::new).insert(
device_id.to_owned(),
ShareInfo { sender_key: sender_key.to_owned(), message_index: 0 },
);
}
/// Get the list of requests that need to be sent out for this session to be
@ -498,8 +504,7 @@ impl OutboundGroupSession {
.map(|u| {
(
u.key().clone(),
#[allow(clippy::map_clone)]
u.value().iter().map(|d| (d.key().clone(), *d.value())).collect(),
u.value().iter().map(|d| (d.key().clone(), d.value().clone())).collect(),
)
})
.collect(),
@ -555,9 +560,9 @@ pub struct PickledOutboundGroupSession {
/// Has the session been invalidated.
pub invalidated: bool,
/// The set of users the session has been already shared with.
pub shared_with_set: BTreeMap<UserId, BTreeMap<DeviceIdBox, u32>>,
pub shared_with_set: BTreeMap<UserId, BTreeMap<DeviceIdBox, ShareInfo>>,
/// Requests that need to be sent out to share the session.
pub requests: BTreeMap<Uuid, (Arc<ToDeviceRequest>, u32)>,
pub requests: BTreeMap<Uuid, (Arc<ToDeviceRequest>, ShareInfoSet)>,
}
#[cfg(test)]

View file

@ -27,7 +27,7 @@ pub(crate) use account::{Account, OlmDecryptionInfo, SessionType};
pub use account::{AccountPickle, OlmMessageHash, PickledAccount, ReadOnlyAccount};
pub use group_sessions::{
EncryptionSettings, ExportedRoomKey, InboundGroupSession, InboundGroupSessionPickle,
OutboundGroupSession, PickledInboundGroupSession, PickledOutboundGroupSession,
OutboundGroupSession, PickledInboundGroupSession, PickledOutboundGroupSession, ShareInfo,
};
pub(crate) use group_sessions::{GroupSessionKey, ShareState};
use matrix_sdk_common::instant::{Duration, Instant};

View file

@ -33,7 +33,7 @@ use tracing::{debug, info, trace};
use crate::{
error::{EventError, MegolmResult, OlmResult},
olm::{Account, InboundGroupSession, OutboundGroupSession, Session, ShareState},
olm::{Account, InboundGroupSession, OutboundGroupSession, Session, ShareInfo, ShareState},
store::{Changes, Result as StoreResult, Store},
Device, EncryptionSettings, OlmError, ToDeviceRequest,
};
@ -226,21 +226,43 @@ impl GroupSessionManager {
async fn encrypt_session_for(
content: AnyToDeviceEventContent,
devices: Vec<Device>,
) -> OlmResult<(Uuid, ToDeviceRequest, Vec<Session>)> {
message_index: u32,
) -> OlmResult<(
Uuid,
ToDeviceRequest,
BTreeMap<UserId, BTreeMap<DeviceIdBox, ShareInfo>>,
Vec<Session>,
)> {
let mut messages = BTreeMap::new();
let mut changed_sessions = Vec::new();
let mut share_infos = BTreeMap::new();
let encrypt = |device: Device, content: AnyToDeviceEventContent| async move {
let mut message = BTreeMap::new();
let mut share_infos = BTreeMap::new();
let encrypted = device.encrypt(content.clone()).await;
let used_session = match encrypted {
Ok((session, encrypted)) => {
message.entry(device.user_id().clone()).or_insert_with(BTreeMap::new).insert(
DeviceIdOrAllDevices::DeviceId(device.device_id().into()),
Raw::from(AnyToDeviceEventContent::RoomEncrypted(encrypted)),
);
message
.entry(device.user_id().to_owned())
.or_insert_with(BTreeMap::new)
.insert(
DeviceIdOrAllDevices::DeviceId(device.device_id().into()),
Raw::from(AnyToDeviceEventContent::RoomEncrypted(encrypted)),
);
share_infos
.entry(device.user_id().to_owned())
.or_insert_with(BTreeMap::new)
.insert(
device.device_id().to_owned(),
ShareInfo {
sender_key: session.sender_key().to_owned(),
message_index,
},
);
Some(session)
}
// TODO we'll want to create m.room_key.withheld here.
@ -249,7 +271,7 @@ impl GroupSessionManager {
Err(e) => return Err(e),
};
Ok((used_session, message))
Ok((used_session, share_infos, message))
};
let tasks: Vec<_> =
@ -258,7 +280,7 @@ impl GroupSessionManager {
let results = join_all(tasks).await;
for result in results {
let (used_session, message) = result.expect("Encryption task panicked")?;
let (used_session, infos, message) = result.expect("Encryption task panicked")?;
if let Some(session) = used_session {
changed_sessions.push(session);
@ -267,6 +289,10 @@ impl GroupSessionManager {
for (user, device_messages) in message.into_iter() {
messages.entry(user).or_insert_with(BTreeMap::new).extend(device_messages);
}
for (user, infos) in infos.into_iter() {
share_infos.entry(user).or_insert_with(BTreeMap::new).extend(infos);
}
}
let id = Uuid::new_v4();
@ -280,7 +306,7 @@ impl GroupSessionManager {
"Created a to-device request carrying a room_key"
);
Ok((id, request, changed_sessions))
Ok((id, request, share_infos, changed_sessions))
}
/// Given a list of user and an outbound session, return the list of users
@ -380,11 +406,11 @@ impl GroupSessionManager {
message_index: u32,
being_shared: Arc<DashMap<Uuid, OutboundGroupSession>>,
) -> OlmResult<Vec<Session>> {
let (id, request, used_sessions) =
Self::encrypt_session_for(content.clone(), chunk).await?;
let (id, request, share_infos, used_sessions) =
Self::encrypt_session_for(content.clone(), chunk, message_index).await?;
if !request.messages.is_empty() {
outbound.add_request(id, request.into(), message_index);
outbound.add_request(id, request.into(), share_infos);
being_shared.insert(id, outbound.clone());
}
@ -453,12 +479,8 @@ impl GroupSessionManager {
let devices: Vec<Device> = devices
.into_iter()
.map(|(_, d)| {
d.into_iter().filter(|d| {
matches!(
outbound.is_shared_with(d.user_id(), d.device_id()),
ShareState::NotShared
)
})
d.into_iter()
.filter(|d| matches!(outbound.is_shared_with(d), ShareState::NotShared))
})
.flatten()
.collect();

View file

@ -14,7 +14,7 @@
use std::{
collections::{HashMap, HashSet},
convert::TryFrom,
convert::{TryFrom, TryInto},
path::{Path, PathBuf},
sync::{Arc, RwLock},
};
@ -31,6 +31,7 @@ use sled::{
transaction::{ConflictableTransactionError, TransactionError},
Config, Db, Transactional, Tree,
};
use tracing::trace;
use uuid::Uuid;
use super::{
@ -46,6 +47,7 @@ use crate::{
/// This needs to be 32 bytes long since AES-GCM requires it, otherwise we will
/// panic once we try to pickle a Signing object.
const DEFAULT_PICKLE: &str = "DEFAULT_PICKLE_PASSPHRASE_123456";
const DATABASE_VERSION: u8 = 1;
trait EncodeKey {
const SEPARATOR: u8 = 0xff;
@ -204,12 +206,43 @@ impl SledStore {
self.account_info.read().unwrap().clone()
}
fn upgrade_databse(db: &Db) -> Result<()> {
let version = db
.get("version")?
.map(|v| {
let (version_bytes, _) = v.split_at(std::mem::size_of::<u8>());
u8::from_be_bytes(version_bytes.try_into().unwrap_or_default())
})
.unwrap_or_default();
if version != DATABASE_VERSION {
trace!(
version = version,
new_version = DATABASE_VERSION,
"Upgrading the Sled crypto store"
);
}
if version == 0 {
// We changed the schema but migrating this isn't important since we
// rotate the group sessions relatively often anyways so we just
// drop it.
db.drop_tree("outbound_group_sessions")?;
}
db.insert("version", DATABASE_VERSION.to_be_bytes().as_ref())?;
Ok(())
}
fn open_helper(db: Db, path: Option<PathBuf>, passphrase: Option<&str>) -> Result<Self> {
Self::upgrade_databse(&db)?;
let account = db.open_tree("account")?;
let private_identity = db.open_tree("private_identity")?;
let sessions = db.open_tree("session")?;
let inbound_group_sessions = db.open_tree("inbound_group_sessions")?;
let outbound_group_sessions = db.open_tree("outbound_group_sessions")?;
let tracked_users = db.open_tree("tracked_users")?;