[master] Terminate strings before calling regexec

    Make sure strings are terminated before callng regexec.
    If they are we can simply copy the pointers, if they
    aren't we need to copy the string into a new block
    of memory.

    Fix a boundary error in data_string_new()

5 files changed, 132 insertions, 14 deletions

diff --git a/RELNOTES b/RELNOTES
index 55ee8dbb..b6331225 100644
--- a/RELNOTES
+++ b/RELNOTES
@@ -145,6 +145,11 @@ by Eric Young (eay@cryptsoft.com).
   the software to fail.
   [ISC-Bugs #40371]
 
+- Properly terminate strings before passing them to regex and fix
+  a boudnary error when creating certain new data strings.
+  Thanks to Andrey Jr. Melnikov for the bug report.
+  [ISC-Bugs #41217]
+
 			Changes since 4.3.3b1
 - None
 
diff --git a/common/alloc.c b/common/alloc.c
index a55f4712..f90556fc 100644
--- a/common/alloc.c
+++ b/common/alloc.c
@@ -3,7 +3,7 @@
    Memory allocation... */
 
 /*
- * Copyright (c) 2009,2013-2014 by Internet Systems Consortium, Inc. ("ISC")
+ * Copyright (c) 2009,2013-2014,2016 by Internet Systems Consortium, Inc. ("ISC")
  * Copyright (c) 2004-2007 by Internet Systems Consortium, Inc. ("ISC")
  * Copyright (c) 1996-2003 by Internet Software Consortium
  *
@@ -240,7 +240,7 @@ int group_dereference (ptr, file, line)
 
 	if (group -> object)
 		group_object_dereference (&group -> object, file, line);
-	if (group -> subnet)	
+	if (group -> subnet)
 		subnet_dereference (&group -> subnet, file, line);
 	if (group -> shared_network)
 		shared_network_dereference (&group -> shared_network,
@@ -497,7 +497,7 @@ void relinquish_free_expressions ()
 #endif
 
 struct binding_value *free_binding_values;
-				
+
 int binding_value_allocate (cptr, file, line)
 	struct binding_value **cptr;
 	const char *file;
@@ -685,7 +685,7 @@ int buffer_allocate (ptr, len, file, line)
 {
 	struct buffer *bp;
 
-	/* XXXSK: should check for bad ptr values, otherwise we 
+	/* XXXSK: should check for bad ptr values, otherwise we
 		  leak memory if they are wrong */
 	bp = dmalloc (len + sizeof *bp, file, line);
 	if (!bp)
@@ -1279,25 +1279,25 @@ data_string_new(struct data_string *new_string,
 	if (new_string == NULL) {
 		log_error("data_string_new: new_string cannot be NULL %s(%d)",
 			  file, line);
-                return (0);
+		return (0);
 	}
 
 	if (src == NULL) {
 		log_error("data_string_new: src cannot be NULL %s(%d)",
 			  file, line);
-                return (0);
+		return (0);
 	}
 
 	memset(new_string, 0, sizeof (struct data_string));
 
 	/* If we already have a NULL back off length by one. This lets
 	 * us always just add a NULL at the end. */
-	copy_len = (len > 0 && src[len - 1 ] == 0) ? len - 1 : len;
+	copy_len = (len > 0 && src[len - 1] == 0) ? len - 1 : len;
 
 	/* Allocate the buffer, accounting for terminating null */
 	if (!buffer_allocate(&(new_string->buffer), copy_len + 1,  MDL)) {
 		log_error("data_string_new: No memory %s(%d)", file, line);
-                return (0);
+		return (0);
 	}
 
 	/* Only copy if there's something to copy */
@@ -1306,7 +1306,7 @@ data_string_new(struct data_string *new_string,
 	}
 
 	/* Always tack on the null */
-	new_string->buffer->data[copy_len + 1] = 0;
+	new_string->buffer->data[copy_len] = 0;
 
 	/* Update data_string accessor values.  Note len does NOT include
 	 * the NULL.  */
@@ -1347,7 +1347,7 @@ void data_string_forget (data, file, line)
 	memset (data, 0, sizeof *data);
 }
 
-/* If the data_string is larger than the specified length, reduce 
+/* If the data_string is larger than the specified length, reduce
    the data_string to the specified size. */
 
 void data_string_truncate (dp, len)
@@ -1360,3 +1360,49 @@ void data_string_truncate (dp, len)
 		dp -> len = len;
 	}
 }
