From 6dc8bc05a95324de512ef7999b47507e712ccb76 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Thu, 4 Mar 2021 14:25:21 +0100 Subject: Tighten up against malformed user ids in events A few months ago 3c85f049 introduced validation of user ids but the rest of the library code wasn't updated to the fact that Connection::user() may quite legitimately (if not routinely) return nullptr, leading to crashes particularaly when malformed ids come from the wire. This commit adds necessary checks before using the value returned from user(). Closes #456. --- lib/connection.cpp | 10 ++++++++-- lib/room.cpp | 37 ++++++++++++++++++++++++------------- lib/uriresolver.cpp | 6 +++--- 3 files changed, 35 insertions(+), 18 deletions(-) (limited to 'lib') diff --git a/lib/connection.cpp b/lib/connection.cpp index 0fb2d003..c4587f30 100644 --- a/lib/connection.cpp +++ b/lib/connection.cpp @@ -691,7 +691,7 @@ void Connection::Private::consumeAccountData(Events&& accountDataEvents) DirectChatsMap remoteAdditions; for (auto it = usersToDCs.begin(); it != usersToDCs.end(); ++it) { - if (auto* u = q->user(it.key())) { + if (auto* const u = q->user(it.key())) { if (!directChats.contains(u, it.value()) && !dcLocalRemovals.contains(u, it.value())) { Q_ASSERT(!directChatUsers.contains(it.value(), u)); @@ -924,6 +924,12 @@ Connection::createRoom(RoomVisibility visibility, const QString& alias, const QJsonObject& creationContent) { invites.removeOne(userId()); // The creator is by definition in the room + for (const auto& i : invites) + if (!user(i)) { + qCWarning(MAIN) << "Won't create a room with malformed invitee ids"; + return nullptr; + } + auto job = callApi(visibility == PublishRoom ? QStringLiteral("public") : QStringLiteral("private"), @@ -958,7 +964,7 @@ void Connection::requestDirectChat(User* u) void Connection::doInDirectChat(const QString& userId, const std::function& operation) { - if (auto* u = user(userId)) + if (auto* const u = user(userId)) doInDirectChat(u, operation); else qCCritical(MAIN) diff --git a/lib/room.cpp b/lib/room.cpp index c42e618e..a5940eb2 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -1460,7 +1460,9 @@ QString Room::roomMembername(const User* u) const QString Room::roomMembername(const QString& userId) const { - return roomMembername(user(userId)); + if (auto* const u = user(userId)) + return roomMembername(u); + return {}; } QString Room::safeMemberName(const QString& userId) const @@ -2349,7 +2351,7 @@ Room::Changes Room::Private::addNewMessageEvents(RoomEvents&& events) // the new message events. if (const auto senderId = (*from)->senderId(); !senderId.isEmpty()) { auto* const firstWriter = q->user(senderId); - if (q->readMarker(firstWriter) != timeline.crend()) { + if (firstWriter && q->readMarker(firstWriter) != timeline.crend()) { roomChanges |= promoteReadMarker(firstWriter, rev_iter_t(from) - 1); qCDebug(MESSAGES) @@ -2418,6 +2420,13 @@ Room::Changes Room::processStateEvent(const RoomEvent& e) if (!e.isStateEvent()) return Change::NoChange; + auto* const sender = user(e.senderId()); + if (!sender) { + qCWarning(MAIN) << "State event" << e.id() + << "is invalid and won't be processed"; + return Change::NoChange; + } + // Find a value (create an empty one if necessary) and get a reference // to it. Can't use getCurrentState<>() because it (creates and) returns // a stub if a value is not found, and what's needed here is a "real" event @@ -2426,9 +2435,9 @@ Room::Changes Room::processStateEvent(const RoomEvent& e) // Prepare for the state change const auto oldRme = static_cast(curStateEvent); visit(e, [this, &oldRme](const RoomMemberEvent& rme) { - auto* u = user(rme.userId()); - if (!u) { // ??? - qCCritical(MAIN) + auto* const u = user(rme.userId()); + if (!u) { // Invalid user id? + qCWarning(MAIN) << "Could not get a user object for" << rme.userId(); return; } @@ -2517,9 +2526,11 @@ Room::Changes Room::processStateEvent(const RoomEvent& e) emit avatarChanged(); return AvatarChange; } - , [this,oldRme] (const RoomMemberEvent& evt) { + , [this,oldRme,sender] (const RoomMemberEvent& evt) { // clang-format on auto* u = user(evt.userId()); + if (!u) + return NoChange; // Already warned earlier // TODO: remove in 0.7 u->processEvent(evt, this, oldRme == nullptr); @@ -2539,7 +2550,7 @@ Room::Changes Room::processStateEvent(const RoomEvent& e) if (!d->usersInvited.contains(u)) d->usersInvited.push_back(u); if (u == localUser() && evt.isDirect()) - connection()->addToDirectChats(this, user(evt.senderId())); + connection()->addToDirectChats(this, sender); break; case MembershipType::Knock: case MembershipType::Ban: @@ -2599,8 +2610,8 @@ Room::Changes Room::processEphemeralEvent(EventPtr&& event) if (auto* evt = eventCast(event)) { d->usersTyping.clear(); for (const QString& userId : qAsConst(evt->users())) { - auto u = user(userId); - if (memberJoinState(u) == JoinState::Join) + auto* const u = user(userId); + if (u && memberJoinState(u) == JoinState::Join) d->usersTyping.append(u); } if (evt->users().size() > 3 || et.nsecsElapsed() >= profilerMinNsecs()) @@ -2625,8 +2636,8 @@ Room::Changes Room::processEphemeralEvent(EventPtr&& event) for (const Receipt& r : p.receipts) { if (r.userId == connection()->userId()) continue; // FIXME, #185 - auto u = user(r.userId); - if (memberJoinState(u) == JoinState::Join) + auto* const u = user(r.userId); + if (u && memberJoinState(u) == JoinState::Join) changes |= d->promoteReadMarker(u, newMarker); } } else { @@ -2639,8 +2650,8 @@ Room::Changes Room::processEphemeralEvent(EventPtr&& event) for (const Receipt& r : p.receipts) { if (r.userId == connection()->userId()) continue; // FIXME, #185 - auto u = user(r.userId); - if (memberJoinState(u) == JoinState::Join + auto* const u = user(r.userId); + if (u && memberJoinState(u) == JoinState::Join && readMarker(u) == timelineEdge()) changes |= d->setLastReadEvent(u, p.evtId); } diff --git a/lib/uriresolver.cpp b/lib/uriresolver.cpp index 27360bcc..e5f19a96 100644 --- a/lib/uriresolver.cpp +++ b/lib/uriresolver.cpp @@ -24,9 +24,9 @@ UriResolveResult UriResolverBase::visitResource(Connection* account, case Uri::UserId: { if (uri.action() == "join") return IncorrectAction; - auto* user = account->user(uri.primaryId()); - Q_ASSERT(user != nullptr); - return visitUser(user, uri.action()); + if (auto* const user = account->user(uri.primaryId())) + return visitUser(user, uri.action()); + return InvalidUri; } case Uri::RoomId: case Uri::RoomAlias: { -- cgit v1.2.3