Harden some stuff around COMs and members of networks.

node/CertificateOfMembership.cpp

@@ -28,10 +28,10 @@ bool CertificateOfMembership::agreesWith(const CertificateOfMembership &other) c
 	// conditions that could introduce a vulnerability.
 
 	if (other._timestamp > _timestamp) {
-		if ((other._timestamp - _timestamp) > _timestampMaxDelta)
+		if ((other._timestamp - _timestamp) > std::min(_timestampMaxDelta,other._timestampMaxDelta))
 			return false;
 	} else {
-		if ((_timestamp - other._timestamp) > _timestampMaxDelta)
+		if ((_timestamp - other._timestamp) > std::min(_timestampMaxDelta,other._timestampMaxDelta))
 			return false;
 	}
 
@@ -81,7 +81,7 @@ bool CertificateOfMembership::agreesWith(const CertificateOfMembership &other) c
 
 	// SECURITY: check for issued-to inequality is a sanity check. This should be impossible since elsewhere
 	// in the code COMs are checked to ensure that they do in fact belong to their issued-to identities.
-	return (other._networkId != _networkId) && (other._issuedTo.address() != _issuedTo.address());
+	return (other._networkId == _networkId) && (_networkId != 0) && (other._issuedTo.address() != _issuedTo.address());
 }
 
 bool CertificateOfMembership::sign(const Identity &with) noexcept
@@ -214,7 +214,7 @@ int CertificateOfMembership::unmarshal(const uint8_t *data,int len) noexcept
 		if ((p + 2) > len)
 			return -1;
 		_signatureLength = Utils::loadBigEndian<uint16_t>(data + p);
-		if ((_signatureLength > sizeof(_signature))||((p + _signatureLength) > len))
+		if ((_signatureLength > (unsigned int)sizeof(_signature))||((p + (int)_signatureLength) > len))
 			return -1;
 		memcpy(_signature,data + p,_signatureLength);
 		return p + (int)_signatureLength;

node/CertificateOfMembership.hpp

@@ -179,8 +179,10 @@ public:
 	 */
 	ZT_INLINE Credential::VerifyResult verify(const RuntimeEnvironment *RR,void *tPtr) const { return _verify(RR,tPtr,*this); }
 
+	// NOTE: right now we use v1 serialization format which works with both ZeroTier 1.x and 2.x. V2 format
+	// will be switched on once 1.x is pretty much dead and out of support.
 	static constexpr int marshalSizeMax() noexcept { return ZT_CERTIFICATEOFMEMBERSHIP_MARSHAL_SIZE_MAX; }
-	int marshal(uint8_t data[ZT_CERTIFICATEOFMEMBERSHIP_MARSHAL_SIZE_MAX],bool v2) const noexcept;
+	int marshal(uint8_t data[ZT_CERTIFICATEOFMEMBERSHIP_MARSHAL_SIZE_MAX],bool v2 = false) const noexcept;
 	int unmarshal(const uint8_t *data,int len) noexcept;
 
 private:

node/Membership.cpp

