Improve security posture by eliminating non-const data() accessor from Buffer.

5 files changed, 85 insertions, 60 deletions

diff --git a/node/Buffer.hpp b/node/Buffer.hpp
index bc74f048..64176e58 100644
--- a/node/Buffer.hpp
+++ b/node/Buffer.hpp
@@ -163,11 +163,13 @@ public:
 		return ((unsigned char *)_b)[i];
 	}
 
-	unsigned char *data() throw() { return (unsigned char *)_b; }
-	const unsigned char *data() const throw() { return (const unsigned char *)_b; }
-
 	/**
-	 * Safe way to get a pointer to a field from data() with bounds checking
+	 * Get a raw pointer to a field with bounds checking
+	 *
+	 * This isn't perfectly safe in that the caller could still overflow
+	 * the pointer, but its use provides both a sanity check and
+	 * documentation / reminder to the calling code to treat the returned
+	 * pointer as being of size [l].
 	 * 
 	 * @param i Index of field in buffer
 	 * @param l Length of field in bytes
@@ -304,8 +306,9 @@ public:
 	/**
 	 * Increment size and return pointer to field of specified size
 	 *
-	 * The memory isn't actually written, so this is a shortcut for a multi-step
-	 * process involving getting the current pointer and adding size.
+	 * Nothing is actually written to the memory. This is a shortcut
+	 * for addSize() followed by field() to reference the previous
+	 * position and the new size.
 	 *
 	 * @param l Length of field to append
 	 * @return Pointer to beginning of appended field of length 'l'
@@ -353,6 +356,22 @@ public:
 	}
 
 	/**
+	 * Move everything after 'at' to the buffer's front and truncate
+	 *
+	 * @param at Truncate before this position
+	 * @throw std::out_of_range Position is beyond size of buffer
+	 */
+	inline void behead(const unsigned int at)
+		throw(std::out_of_range)
+	{
+		if (!at)
+			return;
+		if (at > _l)
+			throw std::out_of_range("Buffer: behead() beyond capacity");
+		::memmove(_b,_b + at,_l -= at);
+	}
+
+	/**
 	 * Set buffer data length to zero
 	 */
 	inline void clear()
@@ -389,6 +408,11 @@ public:
 	}
 
 	/**
+	 * @return Constant pointer to data in buffer
+	 */
+	inline const void *data() const throw() { return _b; }
+
+	/**
 	 * @return Size of data in buffer
 	 */
 	inline unsigned int size() const throw() { return _l; }
