DNP3: fix reachable assertion and buffer over-read/overflow.

A DNP3 packet using a link layer header that specifies a zero length can
trigger an assertion failure if assertions are enabled.  Assertions are
enabled unless Bro is compiled with the NDEBUG preprocessor macro
defined.  The default configuration of Bro will define this macro and so
disables assertions, but using the --enable-debug option in the
configure script will enable assertions.  When assertions are disabled,
or also for certain length values, the DNP3 parser may attempt to pass a
negative value as the third argument to memcpy (number of bytes to copy)
and result in a buffer over-read or overflow.

Reported by Travis Emmert.

CHANGES

@@ -1,4 +1,15 @@
 
+2.3-396 | 2015-01-23 10:49:15 -0600
+
+  * DNP3: fix reachable assertion and buffer over-read/overflow.
+    CVE number pending. (Travis Emmert, Jon Siwek)
+
+  * Update binpac: Fix potential out-of-bounds memory reads in generated
+    code. CVE-2014-9586. (John Villamil and Chris Rohlf - Yahoo
+    Paranoids, Jon Siwek)
+
+  * Fixing (harmless) Coverity warning. (Robin Sommer)
+
 2.3-392 | 2015-01-15 09:44:15 -0800
 
   * Small changes to EC curve names in a newer draft. (Johanna Amann)

VERSION

@@ -1 +1 @@
-2.3-392
+2.3-396

src/analyzer/protocol/dnp3/DNP3.cc

@@ -138,9 +138,14 @@ bool DNP3_Base::ProcessData(int len, const u_char* data, bool orig)
 		if ( endp->in_hdr )
 			{
 			// We're parsing the DNP3 header and link layer, get that in full.
-			if ( ! AddToBuffer(endp, PSEUDO_APP_LAYER_INDEX, &data, &len) )
+			int res = AddToBuffer(endp, PSEUDO_APP_LAYER_INDEX, &data, &len);
+
+			if ( res == 0 )
 				return true;
 
+			if ( res < 0 )
+				return false;
+
 			// The first two bytes must always be 0x0564.
 			if( endp->buffer[0] != 0x05 || endp->buffer[1] != 0x64 )
 				{
@@ -186,7 +191,11 @@ bool DNP3_Base::ProcessData(int len, const u_char* data, bool orig)
 
 		if ( ! endp->in_hdr )
 			{
-			assert(endp->pkt_length);
+			if ( endp->pkt_length <= 0 )
+				{
+				analyzer->Weird("dnp3_negative_or_zero_length_link_layer");
+				return false;
+				}
 
 			// We're parsing the DNP3 application layer, get that
 			// in full now as well. We calculate the number of
@@ -197,9 +206,14 @@ bool DNP3_Base::ProcessData(int len, const u_char* data, bool orig)
 			int n = PSEUDO_APP_LAYER_INDEX + (endp->pkt_length - 5) + ((endp->pkt_length - 5) / 16) * 2
 					+ 2 * ( ((endp->pkt_length - 5) % 16 == 0) ? 0 : 1) - 1 ;
 
-			if ( ! AddToBuffer(endp, n, &data, &len) )
+			int res = AddToBuffer(endp, n, &data, &len);
+
+			if ( res == 0 )
 				return true;
 
+			if ( res < 0 )
+				return false;
+
 			// Parse the the application layer data.
 			if ( ! ParseAppLayer(endp) )
 				return false;
@@ -213,19 +227,42 @@ bool DNP3_Base::ProcessData(int len, const u_char* data, bool orig)
 	return true;
 	}
 
-bool DNP3_Base::AddToBuffer(Endpoint* endp, int target_len, const u_char** data, int* len)
+int DNP3_Base::AddToBuffer(Endpoint* endp, int target_len, const u_char** data, int* len)
 	{
 	if ( ! target_len )
-		return true;
+		return 1;
+
+	if ( *len < 0 )
+		{
+		reporter->AnalyzerError(analyzer, "dnp3 negative input length: %d", *len);
+		return -1;
+		}
+
+	if ( target_len < endp->buffer_len )
+		{
+		reporter->AnalyzerError(analyzer, "dnp3 invalid target length: %d - %d",
+		                        target_len, endp->buffer_len);
+		return -1;
+		}
 
 	int to_copy = min(*len, target_len - endp->buffer_len);
 
+	if ( endp->buffer_len + to_copy > MAX_BUFFER_SIZE )
+		{
+		reporter->AnalyzerError(analyzer, "dnp3 buffer length exceeded: %d + %d",
+		                        endp->buffer_len, to_copy);
+		return -1;
+		}
+
 	memcpy(endp->buffer + endp->buffer_len, *data, to_copy);
 	*data += to_copy;
 	*len -= to_copy;
 	endp->buffer_len += to_copy;
 
-	return endp->buffer_len == target_len;
+	if ( endp->buffer_len == target_len )
+		return 1;
+
+	return 0;
 	}
 
 bool DNP3_Base::ParseAppLayer(Endpoint* endp)
@@ -256,8 +293,15 @@ bool DNP3_Base::ParseAppLayer(Endpoint* endp)
 		if ( ! CheckCRC(n, data, data + n, "app_chunk") )
 			return false;
 
+		if ( data + n >= endp->buffer + endp->buffer_len )
+			{
+			reporter->AnalyzerError(analyzer,
+			                        "dnp3 app layer parsing overflow %d - %d",
+			                        endp->buffer_len, n);
+			return false;
+			}
+
 		// Pass on to BinPAC.
-		assert(data + n < endp->buffer + endp->buffer_len);
 		flow->flow_buffer()->BufferData(data + transport, data + n);
 		transport = 0;
 

src/analyzer/protocol/dnp3/DNP3.h

@@ -31,7 +31,21 @@ protected:
 
 	bool ProcessData(int len, const u_char* data, bool orig);
 	void ClearEndpointState(bool orig);
-	bool AddToBuffer(Endpoint* endp, int target_len, const u_char** data, int* len);
+
+	/**
+	 * Buffers packet data until it reaches a specified length.
+	 * @param endp an endpoint speaking DNP3 to which data will be buffered.
+	 * @param target_len the required length of the buffer
+	 * @param data source buffer to copy bytes from.  Will be incremented
+	 * by the number of bytes copied by this function.
+	 * @param len the number of bytes available in \a data.  Will be decremented
+	 * by the number of bytes copied by this function.
+	 * @return -1 if invalid input parameters were supplied, 0 if the endpoint's
+	 * buffer is not yet \a target_len bytes in size, or 1 the buffer is the
+	 * required size.
+	 */
+	int AddToBuffer(Endpoint* endp, int target_len, const u_char** data, int* len);
+
 	bool ParseAppLayer(Endpoint* endp);
 	bool CheckCRC(int len, const u_char* data, const u_char* crc16, const char* where);
 	unsigned int CalcCRC(int len, const u_char* data);