diff --git a/src/jamidht/conversationrepository.cpp b/src/jamidht/conversationrepository.cpp
index 7e76bf92911cd2f1f5f10dcd0113516cbf76f73a..7c3c02b6e255f657e1f91c7cd3c8f610e3f99930 100644
--- a/src/jamidht/conversationrepository.cpp
+++ b/src/jamidht/conversationrepository.cpp
@@ -203,6 +203,64 @@ public:
}
mutable std::mutex deviceToUriMtx_;
mutable std::map<std::string, std::string> deviceToUri_;
+
+ /**
+ * Verify that a certificate modification is correct
+ * @param certPath Where the certificate is saved (relative path)
+ * @param userUri Account we want for this certificate
+ * @param oldCert Previous certificate. getId() should return the same id as the new
+ * certificate.
+ * @note There is a few exception because JAMS certificates are buggy right now
+ */
+ bool verifyCertificate(const std::string& certContent,
+ const std::string& userUri,
+ const std::string& oldCert = "") const
+ {
+ auto cert = dht::crypto::Certificate(certContent);
+ auto isDeviceCertificate = cert.getId().toString() != userUri;
+ auto issuerUid = cert.getIssuerUID();
+ if (isDeviceCertificate && issuerUid.empty()) {
+ // Err for Jams certificates
+ JAMI_ERR("Empty issuer for %s", cert.getId().to_c_str());
+ }
+ if (!oldCert.empty()) {
+ auto deviceCert = dht::crypto::Certificate(oldCert);
+ if (isDeviceCertificate) {
+ if (issuerUid != deviceCert.getIssuerUID()) {
+ // NOTE: Here, because JAMS certificate can be incorrectly formatted, there is
+ // just one valid possibility: passing from an empty issuer to
+ // the valid issuer.
+ if (issuerUid != userUri) {
+ JAMI_ERR("Device certificate with a bad issuer %s", cert.getId().to_c_str());
+ return false;
+ }
+ }
+ } else if (cert.getId().toString() != userUri) {
+ JAMI_ERR("Certificate with a bad Id %s", cert.getId().to_c_str());
+ return false;
+ }
+ if (cert.getId() != deviceCert.getId()) {
+ JAMI_ERR("Certificate with a bad Id %s", cert.getId().to_c_str());
+ return false;
+ }
+ return true;
+ }
+
+ // If it's a device certificate, we need to verify that the issuer is not modified
+ if (isDeviceCertificate) {
+ // Check that issuer is the one we want.
+ // NOTE: Still one case due to incorrectly formatted certificates from JAMS
+ if (issuerUid != userUri && !issuerUid.empty()) {
+ JAMI_ERR("Device certificate with a bad issuer %s", cert.getId().to_c_str());
+ return false;
+ }
+ } else if (cert.getId().toString() != userUri) {
+ JAMI_ERR("Certificate with a bad Id %s", cert.getId().to_c_str());
+ return false;
+ }
+
+ return true;
+ }
};
/////////////////////////////////////////////////////////////////////////////////
@@ -729,21 +787,46 @@ ConversationRepository::Impl::checkValidUserDiff(const std::string& userDevice,
std::string adminsFile = fmt::format("admins/{}.crt", userUri);
std::string membersFile = fmt::format("members/{}.crt", userUri);
+ auto treeOld = treeAtCommit(repo.get(), parentId);
+ if (not treeNew or not treeOld)
+ return false;
for (const auto& changedFile : changedFiles) {
if (changedFile == adminsFile || changedFile == membersFile) {
// In this case, we should verify it's not added (normal commit, not a member change)
// but only updated
- auto treeOld = treeAtCommit(repo.get(), parentId);
- if (not treeNew or not treeOld)
- return false;
-
- if (!fileAtTree(changedFile, treeOld)) {
+ auto oldFile = fileAtTree(changedFile, treeOld);
+ if (!oldFile) {
JAMI_ERR("Invalid file modified: %s", changedFile.c_str());
return false;
}
+ auto* blob = reinterpret_cast<git_blob*>(oldFile.get());
+ auto oldCert = std::string(static_cast<const char*>(git_blob_rawcontent(blob)),
+ git_blob_rawsize(blob));
+ auto newFile = fileAtTree(changedFile, treeNew);
+ blob = reinterpret_cast<git_blob*>(newFile.get());
+ auto newCert = std::string(static_cast<const char*>(git_blob_rawcontent(blob)),
+ git_blob_rawsize(blob));
+ if (!verifyCertificate(newCert, userUri, oldCert)) {
+ JAMI_ERR("Invalid certificate %s", changedFile.c_str());
+ return false;
+ }
} else if (changedFile == deviceFile) {
// In this case, device is added or modified (certificate expiration)
- continue;
+ auto oldFile = fileAtTree(changedFile, treeOld);
+ std::string oldCert;
+ if (oldFile) {
+ auto* blob = reinterpret_cast<git_blob*>(oldFile.get());
+ auto oldCert = std::string(static_cast<const char*>(git_blob_rawcontent(blob)),
+ git_blob_rawsize(blob));
+ }
+ auto newFile = fileAtTree(changedFile, treeNew);
+ auto* blob = reinterpret_cast<git_blob*>(newFile.get());
+ auto newCert = std::string(static_cast<const char*>(git_blob_rawcontent(blob)),
+ git_blob_rawsize(blob));
+ if (!verifyCertificate(newCert, userUri, oldCert)) {
+ JAMI_ERR("Invalid certificate %s", changedFile.c_str());
+ return false;
+ }
} else {
// Invalid file detected
JAMI_ERR("Invalid add file detected: %s %u", changedFile.c_str(), (int) *mode_);
@@ -1265,8 +1348,11 @@ ConversationRepository::Impl::checkInitialCommit(const std::string& userDevice,
const std::string& commitMsg) const
{
auto account = account_.lock();
- if (!account)
+ auto repo = repository();
+ if (not account or not repo) {
+ JAMI_WARN("Invalid repository detected");
return false;
+ }
auto userUri = uriFromDevice(userDevice);
auto changedFiles = ConversationRepository::changedFiles(diffStats(commitId, ""));
// NOTE: libgit2 return a diff with /, not DIR_SEPARATOR_DIR
@@ -1296,6 +1382,22 @@ ConversationRepository::Impl::checkInitialCommit(const std::string& userDevice,
std::string deviceFile = fmt::format("devices/{}.crt", userDevice);
std::string crlFile = fmt::format("CRLs/{}", userUri);
std::string invitedFile = fmt::format("invited/{}", invited);
+
+ git_oid oid;
+ git_commit* commit = nullptr;
+ if (git_oid_fromstr(&oid, commitId.c_str()) < 0
+ || git_commit_lookup(&commit, repo.get(), &oid) < 0) {
+ JAMI_WARN("Failed to look up commit %s", commitId.c_str());
+ return false;
+ }
+ GitCommit gc = {commit, git_commit_free};
+ git_tree* tNew = nullptr;
+ if (git_commit_tree(&tNew, gc.get()) < 0) {
+ JAMI_ERR("Could not look up initial tree");
+ return false;
+ }
+ GitTree treeNew = {tNew, git_tree_free};
+
// Check that admin cert is added
// Check that device cert is added
// Check CRLs added
@@ -1304,10 +1406,27 @@ ConversationRepository::Impl::checkInitialCommit(const std::string& userDevice,
for (const auto& changedFile : changedFiles) {
if (changedFile == adminsFile) {
hasAdmin = true;
+ auto newFile = fileAtTree(changedFile, treeNew);
+ auto* blob = reinterpret_cast<git_blob*>(newFile.get());
+ auto newCert = std::string(static_cast<const char*>(git_blob_rawcontent(blob)),
+ git_blob_rawsize(blob));
+ if (!verifyCertificate(newCert, userUri)) {
+ JAMI_ERR("Invalid certificate found %s", changedFile.c_str());
+ return false;
+ }
} else if (changedFile == deviceFile) {
hasDevice = true;
+ auto newFile = fileAtTree(changedFile, treeNew);
+ auto* blob = reinterpret_cast<git_blob*>(newFile.get());
+ auto newCert = std::string(static_cast<const char*>(git_blob_rawcontent(blob)),
+ git_blob_rawsize(blob));
+ if (!verifyCertificate(newCert, userUri)) {
+ JAMI_ERR("Invalid certificate found %s", changedFile.c_str());
+ return false;
+ }
} else if (changedFile == crlFile || changedFile == invitedFile) {
// Nothing to do
+ continue;
} else {
// Invalid file detected
JAMI_ERR("Invalid add file detected: %s %u", changedFile.c_str(), (int) *mode_);
@@ -2849,7 +2968,7 @@ ConversationRepository::leave()
// Devices
for (const auto& d : account->getKnownDevices()) {
- std::string deviceFile = repoPath + "devices" + "/" + d.first + ".crt";
+ std::string deviceFile = fmt::format("{}/devices/{}.crt", repoPath, d.first);
if (fileutils::isFile(deviceFile)) {
fileutils::removeAll(deviceFile, true);
}
diff --git a/test/unitTest/conversation/conversation.cpp b/test/unitTest/conversation/conversation.cpp
index dccdd208e81d0bbb0a4517da9f97afef2e165fdc..95ed632f2275dbcc42c6cb6cbeb787347f7e0a5e 100644
--- a/test/unitTest/conversation/conversation.cpp
+++ b/test/unitTest/conversation/conversation.cpp
@@ -37,6 +37,7 @@
#include "fileutils.h"
#include "jami.h"
#include "manager.h"
+#include "security/certstore.h"
using namespace std::string_literals;
using namespace DRing::Account;
@@ -61,7 +62,8 @@ public:
static std::string name() { return "Conversation"; }
void setUp();
void tearDown();
- std::string createFakeConversation(std::shared_ptr<JamiAccount> account);
+ std::string createFakeConversation(std::shared_ptr<JamiAccount> account,
+ const std::string& fakeCert = "");
std::string aliceId;
std::string bobId;
@@ -74,6 +76,7 @@ private:
void testGetConversationsAfterRm();
void testRemoveInvalidConversation();
void testSendMessage();
+ void testReplaceWithBadCertificate();
void testSendMessageTriggerMessageReceived();
void testMergeTwoDifferentHeads();
void testSendMessageToMultipleParticipants();
@@ -92,6 +95,7 @@ private:
// LATER void testAdminRemoveConversationShouldPromoteOther();
void testNoBadFileInInitialCommit();
+ void testNoBadCertInInitialCommit();
void testPlainTextNoBadFile();
void testVoteNoBadFile();
void testETooBigClone();
@@ -116,6 +120,7 @@ private:
CPPUNIT_TEST(testGetConversationsAfterRm);
CPPUNIT_TEST(testRemoveInvalidConversation);
CPPUNIT_TEST(testSendMessage);
+ CPPUNIT_TEST(testReplaceWithBadCertificate);
CPPUNIT_TEST(testSendMessageTriggerMessageReceived);
CPPUNIT_TEST(testSendMessageToMultipleParticipants);
CPPUNIT_TEST(testPingPongMessages);
@@ -124,6 +129,7 @@ private:
CPPUNIT_TEST(testSetMessageDisplayedPreference);
CPPUNIT_TEST(testVoteNonEmpty);
CPPUNIT_TEST(testNoBadFileInInitialCommit);
+ CPPUNIT_TEST(testNoBadCertInInitialCommit);
CPPUNIT_TEST(testPlainTextNoBadFile);
CPPUNIT_TEST(testVoteNoBadFile);
CPPUNIT_TEST(testETooBigClone);
@@ -359,7 +365,99 @@ ConversationTest::testSendMessage()
DRing::sendMessage(aliceId, convId, "hi"s, "");
cv.wait_for(lk, std::chrono::seconds(30), [&]() { return messageBobReceived == 1; });
- DRing::unregisterSignalHandlers();
+}
+
+void
+ConversationTest::testReplaceWithBadCertificate()
+{
+ auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId);
+ auto bobAccount = Manager::instance().getAccount<JamiAccount>(bobId);
+ auto bobUri = bobAccount->getUsername();
+
+ std::mutex mtx;
+ std::unique_lock<std::mutex> lk {mtx};
+ std::condition_variable cv;
+ std::map<std::string, std::shared_ptr<DRing::CallbackWrapperBase>> confHandlers;
+ auto messageBobReceived = 0, messageAliceReceived = 0;
+ confHandlers.insert(DRing::exportable_callback<DRing::ConversationSignal::MessageReceived>(
+ [&](const std::string& accountId,
+ const std::string& /* conversationId */,
+ std::map<std::string, std::string> /*message*/) {
+ if (accountId == bobId) {
+ messageBobReceived += 1;
+ } else {
+ messageAliceReceived += 1;
+ }
+ cv.notify_one();
+ }));
+ auto requestReceived = false;
+ confHandlers.insert(
+ DRing::exportable_callback<DRing::ConversationSignal::ConversationRequestReceived>(
+ [&](const std::string& /*accountId*/,
+ const std::string& /* conversationId */,
+ std::map<std::string, std::string> /*metadatas*/) {
+ requestReceived = true;
+ cv.notify_one();
+ }));
+ auto conversationReady = false;
+ confHandlers.insert(DRing::exportable_callback<DRing::ConversationSignal::ConversationReady>(
+ [&](const std::string& accountId, const std::string& /* conversationId */) {
+ if (accountId == bobId) {
+ conversationReady = true;
+ cv.notify_one();
+ }
+ }));
+ auto errorDetected = false;
+ confHandlers.insert(DRing::exportable_callback<DRing::ConversationSignal::OnConversationError>(
+ [&](const std::string& /* accountId */,
+ const std::string& /* conversationId */,
+ int code,
+ const std::string& /* what */) {
+ if (code == 3)
+ errorDetected = true;
+ cv.notify_one();
+ }));
+ DRing::registerSignalHandlers(confHandlers);
+
+ auto convId = DRing::startConversation(aliceId);
+
+ DRing::addConversationMember(aliceId, convId, bobUri);
+ CPPUNIT_ASSERT(cv.wait_for(lk, std::chrono::seconds(30), [&]() { return requestReceived; }));
+
+ DRing::acceptConversationRequest(bobId, convId);
+ CPPUNIT_ASSERT(cv.wait_for(lk, std::chrono::seconds(30), [&]() { return conversationReady; }));
+
+ // Wait that alice sees Bob
+ cv.wait_for(lk, std::chrono::seconds(30), [&]() { return messageAliceReceived == 2; });
+
+ // Replace alice's certificate with a bad one.
+ auto repoPath = fmt::format("{}/{}/conversations/{}",
+ fileutils::get_data_dir(),
+ aliceAccount->getAccountID(),
+ convId);
+ auto aliceDevicePath = fmt::format("{}/devices/{}.crt",
+ repoPath,
+ aliceAccount->currentDeviceId());
+ auto bobDevicePath = fmt::format("{}/devices/{}.crt", repoPath, bobAccount->currentDeviceId());
+ std::filesystem::copy(bobDevicePath,
+ aliceDevicePath,
+ std::filesystem::copy_options::overwrite_existing);
+ addAll(aliceAccount, convId);
+
+ // Note: Do not use DRing::sendMessage as it will replace the invalid certificate by a valid one
+ Json::Value root;
+ root["type"] = "text/plain";
+ root["body"] = "hi";
+ Json::StreamWriterBuilder wbuilder;
+ wbuilder["commentStyle"] = "None";
+ wbuilder["indentation"] = "";
+ auto message = Json::writeString(wbuilder, root);
+ messageBobReceived = 0;
+ commitInRepo(repoPath, aliceAccount, message);
+ // now we need to sync!
+ DRing::sendMessage(aliceId, convId, "trigger sync!"s, "");
+ // We should detect the incorrect commit!
+ cv.wait_for(lk, std::chrono::seconds(30), [&]() { return errorDetected; });
}
void
@@ -1035,7 +1133,8 @@ ConversationTest::testBanDevice()
}*/
std::string
-ConversationTest::createFakeConversation(std::shared_ptr<JamiAccount> account)
+ConversationTest::createFakeConversation(std::shared_ptr<JamiAccount> account,
+ const std::string& fakeCert)
{
auto repoPath = fileutils::get_data_dir() + DIR_SEPARATOR_STR + account->getAccountID()
+ DIR_SEPARATOR_STR + "conversations" + DIR_SEPARATOR_STR + "tmp";
@@ -1084,21 +1183,23 @@ ConversationTest::createFakeConversation(std::shared_ptr<JamiAccount> account)
}
// /devices
- std::string devicePath = devicesPath + DIR_SEPARATOR_STR + cert->getLongId().toString()
- + ".crt";
+ std::string devicePath = fmt::format("{}/{}.crt", devicesPath, cert->getLongId().toString());
file = fileutils::ofstream(devicePath, std::ios::trunc | std::ios::binary);
if (!file.is_open()) {
JAMI_ERR("Could not write data to %s", devicePath.c_str());
}
- file << deviceCert;
+ file << (fakeCert.empty() ? deviceCert : fakeCert);
file.close();
if (!fileutils::recursive_mkdir(crlsPath, 0700)) {
JAMI_ERR("Error when creating %s. Abort create conversations", crlsPath.c_str());
}
- std::string badFile = repoPath + DIR_SEPARATOR_STR + "BAD";
- file = fileutils::ofstream(badFile, std::ios::trunc | std::ios::binary);
+ if (fakeCert.empty()) {
+ // Add a unwanted file
+ std::string badFile = repoPath + DIR_SEPARATOR_STR + "BAD";
+ file = fileutils::ofstream(badFile, std::ios::trunc | std::ios::binary);
+ }
addAll(account, "tmp");
@@ -1362,6 +1463,79 @@ ConversationTest::testNoBadFileInInitialCommit()
CPPUNIT_ASSERT(cv.wait_for(lk, std::chrono::seconds(30), [&]() { return errorDetected; }));
}
+void
+ConversationTest::testNoBadCertInInitialCommit()
+{
+ auto aliceAccount = Manager::instance().getAccount<JamiAccount>(aliceId);
+ auto carlaAccount = Manager::instance().getAccount<JamiAccount>(carlaId);
+ auto carlaUri = carlaAccount->getUsername();
+ auto aliceUri = aliceAccount->getUsername();
+ auto fakeCert = jami::tls::CertificateStore::instance().getCertificate(
+ std::string(aliceAccount->currentDeviceId()));
+ auto carlaCert = jami::tls::CertificateStore::instance().getCertificate(
+ std::string(carlaAccount->currentDeviceId()));
+
+ CPPUNIT_ASSERT(fakeCert);
+ // Create a conversation from Carla with Alice's device
+ auto convId = createFakeConversation(carlaAccount, fakeCert->toString(false));
+
+ std::mutex mtx;
+ std::unique_lock<std::mutex> lk {mtx};
+ std::condition_variable cv;
+ std::map<std::string, std::shared_ptr<DRing::CallbackWrapperBase>> confHandlers;
+ auto messageBobReceived = 0, messageAliceReceived = 0;
+ bool requestReceived = false;
+ bool carlaConnected = false;
+ bool errorDetected = false;
+ confHandlers.insert(DRing::exportable_callback<DRing::ConversationSignal::MessageReceived>(
+ [&](const std::string& accountId,
+ const std::string& /* conversationId */,
+ std::map<std::string, std::string> /*message*/) {
+ if (accountId == bobId) {
+ messageBobReceived += 1;
+ } else {
+ messageAliceReceived += 1;
+ }
+ cv.notify_one();
+ }));
+ confHandlers.insert(
+ DRing::exportable_callback<DRing::ConversationSignal::ConversationRequestReceived>(
+ [&](const std::string& /*accountId*/,
+ const std::string& /* conversationId */,
+ std::map<std::string, std::string> /*metadatas*/) {
+ requestReceived = true;
+ cv.notify_one();
+ }));
+ confHandlers.insert(
+ DRing::exportable_callback<DRing::ConfigurationSignal::VolatileDetailsChanged>(
+ [&](const std::string&, const std::map<std::string, std::string>&) {
+ auto details = carlaAccount->getVolatileAccountDetails();
+ auto deviceAnnounced = details[DRing::Account::VolatileProperties::DEVICE_ANNOUNCED];
+ if (deviceAnnounced == "true") {
+ carlaConnected = true;
+ cv.notify_one();
+ }
+ }));
+ confHandlers.insert(DRing::exportable_callback<DRing::ConversationSignal::OnConversationError>(
+ [&](const std::string& /* accountId */,
+ const std::string& /* conversationId */,
+ int code,
+ const std::string& /* what */) {
+ if (code == 3)
+ errorDetected = true;
+ cv.notify_one();
+ }));
+ DRing::registerSignalHandlers(confHandlers);
+
+ Manager::instance().sendRegister(carlaId, true);
+ CPPUNIT_ASSERT(cv.wait_for(lk, std::chrono::seconds(30), [&] { return carlaConnected; }));
+ DRing::addConversationMember(carlaId, convId, aliceUri);
+ CPPUNIT_ASSERT(cv.wait_for(lk, std::chrono::seconds(30), [&]() { return requestReceived; }));
+
+ DRing::acceptConversationRequest(aliceId, convId);
+ CPPUNIT_ASSERT(cv.wait_for(lk, std::chrono::seconds(30), [&]() { return errorDetected; }));
+}
+
void
ConversationTest::testPlainTextNoBadFile()
{