diff --git a/node/IncomingPacket.cpp b/node/IncomingPacket.cpp
index bb4f2dc5..042b3ccd 100644
--- a/node/IncomingPacket.cpp
+++ b/node/IncomingPacket.cpp
@@ -46,50 +46,56 @@ namespace ZeroTier {
 
 bool IncomingPacket::tryDecode(const RuntimeEnvironment *RR)
 {
-	if ((!encrypted())&&(verb() == Packet::VERB_HELLO)) {
-		// Unencrypted HELLOs are handled here since they are used to
-		// populate our identity cache in the first place. _doHELLO() is special
-		// in that it contains its own authentication logic.
-		//TRACE("<< HELLO from %s(%s) (normal unencrypted HELLO)",source().toString().c_str(),_remoteAddress.toString().c_str());
-		return _doHELLO(RR);
-	}
-
-	SharedPtr<Peer> peer = RR->topology->getPeer(source());
-	if (peer) {
-		if (!dearmor(peer->key())) {
-			TRACE("dropped packet from %s(%s), MAC authentication failed (size: %u)",source().toString().c_str(),_remoteAddress.toString().c_str(),size());
-			return true;
-		}
-		if (!uncompress()) {
-			TRACE("dropped packet from %s(%s), compressed data invalid",source().toString().c_str(),_remoteAddress.toString().c_str());
-			return true;
+	try {
+		if ((!encrypted())&&(verb() == Packet::VERB_HELLO)) {
+			// Unencrypted HELLOs are handled here since they are used to
+			// populate our identity cache in the first place. _doHELLO() is special
+			// in that it contains its own authentication logic.
+			return _doHELLO(RR);
 		}
 
-		//TRACE("<< %s from %s(%s)",Packet::verbString(verb()),source().toString().c_str(),_remoteAddress.toString().c_str());
-
-		switch(verb()) {
-			//case Packet::VERB_NOP:
-			default: // ignore unknown verbs, but if they pass auth check they are still valid
-				peer->receive(RR,_fromSock,_remoteAddress,hops(),packetId(),verb(),0,Packet::VERB_NOP,Utils::now());
+		SharedPtr<Peer> peer = RR->topology->getPeer(source());
+		if (peer) {
+			if (!dearmor(peer->key())) {
+				TRACE("dropped packet from %s(%s), MAC authentication failed (size: %u)",source().toString().c_str(),_remoteAddress.toString().c_str(),size());
+				return true;
+			}
+			if (!uncompress()) {
+				TRACE("dropped packet from %s(%s), compressed data invalid",source().toString().c_str(),_remoteAddress.toString().c_str());
 				return true;
-			case Packet::VERB_HELLO:                          return _doHELLO(RR);
-			case Packet::VERB_ERROR:                          return _doERROR(RR,peer);
-			case Packet::VERB_OK:                             return _doOK(RR,peer);
-			case Packet::VERB_WHOIS:                          return _doWHOIS(RR,peer);
-			case Packet::VERB_RENDEZVOUS:                     return _doRENDEZVOUS(RR,peer);
-			case Packet::VERB_FRAME:                          return _doFRAME(RR,peer);
-			case Packet::VERB_EXT_FRAME:                      return _doEXT_FRAME(RR,peer);
-			case Packet::VERB_P5_MULTICAST_FRAME:             return _doP5_MULTICAST_FRAME(RR,peer);
-			case Packet::VERB_MULTICAST_LIKE:                 return _doMULTICAST_LIKE(RR,peer);
-			case Packet::VERB_NETWORK_MEMBERSHIP_CERTIFICATE: return _doNETWORK_MEMBERSHIP_CERTIFICATE(RR,peer);
-			case Packet::VERB_NETWORK_CONFIG_REQUEST:         return _doNETWORK_CONFIG_REQUEST(RR,peer);
-			case Packet::VERB_NETWORK_CONFIG_REFRESH:         return _doNETWORK_CONFIG_REFRESH(RR,peer);
-			case Packet::VERB_MULTICAST_GATHER:               return _doMULTICAST_GATHER(RR,peer);
-			case Packet::VERB_MULTICAST_FRAME:                return _doMULTICAST_FRAME(RR,peer);
+			}
+
+			//TRACE("<< %s from %s(%s)",Packet::verbString(verb()),source().toString().c_str(),_remoteAddress.toString().c_str());
+
+			switch(verb()) {
+				//case Packet::VERB_NOP:
+				default: // ignore unknown verbs, but if they pass auth check they are "received"
+					peer->receive(RR,_fromSock,_remoteAddress,hops(),packetId(),verb(),0,Packet::VERB_NOP,Utils::now());
+					return true;
+				case Packet::VERB_HELLO:                          return _doHELLO(RR);
+				case Packet::VERB_ERROR:                          return _doERROR(RR,peer);
+				case Packet::VERB_OK:                             return _doOK(RR,peer);
+				case Packet::VERB_WHOIS:                          return _doWHOIS(RR,peer);
+				case Packet::VERB_RENDEZVOUS:                     return _doRENDEZVOUS(RR,peer);
+				case Packet::VERB_FRAME:                          return _doFRAME(RR,peer);
+				case Packet::VERB_EXT_FRAME:                      return _doEXT_FRAME(RR,peer);
+				case Packet::VERB_P5_MULTICAST_FRAME:             return _doP5_MULTICAST_FRAME(RR,peer);
+				case Packet::VERB_MULTICAST_LIKE:                 return _doMULTICAST_LIKE(RR,peer);
+				case Packet::VERB_NETWORK_MEMBERSHIP_CERTIFICATE: return _doNETWORK_MEMBERSHIP_CERTIFICATE(RR,peer);
+				case Packet::VERB_NETWORK_CONFIG_REQUEST:         return _doNETWORK_CONFIG_REQUEST(RR,peer);
+				case Packet::VERB_NETWORK_CONFIG_REFRESH:         return _doNETWORK_CONFIG_REFRESH(RR,peer);
+				case Packet::VERB_MULTICAST_GATHER:               return _doMULTICAST_GATHER(RR,peer);
+				case Packet::VERB_MULTICAST_FRAME:                return _doMULTICAST_FRAME(RR,peer);
+			}
+		} else {
+			RR->sw->requestWhois(source());
+			return false;
 		}
-	} else {
-		RR->sw->requestWhois(source());
-		return false;
+	} catch ( ... ) {
+		// Exceptions are more informatively caught in _do...() handlers but
+		// this outer try/catch will catch anything else odd.
+		TRACE("dropped ??? from %s(%s): unexpected exception in tryDecode()",source().toString().c_str(),_remoteAddress.toString().c_str());
+		return true;
 	}
 }
 
@@ -430,7 +436,8 @@ bool IncomingPacket::_doFRAME(const RuntimeEnvironment *RR,const SharedPtr<Peer>
 					return true;
 				}
 
-				network->tapPut(MAC(peer->address(),network->id()),network->mac(),etherType,data() + ZT_PROTO_VERB_FRAME_IDX_PAYLOAD,size() - ZT_PROTO_VERB_FRAME_IDX_PAYLOAD);
+				unsigned int payloadLen = size() - ZT_PROTO_VERB_FRAME_IDX_PAYLOAD;
+				network->tapPut(MAC(peer->address(),network->id()),network->mac(),etherType,field(ZT_PROTO_VERB_FRAME_IDX_PAYLOAD,payloadLen),payloadLen);
 			}
 
 			peer->receive(RR,_fromSock,_remoteAddress,hops(),packetId(),Packet::VERB_FRAME,0,Packet::VERB_NOP,Utils::now());
diff --git a/node/Network.cpp b/node/Network.cpp
index 0dc5c8b6..b9d605f5 100644
--- a/node/Network.cpp
+++ b/node/Network.cpp
@@ -505,7 +505,7 @@ void Network::_restoreState()
 		}
 	}
 
-	// Read most recent multicast cert dump
+	// Read most recent membership cert dump
 	if ((_config)&&(!_config->isPublic())&&(Utils::fileExists(mcdbPath.c_str()))) {
 		CertificateOfMembership com;
 		Mutex::Lock _l(_lock);
@@ -519,7 +519,7 @@ void Network::_restoreState()
 				if ((fread(magic,6,1,mcdb) == 1)&&(!memcmp("ZTMCD0",magic,6))) {
 					long rlen = 0;
 					do {
-						long rlen = (long)fread(buf.data() + buf.size(),1,ZT_NETWORK_CERT_WRITE_BUF_SIZE - buf.size(),mcdb);
+						long rlen = (long)fread(const_cast<char *>(static_cast<const char *>(buf.data())) + buf.size(),1,ZT_NETWORK_CERT_WRITE_BUF_SIZE - buf.size(),mcdb);
 						if (rlen < 0) rlen = 0;
 						buf.setSize(buf.size() + (unsigned int)rlen);
 						unsigned int ptr = 0;
@@ -528,10 +528,7 @@ void Network::_restoreState()
 							if (com.issuedTo())
 								_membershipCertificates[com.issuedTo()] = com;
 						}
-						if (ptr) {
-							memmove(buf.data(),buf.data() + ptr,buf.size() - ptr);
-							buf.setSize(buf.size() - ptr);
-						}
+						buf.behead(ptr);
 					} while (rlen > 0);
 					fclose(mcdb);
 				} else {
diff --git a/node/Packet.hpp b/node/Packet.hpp
index 6786a8e5..3cf77b33 100644
--- a/node/Packet.hpp
+++ b/node/Packet.hpp
@@ -383,13 +383,13 @@ public:
 			setSize(fragLen + ZT_PROTO_MIN_FRAGMENT_LENGTH);
 
 			// NOTE: this copies both the IV/packet ID and the destination address.
-			memcpy(field(ZT_PACKET_FRAGMENT_IDX_PACKET_ID,13),p.data() + ZT_PACKET_IDX_IV,13);
+			memcpy(field(ZT_PACKET_FRAGMENT_IDX_PACKET_ID,13),field(ZT_PACKET_IDX_IV,13),13);
 
 			(*this)[ZT_PACKET_FRAGMENT_IDX_FRAGMENT_INDICATOR] = ZT_PACKET_FRAGMENT_INDICATOR;
 			(*this)[ZT_PACKET_FRAGMENT_IDX_FRAGMENT_NO] = (char)(((fragTotal & 0xf) << 4) | (fragNo & 0xf));
 			(*this)[ZT_PACKET_FRAGMENT_IDX_HOPS] = 0;
 
-			memcpy(field(ZT_PACKET_FRAGMENT_IDX_PAYLOAD,fragLen),p.data() + fragStart,fragLen);
+			memcpy(field(ZT_PACKET_FRAGMENT_IDX_PAYLOAD,fragLen),field(fragStart,fragLen),fragLen);
 		}
 
 		/**
diff --git a/node/Topology.cpp b/node/Topology.cpp
index 86c1befb..5f8401dd 100644
--- a/node/Topology.cpp
+++ b/node/Topology.cpp
@@ -356,7 +356,7 @@ void Topology::_loadPeers()
 		if ((fread(magic,5,1,pd) == 1)&&(!memcmp("ZTPD0",magic,5))) {
 			long rlen = 0;
 			do {
-				long rlen = (long)fread(buf.data() + buf.size(),1,ZT_PEER_WRITE_BUF_SIZE - buf.size(),pd);
+				long rlen = (long)fread(const_cast<char *>(static_cast<const char *>(buf.data())) + buf.size(),1,ZT_PEER_WRITE_BUF_SIZE - buf.size(),pd);
 				if (rlen < 0) rlen = 0;
 				buf.setSize(buf.size() + (unsigned int)rlen);
 				unsigned int ptr = 0;
@@ -366,10 +366,7 @@ void Topology::_loadPeers()
 					_activePeers[p->address()] = p;
 					saveIdentity(p->identity());
 				}
-				if (ptr) {
-					memmove(buf.data(),buf.data() + ptr,buf.size() - ptr);
-					buf.setSize(buf.size() - ptr);
-				}
+				buf.behead(ptr);
 			} while (rlen > 0);
 		}
 	} catch ( ... ) {