From ef933c9e76308a59f5a1a8f02d29016327be2a37 Mon Sep 17 00:00:00 2001
From: Jon Siwek <jsiwek@corelight.com>
Date: Wed, 26 Jun 2019 23:31:59 -0700
Subject: [PATCH] binpac: Fix signed integer overflow in array bounds checks

Array lengths use signed integer storage, so multiplication of that
by the element size for purpose of bounds checking against available
data may produce a signed integer overlow, which is undefined behavior.
---
 tools/binpac/src/pac_array.cc | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/tools/binpac/src/pac_array.cc b/tools/binpac/src/pac_array.cc
index fc07e77517..16d961d80b 100644
--- a/tools/binpac/src/pac_array.cc
+++ b/tools/binpac/src/pac_array.cc
@@ -292,7 +292,7 @@ void ArrayType::GenArrayLength(Output *out_cc, Env *env, const DataPtr& data)
 		out_cc->println("}");
 		out_cc->dec_indent();
 
-		string array_size;
+		int element_size;
 
 		if ( elemtype_->StaticSize(env) == -1 )
 			{
@@ -307,7 +307,7 @@ void ArrayType::GenArrayLength(Output *out_cc, Env *env, const DataPtr& data)
 			// buffer.
 			out_cc->println("// Check array element quantity: %s",
 			                data_id_str_.c_str());
-			array_size = strfmt("(%s)", env->RValue(arraylength_var()));
+			element_size = 1;
 			}
 		else
 			{
@@ -315,21 +315,34 @@ void ArrayType::GenArrayLength(Output *out_cc, Env *env, const DataPtr& data)
 			out_cc->println("// Check bounds for static-size array: %s",
 			                data_id_str_.c_str());
 			elemtype_->SetBoundaryChecked();
-			array_size = strfmt("((%d) * (%s))",
-			                    elemtype_->StaticSize(env),
-			                    env->RValue(arraylength_var()));
+			element_size = elemtype_->StaticSize(env);
+
+			if ( element_size == 0 )
+				{
+				// If we know we have an array of empty elements, probably
+				// better to structure the parser as just a single empty
+				// field to avoid DoS vulnerability of allocating
+				// arbitrary number of empty records (i.e. cheap for them,
+				// but costly for us unless we have special optimization
+				// for this scenario to forgo the usual allocation).
+				throw Exception(this, "using an array of known-to-be-empty elements is possibly a bad idea");
+				}
 			}
 
 		const char* array_ptr_expr = data.ptr_expr();
+		string max_elements_available = strfmt("((%s - %s) / %d)",
+		                                       env->RValue(end_of_data),
+		                                       array_ptr_expr,
+		                                       element_size);
 
-		out_cc->println("if ( %s + %s > %s || %s + %s < %s )",
-		                array_ptr_expr, array_size.c_str(), env->RValue(end_of_data),
-		                array_ptr_expr, array_size.c_str(), array_ptr_expr);
+		out_cc->println("if ( %s > %s )",
+		                env->RValue(arraylength_var()),
+		                max_elements_available.c_str());
 		out_cc->inc_indent();
 		out_cc->println("throw binpac::ExceptionOutOfBound(\"%s\",",
 		                data_id_str_.c_str());
 		out_cc->println("  %s, (%s) - (%s));",
-		                array_size.c_str(),
+		                env->RValue(arraylength_var()),
 		                env->RValue(end_of_data),
 		                array_ptr_expr);
 		out_cc->dec_indent();