From 5660c73175d77dedd6f8f19ff3dc70963358011a Mon Sep 17 00:00:00 2001 From: Gioacchino Mazzurco Date: Fri, 27 Sep 2019 14:15:55 +0200 Subject: [PATCH 1/3] Allow friend information update from short invite Add comments explaining security of addSslOnlyFriend --- libretroshare/src/pqi/p3peermgr.cc | 104 +++++++++++++++++++---------- 1 file changed, 69 insertions(+), 35 deletions(-) diff --git a/libretroshare/src/pqi/p3peermgr.cc b/libretroshare/src/pqi/p3peermgr.cc index 1adf191d1..01f245a25 100644 --- a/libretroshare/src/pqi/p3peermgr.cc +++ b/libretroshare/src/pqi/p3peermgr.cc @@ -1071,45 +1071,81 @@ bool p3PeerMgrIMPL::addFriend(const RsPeerId& input_id, const RsPgpId& input_gpg } -bool p3PeerMgrIMPL::addSslOnlyFriend( const RsPeerId& sslId, const RsPgpId& pgp_id,const RsPeerDetails& dt ) +bool p3PeerMgrIMPL::addSslOnlyFriend( + const RsPeerId& sslId, const RsPgpId& pgp_id, const RsPeerDetails& dt ) { - if(sslId.isNull() || sslId == getOwnId()) - { - RsErr() <<"Attempt to add yourself or a null ID as SSL-only friend (id=" << sslId << ")" << std::endl; - return false; - } - - peerState pstate; - -// { -// RS_STACK_MUTEX(mPeerMtx); -// -// /* If in mOthersList -> move over */ -// auto it = mOthersList.find(sslId); -// if (it != mOthersList.end()) -// { -// pstate = it->second; -// mOthersList.erase(it); -// } -// -// -// } // RS_STACK_MUTEX(mPeerMtx); - - if(!pstate.gpg_id.isNull() && AuthGPG::getAuthGPG()->isGPGAccepted(pstate.gpg_id)) + if(sslId.isNull()) { - RsErr() << "Trying to add as SSL-only friend a peer which PGP id is already a friend. This means the code is inconsistent. Not doing this!" << std::endl; + RsErr() << __PRETTY_FUNCTION__ << " Cannot add a null " + << "ID as SSL-only friend " << std::endl; return false; } - if(pgp_id.isNull()) - { - RsErr() << "Null pgp id for friend added with skip_pgp_signature_validaiton flag. This is not allowed." << std::endl; - return false; - } + if(pgp_id.isNull()) + { + RsErr() << __PRETTY_FUNCTION__ << " Cannot add as SSL-only friend a " + << "peer with null PGP" << std::endl; + return false; + } + + if(sslId == getOwnId()) + { + RsErr() << __PRETTY_FUNCTION__ << " Cannot add yourself as SSL-only " + << "friend (id=" << sslId << ")" << std::endl; + return false; + } + + bool alreadySslFriend = false; + peerState pstate; + + { RS_STACK_MUTEX(mPeerMtx); + auto it = mFriendList.find(sslId); + if( it != mFriendList.end() ) + { + alreadySslFriend = true; + + /* If it is already friend override pstate so we don't loose already + * known information about the peer, in particular overriding + * pstate.skip_pgp_signature_validation is important for security. + */ + pstate = it->second; + } + } // RS_STACK_MUTEX(mPeerMtx); + + /* If it is already friend check if PGP id of the invite matches with the + * PGP id we already know, to avoid nasty tricks with malevolently forged + * short invites.*/ + if(alreadySslFriend && pstate.gpg_id != pgp_id) + { + RsErr() << __PRETTY_FUNCTION__ << " Cannot SSL-only friend for " + << "a pre-existing friend with mismatching PGP-id " + << "known: " << pstate.gpg_id << " new: " << pgp_id + << std::endl; + return false; + } + + /* It is very important to be expecially carefull setting + * pstate.skip_pgp_signature_validation to true because this effectively + * disables PGP signature verification on connection attempt. + * This check in particular avoid someone attempting to trick the user into + * accepting as SSL-only friend a malevolently forged short invite, with the + * PGP id of an already known friend but the SSL-id of a location generated + * by the attacker which doesn't have access to the legitimate PGP + * certificate. + * In that case being pstate.skip_pgp_signature_validation false on + * connection attempt the PGP signaure verification would fail and the + * connection closed. + * Instead if pstate.skip_pgp_signature_validation would have been + * superficially set to true the PGP signature verification would have been + * skipped and the attacker connection would be accepted. */ + if(!AuthGPG::getAuthGPG()->isGPGAccepted(pgp_id)) + pstate.skip_pgp_signature_validation = true; pstate.gpg_id = pgp_id; pstate.id = sslId; + /* At this point if we got info about the peer just update with the new + * values. */ if(!dt.name.empty()) pstate.name = dt.name; if(!dt.dyndns.empty()) pstate.dyndns = dt.dyndns; pstate.hiddenNode = dt.isHiddenNode; @@ -1119,19 +1155,17 @@ bool p3PeerMgrIMPL::addSslOnlyFriend( const RsPeerId& sslId, const RsPgpId& pgp_ if(dt.hiddenType) pstate.hiddenType = dt.hiddenType; if(!dt.location.empty()) pstate.location = dt.location; - pstate.skip_pgp_signature_validation = true; - { RS_STACK_MUTEX(mPeerMtx); - mFriendList[sslId] = pstate; mStatusChanged = true; - } // RS_STACK_MUTEX(mPeerMtx); IndicateConfigChanged(); mLinkMgr->addFriend(sslId, dt.vs_dht != RS_VS_DHT_OFF); - // To update IP addresses is much more confortable to use locators + /* To update IP addresses is much more confortable to use locators, beside + * of the easy to use another benefit is that this way we don't loose + * previously known IP addresses */ if(!dt.isHiddenNode) { for(const std::string& locator : dt.ipAddressList) From 4e3ac4a9f46b83ec306dc1b3bcf16fd4a798a90f Mon Sep 17 00:00:00 2001 From: Gioacchino Mazzurco Date: Sat, 28 Sep 2019 14:15:12 +0200 Subject: [PATCH 2/3] Make SSL-only friend criteria stricter --- libretroshare/src/pgp/pgphandler.cc | 3 +++ libretroshare/src/pgp/pgphandler.h | 9 +++++++++ libretroshare/src/pqi/p3peermgr.cc | 2 +- 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/libretroshare/src/pgp/pgphandler.cc b/libretroshare/src/pgp/pgphandler.cc index 48915aa85..d0e95bcb4 100644 --- a/libretroshare/src/pgp/pgphandler.cc +++ b/libretroshare/src/pgp/pgphandler.cc @@ -1667,6 +1667,9 @@ bool PGPHandler::getGPGFilteredList(std::list& list,bool (*filter)(cons return true ; } +bool PGPHandler::isPgpPubKeyAvailable(const RsPgpId &id) +{ return _public_keyring_map.find(id) != _public_keyring_map.end(); } + bool PGPHandler::isGPGId(const RsPgpId &id) { return _public_keyring_map.find(id) != _public_keyring_map.end() ; diff --git a/libretroshare/src/pgp/pgphandler.h b/libretroshare/src/pgp/pgphandler.h index fee240e6a..51c9dbfcd 100644 --- a/libretroshare/src/pgp/pgphandler.h +++ b/libretroshare/src/pgp/pgphandler.h @@ -148,6 +148,15 @@ class PGPHandler const PGPCertificateInfo *getCertificateInfo(const RsPgpId& id) const ; + /** + * @brief Check if a PGP publick key is available + * @param id id of the key to check + * @return true if the public key for the given id is available, + * false otherwise + */ + bool isPgpPubKeyAvailable(const RsPgpId& id); + + RS_DEPRECATED_FOR(isPgpPubKeyAvailable) bool isGPGId(const RsPgpId &id); bool isGPGSigned(const RsPgpId &id); bool isGPGAccepted(const RsPgpId &id); diff --git a/libretroshare/src/pqi/p3peermgr.cc b/libretroshare/src/pqi/p3peermgr.cc index 01f245a25..e48576d90 100644 --- a/libretroshare/src/pqi/p3peermgr.cc +++ b/libretroshare/src/pqi/p3peermgr.cc @@ -1138,7 +1138,7 @@ bool p3PeerMgrIMPL::addSslOnlyFriend( * Instead if pstate.skip_pgp_signature_validation would have been * superficially set to true the PGP signature verification would have been * skipped and the attacker connection would be accepted. */ - if(!AuthGPG::getAuthGPG()->isGPGAccepted(pgp_id)) + if(!AuthGPG::getAuthGPG()->isPgpPubKeyAvailable(pgp_id)) pstate.skip_pgp_signature_validation = true; pstate.gpg_id = pgp_id; From 1e9adc1e23a15b22628975de86531cac1b90545f Mon Sep 17 00:00:00 2001 From: Gioacchino Mazzurco Date: Sat, 28 Sep 2019 14:34:07 +0200 Subject: [PATCH 3/3] Add some documentation and fix some compiler warnings --- libretroshare/src/pgp/pgphandler.cc | 15 ++++++------ libretroshare/src/pgp/pgphandler.h | 36 ++++++++++++++++++++--------- 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/libretroshare/src/pgp/pgphandler.cc b/libretroshare/src/pgp/pgphandler.cc index d0e95bcb4..4ed342629 100644 --- a/libretroshare/src/pgp/pgphandler.cc +++ b/libretroshare/src/pgp/pgphandler.cc @@ -1581,24 +1581,25 @@ void PGPHandler::locked_updateOwnSignatureFlag(PGPCertificateInfo& cert,const Rs cert._flags &= ~PGPCertificateInfo::PGP_CERTIFICATE_FLAG_HAS_SIGNED_ME ; } -RsPgpId PGPHandler::pgpIdFromFingerprint(const PGPFingerprintType& f) +/*static*/ RsPgpId PGPHandler::pgpIdFromFingerprint(const RsPgpFingerprint& f) { - return RsPgpId(f.toByteArray() + _RsIdSize::PGP_FINGERPRINT - _RsIdSize::PGP_ID); + return RsPgpId::fromBufferUnsafe( + f.toByteArray() + + RsPgpFingerprint::SIZE_IN_BYTES - RsPgpId::SIZE_IN_BYTES ); } -bool PGPHandler::getKeyFingerprint(const RsPgpId& id,PGPFingerprintType& fp) const +bool PGPHandler::getKeyFingerprint(const RsPgpId& id, RsPgpFingerprint& fp) const { - RsStackMutex mtx(pgphandlerMtx) ; // lock access to PGP memory structures. + RS_STACK_MUTEX(pgphandlerMtx); const ops_keydata_t *key = locked_getPublicKey(id,false) ; - if(key == NULL) - return false ; + if(!key) return false; ops_fingerprint_t f ; ops_fingerprint(&f,&key->key.pkey) ; - fp = PGPFingerprintType(f.fingerprint) ; + fp = RsPgpFingerprint::fromBufferUnsafe(f.fingerprint); return true ; } diff --git a/libretroshare/src/pgp/pgphandler.h b/libretroshare/src/pgp/pgphandler.h index 51c9dbfcd..0b785285a 100644 --- a/libretroshare/src/pgp/pgphandler.h +++ b/libretroshare/src/pgp/pgphandler.h @@ -79,7 +79,7 @@ class PGPCertificateInfo /// This class offer an abstract pgp handler to be used in RetroShare. class PGPHandler { - public: +public: PGPHandler( const std::string& path_to_public_keyring, const std::string& path_to_secret_keyring, const std::string& path_to_trust_database, @@ -124,7 +124,6 @@ class PGPHandler bool encryptTextToFile(const RsPgpId& key_id,const std::string& text,const std::string& outfile) ; bool decryptTextFromFile(const RsPgpId& key_id,std::string& text,const std::string& encrypted_inputfile) ; - bool getKeyFingerprint(const RsPgpId& id,PGPFingerprintType& fp) const ; void setAcceptConnexion(const RsPgpId&,bool) ; void updateOwnSignatureFlag(const RsPgpId& ownId) ; @@ -148,14 +147,6 @@ class PGPHandler const PGPCertificateInfo *getCertificateInfo(const RsPgpId& id) const ; - /** - * @brief Check if a PGP publick key is available - * @param id id of the key to check - * @return true if the public key for the given id is available, - * false otherwise - */ - bool isPgpPubKeyAvailable(const RsPgpId& id); - RS_DEPRECATED_FOR(isPgpPubKeyAvailable) bool isGPGId(const RsPgpId &id); bool isGPGSigned(const RsPgpId &id); @@ -163,7 +154,30 @@ class PGPHandler static void setPassphraseCallback(PassphraseCallback cb) ; static PassphraseCallback passphraseCallback() { return _passphrase_callback ; } - static RsPgpId pgpIdFromFingerprint(const PGPFingerprintType& f) ; + + /** + * @brief Check if a PGP publick key is available + * @param id id of the key to check + * @return true if the public key for the given id is available, + * false otherwise + */ + bool isPgpPubKeyAvailable(const RsPgpId& id); + + /** + * @brief Convert PGP fingerprint to PGP 64bit id + * @param f PGP fingerprint to convert + * @return PGP 64bit id extracted from fingerprint + */ + static RsPgpId pgpIdFromFingerprint(const RsPgpFingerprint& f); + + /** + * @brief Get PGP fingerprint for the given key + * @param id PGP 64bit key id + * @param fp storage for the retrived key fingerpring, the contained value + * is meaningfull only if true is returned + * @return true if the key was found, false if not + */ + bool getKeyFingerprint(const RsPgpId& id, RsPgpFingerprint& fp) const; // Gets info about the key. Who are the signers, what's the owner's name, etc. //