From 239d136788632760eadda1cda1241edceda393cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Blin?= <sebastien.blin@savoirfairelinux.com> Date: Fri, 31 May 2024 11:18:56 -0400 Subject: [PATCH] jamiaccount: check if uri is valid before adding contact Else, if a client calls addContact with an invalid uri, it will create a conversation with an invalid contact and the client will be in a bugguy state. git.jami.net/savoirfairelinux/jami-client-android/-/issues/1681 Change-Id: Id6227c45c279c78aac0a191b6ae688ebe0d3d1c4 --- src/jamidht/account_manager.cpp | 17 ++++----- src/jamidht/account_manager.h | 2 +- src/jamidht/conversation_module.cpp | 8 ++--- src/jamidht/conversation_module.h | 2 +- src/jamidht/jamiaccount.cpp | 35 +++++++++++++------ test/unitTest/conversation/conversation.cpp | 2 +- .../conversation/conversationMembersEvent.cpp | 17 ++++++++- 7 files changed, 54 insertions(+), 29 deletions(-) diff --git a/src/jamidht/account_manager.cpp b/src/jamidht/account_manager.cpp index 9d0a4dd6b9..6c08d77693 100644 --- a/src/jamidht/account_manager.cpp +++ b/src/jamidht/account_manager.cpp @@ -480,19 +480,14 @@ AccountManager::onPeerCertificate(const std::shared_ptr<dht::crypto::Certificate } bool -AccountManager::addContact(const std::string& uri, bool confirmed, const std::string& conversationId) +AccountManager::addContact(const dht::InfoHash& uri, bool confirmed, const std::string& conversationId) { - JAMI_WARN("AccountManager::addContact %d", confirmed); - dht::InfoHash h(uri); - if (not h) { - JAMI_ERR("addContact: invalid contact URI"); - return false; - } + JAMI_WARNING("AccountManager::addContact {}", confirmed); if (not info_) { - JAMI_ERR("addContact(): account not loaded"); + JAMI_ERROR("addContact(): account not loaded"); return false; } - if (info_->contacts->addContact(h, confirmed, conversationId)) { + if (info_->contacts->addContact(uri, confirmed, conversationId)) { syncDevices(); return true; } @@ -504,11 +499,11 @@ AccountManager::removeContact(const std::string& uri, bool banned) { dht::InfoHash h(uri); if (not h) { - JAMI_ERR("removeContact: invalid contact URI"); + JAMI_ERROR("removeContact: invalid contact URI"); return; } if (not info_) { - JAMI_ERR("addContact(): account not loaded"); + JAMI_ERROR("addContact(): account not loaded"); return; } if (info_->contacts->removeContact(h, banned)) diff --git a/src/jamidht/account_manager.h b/src/jamidht/account_manager.h index 20696a249f..c784d19d52 100644 --- a/src/jamidht/account_manager.h +++ b/src/jamidht/account_manager.h @@ -219,7 +219,7 @@ public: * Add contact to the account contact list. * Set confirmed if we know the contact also added us. */ - bool addContact(const std::string& uri, + bool addContact(const dht::InfoHash& uri, bool confirmed = false, const std::string& conversationId = ""); void removeContact(const std::string& uri, bool banned = true); diff --git a/src/jamidht/conversation_module.cpp b/src/jamidht/conversation_module.cpp index 53a73f3a75..4f2af546eb 100644 --- a/src/jamidht/conversation_module.cpp +++ b/src/jamidht/conversation_module.cpp @@ -2024,7 +2024,7 @@ ConversationModule::declineConversationRequest(const std::string& conversationId } std::string -ConversationModule::startConversation(ConversationMode mode, const std::string& otherMember) +ConversationModule::startConversation(ConversationMode mode, const dht::InfoHash& otherMember) { auto acc = pimpl_->account_.lock(); if (!acc) @@ -2035,7 +2035,7 @@ ConversationModule::startConversation(ConversationMode mode, const std::string& // Create the conversation object std::shared_ptr<Conversation> conversation; try { - conversation = std::make_shared<Conversation>(acc, mode, otherMember); + conversation = std::make_shared<Conversation>(acc, mode, otherMember.toString()); auto conversationId = conversation->id(); conversation->onMessageStatusChanged([this, conversationId](const auto& status) { auto msg = std::make_shared<SyncMsg>(); @@ -2068,8 +2068,8 @@ ConversationModule::startConversation(ConversationMode mode, const std::string& std::unique_lock lk(conv->mtx); conv->info.created = std::time(nullptr); conv->info.members.emplace(pimpl_->username_); - if (!otherMember.empty()) - conv->info.members.emplace(otherMember); + if (!otherMember) + conv->info.members.emplace(otherMember.toString()); conv->conversation = conversation; addConvInfo(conv->info); lk.unlock(); diff --git a/src/jamidht/conversation_module.h b/src/jamidht/conversation_module.h index 4b792174c5..acd2ea75ef 100644 --- a/src/jamidht/conversation_module.h +++ b/src/jamidht/conversation_module.h @@ -193,7 +193,7 @@ public: * @return conversation's id */ std::string startConversation(ConversationMode mode = ConversationMode::INVITES_ONLY, - const std::string& otherMember = ""); + const dht::InfoHash& otherMember = {}); // Message send/load void sendMessage(const std::string& conversationId, diff --git a/src/jamidht/jamiaccount.cpp b/src/jamidht/jamiaccount.cpp index 57dc05e8db..90dfd41afc 100644 --- a/src/jamidht/jamiaccount.cpp +++ b/src/jamidht/jamiaccount.cpp @@ -2782,14 +2782,19 @@ JamiAccount::getContactHeader(const std::shared_ptr<SipTransport>& sipTransport) void JamiAccount::addContact(const std::string& uri, bool confirmed) { + dht::InfoHash h(uri); + if (not h) { + JAMI_ERROR("addContact: invalid contact URI"); + return; + } auto conversation = convModule()->getOneToOneConversation(uri); if (!confirmed && conversation.empty()) - conversation = convModule()->startConversation(ConversationMode::ONE_TO_ONE, uri); + conversation = convModule()->startConversation(ConversationMode::ONE_TO_ONE, h); std::unique_lock<std::recursive_mutex> lock(configurationMutex_); if (accountManager_) - accountManager_->addContact(uri, confirmed, conversation); + accountManager_->addContact(h, confirmed, conversation); else - JAMI_WARN("[Account %s] addContact: account not loaded", getAccountID().c_str()); + JAMI_WARNING("[Account {}] addContact: account not loaded", getAccountID()); } void @@ -2799,7 +2804,7 @@ JamiAccount::removeContact(const std::string& uri, bool ban) if (accountManager_) accountManager_->removeContact(uri, ban); else - JAMI_WARN("[Account %s] removeContact: account not loaded", getAccountID().c_str()); + JAMI_WARNING("[Account {}] removeContact: account not loaded", getAccountID()); } bool @@ -2852,16 +2857,21 @@ JamiAccount::getTrustRequests() const bool JamiAccount::acceptTrustRequest(const std::string& from, bool includeConversation) { + dht::InfoHash h(from); + if (not h) { + JAMI_ERROR("addContact: invalid contact URI"); + return false; + } std::unique_lock<std::recursive_mutex> lock(configurationMutex_); if (accountManager_) { if (!accountManager_->acceptTrustRequest(from, includeConversation)) { // Note: unused for swarm // Typically the case where the trust request doesn't exists, only incoming DHT messages - return accountManager_->addContact(from, true); + return accountManager_->addContact(h, true); } return true; } - JAMI_WARN("[Account %s] acceptTrustRequest: account not loaded", getAccountID().c_str()); + JAMI_WARNING("[Account {}] acceptTrustRequest: account not loaded", getAccountID()); return false; } @@ -2909,13 +2919,18 @@ JamiAccount::declineConversationRequest(const std::string& conversationId) void JamiAccount::sendTrustRequest(const std::string& to, const std::vector<uint8_t>& payload) { + dht::InfoHash h(to); + if (not h) { + JAMI_ERROR("addContact: invalid contact URI"); + return; + } // Here we cache payload sent by the client auto requestPath = cachePath_ / "requests"; dhtnet::fileutils::recursive_mkdir(requestPath, 0700); auto cachedFile = requestPath / to; std::ofstream req(cachedFile, std::ios::trunc | std::ios::binary); if (!req.is_open()) { - JAMI_ERR("Could not write data to %s", cachedFile.c_str()); + JAMI_ERROR("Could not write data to {}", cachedFile); return; } @@ -2929,7 +2944,7 @@ JamiAccount::sendTrustRequest(const std::string& to, const std::vector<uint8_t>& auto conversation = convModule()->getOneToOneConversation(to); if (conversation.empty()) - conversation = convModule()->startConversation(ConversationMode::ONE_TO_ONE, to); + conversation = convModule()->startConversation(ConversationMode::ONE_TO_ONE, h); if (not conversation.empty()) { std::lock_guard lock(configurationMutex_); if (accountManager_) @@ -2938,9 +2953,9 @@ JamiAccount::sendTrustRequest(const std::string& to, const std::vector<uint8_t>& payload.size() >= 64000 ? std::vector<uint8_t> {} : payload); else - JAMI_WARN("[Account %s] sendTrustRequest: account not loaded", getAccountID().c_str()); + JAMI_WARNING("[Account {}] sendTrustRequest: account not loaded", getAccountID()); } else - JAMI_WARN("[Account %s] sendTrustRequest: account not loaded", getAccountID().c_str()); + JAMI_WARNING("[Account {}] sendTrustRequest: account not loaded", getAccountID()); } void diff --git a/test/unitTest/conversation/conversation.cpp b/test/unitTest/conversation/conversation.cpp index 4f49b61bba..0979182f95 100644 --- a/test/unitTest/conversation/conversation.cpp +++ b/test/unitTest/conversation/conversation.cpp @@ -2175,7 +2175,7 @@ ConversationTest::testRemoveOneToOneNotInDetails() auto firstConv = aliceData.conversationId; // Create a duplicate std::this_thread::sleep_for(2s); // Avoid to get same id - aliceAccount->convModule()->startConversation(ConversationMode::ONE_TO_ONE, bobUri); + aliceAccount->convModule()->startConversation(ConversationMode::ONE_TO_ONE, dht::InfoHash(bobUri)); CPPUNIT_ASSERT(cv.wait_for(lk, 10s, [&]() { return firstConv != aliceData.conversationId; })); // Assert that repository exists diff --git a/test/unitTest/conversation/conversationMembersEvent.cpp b/test/unitTest/conversation/conversationMembersEvent.cpp index 4d8dcefb9e..20254664b4 100644 --- a/test/unitTest/conversation/conversationMembersEvent.cpp +++ b/test/unitTest/conversation/conversationMembersEvent.cpp @@ -73,6 +73,7 @@ public: const std::string& convId, const std::string& uri); + void testAddInvalidUri(); void testRemoveConversationNoMember(); void testRemoveConversationWithMember(); void testAddMember(); @@ -125,6 +126,7 @@ public: private: CPPUNIT_TEST_SUITE(ConversationMembersEventTest); + CPPUNIT_TEST(testAddInvalidUri); CPPUNIT_TEST(testRemoveConversationNoMember); CPPUNIT_TEST(testRemoveConversationWithMember); CPPUNIT_TEST(testAddMember); @@ -389,7 +391,9 @@ ConversationMembersEventTest::connectSignals() })); confHandlers.insert(libjami::exportable_callback<libjami::ConfigurationSignal::ContactAdded>( [&](const std::string& accountId, const std::string&, bool) { - if (accountId == bobId) { + if (accountId == aliceId) { + aliceData.contactAdded = true; + } else if (accountId == bobId) { bobData.contactAdded = true; } else if (accountId == bob2Id) { bob2Data.contactAdded = true; @@ -445,6 +449,17 @@ ConversationMembersEventTest::generateFakeInvite(std::shared_ptr<JamiAccount> ac ""); } +void +ConversationMembersEventTest::testAddInvalidUri() +{ + connectSignals(); + + auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId); + aliceAccount->addContact("Shitty/Uri"); + CPPUNIT_ASSERT(!cv.wait_for(lk, 5s, [&]() { return aliceData.contactAdded; })); + CPPUNIT_ASSERT(aliceData.conversationId.empty()); +} + void ConversationMembersEventTest::testRemoveConversationNoMember() { -- GitLab