From eb641097e3bae1f0705452d26ac5372a8239217e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Blin?=
<sebastien.blin@savoirfairelinux.com>
Date: Tue, 8 Aug 2023 14:23:49 -0400
Subject: [PATCH] jamiaccount: do not open swarm channels with banned devices
Else, opening p2p socket will allow the peer to perform call actions
causing weird experiences where a banned contact is calling the user.
GitLab: #886
Change-Id: Ia76ddda03f41b6eaf6b937c11c362aa72d983cd5
---
contrib/src/dhtnet/rules.mak | 1 +
contrib/src/dhtnet/test.patch | 43 ++++++++
src/jamidht/account_manager.cpp | 2 +-
src/jamidht/jamiaccount.cpp | 2 +-
test/unitTest/certstore.cpp | 25 +++++
.../conversation/conversationRequest.cpp | 99 +++++++++++++++++++
6 files changed, 170 insertions(+), 2 deletions(-)
create mode 100644 contrib/src/dhtnet/test.patch
diff --git a/contrib/src/dhtnet/rules.mak b/contrib/src/dhtnet/rules.mak
index 5f62ffd7d4..b158b70d1a 100644
--- a/contrib/src/dhtnet/rules.mak
+++ b/contrib/src/dhtnet/rules.mak
@@ -20,6 +20,7 @@ dhtnet: dhtnet-$(DHTNET_VERSION).tar.gz
mkdir -p $(UNPACK_DIR)
$(UNPACK) -C $(UNPACK_DIR)
$(APPLY) $(SRC)/dhtnet/libjami-testable.patch
+ $(APPLY) $(SRC)/dhtnet/test.patch
$(MOVE)
.dhtnet: dhtnet toolchain.cmake .sum-dhtnet
diff --git a/contrib/src/dhtnet/test.patch b/contrib/src/dhtnet/test.patch
new file mode 100644
index 0000000000..63ec14e2a6
--- /dev/null
+++ b/contrib/src/dhtnet/test.patch
@@ -0,0 +1,43 @@
+ src/security/certstore.cpp | 28 +++++++++++++++++++++-------
+ 1 file changed, 21 insertions(+), 7 deletions(-)
+
+diff --git a/src/security/certstore.cpp b/src/security/certstore.cpp
+index 9b6bb96..2a3fa8a 100644
+--- a/src/security/certstore.cpp
++++ b/src/security/certstore.cpp
+@@ -591,14 +591,28 @@ TrustStore::PermissionStatus
+ TrustStore::getCertificateStatus(const std::string& cert_id) const
+ {
+ std::lock_guard<std::recursive_mutex> lk(mutex_);
+- auto s = certStatus_.find(cert_id);
+- if (s == std::end(certStatus_)) {
+- auto us = unknownCertStatus_.find(cert_id);
+- if (us == std::end(unknownCertStatus_))
+- return PermissionStatus::UNDEFINED;
+- return us->second.allowed ? PermissionStatus::ALLOWED : PermissionStatus::BANNED;
++ auto cert = certStore_.getCertificate(cert_id);
++ if (!cert)
++ return PermissionStatus::UNDEFINED;
++ auto banned = false;
++ auto unknown = true;
++ while (cert) {
++ auto s = certStatus_.find(cert->getId().toString());
++ unknown &= s == std::end(certStatus_);
++ if (s != std::end(certStatus_) && !s->second.second.allowed) {
++ return PermissionStatus::BANNED;
++ } else {
++ auto us = unknownCertStatus_.find(cert->getId().toString());
++ unknown &= us == std::end(unknownCertStatus_);
++ if (!us->second.allowed)
++ return PermissionStatus::BANNED;
++ }
++ if (cert->getUID() == cert->getIssuerUID())
++ break;
++ cert = certStore_.getCertificate(cert->getIssuerUID());
+ }
+- return s->second.second.allowed ? PermissionStatus::ALLOWED : PermissionStatus::BANNED;
++
++ return unknown ? PermissionStatus::UNDEFINED : PermissionStatus::ALLOWED;
+ }
+
+ std::vector<std::string>
diff --git a/src/jamidht/account_manager.cpp b/src/jamidht/account_manager.cpp
index 3559ca1fa9..47512cd36b 100644
--- a/src/jamidht/account_manager.cpp
+++ b/src/jamidht/account_manager.cpp
@@ -112,7 +112,7 @@ AccountManager::loadIdentity(const std::string& accountId,
const std::string& key_path,
const std::string& key_pwd) const
{
- // Return to avoid unnecessary log if certificate or key is missing. Example case: when
+ // Return to avoid unnecessary log if certificate or key is missing. Example case: when
// importing an account when the certificate has not been unpacked from the archive.
if (crt_path.empty() or key_path.empty())
return {};
diff --git a/src/jamidht/jamiaccount.cpp b/src/jamidht/jamiaccount.cpp
index ea0fc7483a..1cbd04cff1 100644
--- a/src/jamidht/jamiaccount.cpp
+++ b/src/jamidht/jamiaccount.cpp
@@ -2166,7 +2166,7 @@ JamiAccount::convModule()
if (!shared)
return;
std::lock_guard<std::mutex> lkCM(shared->connManagerMtx_);
- if (!shared->connectionManager_) {
+ if (!shared->connectionManager_ || shared->convModule()->isBanned(convId, deviceId)) {
Manager::instance().ioContext()->post([cb] { cb({}); });
return;
}
diff --git a/test/unitTest/certstore.cpp b/test/unitTest/certstore.cpp
index a16f5320c1..c6f1f31477 100644
--- a/test/unitTest/certstore.cpp
+++ b/test/unitTest/certstore.cpp
@@ -56,10 +56,12 @@ public:
private:
void trustStoreTest();
void getCertificateWithSplitted();
+ void testBannedParent();
CPPUNIT_TEST_SUITE(CertStoreTest);
CPPUNIT_TEST(trustStoreTest);
CPPUNIT_TEST(getCertificateWithSplitted);
+ CPPUNIT_TEST(testBannedParent);
CPPUNIT_TEST_SUITE_END();
};
@@ -214,6 +216,29 @@ CertStoreTest::getCertificateWithSplitted()
&& fullCert->issuer->issuer->getUID() == caCert->getUID());
}
+void
+CertStoreTest::testBannedParent()
+{
+ auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId);
+
+ auto ca = dht::crypto::generateIdentity("test CA");
+ auto account = dht::crypto::generateIdentity("test account", ca, 4096, true);
+ auto device = dht::crypto::generateIdentity("test device", account);
+ auto device2 = dht::crypto::generateIdentity("test device 2", account);
+ auto id = ca.second->getId().toString();
+ auto pinned = aliceAccount->certStore().getPinnedCertificates();
+ CPPUNIT_ASSERT(std::find_if(pinned.begin(), pinned.end(), [&](auto v) { return v == id; })
+ == pinned.end());
+
+ // Ban account
+ aliceAccount->setCertificateStatus(account.second, dhtnet::tls::TrustStore::PermissionStatus::BANNED);
+ CPPUNIT_ASSERT(aliceAccount->accountManager()->getCertificateStatus(account.second->getId().toString())
+ == dhtnet::tls::TrustStore::PermissionStatus::BANNED);
+ CPPUNIT_ASSERT(not aliceAccount->accountManager()->isAllowed(*account.second));
+ CPPUNIT_ASSERT(not aliceAccount->accountManager()->isAllowed(*device2.second));
+ CPPUNIT_ASSERT(not aliceAccount->accountManager()->isAllowed(*device.second));
+}
+
} // namespace test
} // namespace jami
diff --git a/test/unitTest/conversation/conversationRequest.cpp b/test/unitTest/conversation/conversationRequest.cpp
index 09b7fee580..bc69699fa2 100644
--- a/test/unitTest/conversation/conversationRequest.cpp
+++ b/test/unitTest/conversation/conversationRequest.cpp
@@ -68,6 +68,7 @@ public:
void testRemoveSelfDoesntRemoveConversation();
void testRemoveConversationUpdateContactDetails();
void testBanContact();
+ void testBanContactRestartAccount();
void testBanContactRemoveTrustRequest();
void testAddOfflineContactThenConnect();
void testDeclineTrustRequestDoNotGenerateAnother();
@@ -97,6 +98,7 @@ private:
CPPUNIT_TEST(testRemoveSelfDoesntRemoveConversation);
CPPUNIT_TEST(testRemoveConversationUpdateContactDetails);
CPPUNIT_TEST(testBanContact);
+ CPPUNIT_TEST(testBanContactRestartAccount);
CPPUNIT_TEST(testBanContactRemoveTrustRequest);
CPPUNIT_TEST(testAddOfflineContactThenConnect);
CPPUNIT_TEST(testDeclineTrustRequestDoNotGenerateAnother);
@@ -985,6 +987,103 @@ ConversationRequestTest::testBanContact()
CPPUNIT_ASSERT(fileutils::isDirectory(repoPath));
}
+
+void
+ConversationRequestTest::testBanContactRestartAccount()
+{
+ auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId);
+ auto bobAccount = Manager::instance().getAccount<JamiAccount>(bobId);
+ auto bobUri = bobAccount->getUsername();
+ auto aliceUri = aliceAccount->getUsername();
+ std::mutex mtx;
+ std::unique_lock<std::mutex> lk {mtx};
+ std::condition_variable cv;
+ std::map<std::string, std::shared_ptr<libjami::CallbackWrapperBase>> confHandlers;
+ bool conversationReady = false, requestReceived = false, memberMessageGenerated = false;
+ std::string convId = "";
+ confHandlers.insert(
+ libjami::exportable_callback<libjami::ConfigurationSignal::IncomingTrustRequest>(
+ [&](const std::string& account_id,
+ const std::string& /*from*/,
+ const std::string& /*conversationId*/,
+ const std::vector<uint8_t>& /*payload*/,
+ time_t /*received*/) {
+ if (account_id == bobId)
+ requestReceived = true;
+ cv.notify_one();
+ }));
+ confHandlers.insert(libjami::exportable_callback<libjami::ConversationSignal::ConversationReady>(
+ [&](const std::string& accountId, const std::string& conversationId) {
+ if (accountId == aliceId) {
+ convId = conversationId;
+ } else if (accountId == bobId) {
+ conversationReady = true;
+ }
+ cv.notify_one();
+ }));
+ confHandlers.insert(libjami::exportable_callback<libjami::ConversationSignal::MessageReceived>(
+ [&](const std::string& accountId,
+ const std::string& conversationId,
+ std::map<std::string, std::string> message) {
+ if (accountId == aliceId && conversationId == convId && message["type"] == "member") {
+ memberMessageGenerated = true;
+ }
+ cv.notify_one();
+ }));
+ bool contactRemoved = false;
+ confHandlers.insert(libjami::exportable_callback<libjami::ConfigurationSignal::ContactRemoved>(
+ [&](const std::string& accountId, const std::string& uri, bool) {
+ if (accountId == bobId && uri == aliceUri) {
+ contactRemoved = true;
+ }
+ cv.notify_one();
+ }));
+ auto bobConnected = false;
+ confHandlers.insert(
+ libjami::exportable_callback<libjami::ConfigurationSignal::VolatileDetailsChanged>(
+ [&](const std::string&, const std::map<std::string, std::string>&) {
+ auto details = bobAccount->getVolatileAccountDetails();
+ auto daemonStatus = details[libjami::Account::ConfProperties::Registration::STATUS];
+ if (daemonStatus == "REGISTERED") {
+ bobConnected = true;
+ } else if (daemonStatus == "UNREGISTERED") {
+ bobConnected = false;
+ }
+ cv.notify_one();
+ }));
+ libjami::registerSignalHandlers(confHandlers);
+
+ aliceAccount->addContact(bobUri);
+ aliceAccount->sendTrustRequest(bobUri, {});
+ CPPUNIT_ASSERT(cv.wait_for(lk, 5s, [&]() { return !convId.empty(); }));
+ // Check created files
+ CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return requestReceived; }));
+ memberMessageGenerated = false;
+ CPPUNIT_ASSERT(bobAccount->acceptTrustRequest(aliceUri));
+ CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return memberMessageGenerated; }));
+
+ memberMessageGenerated = false;
+ bobAccount->removeContact(aliceUri, true);
+ CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return contactRemoved; }));
+ std::this_thread::sleep_for(10s); // Wait a bit to ensure that everything is updated
+ bobConnected = true;
+ Manager::instance().sendRegister(bobId, false);
+ CPPUNIT_ASSERT(cv.wait_for(lk, 30s, [&]() { return !bobConnected; }));
+ std::this_thread::sleep_for(5s); // Wait a bit to ensure that everything is updated
+
+ // Connect bob, it will trigger the bootstrap
+ auto statusBootstrap = Conversation::BootstrapStatus::SUCCESS;
+ // alice is banned so bootstrap MUST fail
+ bobAccount->convModule()->onBootstrapStatus(
+ [&](std::string /*convId*/, Conversation::BootstrapStatus status) {
+ statusBootstrap = status;
+ cv.notify_one();
+ });
+ Manager::instance().sendRegister(bobId, true);
+ CPPUNIT_ASSERT(cv.wait_for(lk, 60s, [&]() { return statusBootstrap == Conversation::BootstrapStatus::FAILED; }));
+
+}
+
void
ConversationRequestTest::testBanContactRemoveTrustRequest()
{
--
GitLab