Git-Mirrors/RetroShare
1
0
Fork 0
mirror of https://github.com/RetroShare/RetroShare.git synced 2025-08-08 14:22:31 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions

Allow friend information update from short invite

Browse source 
Add comments explaining security of addSslOnlyFriend
This commit is contained in:
Gioacchino Mazzurco 2019-09-27 14:15:55 +02:00
parent 1705a930d4
commit 5660c73175
No known key found for this signature in database
GPG key ID: A1FBCA3872E87051
1 changed files with 69 additions and 35 deletions
Download patch file Download diff file Expand all files Collapse all files

96
libretroshare/src/pqi/p3peermgr.cc
View file

@ -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; RsErr() << __PRETTY_FUNCTION__ << " Cannot add a null "
return false; << "ID as SSL-only friend " << std::endl;
}
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;
return false; 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 "
<< "peer with null PGP" << std::endl;
return false; 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.gpg_id = pgp_id;
pstate.id = sslId; 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.name.empty()) pstate.name = dt.name;
if(!dt.dyndns.empty()) pstate.dyndns = dt.dyndns; if(!dt.dyndns.empty()) pstate.dyndns = dt.dyndns;
pstate.hiddenNode = dt.isHiddenNode; 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.hiddenType) pstate.hiddenType = dt.hiddenType;
if(!dt.location.empty()) pstate.location = dt.location; if(!dt.location.empty()) pstate.location = dt.location;
pstate.skip_pgp_signature_validation = true;
{ RS_STACK_MUTEX(mPeerMtx); { RS_STACK_MUTEX(mPeerMtx);
mFriendList[sslId] = pstate; mFriendList[sslId] = pstate;
mStatusChanged = true; mStatusChanged = true;
} // RS_STACK_MUTEX(mPeerMtx); } // RS_STACK_MUTEX(mPeerMtx);
IndicateConfigChanged(); IndicateConfigChanged();
mLinkMgr->addFriend(sslId, dt.vs_dht != RS_VS_DHT_OFF); 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) if(!dt.isHiddenNode)
{ {
for(const std::string& locator : dt.ipAddressList) for(const std::string& locator : dt.ipAddressList)