+
+/* \brief Converts a data_string to a null-terminated data string
+ *
+ * If the given string isn't null-terminated, replace it with a
+ * null-terminated version and free the current string decrementing
+ * the referecne count.  If the string is null-terminated it is left
+ * as is.
+ *
+ * Currently this routine doesn't check if the string is 0 length
+ * that must be checked by the caller.
+ *
+ * \param [in/out] str the data_string to convert
+ * \param file the file this routine was called from
+ * \param line the line this routine was called from
+ *
+ * \return 1 if the string was converted successfully (or already terminated),
+ * 0 if the conversion failed.  Failure is only possible if memory for the new
+ * string could not be allocated.  If the conversion fails, the original
+ * string's content is lost.
+ */
+int data_string_terminate(str, file, line)
+	struct data_string* str;
+	const char *file;
+	int line;
+{
+	int ret_val = 1;
+
+	if (str->terminated == 0) {
+		struct data_string temp;
+		memset(&temp, 0, sizeof(temp));
+
+		data_string_copy(&temp, str, file, line);
+		data_string_forget(str, file, line);
+		if (data_string_new(str, (const char*)temp.data, temp.len,
+				    file, line) == 0) {
+			/* couldn't create a copy, probably a memory issue,
+			 * an error message has already been logged. */
+			ret_val = 0;
+		}
+
+		/* get rid of temp string */
+		data_string_forget(&temp, file, line);
+	}
+
+	return (ret_val);
+}
diff --git a/common/tests/test_alloc.c b/common/tests/test_alloc.c
index 9e082135..41e3848f 100644
--- a/common/tests/test_alloc.c
+++ b/common/tests/test_alloc.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2007,2009-2014 by Internet Systems Consortium, Inc. ("ISC")
+ * Copyright (c) 2007,2009-2014,2016 by Internet Systems Consortium, Inc. ("ISC")
  *
  * We test the functions provided in alloc.c here. These are very
  * basic functions, and it is very important that they work correctly.
@@ -485,6 +485,59 @@ const char* checkString (struct data_string* string,
     return (NULL);
 }
 
+ATF_TC(data_string_terminate);
+
+ATF_TC_HEAD(data_string_terminate, tc) {
+    atf_tc_set_md_var(tc, "descr", "data_string_terminate test, "
+                      "exercises data_string_terminate function");
+}
+
+ATF_TC_BODY(data_string_terminate, tc) {
+    struct data_string new_string, copy_string;
+    const char *src = "Boring test string";
+
+    /* Case 1: Call with an already terminated string.  The
+     * original structure shouldn't be touched.
+     */
+    memset(&new_string, 0, sizeof(new_string));
+    memset(&copy_string, 0, sizeof(copy_string));
+    if (data_string_new(&new_string, src, strlen(src), MDL) == 0) {
+	atf_tc_fail("Case 1: unable to create new string");
+    }
+    memcpy(&copy_string, &new_string, sizeof(new_string));
+    if (data_string_terminate(&new_string, MDL) == 0) {
+	atf_tc_fail("Case 1: unable to terminate string");
+    }
+    if (memcmp(&copy_string, &new_string, sizeof(new_string)) != 0) {
+	atf_tc_fail("Case 1: structure modified");
+    }
+
+    /* Case 2: Call with an unterminated string.  The
+     * original structure should be modified with a pointer
+     * to new memory for the string.
+     */
+    /* clear the termination flag, and shrink the string */
+    new_string.terminated = 0;
+    new_string.len -= 2;
+    memcpy(&copy_string, &new_string, sizeof(new_string));
+
+    if (data_string_terminate(&new_string, MDL) == 0) {
+	atf_tc_fail("Case 2: unable to terminate string");
+    }
+
+    /* We expect the same string but in a differnet block of memory */
+    if ((new_string.terminated == 0) ||
+	(&new_string.buffer == &copy_string.buffer) ||
+	(new_string.len != copy_string.len) ||
+	memcmp(new_string.data, src, new_string.len) ||
+	new_string.data[new_string.len] != 0) {
+	atf_tc_fail("Case 2: structure not modified correctly");
+    }
+
+    /* get rid of the string, no need to get rid of copy as the
+     * string memory was freed during the terminate call */
+    data_string_forget(&new_string, MDL);
+}
 
 ATF_TP_ADD_TCS(tp)
 {
@@ -496,6 +549,7 @@ ATF_TP_ADD_TCS(tp)
     ATF_TP_ADD_TC(tp, data_string_copy);
     ATF_TP_ADD_TC(tp, data_string_copy_nobuf);
     ATF_TP_ADD_TC(tp, data_string_new);
+    ATF_TP_ADD_TC(tp, data_string_terminate);
 
     return (atf_no_error());
 }
