The IpfixParser was very carelessly reading beyond the end of message boundary.

* Tried to find and improve all places where reading beyond set and message bounderies could happen
* Tested the new parser with some forged testpackets, and it seems to work correctly
Note: The handling of variable length fields is still not threat-proof (see FIXMEs in IpfixParser.cpp)


git-svn-id: file:///Users/braun/svn/vermont/trunk/vermont@1517 aef3b71b-58ee-0310-9ba9-8811b9f0742f

concentrator/IpfixParser.hpp

@@ -128,10 +128,10 @@ class IpfixParser : public IpfixPacketProcessor, public FlowSource {
 
 		pthread_mutex_t mutex; /**< Used to give only one IpfixReceiver access to the IpfixPacketProcessor */
 
-		void processDataSet(boost::shared_ptr<IpfixRecord::SourceID> sourceID, boost::shared_array<uint8_t> message, IpfixSetHeader* set);
-		void processTemplateSet(boost::shared_ptr<IpfixRecord::SourceID> sourceID, boost::shared_array<uint8_t> message, IpfixSetHeader* set);
-		void processDataTemplateSet(boost::shared_ptr<IpfixRecord::SourceID> sourceID, boost::shared_array<uint8_t> message, IpfixSetHeader* set);
-		void processOptionsTemplateSet(boost::shared_ptr<IpfixRecord::SourceID> sourceId, boost::shared_array<uint8_t> message, IpfixSetHeader* set);
+		void processDataSet(boost::shared_ptr<IpfixRecord::SourceID> sourceID, boost::shared_array<uint8_t> message, IpfixSetHeader* set, uint8_t* endOfMessage);
+		void processTemplateSet(boost::shared_ptr<IpfixRecord::SourceID> sourceID, boost::shared_array<uint8_t> message, IpfixSetHeader* set, uint8_t* endOfMessage);
+		void processDataTemplateSet(boost::shared_ptr<IpfixRecord::SourceID> sourceID, boost::shared_array<uint8_t> message, IpfixSetHeader* set, uint8_t* endOfMessage);
+		void processOptionsTemplateSet(boost::shared_ptr<IpfixRecord::SourceID> sourceId, boost::shared_array<uint8_t> message, IpfixSetHeader* set, uint8_t* endOfMessage);
 		int processNetflowV9Packet(boost::shared_array<uint8_t> message, uint16_t length, boost::shared_ptr<IpfixRecord::SourceID> sourceId);
 		int processIpfixPacket(boost::shared_array<uint8_t> message, uint16_t length, boost::shared_ptr<IpfixRecord::SourceID> sourceId);
 

concentrator/IpfixRecord.hpp

@@ -69,6 +69,8 @@ class IpfixRecord {
 		 * Template description passed to the callback function when a new Template arrives.
 		 */
 		struct TemplateInfo {
+			TemplateInfo() : fieldInfo(NULL) {}
+
 			~TemplateInfo() {
 				free(fieldInfo);
 			}
@@ -326,6 +328,9 @@ class IpfixRecord {
 		 * Note that - other than in [PROTO] - fieldCount specifies only the number of regular fields
 		 */
 		struct OptionsTemplateInfo {
+			OptionsTemplateInfo() : scopeInfo(NULL), fieldInfo(NULL) {
+			}
+
 			~OptionsTemplateInfo() {
 				free(fieldInfo);
 				free(scopeInfo);
@@ -343,7 +348,7 @@ class IpfixRecord {
 		 * DataTemplate description passed to the callback function when a new DataTemplate arrives.
 		 */
 		struct DataTemplateInfo {
-			DataTemplateInfo() : freePointers(true) {
+			DataTemplateInfo() : fieldInfo(NULL), dataInfo(NULL), data(NULL), freePointers(true) {
 			}
 
 			~DataTemplateInfo() {

concentrator/TemplateBuffer.cpp

@@ -62,32 +62,32 @@ void TemplateBuffer::destroyBufferedTemplate(boost::shared_ptr<IpfixRecord::Sour
 {
 	TemplateBuffer::BufferedTemplate* predecessor = 0;
 	TemplateBuffer::BufferedTemplate* bt = head;
+	bool found = false;
 	while (bt != 0) {
-		if ((bt->sourceID->observationDomainId == sourceId->observationDomainId) && (bt->templateID == templateId)) break;
-		predecessor = bt;
-		bt = (TemplateBuffer::BufferedTemplate*)bt->next;
-	}
-	if (bt == 0) return;
-	if (predecessor != 0) {
-		predecessor->next = bt->next;
-	} else {
-		head = (TemplateBuffer::BufferedTemplate*)bt->next;
-	}
-	if (bt->setID == IPFIX_SetId_Template) {
-		/* Invoke all registered callback functions */
-		boost::shared_ptr<IpfixTemplateDestructionRecord> ipfixRecord(new IpfixTemplateDestructionRecord);
-		ipfixRecord->sourceID = sourceId;
-		ipfixRecord->templateInfo = bt->templateInfo;
-		ipfixParser->push(ipfixRecord);
-	} else
+		/* templateId == setID means that all templates of this set type shall be removed */
+		if ((bt->sourceID->observationDomainId == sourceId->observationDomainId) 
+				&& ((bt->templateID == templateId) || (bt->setID == templateId))) {
+			found = true;
+			if (predecessor != 0) {
+				predecessor->next = bt->next;
+			} else {
+				head = (TemplateBuffer::BufferedTemplate*)bt->next;
+			}
+			if (bt->setID == IPFIX_SetId_Template) {
+				/* Invoke all registered callback functions */
+				boost::shared_ptr<IpfixTemplateDestructionRecord> ipfixRecord(new IpfixTemplateDestructionRecord);
+				ipfixRecord->sourceID = sourceId;
+				ipfixRecord->templateInfo = bt->templateInfo;
+				ipfixParser->push(ipfixRecord);
+			} else
 #ifdef SUPPORT_NETFLOWV9
-		if (bt->setID == NetflowV9_SetId_Template) {
-			/* Invoke all registered callback functions */
-			boost::shared_ptr<IpfixTemplateDestructionRecord> ipfixRecord(new IpfixTemplateDestructionRecord);
-			ipfixRecord->sourceID = sourceId;
-			ipfixRecord->templateInfo = bt->templateInfo;
-			ipfixParser->push(ipfixRecord);
-		} else
+			if (bt->setID == NetflowV9_SetId_Template) {
+				/* Invoke all registered callback functions */
+				boost::shared_ptr<IpfixTemplateDestructionRecord> ipfixRecord(new IpfixTemplateDestructionRecord);
+				ipfixRecord->sourceID = sourceId;
+				ipfixRecord->templateInfo = bt->templateInfo;
+				ipfixParser->push(ipfixRecord);
+			} else
 #endif
 			if (bt->setID == IPFIX_SetId_OptionsTemplate) {
 				/* Invoke all registered callback functions */
@@ -95,19 +95,28 @@ void TemplateBuffer::destroyBufferedTemplate(boost::shared_ptr<IpfixRecord::Sour
 				ipfixRecord->sourceID = sourceId;
 				ipfixRecord->optionsTemplateInfo = bt->optionsTemplateInfo;
 				ipfixParser->push(ipfixRecord);
-			} else {
-				if (bt->setID == IPFIX_SetId_DataTemplate) {
-					/* Invoke all registered callback functions */
-					boost::shared_ptr<IpfixDataTemplateDestructionRecord> ipfixRecord(new IpfixDataTemplateDestructionRecord);
-					ipfixRecord->sourceID = sourceId;
-					ipfixRecord->dataTemplateInfo = bt->dataTemplateInfo;
-					ipfixParser->push(ipfixRecord);
+			} else if (bt->setID == IPFIX_SetId_DataTemplate) {
+				/* Invoke all registered callback functions */
+				boost::shared_ptr<IpfixDataTemplateDestructionRecord> ipfixRecord(new IpfixDataTemplateDestructionRecord);
+				ipfixRecord->sourceID = sourceId;
+				ipfixRecord->dataTemplateInfo = bt->dataTemplateInfo;
+				ipfixParser->push(ipfixRecord);
 
-				} else {
-					msg(MSG_FATAL, "Unknown template type requested to be freed: %d", bt->setID);
-				}
+			} else {
+				msg(MSG_FATAL, "Unknown template type requested to be freed: %d", bt->setID);
 			}
-		delete bt;
+			TemplateBuffer::BufferedTemplate* toBeFreed = bt;
+			bt = (TemplateBuffer::BufferedTemplate*)bt->next;
+			delete toBeFreed;
+		} else {
+			predecessor = bt;
+			bt = (TemplateBuffer::BufferedTemplate*)bt->next;
+		}
+	}
+	if (!found) {
+		DPRINTF("Destroy template - no matching template found (id=%u)", templateId);
+	}
+		
 }
 
 /**

sampler/Observer.cpp

@@ -82,9 +82,9 @@ Observer::Observer(const std::string& interface, InstanceManager<Packet>* manage
 	captureDevice(NULL), capturelen(PCAP_DEFAULT_CAPTURE_LENGTH), pcap_timeout(PCAP_TIMEOUT), 
 	pcap_promisc(1), ready(false), filter_exp(0), packetManager(manager),
 	receivedBytes(0), lastReceivedBytes(0), processedPackets(0), 
-	lastProcessedPackets(0), exitFlag(false), 
+	lastProcessedPackets(0), 
 	captureInterface(NULL), fileName(NULL), replaceTimestampsFromFile(false),
-	stretchTimeInt(1), stretchTime(1.0)
+	stretchTimeInt(1), stretchTime(1.0), exitFlag(false)
 {
 	if(offline) {
 		readFromFile = true;
@@ -295,7 +295,7 @@ void *Observer::observerThread(void *arg)
 			p->init((char*)pcapData, 
 				// in constrast to live capturing, the data length is not limited
 				// to any snap length when reading from a pcap file
-				(packetHeader.caplen<obs->capturelen) ? packetHeader.caplen : obs->capturelen, 
+				((int)packetHeader.caplen < obs->capturelen) ? (int)packetHeader.caplen : obs->capturelen, 
 				packetHeader.ts);
 
 			obs->receivedBytes += packetHeader.caplen;

tests/testpackets/README

@@ -0,0 +1,7 @@
+IPFIX Test Packets
+=================
+
+Use netcat (nc) to send them to the UDP (test) collector.
+
+Use hex editor (e.g., ghex) to conveniently create your own packets.
+

tools/testCollector.cpp

@@ -54,7 +54,7 @@ int main(int argc, char *argv[]) {
 
 	int lport = DEFAULT_LISTEN_PORT;
 
-	msg_setlevel(MSG_DEFAULT);
+	msg_setlevel(MSG_DEBUG);
 
 	signal(SIGINT, sigint);