]> git.ipfire.org Git - thirdparty/dhcp.git/commitdiff
[v4_1_esv] Terminate strings before calling regexec
authorShawn Routhier <sar@isc.org>
Sat, 16 Jan 2016 03:54:39 +0000 (19:54 -0800)
committerShawn Routhier <sar@isc.org>
Sat, 16 Jan 2016 03:54:39 +0000 (19:54 -0800)
    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()

RELNOTES
common/alloc.c
common/tests/test_alloc.c
common/tree.c
includes/dhcpd.h

index 78de6431de355e1716d78f9870ebe809044bae92..af06ba69111e5a327510f2109fcf662d1c7ea6d8 100644 (file)
--- a/RELNOTES
+++ b/RELNOTES
@@ -133,6 +133,11 @@ by Eric Young (eay@cryptsoft.com).
   to build 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.1-ESV-R12b1
 
 - None
index a55f4712bfc76061b07e1ad211266d3a5d1e2ef9..f90556fc5f86307b2a60a5f99c6faf2e7e187995 100644 (file)
@@ -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);
+}
index 9e082135747baaf835a9df4d74879d2bc898b259..41e3848f7989b090caa0931188fca6685cab64c2 100644 (file)
@@ -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());
 }
index 204a75aa0089bc4264371db1a256bc02d79bed19..2c4ab1d05f0620eb5e542d3b36f2e4486205b5fb 100644 (file)
@@ -3,7 +3,7 @@
    Routines for manipulating parse trees... */
 
 /*
- * Copyright (c) 2011-2015 by Internet Systems Consortium, Inc. ("ISC")
+ * Copyright (c) 2011-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
  *
@@ -1109,6 +1109,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,
@@ -1120,7 +1132,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;
 
@@ -1144,7 +1156,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;
index c310adb7f74d2f20bfd0e7f31a239b642c635fd7..a5c7ff551921ca74183eec1373126871567fd446 100644 (file)
@@ -2137,6 +2137,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 **,