Skip to content

Commit

Permalink
fix: Use the DisplayName struct to protect against homoglyph attacks
Browse files Browse the repository at this point in the history
  • Loading branch information
poljar committed Nov 8, 2024
1 parent 563d649 commit f3a80cb
Show file tree
Hide file tree
Showing 12 changed files with 315 additions and 80 deletions.
12 changes: 8 additions & 4 deletions crates/matrix-sdk-base/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
#[cfg(feature = "e2e-encryption")]
use std::sync::Arc;
use std::{
collections::{BTreeMap, BTreeSet},
collections::{BTreeMap, BTreeSet, HashMap},
fmt, iter,
ops::Deref,
};
Expand Down Expand Up @@ -70,7 +70,7 @@ use crate::latest_event::{is_suitable_for_latest_event, LatestEvent, PossibleLat
#[cfg(feature = "e2e-encryption")]
use crate::RoomMemberships;
use crate::{
deserialized_responses::{RawAnySyncOrStrippedTimelineEvent, SyncTimelineEvent},
deserialized_responses::{DisplayName, RawAnySyncOrStrippedTimelineEvent, SyncTimelineEvent},
error::{Error, Result},
event_cache_store::EventCacheStoreLock,
response_processors::AccountDataProcessor,
Expand Down Expand Up @@ -1332,7 +1332,7 @@ impl BaseClient {
#[cfg(feature = "e2e-encryption")]
let mut user_ids = BTreeSet::new();

let mut ambiguity_map: BTreeMap<String, BTreeSet<OwnedUserId>> = BTreeMap::new();
let mut ambiguity_map: HashMap<DisplayName, BTreeSet<OwnedUserId>> = Default::default();

for raw_event in &response.chunk {
let member = match raw_event.deserialize() {
Expand Down Expand Up @@ -1363,7 +1363,11 @@ impl BaseClient {

if let StateEvent::Original(e) = &member {
if let Some(d) = &e.content.displayname {
ambiguity_map.entry(d.clone()).or_default().insert(member.state_key().clone());
let display_name = DisplayName::new(d);
ambiguity_map
.entry(display_name)
.or_default()
.insert(member.state_key().clone());
}
}

Expand Down
10 changes: 6 additions & 4 deletions crates/matrix-sdk-base/src/deserialized_responses.rs
Original file line number Diff line number Diff line change
Expand Up @@ -446,10 +446,12 @@ impl MemberEvent {
///
/// It there is no `displayname` in the event's content, the localpart or
/// the user ID is returned.
pub fn display_name(&self) -> &str {
self.original_content()
.and_then(|c| c.displayname.as_deref())
.unwrap_or_else(|| self.user_id().localpart())
pub fn display_name(&self) -> DisplayName {
DisplayName::new(
self.original_content()
.and_then(|c| c.displayname.as_deref())
.unwrap_or_else(|| self.user_id().localpart()),
)
}
}

Expand Down
13 changes: 8 additions & 5 deletions crates/matrix-sdk-base/src/rooms/members.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// limitations under the License.

use std::{
collections::{BTreeMap, BTreeSet},
collections::{BTreeSet, HashMap},
sync::Arc,
};

Expand All @@ -30,7 +30,8 @@ use ruma::{
};

use crate::{
deserialized_responses::{MemberEvent, SyncOrStrippedState},
deserialized_responses::{DisplayName, MemberEvent, SyncOrStrippedState},
store::ambiguity_map::is_display_name_ambiguous,
MinimalRoomMemberEvent,
};

Expand Down Expand Up @@ -67,8 +68,10 @@ impl RoomMember {
} = room_info;

let is_room_creator = room_creator.as_deref() == Some(event.user_id());
let display_name_ambiguous =
users_display_names.get(event.display_name()).is_some_and(|s| s.len() > 1);
let display_name = event.display_name();
let display_name_ambiguous = users_display_names
.get(&display_name)
.is_some_and(|s| is_display_name_ambiguous(&display_name, s));
let is_ignored = ignored_users.as_ref().is_some_and(|s| s.contains(event.user_id()));

Self {
Expand Down Expand Up @@ -245,6 +248,6 @@ pub(crate) struct MemberRoomInfo<'a> {
pub(crate) power_levels: Arc<Option<SyncOrStrippedState<RoomPowerLevelsEventContent>>>,
pub(crate) max_power_level: i64,
pub(crate) room_creator: Option<OwnedUserId>,
pub(crate) users_display_names: BTreeMap<&'a str, BTreeSet<OwnedUserId>>,
pub(crate) users_display_names: HashMap<&'a DisplayName, BTreeSet<OwnedUserId>>,
pub(crate) ignored_users: Option<BTreeSet<OwnedUserId>>,
}
9 changes: 4 additions & 5 deletions crates/matrix-sdk-base/src/rooms/normal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ use super::{
#[cfg(feature = "experimental-sliding-sync")]
use crate::latest_event::LatestEvent;
use crate::{
deserialized_responses::{MemberEvent, RawSyncOrStrippedState},
deserialized_responses::{DisplayName, MemberEvent, RawSyncOrStrippedState},
notification_settings::RoomNotificationMode,
read_receipts::RoomReadReceipts,
store::{DynStateStore, Result as StoreResult, StateStoreExt},
Expand Down Expand Up @@ -819,8 +819,7 @@ impl Room {
})
.collect::<BTreeMap<_, _>>();

let display_names =
member_events.iter().map(|e| e.display_name().to_owned()).collect::<Vec<_>>();
let display_names = member_events.iter().map(|e| e.display_name()).collect::<Vec<_>>();
let room_info = self.member_room_info(&display_names).await?;

let mut members = Vec::new();
Expand Down Expand Up @@ -900,7 +899,7 @@ impl Room {

let profile = self.store.get_profile(self.room_id(), user_id).await?;

let display_names = [event.display_name().to_owned()];
let display_names = [event.display_name()];
let room_info = self.member_room_info(&display_names).await?;

Ok(Some(RoomMember::from_parts(event, profile, presence, &room_info)))
Expand All @@ -911,7 +910,7 @@ impl Room {
/// Async because it can read from storage.
async fn member_room_info<'a>(
&self,
display_names: &'a [String],
display_names: &'a [DisplayName],
) -> StoreResult<MemberRoomInfo<'a>> {
let max_power_level = self.max_power_level();
let room_creator = self.inner.read().creator().map(ToOwned::to_owned);
Expand Down
8 changes: 6 additions & 2 deletions crates/matrix-sdk-base/src/sliding_sync/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -695,6 +695,8 @@ async fn cache_latest_events(
changes: Option<&StateChanges>,
store: Option<&Store>,
) {
use crate::{deserialized_responses::DisplayName, store::ambiguity_map::is_display_name_ambiguous};

let mut encrypted_events =
Vec::with_capacity(room.latest_encrypted_events.read().unwrap().capacity());

Expand Down Expand Up @@ -752,11 +754,13 @@ async fn cache_latest_events(
.as_original()
.and_then(|profile| profile.content.displayname.as_ref())
.and_then(|display_name| {
let display_name = DisplayName::new(&display_name);

changes.ambiguity_maps.get(room.room_id()).and_then(
|map_for_room| {
map_for_room
.get(display_name)
.map(|user_ids| user_ids.len() > 1)
.get(&display_name)
.map(|users| is_display_name_ambiguous(&display_name, users))
},
)
});
Expand Down
Loading

0 comments on commit f3a80cb

Please sign in to comment.