From a8db4a8d2d214c2fcee33625d52d72dafc1ed863 Mon Sep 17 00:00:00 2001 From: Adam Ierymenko Date: Mon, 2 Mar 2020 10:25:15 -0800 Subject: [PATCH] Minor future proofing and cleanup in CertificateOfMembership, improve comments in a few places. --- node/CertificateOfMembership.cpp | 40 ++++++++++++++++++-------------- node/CertificateOfMembership.hpp | 6 +++++ node/FCV.hpp | 12 +++++----- node/Node.cpp | 3 ++- node/Protocol.cpp | 5 ++-- node/RuntimeEnvironment.hpp | 19 ++++++++------- node/Tests.cpp | 2 +- node/Tests.h | 34 ++++++++++++++++----------- node/Topology.hpp | 1 + 9 files changed, 74 insertions(+), 48 deletions(-) diff --git a/node/CertificateOfMembership.cpp b/node/CertificateOfMembership.cpp index 73ed65862..2c7685655 100644 --- a/node/CertificateOfMembership.cpp +++ b/node/CertificateOfMembership.cpp @@ -17,15 +17,15 @@ namespace ZeroTier { CertificateOfMembership::CertificateOfMembership(uint64_t timestamp,uint64_t timestampMaxDelta,uint64_t nwid,const Address &issuedTo) { - _qualifiers[0].id = COM_RESERVED_ID_TIMESTAMP; - _qualifiers[0].value = timestamp; - _qualifiers[0].maxDelta = timestampMaxDelta; - _qualifiers[1].id = COM_RESERVED_ID_NETWORK_ID; - _qualifiers[1].value = nwid; - _qualifiers[1].maxDelta = 0; - _qualifiers[2].id = COM_RESERVED_ID_ISSUED_TO; - _qualifiers[2].value = issuedTo.toInt(); - _qualifiers[2].maxDelta = 0xffffffffffffffffULL; + _qualifiers[COM_RESERVED_ID_TIMESTAMP].id = COM_RESERVED_ID_TIMESTAMP; + _qualifiers[COM_RESERVED_ID_TIMESTAMP].value = timestamp; + _qualifiers[COM_RESERVED_ID_TIMESTAMP].maxDelta = timestampMaxDelta; + _qualifiers[COM_RESERVED_ID_NETWORK_ID].id = COM_RESERVED_ID_NETWORK_ID; + _qualifiers[COM_RESERVED_ID_NETWORK_ID].value = nwid; + _qualifiers[COM_RESERVED_ID_NETWORK_ID].maxDelta = 0; + _qualifiers[COM_RESERVED_ID_ISSUED_TO].id = COM_RESERVED_ID_ISSUED_TO; + _qualifiers[COM_RESERVED_ID_ISSUED_TO].value = issuedTo.toInt(); + _qualifiers[COM_RESERVED_ID_ISSUED_TO].maxDelta = 0xffffffffffffffffULL; _qualifierCount = 3; _signatureLength = 0; } @@ -107,7 +107,7 @@ bool CertificateOfMembership::sign(const Identity &with) int CertificateOfMembership::marshal(uint8_t data[ZT_CERTIFICATEOFMEMBERSHIP_MARSHAL_SIZE_MAX]) const noexcept { - data[0] = 1; + data[0] = 1; // 96-byte signature length; a v2 is supported in unmarshal code where signature is length prefixed Utils::storeBigEndian(data + 1,(uint16_t)_qualifierCount); int p = 3; for(unsigned int i=0;i<_qualifierCount;++i) { @@ -117,10 +117,6 @@ int CertificateOfMembership::marshal(uint8_t data[ZT_CERTIFICATEOFMEMBERSHIP_MAR } _signedBy.copyTo(data + p); p += ZT_ADDRESS_LENGTH; if ((_signedBy)&&(_signatureLength == 96)) { - // UGLY: Ed25519 signatures in ZT are 96 bytes (64 + 32 bytes of hash). - // P-384 signatures are also 96 bytes, praise the horned one. That means - // we don't need to include a length. If we ever do we will need a new - // serialized object version, but only for those with length != 96. memcpy(data + p,_signature,96); p += 96; } return p; @@ -128,7 +124,7 @@ int CertificateOfMembership::marshal(uint8_t data[ZT_CERTIFICATEOFMEMBERSHIP_MAR int CertificateOfMembership::unmarshal(const uint8_t *data,int len) noexcept { - if ((len < 3)||(data[0] != 1)) + if ((len < 3)||(data[0] == 0)) return -1; unsigned int numq = Utils::loadBigEndian(data + 1); if (numq > ZT_NETWORK_COM_MAX_QUALIFIERS) @@ -146,9 +142,19 @@ int CertificateOfMembership::unmarshal(const uint8_t *data,int len) noexcept return -1; _signedBy.setTo(data + p); p += ZT_ADDRESS_LENGTH; if (_signedBy) { - if ((p + 96) > len) - return -1; _signatureLength = 96; + if (data[0] > 1) { + // If the version byte is >1, signatures come prefixed by a length. This is the + // way it should have been in the first place. Version byte 1 indicates 96 byte + // signatures and is backward compatible with <2.x nodes. + if ((p + 2) >= len) + return -1; + _signatureLength = Utils::loadBigEndian(data + p); p += 2; + if (_signatureLength == 0) + return -1; + } + if ((p + _signatureLength) > len) + return -1; memcpy(_signature,data + p,96); p += 96; } diff --git a/node/CertificateOfMembership.hpp b/node/CertificateOfMembership.hpp index bcfb5b755..db4253c2a 100644 --- a/node/CertificateOfMembership.hpp +++ b/node/CertificateOfMembership.hpp @@ -129,6 +129,8 @@ public: */ ZT_ALWAYS_INLINE int64_t timestamp() const noexcept { + if (_qualifiers[COM_RESERVED_ID_TIMESTAMP].id == COM_RESERVED_ID_TIMESTAMP) + return (int64_t)_qualifiers[0].value; for(unsigned int i=0;i<_qualifierCount;++i) { if (_qualifiers[i].id == COM_RESERVED_ID_TIMESTAMP) return (int64_t)_qualifiers[i].value; @@ -141,6 +143,8 @@ public: */ ZT_ALWAYS_INLINE Address issuedTo() const noexcept { + if (_qualifiers[COM_RESERVED_ID_ISSUED_TO].id == COM_RESERVED_ID_ISSUED_TO) + return Address(_qualifiers[2].value); for(unsigned int i=0;i<_qualifierCount;++i) { if (_qualifiers[i].id == COM_RESERVED_ID_ISSUED_TO) return Address(_qualifiers[i].value); @@ -153,6 +157,8 @@ public: */ ZT_ALWAYS_INLINE uint64_t networkId() const noexcept { + if (_qualifiers[COM_RESERVED_ID_NETWORK_ID].id == COM_RESERVED_ID_NETWORK_ID) + return _qualifiers[COM_RESERVED_ID_NETWORK_ID].value; for(unsigned int i=0;i<_qualifierCount;++i) { if (_qualifiers[i].id == COM_RESERVED_ID_NETWORK_ID) return _qualifiers[i].value; diff --git a/node/FCV.hpp b/node/FCV.hpp index 2092b8597..ea4f35b34 100644 --- a/node/FCV.hpp +++ b/node/FCV.hpp @@ -242,7 +242,7 @@ public: } } - ZT_ALWAYS_INLINE bool operator==(const FCV &v) const + ZT_ALWAYS_INLINE bool operator==(const FCV &v) const noexcept { if (_s == v._s) { for(unsigned int i=0;i<_s;++i) { @@ -253,11 +253,11 @@ public: } return false; } - ZT_ALWAYS_INLINE bool operator!=(const FCV &v) const { return (!(*this == v)); } - ZT_ALWAYS_INLINE bool operator<(const FCV &v) const { return std::lexicographical_compare(begin(),end(),v.begin(),v.end()); } - ZT_ALWAYS_INLINE bool operator>(const FCV &v) const { return (v < *this); } - ZT_ALWAYS_INLINE bool operator<=(const FCV &v) const { return !(v < *this); } - ZT_ALWAYS_INLINE bool operator>=(const FCV &v) const { return !(*this < v); } + ZT_ALWAYS_INLINE bool operator!=(const FCV &v) const noexcept { return (!(*this == v)); } + ZT_ALWAYS_INLINE bool operator<(const FCV &v) const noexcept { return std::lexicographical_compare(begin(),end(),v.begin(),v.end()); } + ZT_ALWAYS_INLINE bool operator>(const FCV &v) const noexcept { return (v < *this); } + ZT_ALWAYS_INLINE bool operator<=(const FCV &v) const noexcept { return !(v < *this); } + ZT_ALWAYS_INLINE bool operator>=(const FCV &v) const noexcept { return !(*this < v); } private: unsigned int _s; diff --git a/node/Node.cpp b/node/Node.cpp index f40824650..1bbe1fd86 100644 --- a/node/Node.cpp +++ b/node/Node.cpp @@ -25,7 +25,6 @@ #include "SelfAwareness.hpp" #include "Network.hpp" #include "Trace.hpp" -#include "ScopedPtr.hpp" #include "Locator.hpp" #include "Protocol.hpp" #include "Expect.hpp" @@ -112,6 +111,8 @@ Node::Node(void *uPtr,void *tPtr,const struct ZT_Node_Callbacks *callbacks,int64 stateObjectPut(tPtr,ZT_STATE_OBJECT_IDENTITY_PUBLIC,idtmp,RR->publicIdentityStr,(unsigned int)strlen(RR->publicIdentityStr)); } + RR->identity.hashWithPrivate(RR->localCacheSymmetricKey); + // This constructs all the components of the ZeroTier core within a single contiguous memory container, // which reduces memory fragmentation and may improve cache locality. _objects = new _NodeObjects(RR,tPtr); diff --git a/node/Protocol.cpp b/node/Protocol.cpp index e1ddba92e..66afc96d2 100644 --- a/node/Protocol.cpp +++ b/node/Protocol.cpp @@ -27,6 +27,9 @@ namespace ZeroTier { namespace Protocol { +// The counter used to assign packet IDs / cryptographic nonces. +std::atomic _s_packetIdCtr((uint64_t)time(nullptr) << 32U); + uint64_t createProbe(const Identity &sender,const Identity &recipient,const uint8_t key[ZT_PEER_SECRET_KEY_LENGTH]) noexcept { uint8_t tmp[ZT_IDENTITY_HASH_SIZE + ZT_IDENTITY_HASH_SIZE]; @@ -37,8 +40,6 @@ uint64_t createProbe(const Identity &sender,const Identity &recipient,const uint return hash[0]; } -std::atomic _s_packetIdCtr((uint64_t)time(nullptr) << 32U); - void armor(Buf &pkt,int packetSize,const uint8_t key[ZT_PEER_SECRET_KEY_LENGTH],uint8_t cipherSuite) noexcept { Protocol::Header &ph = pkt.as(); diff --git a/node/RuntimeEnvironment.hpp b/node/RuntimeEnvironment.hpp index f9c8e2ff4..4dec6532d 100644 --- a/node/RuntimeEnvironment.hpp +++ b/node/RuntimeEnvironment.hpp @@ -30,12 +30,16 @@ class Trace; class Expect; /** - * Holds global state for an instance of ZeroTier::Node + * ZeroTier::Node execution context + * + * This just holds pointers and various other information used by all the + * various moving parts of a node. It's stored or passed as 'RR' to give it + * a common name througout the code. */ class RuntimeEnvironment { public: - ZT_ALWAYS_INLINE RuntimeEnvironment(Node *n) : + ZT_ALWAYS_INLINE RuntimeEnvironment(Node *n) noexcept : node(n), localNetworkController(nullptr), rtmem(nullptr), @@ -53,6 +57,7 @@ public: ZT_ALWAYS_INLINE ~RuntimeEnvironment() { Utils::burn(secretIdentityStr,sizeof(secretIdentityStr)); + Utils::burn(localCacheSymmetricKey,sizeof(localCacheSymmetricKey)); } // Node instance that owns this RuntimeEnvironment @@ -64,12 +69,6 @@ public: // Memory actually occupied by Trace, Switch, etc. void *rtmem; - /* Order matters a bit here. These are constructed in this order - * and then deleted in the opposite order on Node exit. The order ensures - * that things that are needed are there before they're needed. - * - * These are constant and never null after startup unless indicated. */ - Trace *t; Expect *expect; VL2 *vl2; @@ -81,6 +80,10 @@ public: Identity identity; char publicIdentityStr[ZT_IDENTITY_STRING_BUFFER_LENGTH]; char secretIdentityStr[ZT_IDENTITY_STRING_BUFFER_LENGTH]; + + // A hash of this node identity's public and private keys that is used as + // a secret key to encrypt locally cached sensitive information. + uint8_t localCacheSymmetricKey[ZT_IDENTITY_HASH_SIZE]; }; } // namespace ZeroTier diff --git a/node/Tests.cpp b/node/Tests.cpp index dbc57e5e0..29d523a94 100644 --- a/node/Tests.cpp +++ b/node/Tests.cpp @@ -1198,7 +1198,7 @@ int main(int argc,char **argv) ok &= ZTT_crypto() == nullptr; ok &= ZTT_benchmarkCrypto() == nullptr; if (!ok) { - ZT_T_PRINTF(ZT_EOL_S "At least one error occurred!" ZT_EOL_S); + ZT_T_PRINTF(ZT_EOL_S "*** AT LEAST ONE TEST FAILED! ***" ZT_EOL_S); return 1; } return 0; diff --git a/node/Tests.h b/node/Tests.h index f30c17b04..99a3a6d8a 100644 --- a/node/Tests.h +++ b/node/Tests.h @@ -18,20 +18,24 @@ * To build these ensure that ZT_ENABLE_TESTS is defined during build time. * Otherwise they are omitted. * + * The macro ZT_STANDALONE_TESTS will also build a main() function for these + * tests for creating a stand-alone test executable. It will return zero if + * all tests pass and non-zero if at least one test fails. + * * These symbols are defined extern "C" so tests can be called from regular * C code, which is important for use via CGo or in plain C projects. * * The ZT_T_PRINTF macro defaults to printf() but if it's defined at compile - * time (also must be set while building Tests.cpp) it can specify another + * time (it must be set while building Tests.cpp) it can specify another * function to use for output. Defining it to a no-op can be used to disable * output. * * Each test function returns NULL if the tests succeeds or an error message * on test failure. * - * Only the fuzzing test functions are thread-safe. Other test functions - * should not be called concurrently. It's okay to call different tests from - * different threads, but not the same test. + * Be aware that fuzzing tests can and will crash the program if a serious + * error is discovered. This is the point. It's also beneficial to run these + * in "valgrind" or a similar tool to detect marginal bad behvaior. */ #ifndef ZT_TESTS_HPP @@ -52,20 +56,24 @@ extern "C" { #define ZT_T_PRINTF(fmt,...) printf((fmt),##__VA_ARGS__),fflush(stdout) #endif -// Basic self-tests --------------------------------------------------------------------------------------------------- - +/** + * Test platform, compiler behavior, utility functions, and core classes + */ const char *ZTT_general(); + +/** + * Test crypto using test vectors and simple scenarios + * + * This is not an absolutely exhaustive test, just a sanity check to make sure + * crypto routines are basically working. + */ const char *ZTT_crypto(); -// Benchmarks --------------------------------------------------------------------------------------------------------- - +/** + * Run benchmarks of cryptographic routines and common constructions + */ const char *ZTT_benchmarkCrypto(); -// Fuzzing ------------------------------------------------------------------------------------------------------------ - - -// -------------------------------------------------------------------------------------------------------------------- - #ifdef __cplusplus } #endif diff --git a/node/Topology.hpp b/node/Topology.hpp index b2f21f02d..decb430ba 100644 --- a/node/Topology.hpp +++ b/node/Topology.hpp @@ -327,6 +327,7 @@ public: void saveAll(void *tPtr); private: + // Load cached peer and set 'peer' to it, if one is found. void _loadCached(void *tPtr,const Address &zta,SharedPtr &peer); // This is a secure random integer created at startup to salt the calculation of path hash map keys