]> git.ipfire.org Git - thirdparty/dhcp.git/commitdiff
[master] Terminate strings before calling regexec
authorShawn Routhier <sar@isc.org>
Sat, 16 Jan 2016 03:49:23 +0000 (19:49 -0800)
committerShawn Routhier <sar@isc.org>
Sat, 16 Jan 2016 03:49:23 +0000 (19:49 -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 55ee8dbb54d78ce653cd33285cebf6505435cdc8..b63312259080b8af580654662e200f2c3d23a8f1 100644 (file)
--- 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
 
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 3e2449945f25d86cac5ed3ca0e478503a8ac6a42..c4965ff129a04420a4a2c4da3b176691991f56e8 100644 (file)
@@ -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;
index 230df59f82a3e3bea03b297ff96bd8a3a20cd617..4270edcace4bac152ad96ce88ba0032287ab0959 100644 (file)
@@ -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 **,