CVE-2015-1521: Improper Restriction of Operations within the Bounds of a Memory Buffer (CWE-119)
value: analyzer/protocol/dnp3/DNP3.cc in Bro before 2.3.2 does not properly handle zero values of a packet length, which allows remote attackers to cause a denial of service (buffer overflow or buffer over-read if NDEBUG; otherwise assertion failure) via a crafted DNP3 packet.
CWE description: The product performs operations on a memory buffer, but it reads from or writes to a memory location outside the buffer's intended boundary. This may result in read or write operations on unexpected memory locations that could be linked to other variables, data structures, or internal program data.
CVSS v3 base score: 7.5
Published: 2017-04-24T06:59Z
Repository: https://github.com/bro/bro
Language: Other
Files changed: CHANGES, VERSION, src/analyzer/protocol/dnp3/DNP3.cc, src/analyzer/protocol/dnp3/DNP3.h
Commit date: 2015-01-23 11:25:00 -0600
Commit message: 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.

Diff: diff --git a/CHANGES b/CHANGES
index 4087615fe..a50dc265c 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,17 @@
 
+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)
 
 2.3-390 | 2015-01-14 13:27:34 -0800
diff --git a/VERSION b/VERSION
index 1cb805162..081c98cc5 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-2.3-392
+2.3-396
diff --git a/src/analyzer/protocol/dnp3/DNP3.cc b/src/analyzer/protocol/dnp3/DNP3.cc
index 135100eb6..b04dbf64e 100644
--- a/src/analyzer/protocol/dnp3/DNP3.cc
+++ b/src/analyzer/protocol/dnp3/DNP3.cc
@@ -136,13 +136,18 @@ bool DNP3_Base::ProcessData(int len, const u_char* data, bool orig)
 	while ( len )
 		{
 		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 )
 				{
 				analyzer->Weird("dnp3_header_lacks_magic");
 				return false;
@@ -184,24 +189,33 @@ bool DNP3_Base::ProcessData(int len, const u_char* data, bool orig)
 				interp->NewData(orig, endp->buffer, endp->buffer + PSEUDO_LINK_LAYER_LEN);
 			}
 
 		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
 			// raw bytes the application layer consists of from
 			// the packet length by determining how much 16-byte
 			// chunks fit in there, and then add 2 bytes CRC for
 			// each.
 			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;
 
 			// Done with this packet, prepare for next.
@@ -211,23 +225,46 @@ 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)
 	{
 	bool orig = (endp == &orig_state);
@@ -254,12 +291,19 @@ bool DNP3_Base::ParseAppLayer(Endpoint* endp)
 
 		// Make sure chunk has a correct checksum.
 		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;
 
 		data += n + 2;
 		len -= n;
diff --git a/src/analyzer/protocol/dnp3/DNP3.h b/src/analyzer/protocol/dnp3/DNP3.h
index 12c3624cd..aa4ef7847 100644
--- a/src/analyzer/protocol/dnp3/DNP3.h
+++ b/src/analyzer/protocol/dnp3/DNP3.h
@@ -29,11 +29,25 @@ protected:
 		bool encountered_first_chunk;
 		};
 
 	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);
 
 	static void PrecomputeCRCTable();

Vulnerable code: 2.3-392
			if ( ! AddToBuffer(endp, PSEUDO_APP_LAYER_INDEX, &data, &len) )
			assert(endp->pkt_length);
			if ( ! AddToBuffer(endp, n, &data, &len) )
bool DNP3_Base::AddToBuffer(Endpoint* endp, int target_len, const u_char** data, int* len)
		return true;
	return endp->buffer_len == target_len;
		assert(data + n < endp->buffer + endp->buffer_len);
	bool AddToBuffer(Endpoint* endp, int target_len, const u_char** data, int* len);
Fixed code: 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-396
			int res = AddToBuffer(endp, PSEUDO_APP_LAYER_INDEX, &data, &len);

			if ( res == 0 )
			if ( res < 0 )
				return false;

			if ( endp->pkt_length <= 0 )
				{
				analyzer->Weird("dnp3_negative_or_zero_length_link_layer");
				return false;
				}
			int res = AddToBuffer(endp, n, &data, &len);

			if ( res == 0 )
			if ( res < 0 )
				return false;

int DNP3_Base::AddToBuffer(Endpoint* endp, int target_len, const u_char** data, int* len)
		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;
		}
	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;
		}

	if ( endp->buffer_len == target_len )
		return 1;

	return 0;
		if ( data + n >= endp->buffer + endp->buffer_len )
			{
			reporter->AnalyzerError(analyzer,
			                        "dnp3 app layer parsing overflow %d - %d",
			                        endp->buffer_len, n);
			return false;
			}


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