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

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'
@@ -352,6 +355,22 @@ public:
 		_l = i;
 	}
 
+	/**
+	 * 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
 	 */
@@ -388,6 +407,11 @@ public:
 		Utils::burn(_b,sizeof(_b));
 	}
 
+	/**
+	 * @return Constant pointer to data in buffer
+	 */
+	inline const void *data() const throw() { return _b; }
+
 	/**
 	 * @return Size of data in buffer
 	 */

node/IncomingPacket.cpp

@@ -46,11 +46,11 @@ namespace ZeroTier {
 
 bool IncomingPacket::tryDecode(const RuntimeEnvironment *RR)
 {
+	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.
-		//TRACE("<< HELLO from %s(%s) (normal unencrypted HELLO)",source().toString().c_str(),_remoteAddress.toString().c_str());
 			return _doHELLO(RR);
 		}
 
@@ -69,7 +69,7 @@ bool IncomingPacket::tryDecode(const RuntimeEnvironment *RR)
 
 			switch(verb()) {
 				//case Packet::VERB_NOP:
-			default: // ignore unknown verbs, but if they pass auth check they are still valid
+				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);
@@ -91,6 +91,12 @@ bool IncomingPacket::tryDecode(const RuntimeEnvironment *RR)
 			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;
+	}
 }
 
 bool IncomingPacket::_doERROR(const RuntimeEnvironment *RR,const SharedPtr<Peer> &peer)
@@ -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());

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 {

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);
 		}
 
 		/**

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 ( ... ) {