From 134d33c2181e5a2b783a0503313b1a03dd89d863 Mon Sep 17 00:00:00 2001 From: Adam Ierymenko Date: Mon, 20 Sep 2021 18:26:49 -0400 Subject: [PATCH] Add a bit of hardening in the network certificate of membership by incorporating a full hash of the identity to which it is issued. This means the recipient need not depend entirely on the root verifying identities properly to make sure impersonation is not occurring. --- controller/EmbeddedNetworkController.cpp | 2 +- node/CertificateOfMembership.cpp | 52 +++++++++++++----------- node/CertificateOfMembership.hpp | 22 ++++++++-- node/Identity.hpp | 12 ++++++ node/Membership.hpp | 7 ++-- node/Network.cpp | 9 ++-- selftest.cpp | 16 ++++---- 7 files changed, 78 insertions(+), 42 deletions(-) diff --git a/controller/EmbeddedNetworkController.cpp b/controller/EmbeddedNetworkController.cpp index 99d59aeef..ea70cb3ae 100644 --- a/controller/EmbeddedNetworkController.cpp +++ b/controller/EmbeddedNetworkController.cpp @@ -1801,7 +1801,7 @@ void EmbeddedNetworkController::_request( nc->certificateOfOwnershipCount = 1; } - CertificateOfMembership com(now,credentialtmd,nwid,identity.address()); + CertificateOfMembership com(now,credentialtmd,nwid,identity); if (com.sign(_signingId)) { nc->com = com; } else { diff --git a/node/CertificateOfMembership.cpp b/node/CertificateOfMembership.cpp index 10cb0863a..2580041c8 100644 --- a/node/CertificateOfMembership.cpp +++ b/node/CertificateOfMembership.cpp @@ -148,37 +148,43 @@ void CertificateOfMembership::fromString(const char *s) #endif // ZT_SUPPORT_OLD_STYLE_NETCONF -bool CertificateOfMembership::agreesWith(const CertificateOfMembership &other) const +bool CertificateOfMembership::agreesWith(const CertificateOfMembership &other, const Identity &otherIdentity) const { - unsigned int myidx = 0; - unsigned int otheridx = 0; - if ((_qualifierCount == 0)||(other._qualifierCount == 0)) return false; - while (myidx < _qualifierCount) { - // Fail if we're at the end of other, since this means the field is - // missing. - if (otheridx >= other._qualifierCount) - return false; + std::map< uint64_t, uint64_t > otherFields; + for(unsigned int i=0;i= other._qualifierCount) + bool fullIdentityVerification = false; + for(unsigned int i=0;i<_qualifierCount;++i) { + const uint64_t qid = _qualifiers[i].id; + if ((qid >= 3)&&(qid <= 6)) { + fullIdentityVerification = true; + } else { + std::map< uint64_t, uint64_t >::iterator otherQ(otherFields.find(qid)); + if (otherQ == otherFields.end()) + return false; + const uint64_t a = _qualifiers[i].value; + const uint64_t b = otherQ->second; + if (((a >= b) ? (a - b) : (b - a)) > _qualifiers[i].maxDelta) return false; } + } - // Compare to determine if the absolute value of the difference - // between these two parameters is within our maxDelta. - const uint64_t a = _qualifiers[myidx].value; - const uint64_t b = other._qualifiers[myidx].value; - if (((a >= b) ? (a - b) : (b - a)) > _qualifiers[myidx].maxDelta) - return false; - - ++myidx; + // If this COM has a full hash of its identity, assume the other must have this as well. + // Otherwise we are on a controller that does not incorporate these. + if (fullIdentityVerification) { + uint64_t idHash[6]; + otherIdentity.publicKeyHash(idHash); + for(unsigned long i=0;i<4;++i) { + std::map< uint64_t, uint64_t >::iterator otherQ(otherFields.find((uint64_t)(i + 3))); + if (otherQ == otherFields.end()) + return false; + if (otherQ->second != Utils::ntoh(idHash[i])) + return false; + } } return true; diff --git a/node/CertificateOfMembership.hpp b/node/CertificateOfMembership.hpp index f8500628d..0e4e04748 100644 --- a/node/CertificateOfMembership.hpp +++ b/node/CertificateOfMembership.hpp @@ -94,6 +94,8 @@ public: * ZeroTier address to whom certificate was issued */ COM_RESERVED_ID_ISSUED_TO = 2 + + // IDs 3-6 reserved for full hash of identity to which this COM was issued. }; /** @@ -110,7 +112,7 @@ public: * @param nwid Network ID * @param issuedTo Certificate recipient */ - CertificateOfMembership(uint64_t timestamp,uint64_t timestampMaxDelta,uint64_t nwid,const Address &issuedTo) + CertificateOfMembership(uint64_t timestamp,uint64_t timestampMaxDelta,uint64_t nwid,const Identity &issuedTo) { _qualifiers[0].id = COM_RESERVED_ID_TIMESTAMP; _qualifiers[0].value = timestamp; @@ -119,9 +121,20 @@ public: _qualifiers[1].value = nwid; _qualifiers[1].maxDelta = 0; _qualifiers[2].id = COM_RESERVED_ID_ISSUED_TO; - _qualifiers[2].value = issuedTo.toInt(); + _qualifiers[2].value = issuedTo.address().toInt(); _qualifiers[2].maxDelta = 0xffffffffffffffffULL; - _qualifierCount = 3; + + // Include hash of full identity public key in COM for hardening purposes. Pack it in + // using the original COM format. Format may be revised in the future to make this cleaner. + uint64_t idHash[6]; + issuedTo.publicKeyHash(idHash); + for(unsigned long i=0;i<4;++i) { + _qualifiers[i + 3].id = (uint64_t)(i + 3); + _qualifiers[i + 3].value = Utils::ntoh(idHash[i]); + _qualifiers[i + 3].maxDelta = 0xffffffffffffffffULL; + } + + _qualifierCount = 7; memset(_signature.data,0,ZT_C25519_SIGNATURE_LEN); } @@ -224,9 +237,10 @@ public: * tuples present in this cert but not in other result in 'false'. * * @param other Cert to compare with + * @param otherIdentity Identity of other node * @return True if certs agree and 'other' may be communicated with */ - bool agreesWith(const CertificateOfMembership &other) const; + bool agreesWith(const CertificateOfMembership &other, const Identity &otherIdentity) const; /** * Sign this certificate diff --git a/node/Identity.hpp b/node/Identity.hpp index e6f658dc3..cc8de5126 100644 --- a/node/Identity.hpp +++ b/node/Identity.hpp @@ -109,6 +109,18 @@ public: */ inline bool hasPrivate() const { return (_privateKey != (C25519::Private *)0); } + /** + * Compute a SHA384 hash of this identity's address and public key(s). + * + * @param sha384buf Buffer with 48 bytes of space to receive hash + */ + inline void publicKeyHash(void *sha384buf) const + { + uint8_t address[ZT_ADDRESS_LENGTH]; + _address.copyTo(address, ZT_ADDRESS_LENGTH); + SHA384(sha384buf, address, ZT_ADDRESS_LENGTH, _publicKey.data, ZT_C25519_PUBLIC_KEY_LEN); + } + /** * Compute the SHA512 hash of our private key (if we have one) * diff --git a/node/Membership.hpp b/node/Membership.hpp index 476987714..63a7c10f5 100644 --- a/node/Membership.hpp +++ b/node/Membership.hpp @@ -91,13 +91,14 @@ public: * Check whether the peer represented by this Membership should be allowed on this network at all * * @param nconf Our network config + * @param otherNodeIdentity Identity of remote node * @return True if this peer is allowed on this network at all */ - inline bool isAllowedOnNetwork(const NetworkConfig &nconf) const + inline bool isAllowedOnNetwork(const NetworkConfig &thisNodeNetworkConfig, const Identity &otherNodeIdentity) const { - if (nconf.isPublic()) return true; + if (thisNodeNetworkConfig.isPublic()) return true; if (_com.timestamp() <= _comRevocationThreshold) return false; - return nconf.com.agreesWith(_com); + return thisNodeNetworkConfig.com.agreesWith(_com, otherNodeIdentity); } inline bool recentlyAssociated(const int64_t now) const diff --git a/node/Network.cpp b/node/Network.cpp index a9007258f..f3138f3ac 100644 --- a/node/Network.cpp +++ b/node/Network.cpp @@ -1227,7 +1227,7 @@ bool Network::gate(void *tPtr,const SharedPtr &peer) try { if (_config) { Membership *m = _memberships.get(peer->address()); - if ( (_config.isPublic()) || ((m)&&(m->isAllowedOnNetwork(_config))) ) { + if ( (_config.isPublic()) || ((m)&&(m->isAllowedOnNetwork(_config, peer->identity()))) ) { if (!m) m = &(_membership(peer->address())); if (m->multicastLikeGate(now)) { @@ -1487,8 +1487,11 @@ void Network::_sendUpdatesToMembers(void *tPtr,const MulticastGroup *const newMu Membership *m = (Membership *)0; Hashtable::Iterator i(_memberships); while (i.next(a,m)) { - if ( ( m->multicastLikeGate(now) || (newMulticastGroup) ) && (m->isAllowedOnNetwork(_config)) && (!std::binary_search(alwaysAnnounceTo.begin(),alwaysAnnounceTo.end(),*a)) ) - _announceMulticastGroupsTo(tPtr,*a,groups); + const Identity remoteIdentity(RR->topology->getIdentity(tPtr, *a)); + if (remoteIdentity) { + if ( ( m->multicastLikeGate(now) || (newMulticastGroup) ) && (m->isAllowedOnNetwork(_config, remoteIdentity)) && (!std::binary_search(alwaysAnnounceTo.begin(),alwaysAnnounceTo.end(),*a)) ) + _announceMulticastGroupsTo(tPtr,*a,groups); + } } } } diff --git a/selftest.cpp b/selftest.cpp index 357e9a026..42e9bc232 100644 --- a/selftest.cpp +++ b/selftest.cpp @@ -561,8 +561,8 @@ static int testCertificate() std::cout << idA.address().toString(buf) << ", " << idB.address().toString(buf) << std::endl; std::cout << "[certificate] Generating certificates A and B..."; - CertificateOfMembership cA(10000,100,1,idA.address()); - CertificateOfMembership cB(10099,100,1,idB.address()); + CertificateOfMembership cA(10000,100,1,idA); + CertificateOfMembership cB(10099,100,1,idB); std::cout << std::endl; std::cout << "[certificate] Signing certificates A and B with authority..."; @@ -574,13 +574,13 @@ static int testCertificate() //std::cout << "[certificate] B: " << cB.toString() << std::endl; std::cout << "[certificate] A agrees with B and B with A... "; - if (cA.agreesWith(cB)) + if (cA.agreesWith(cB, idB)) std::cout << "yes, "; else { std::cout << "FAIL" << std::endl; return -1; } - if (cB.agreesWith(cA)) + if (cB.agreesWith(cA, idA)) std::cout << "yes." << std::endl; else { std::cout << "FAIL" << std::endl; @@ -588,18 +588,18 @@ static int testCertificate() } std::cout << "[certificate] Generating two certificates that should not agree..."; - cA = CertificateOfMembership(10000,100,1,idA.address()); - cB = CertificateOfMembership(10101,100,1,idB.address()); + cA = CertificateOfMembership(10000,100,1,idA); + cB = CertificateOfMembership(10101,100,1,idB); std::cout << std::endl; std::cout << "[certificate] A agrees with B and B with A... "; - if (!cA.agreesWith(cB)) + if (!cA.agreesWith(cB, idB)) std::cout << "no, "; else { std::cout << "FAIL" << std::endl; return -1; } - if (!cB.agreesWith(cA)) + if (!cB.agreesWith(cA, idA)) std::cout << "no." << std::endl; else { std::cout << "FAIL" << std::endl;