From 6b6d556e982d69bf187f76c7a45752e7352e23c9 Mon Sep 17 00:00:00 2001 From: csoler Date: Mon, 1 Nov 2021 16:14:13 +0100 Subject: [PATCH] added nonce system for safe peer removal --- libretroshare/src/friend_server/fsitem.h | 15 +++- retroshare-friendserver/src/friendserver.cc | 80 +++++++++++++++------ retroshare-friendserver/src/friendserver.h | 3 + 3 files changed, 76 insertions(+), 22 deletions(-) diff --git a/libretroshare/src/friend_server/fsitem.h b/libretroshare/src/friend_server/fsitem.h index e3990b19f..d523f5726 100644 --- a/libretroshare/src/friend_server/fsitem.h +++ b/libretroshare/src/friend_server/fsitem.h @@ -76,10 +76,23 @@ class RsFriendServerClientRemoveItem: public RsFriendServerItem public: RsFriendServerClientRemoveItem() : RsFriendServerItem(RS_PKT_SUBTYPE_FS_CLIENT_REMOVE) {} - void serial_process(RsGenericSerializer::SerializeJob /* j */,RsGenericSerializer::SerializeContext& /* ctx */) + void serial_process(RsGenericSerializer::SerializeJob j,RsGenericSerializer::SerializeContext& ctx) { + RS_SERIAL_PROCESS(peer_id); + RS_SERIAL_PROCESS(nonce); } + + // Peer ID for the peer to remove. + + RsPeerId peer_id; + + // Nonce that was returned by the server after the last client request. Should match in order to proceed. This prevents + // a malicious actor from removing peers from the server. Since the nonce is sent through Tor tunnels, it cannot be known by + // anyone else than the client. + + uint64_t nonce; }; + class RsFriendServerEncryptedServerResponseItem: public RsFriendServerItem { public: diff --git a/retroshare-friendserver/src/friendserver.cc b/retroshare-friendserver/src/friendserver.cc index 15b5f3801..b2c2ba38b 100644 --- a/retroshare-friendserver/src/friendserver.cc +++ b/retroshare-friendserver/src/friendserver.cc @@ -66,21 +66,44 @@ void FriendServer::handleClientPublish(const RsFriendServerClientPublishItem *it { try { - RsDbg() << "Received a client publish item from " << item->PeerId() ; + RsDbg() << "Received a client publish item from " << item->PeerId() << ":"; RsDbg() << *item ; // First of all, read PGP key and short invites, parse them, and check that they contain the same information + FriendServer::handleIncomingClientData(item->pgp_public_key_b64,item->short_invite); + + // Respond with a list of potential friends + + + } + catch(std::exception& e) + { + RsErr() << "ERROR: " << e.what() ; + } + + // Close client connection from server side, to tell the client that nothing more is coming. + + RsDbg() << "Closing client connection." ; + + mni->closeConnection(item->PeerId()); +} + +bool FriendServer::handleIncomingClientData(const std::string& pgp_public_key_b64,const std::string& short_invite_b64) +{ RsDbg() << " Checking item data..."; std::string error_string; RsPgpId pgp_id ; std::vector key_binary_data ; - key_binary_data = Radix64::decode(item->pgp_public_key_b64); + key_binary_data = Radix64::decode(pgp_public_key_b64); if(key_binary_data.empty()) - throw std::runtime_error(" Cannot decode client pgp public key: \"" + item->pgp_public_key_b64 + "\". Wrong format??"); + throw std::runtime_error(" Cannot decode client pgp public key: \"" + pgp_public_key_b64 + "\". Wrong format??"); + +// Apparently RsBase64 doesn't work correctly. +// // if(!RsBase64::decode(item->pgp_public_key_b64,key_binary_data)) // throw std::runtime_error(" Cannot decode client pgp public key: \"" + item->pgp_public_key_b64 + "\". Wrong format??"); @@ -94,7 +117,7 @@ void FriendServer::handleClientPublish(const RsFriendServerClientPublishItem *it RsPeerDetails shortInviteDetails; uint32_t errorCode = 0; - if(item->short_invite.empty() || !RsCertificate::decodeRadix64ShortInvite(item->short_invite, shortInviteDetails,errorCode )) + if(short_invite_b64.empty() || !RsCertificate::decodeRadix64ShortInvite(short_invite_b64, shortInviteDetails,errorCode )) throw std::runtime_error("Could not parse short certificate. Error = " + RsUtil::NumberToString(errorCode)); RsDbg() << " Short invite is fine. PGP fingerprint: " << shortInviteDetails.fpr ; @@ -106,38 +129,53 @@ void FriendServer::handleClientPublish(const RsFriendServerClientPublishItem *it if(fpr_test != shortInviteDetails.fpr) throw std::runtime_error("Cannot get fingerprint from keyring for client public key. Something's really wrong.") ; - RsDbg() << " Short invite PGP fingerprint matches the public key fingerprint." ; + RsDbg() << " Short invite PGP fingerprint matches the public key fingerprint."; + RsDbg() << " Sync-ing the PGP keyring on disk"; - // Check the item's data signature + mPgpHandler->syncDatabase(); + + // Check the item's data signature. Is that needed? Not sure, since the data is sent PGP-encrypted, so only the owner + // of the secret PGP key can actually use it. +#warning TODO // All good. -#warning TODO // Store/update the peer info auto& pi(mCurrentClientPeers[shortInviteDetails.id]); - pi.short_certificate = item->short_invite; + pi.short_certificate = short_invite_b64; pi.last_connection_TS = time(nullptr); + pi.last_nonce = RsRandom::random_u64(); - // Respond with a list of potential friends - } - catch(std::exception& e) - { - RsErr() << e.what() ; - } - - // Close client connection from server side, to tell the client that nothing more is coming. - - RsDbg() << "Closing client connection." ; - - mni->closeConnection(item->PeerId()); + return true; } + void FriendServer::handleClientRemove(const RsFriendServerClientRemoveItem *item) { RsDbg() << "Received a client remove item:" << *item ; + + auto it = mCurrentClientPeers.find(item->peer_id); + + if(it == mCurrentClientPeers.end()) + { + RsErr() << " ERROR: Client " << item->peer_id << " is not known to the server." ; + return; + } + + if(it->second.last_nonce != item->nonce) + { + RsErr() << " ERROR: Client supplied a nonce " << std::hex << item->nonce << std::dec << " that is not correct (expected " + << std::hex << it->second.last_nonce << std::dec << ")"; + return; + } + + RsDbg() << " Nonce is correct: " << std::hex << item->nonce << std::dec << ". Removing peer " << item->peer_id ; + + mCurrentClientPeers.erase(it); } + FriendServer::FriendServer(const std::string& base_dir) { RsDbg() << "Creating friend server." ; @@ -191,7 +229,7 @@ void FriendServer::debugPrint() rstime_t now = time(nullptr); for(auto& it:mCurrentClientPeers) - RsDbg() << " " << it.first << ": " << "last contact: " << now - it.second.last_connection_TS; + RsDbg() << " " << it.first << ": nonce=" << std::hex << it.second.last_nonce << std::dec << ", last contact: " << now - it.second.last_connection_TS << " secs ago."; RsDbg() << "==============================================="; diff --git a/retroshare-friendserver/src/friendserver.h b/retroshare-friendserver/src/friendserver.h index 49dd89359..711256d69 100644 --- a/retroshare-friendserver/src/friendserver.h +++ b/retroshare-friendserver/src/friendserver.h @@ -34,6 +34,7 @@ struct PeerInfo { std::string short_certificate; rstime_t last_connection_TS; + uint64_t last_nonce; }; class FriendServer : public RsTickingThread @@ -52,6 +53,8 @@ private: void handleClientRemove(const RsFriendServerClientRemoveItem *item); void handleClientPublish(const RsFriendServerClientPublishItem *item); + bool handleIncomingClientData(const std::string& pgp_public_key_b64,const std::string& short_invite_b64); + void autoWash(); void debugPrint();