Merge pull request #1645 from G10h4ck/short_invites_fixes

Allow friend information  update from short invite

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)
+	if(!key) return false;
-		return false ;
 
 	ops_fingerprint_t f ;
 	ops_fingerprint(&f,&key->key.pkey) ; 
 
-	fp = PGPFingerprintType(f.fingerprint) ;
+	fp = RsPgpFingerprint::fromBufferUnsafe(f.fingerprint);
 
 	return true ;
 }
@@ -1667,6 +1668,9 @@ bool PGPHandler::getGPGFilteredList(std::list<RsPgpId>& 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() ;

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,13 +147,37 @@ class PGPHandler
 
 		const PGPCertificateInfo *getCertificateInfo(const RsPgpId& id) const ;
 
+		RS_DEPRECATED_FOR(isPgpPubKeyAvailable)
 		bool isGPGId(const RsPgpId &id);
 		bool isGPGSigned(const RsPgpId &id);
 		bool isGPGAccepted(const RsPgpId &id);
 
 		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.
 		//

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())
+	if(sslId.isNull())
-    {
-        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))
 	{
-		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())
+	if(pgp_id.isNull())
-    {
+	{
-        RsErr() << "Null pgp id for friend added with skip_pgp_signature_validaiton flag. This is not allowed." << std::endl;
+		RsErr() << __PRETTY_FUNCTION__ << " Cannot add as SSL-only friend a "
-        return false;
+		        << "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()->isPgpPubKeyAvailable(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)