From c9d7845fea15ffe0e09295aedba6389de1bcb59b Mon Sep 17 00:00:00 2001 From: Adam Ierymenko Date: Tue, 9 Aug 2016 17:00:01 -0700 Subject: [PATCH] Minor bug fix and some instrumentation stuff for testing. --- node/IncomingPacket.cpp | 2 +- node/Network.cpp | 7 ++++++- node/Packet.hpp | 1 + node/SelfAwareness.cpp | 42 +++++++++++++++++++++++++++++++++++------ node/SelfAwareness.hpp | 5 +++-- 5 files changed, 47 insertions(+), 10 deletions(-) diff --git a/node/IncomingPacket.cpp b/node/IncomingPacket.cpp index df224c198..53f6b88a3 100644 --- a/node/IncomingPacket.cpp +++ b/node/IncomingPacket.cpp @@ -84,7 +84,7 @@ bool IncomingPacket::tryDecode(const RuntimeEnvironment *RR) } const Packet::Verb v = verb(); - //TRACE("<< %s from %s(%s)",Packet::verbString(v),sourceAddress.toString().c_str(),_remoteAddress.toString().c_str()); + TRACE("<< %s from %s(%s)",Packet::verbString(v),sourceAddress.toString().c_str(),_remoteAddress.toString().c_str()); switch(v) { //case Packet::VERB_NOP: default: // ignore unknown verbs, but if they pass auth check they are "received" diff --git a/node/Network.cpp b/node/Network.cpp index e098c1fd3..b9a2ca1d1 100644 --- a/node/Network.cpp +++ b/node/Network.cpp @@ -676,7 +676,12 @@ void Network::requestConfiguration() const unsigned int rmdSize = rmd.sizeBytes(); outp.append((uint16_t)rmdSize); outp.append((const void *)rmd.data(),rmdSize); - outp.append((_config) ? (uint64_t)_config.revision : (uint64_t)0); + if (_config) { + outp.append((uint64_t)_config.revision); + outp.append((uint64_t)_config.timestamp); + } else { + outp.append((unsigned char)0,16); + } outp.compress(); RR->sw->send(outp,true); diff --git a/node/Packet.hpp b/node/Packet.hpp index 9d4c82894..0524139db 100644 --- a/node/Packet.hpp +++ b/node/Packet.hpp @@ -724,6 +724,7 @@ public: * <[8] 64-bit network ID> * <[2] 16-bit length of request meta-data dictionary> * <[...] string-serialized request meta-data> + * <[8] 64-bit revision of netconf we currently have> * <[8] 64-bit timestamp of netconf we currently have> * * This message requests network configuration from a node capable of diff --git a/node/SelfAwareness.cpp b/node/SelfAwareness.cpp index a4fae3d56..05df53fe3 100644 --- a/node/SelfAwareness.cpp +++ b/node/SelfAwareness.cpp @@ -79,9 +79,10 @@ void SelfAwareness::iam(const Address &reporter,const InetAddress &receivedOnLoc if ( (trusted) && ((now - entry.ts) < ZT_SELFAWARENESS_ENTRY_TIMEOUT) && (!entry.mySurface.ipsEqual(myPhysicalAddress)) ) { // Changes to external surface reported by trusted peers causes path reset in this scope + TRACE("physical address %s for scope %u as seen from %s(%s) differs from %s, resetting paths in scope",myPhysicalAddress.toString().c_str(),(unsigned int)scope,reporter.toString().c_str(),reporterPhysicalAddress.toString().c_str(),entry.mySurface.toString().c_str()); entry.mySurface = myPhysicalAddress; entry.ts = now; - TRACE("physical address %s for scope %u as seen from %s(%s) differs from %s, resetting paths in scope",myPhysicalAddress.toString().c_str(),(unsigned int)scope,reporter.toString().c_str(),reporterPhysicalAddress.toString().c_str(),entry.mySurface.toString().c_str()); + entry.trusted = trusted; // Erase all entries in this scope that were not reported from this remote address to prevent 'thrashing' // due to multiple reports of endpoint change. @@ -113,6 +114,7 @@ void SelfAwareness::iam(const Address &reporter,const InetAddress &receivedOnLoc // Otherwise just update DB to use to determine external surface info entry.mySurface = myPhysicalAddress; entry.ts = now; + entry.trusted = trusted; } } @@ -148,22 +150,50 @@ std::vector SelfAwareness::getSymmetricNatPredictions() bool symmetric = false; { Mutex::Lock _l(_phy_m); + Hashtable< PhySurfaceKey,PhySurfaceEntry >::Iterator i(_phy); PhySurfaceKey *k = (PhySurfaceKey *)0; PhySurfaceEntry *e = (PhySurfaceEntry *)0; + InetAddress lastTrustedSurface; while (i.next(k,e)) { if ((e->mySurface.ss_family == AF_INET)&&(e->mySurface.ipScope() == InetAddress::IP_SCOPE_GLOBAL)) { std::set &s = surfaces[k->receivedOnLocalAddress]; - s.insert(e->mySurface); + + /* MINOR SECURITY FIX: + * + * If the surface was not reported by a trusted (upstream) peer, we do + * not use its report of our surface IP for symmetric NAT prediction. + * Otherwise a peer could poison our external surface cache and then + * use this to coax us into suggesting their IP as an endpoint. This + * in turn could allow them to relay traffic for us. They could not + * decrypt or otherwise mess with it, but they could DOS us or record + * meta-data without anything appearing amiss. + * + * So for surfaces reported by untrusted peers we use the IP reported + * by a trusted peer and then just use the port. + * + * As far as we know this has never been exploited. We discovered it + * because certain weird configurations, such as load balancers and + * gateways that do not preserve IP information, can coax a node into + * reporting back false surface information. */ + if (e->trusted) { + s.insert(e->mySurface); + lastTrustedSurface = e->mySurface; + } else if (lastTrustedSurface) { + InetAddress tmp(lastTrustedSurface); + tmp.setPort(e->mySurface.port()); + s.insert(tmp); + } + symmetric = symmetric||(s.size() > 1); } } } - // If we appear to be symmetrically NATed, generate and return extrapolations - // of those surfaces. Since PUSH_DIRECT_PATHS is sent multiple times, we - // probabilistically generate extrapolations of anywhere from +1 to +5 to - // increase the odds that it will work "eventually". + /* If we appear to be symmetrically NATed, generate and return extrapolations + * of those surfaces. Since PUSH_DIRECT_PATHS is sent multiple times, we + * probabilistically generate extrapolations of anywhere from +1 to +5 to + * increase the odds that it will work "eventually". */ if (symmetric) { std::vector r; for(std::map< InetAddress,std::set >::iterator si(surfaces.begin());si!=surfaces.end();++si) { diff --git a/node/SelfAwareness.hpp b/node/SelfAwareness.hpp index 06c264a91..c7bde87ee 100644 --- a/node/SelfAwareness.hpp +++ b/node/SelfAwareness.hpp @@ -82,9 +82,10 @@ private: { InetAddress mySurface; uint64_t ts; + bool trusted; - PhySurfaceEntry() : mySurface(),ts(0) {} - PhySurfaceEntry(const InetAddress &a,const uint64_t t) : mySurface(a),ts(t) {} + PhySurfaceEntry() : mySurface(),ts(0),trusted(false) {} + PhySurfaceEntry(const InetAddress &a,const uint64_t t) : mySurface(a),ts(t),trusted(false) {} }; const RuntimeEnvironment *RR;