diff --git a/common/tree.c b/common/tree.c
index 3e244994..c4965ff1 100644
--- a/common/tree.c
+++ b/common/tree.c
@@ -3,7 +3,7 @@
    Routines for manipulating parse trees... */
 
 /*
- * Copyright (c) 2011-2014 by Internet Systems Consortium, Inc. ("ISC")
+ * Copyright (c) 2011-2014,2016 by Internet Systems Consortium, Inc. ("ISC")
  * Copyright (c) 2004-2007,2009 by Internet Systems Consortium, Inc. ("ISC")
  * Copyright (c) 1995-2003 by Internet Software Consortium
  *
@@ -804,6 +804,18 @@ int evaluate_boolean_expression (result, packet, lease, client_state,
 						 in_options, cfg_options,
 						 scope,
 						 expr->data.equal[0], MDL);
+
+		/* This is annoying, regexec requires the string being processed
+		 * to be NULL terminated, but left may not be, so pass it into
+		 * the termination function to ensure it's null terminated.
+		 */
+		if (bleft && (data_string_terminate(&left, MDL) == 0)) {
+			/* failed to make a null terminated version, couldn't
+			 * create a copy, probably a memory issue, an error
+			 * message has already been logged */
+			bleft = 0;
+		}
+
 		memset(&right, 0, sizeof right);
 		bright = evaluate_data_expression(&right, packet, lease,
 						  client_state,
@@ -815,7 +827,7 @@ int evaluate_boolean_expression (result, packet, lease, client_state,
 		memset(&re, 0, sizeof(re));
 		if (bleft && bright &&
 		    (left.data != NULL) && (right.data != NULL) &&
-        	    (regcomp(&re, (char *)right.data, regflags) == 0) &&
+		    (regcomp(&re, (char *)right.data, regflags) == 0) &&
 		    (regexec(&re, (char *)left.data, (size_t)0, NULL, 0) == 0))
 				*result = 1;
 
@@ -839,7 +851,7 @@ int evaluate_boolean_expression (result, packet, lease, client_state,
 		 * If we have bleft and bright then we have a good
 		 * syntax, otherwise not.
 		 *
-		 * XXX: we don't warn on invalid regular expression 
+		 * XXX: we don't warn on invalid regular expression
 		 *      syntax, should we?
 		 */
 		return bleft && bright;
diff --git a/includes/dhcpd.h b/includes/dhcpd.h
index 230df59f..4270edca 100644
--- a/includes/dhcpd.h
+++ b/includes/dhcpd.h
@@ -2487,6 +2487,7 @@ void data_string_copy(struct data_string *, const struct data_string *,
 		      const char *, int);
 void data_string_forget (struct data_string *, const char *, int);
 void data_string_truncate (struct data_string *, int);
+int data_string_terminate (struct data_string *, const char *, int);
 int executable_statement_allocate (struct executable_statement **,
 				   const char *, int);
 int executable_statement_reference (struct executable_statement **,