@@ -24,6 +24,8 @@ namespace ZeroTier {
 Membership::Membership() :
 	_comRevocationThreshold(0),
 	_lastPushedCredentials(0),
+	_comAgreementLocalTimestamp(0),
+	_comAgreementRemoteTimestamp(0),
 	_revocations(4),
 	_remoteTags(4),
 	_remoteCaps(4),
@@ -35,7 +37,7 @@ Membership::~Membership()
 {
 }
 
-void Membership::pushCredentials(const RuntimeEnvironment *RR,void *tPtr,const int64_t now,const Address &peerAddress,const NetworkConfig &nconf)
+void Membership::pushCredentials(const RuntimeEnvironment *RR,void *tPtr,const int64_t now,const Identity &to,const NetworkConfig &nconf)
 {
 	if (!nconf.com) // sanity check
 		return;
@@ -48,7 +50,7 @@ void Membership::pushCredentials(const RuntimeEnvironment *RR,void *tPtr,const i
 	bool complete = false;
 	while (!complete) {
 		ph.packetId = Protocol::getPacketId();
-		peerAddress.copyTo(ph.destination);
+		to.address().copyTo(ph.destination);
 		RR->identity.address().copyTo(ph.source);
 		ph.flags = 0;
 		ph.verb = Protocol::VERB_NETWORK_CREDENTIALS;

node/Membership.hpp

@@ -57,29 +57,16 @@ public:
 	 * @param RR Runtime environment
 	 * @param tPtr Thread pointer to be handed through to any callbacks called as a result of this call
 	 * @param now Current time
-	 * @param peerAddress Address of member peer (the one that this Membership describes)
+	 * @param to Peer identity
 	 * @param nconf My network config
 	 */
-	void pushCredentials(const RuntimeEnvironment *RR,void *tPtr,int64_t now,const Address &peerAddress,const NetworkConfig &nconf);
+	void pushCredentials(const RuntimeEnvironment *RR,void *tPtr,int64_t now,const Identity &to,const NetworkConfig &nconf);
 
 	/**
 	 * @return Time we last pushed credentials to this member
 	 */
 	ZT_INLINE int64_t lastPushedCredentials() const noexcept { return _lastPushedCredentials; }
 
-	/**
-	 * Check whether the peer represented by this Membership should be allowed on this network at all
-	 *
-	 * @param nconf Our network config
-	 * @return True if this peer is allowed on this network at all
-	 */
-	ZT_INLINE bool isAllowedOnNetwork(const NetworkConfig &nconf) const noexcept
-	{
-		if (nconf.isPublic()) return true; // public network
-		if (_com.timestamp() <= _comRevocationThreshold) return false; // COM has been revoked
-		return nconf.com.agreesWith(_com); // check timestamp agreement window
-	}
-
 	/**
 	 * Check whether the peer represented by this Membership owns a given address
 	 *
@@ -129,6 +116,42 @@ public:
 	 */
 	static ZT_INLINE uint64_t credentialKey(const ZT_CredentialType &t,const uint32_t i) noexcept { return (((uint64_t)t << 32U) | (uint64_t)i); }
 
+	/**
+	 * Check if our local COM agrees with theirs, with possible memo-ization.
+	 *
+	 * @param localCom
+	 */
+	ZT_INLINE bool certificateOfMembershipAgress(const CertificateOfMembership &localCom,const Identity &remoteIdentity)
+	{
+		if ((_comAgreementLocalTimestamp == localCom.timestamp())&&(_comAgreementRemoteTimestamp == _com.timestamp()))
+			return true;
+		if (_com.agreesWith(localCom)) {
+			// SECURITY: newer network controllers embed the full fingerprint into the COM. If we are
+			// joined to a network managed by one of these, our COM will contain one. If it's present
+			// we compare vs the other and require them to match. If our COM does not contain a full
+			// identity fingerprint we compare by address only, which is a legacy mode supported for
+			// old network controllers. Note that this is safe because the controller issues us our COM
+			// and in so doing indicates if it's new or old. However this will go away after a while
+			// once we can be pretty sure there are no ancient controllers around.
+			if (localCom.issuedTo().haveHash()) {
+				if (localCom.issuedTo() != _com.issuedTo())
+					return false;
+			} else {
+				// LEGACY: support networks run by old controllers.
+				if (localCom.issuedTo().address() != _com.issuedTo().address())
+					return false;
+			}
+
+			// Remember that these two COMs agreed. If any are updated this is invalidated and a full
+			// agreement check will be done again.
+			_comAgreementLocalTimestamp = localCom.timestamp();
+			_comAgreementRemoteTimestamp = _com.timestamp();
+
+			return true;
+		}
+		return false;
+	}
+
 	AddCredentialResult addCredential(const RuntimeEnvironment *RR,void *tPtr,const Identity &sourcePeerIdentity,const NetworkConfig &nconf,const CertificateOfMembership &com);
 	AddCredentialResult addCredential(const RuntimeEnvironment *RR,void *tPtr,const Identity &sourcePeerIdentity,const NetworkConfig &nconf,const Tag &tag);
 	AddCredentialResult addCredential(const RuntimeEnvironment *RR,void *tPtr,const Identity &sourcePeerIdentity,const NetworkConfig &nconf,const Capability &cap);
@@ -173,6 +196,9 @@ private:
 	// Time we last pushed credentials
 	int64_t _lastPushedCredentials;
 
+	// COM timestamps at which we last agreed-- used to memo-ize agreement and avoid having to recompute constantly.
+	int64_t _comAgreementLocalTimestamp,_comAgreementRemoteTimestamp;
+
 	// Remote member's latest network COM
 	CertificateOfMembership _com;
 

node/Network.cpp

@@ -1051,19 +1051,28 @@ int Network::setConfiguration(void *tPtr,const NetworkConfig &nconf,bool saveToD
 	return 0;
 }
 
-bool Network::gate(void *tPtr,const SharedPtr<Peer> &peer)
+bool Network::gate(void *tPtr,const SharedPtr<Peer> &peer) noexcept
 {
-	Mutex::Lock l(_memberships_l);
+	Mutex::Lock lc(_config_l);
+
+	if (!_config)
+		return false;
+	if (_config.isPublic())
+		return true;
+
 	try {
-		if (_config) {
+		Mutex::Lock l(_memberships_l);
-			Membership *m = _memberships.get(peer->address());
+		Membership *m = _memberships.get(peer->address());
-			if ( (_config.isPublic()) || ((m)&&(m->isAllowedOnNetwork(_config))) ) {
+		if (m) {
-				if (!m)
+			// SECURITY: this method in CertificateOfMembership does a full fingerprint check as well as
-					m = &(_memberships[peer->address()]);
+			// checking certificate agreement. See Membership.hpp.
-				return true;
+			return m->certificateOfMembershipAgress(_config.com,peer->identity());
-			}
+		} else {
+			m = &(_memberships[peer->address()]);
+			return false;
 		}
 	} catch ( ... ) {}
+
 	return false;
 }
 
@@ -1147,7 +1156,7 @@ Membership::AddCredentialResult Network::addCredential(void *tPtr,const Identity
 	if (com.networkId() != _id)
 		return Membership::ADD_REJECTED;
 	Mutex::Lock _l(_memberships_l);
-	return _memberships[com.issuedTo()].addCredential(RR,tPtr,sourcePeerIdentity,_config,com);
+	return _memberships[com.issuedTo().address()].addCredential(RR,tPtr,sourcePeerIdentity,_config,com);
 }
 
 Membership::AddCredentialResult Network::addCredential(void *tPtr,const Identity &sourcePeerIdentity,const Capability &cap)
@@ -1208,12 +1217,6 @@ Membership::AddCredentialResult Network::addCredential(void *tPtr,const Identity
 	return _memberships[coo.issuedTo()].addCredential(RR,tPtr,sourcePeerIdentity,_config,coo);
 }
 
-void Network::pushCredentialsNow(void *tPtr,const Address &to,const int64_t now)
-{
-	Mutex::Lock _l(_memberships_l);
-	_memberships[to].pushCredentials(RR,tPtr,now,to,_config);
-}
-
 void Network::destroy()
 {
 	_memberships_l.lock();

node/Network.hpp

@@ -226,7 +226,7 @@ public:
 	 * @param tPtr Thread pointer to be handed through to any callbacks called as a result of this call
 	 * @param peer Peer to check
 	 */
-	bool gate(void *tPtr,const SharedPtr<Peer> &peer);
+	bool gate(void *tPtr,const SharedPtr<Peer> &peer) noexcept;
 
 	/**
 	 * Do periodic cleanup and housekeeping tasks
@@ -292,27 +292,18 @@ public:
 	 */
 	Membership::AddCredentialResult addCredential(void *tPtr,const Identity &sourcePeerIdentity,const CertificateOfOwnership &coo);
 
-	/**
-	 * Force push credentials (COM, etc.) to a peer now
-	 *
-	 * @param tPtr Thread pointer to be handed through to any callbacks called as a result of this call
-	 * @param to Destination peer address
-	 * @param now Current time
-	 */
-	void pushCredentialsNow(void *tPtr,const Address &to,int64_t now);
-
 	/**
 	 * Push credentials if we haven't done so in a long time
 	 *
 	 * @param tPtr Thread pointer to be handed through to any callbacks called as a result of this call
-	 * @param to Destination peer address
+	 * @param to Destination peer
 	 * @param now Current time
 	 */
-	ZT_INLINE void pushCredentialsIfNeeded(void *tPtr,const Address &to,const int64_t now)
+	ZT_INLINE void pushCredentialsIfNeeded(void *tPtr,const Identity &to,const int64_t now)
 	{
 		const int64_t tout = std::min(_config.credentialTimeMaxDelta,(int64_t)ZT_PEER_ACTIVITY_TIMEOUT);
 		Mutex::Lock _l(_memberships_l);
-		Membership &m = _memberships[to];
+		Membership &m = _memberships[to.address()];
 		if (((now - m.lastPushedCredentials()) + 5000) >= tout)
 			m.pushCredentials(RR,tPtr,now,to,_config);
 	}

node/NetworkConfig.cpp

@@ -39,8 +39,9 @@ bool NetworkConfig::toDictionary(Dictionary &d,bool includeLegacy) const
 		d.add(ZT_NETWORKCONFIG_DICT_KEY_NAME,this->name);
 		d.add(ZT_NETWORKCONFIG_DICT_KEY_MTU,this->mtu);
 
-		if (this->com)
+		if (this->com) {
 			d.add(ZT_NETWORKCONFIG_DICT_KEY_COM,tmp,this->com.marshal(tmp));
+		}
 
 		std::vector<uint8_t> *blob = &(d[ZT_NETWORKCONFIG_DICT_KEY_CAPABILITIES]);
 		for (unsigned int i = 0; i < this->capabilityCount; ++i) {

node/Trace.cpp

@@ -116,12 +116,12 @@ void Trace::_tryingNewPath(
 	ev.evSize = ZT_CONST_TO_BE_UINT16(sizeof(ev));
 	ev.evType = ZT_CONST_TO_BE_UINT16(ZT_TRACE_VL1_TRYING_NEW_PATH);
 	ev.codeLocation = Utils::hton(codeLocation);
-	trying.fingerprint().getAPIFingerprint(&ev.peer);
+	memcpy(&ev.peer,trying.fingerprint().apiFingerprint(),sizeof(ev.peer));
 	physicalAddress.forTrace(ev.physicalAddress);
 	triggerAddress.forTrace(ev.triggerAddress);
 	ev.triggeringPacketId = triggeringPacketId;
 	ev.triggeringPacketVerb = triggeringPacketVerb;
-	triggeringPeer.fingerprint().getAPIFingerprint(&ev.triggeringPeer);
+	memcpy(&ev.triggeringPeer,triggeringPeer.fingerprint().apiFingerprint(),sizeof(ev.triggeringPeer));
 	ev.reason = (uint8_t)reason;
 	RR->node->postEvent(tPtr,ZT_EVENT_TRACE,&ev);
 }
@@ -139,7 +139,7 @@ void Trace::_learnedNewPath(
 	ev.evType = ZT_CONST_TO_BE_UINT16(ZT_TRACE_VL1_LEARNED_NEW_PATH);
 	ev.codeLocation = Utils::hton(codeLocation);
 	ev.packetId = packetId; // packet IDs are kept in big-endian
-	peerIdentity.fingerprint().getAPIFingerprint(&ev.peer);
+	memcpy(&ev.peer,peerIdentity.fingerprint().apiFingerprint(),sizeof(ev.peer));
 	physicalAddress.forTrace(ev.physicalAddress);
 	replaced.forTrace(ev.replaced);
 
@@ -163,7 +163,7 @@ void Trace::_incomingPacketDropped(
 	ev.codeLocation = Utils::hton(codeLocation);
 	ev.packetId = packetId; // packet IDs are kept in big-endian
 	ev.networkId = Utils::hton(networkId);
-	peerIdentity.fingerprint().getAPIFingerprint(&ev.peer);
+	memcpy(&ev.peer,peerIdentity.fingerprint().apiFingerprint(),sizeof(ev.peer));
 	physicalAddress.forTrace(ev.physicalAddress);
 	ev.hops = hops;
 	ev.verb = verb;
@@ -226,7 +226,7 @@ void Trace::_incomingNetworkFrameDropped(
 	ev.networkId = Utils::hton(networkId);
 	ev.sourceMac = Utils::hton(sourceMac.toInt());
 	ev.destMac = Utils::hton(destMac.toInt());
-	peerIdentity.fingerprint().getAPIFingerprint(&ev.sender);
+	memcpy(&ev.sender,peerIdentity.fingerprint().apiFingerprint(),sizeof(ev.sender));
 	physicalAddress.forTrace(ev.physicalAddress);
 	ev.hops = hops;
 	ev.frameLength = Utils::hton(frameLength);
@@ -325,7 +325,7 @@ void Trace::_credentialRejected(
 	ev.codeLocation = Utils::hton(codeLocation);
 	ev.networkId = Utils::hton(networkId);
 	if (identity) {
-		identity.fingerprint().getAPIFingerprint(&ev.peer);
+		memcpy(&ev.peer,identity.fingerprint().apiFingerprint(),sizeof(ev.peer));
 	} else {
 		ev.peer.address = address.toInt();
 		memset(ev.peer.hash,0,sizeof(ev.peer.hash));