jami/jami-daemon
1
0
Fork 0
mirror of https://git.jami.net/savoirfairelinux/jami-daemon.git synced 2025-08-12 22:09:25 +08:00
Code Issues Projects Releases Wiki Activity

swarm: verify all certificates changes

Browse Source 
In some cases, a user in the swarm can update its certificates.
However, the new certificate MUST be checked and MUST be signed
by the account.
So, this patch validates two scenarios:
+ Check that a fetch error is sent to the client whenever an invalid
certificate is detected in the original clone
+ Check that a fetch error is sent to the client whenever a
certificate is replaced during the conversation.

Change-Id: Ieb15fb6444dcf4541f00c511a9f4ba0c64617130
This commit is contained in:
Sébastien Blin
2022-01-17 12:31:01 -05:00
parent d2d18bdedc
commit 53cf11bd0c
2 changed files with 309 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

135
src/jamidht/conversationrepository.cpp
View File

@ -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);
}

190
test/unitTest/conversation/conversation.cpp
View File

@ -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()
{