BUG/MEDIUM: pattern: prevent uninitialized reads in pat_match_{str,beg}

Using valgrind when running map_beg or map_str, the following error is
reported:

==242644== Conditional jump or move depends on uninitialised value(s)
==242644==    at 0x2E4AB1: pat_match_str (pattern.c:457)
==242644==    by 0x2E81ED: pattern_exec_match (pattern.c:2560)
==242644==    by 0x343176: sample_conv_map (map.c:211)
==242644==    by 0x27522F: sample_process_cnv (sample.c:1330)
==242644==    by 0x2752DB: sample_process (sample.c:1373)
==242644==    by 0x319917: action_store (vars.c:814)
==242644==    by 0x24D451: http_req_get_intercept_rule (http_ana.c:2697)

In fact, the error is legit, because in pat_match_{beg,str}, we
dereference the buffer on len+1 to check if a value was previously set,
and then decide to force NULL-byte if it wasn't set.

But the approach is no longer compatible with current architecture:
data past str.data is not guaranteed to be initialized in the buffer.
Thus we cannot dereference the value, else we expose us to uninitialized
read errors. Moreover, the check is useless, because we systematically
set the ending byte to 0 when the conditions are met.

Finally, restoring the older value after the lookup is not relevant:
indeed, either the sample is marked as const and in such case it
is already duplicated, or the sample is not const and we forcefully add
a terminating NULL byte outside from the actual string bytes (since we're
past str.data), so as we didn't alter effective string data and that data
past str.data cannot be dereferenced anyway as it isn't guaranteed to be
initialized, there's no point in restoring previous uninitialized data.

It could be backported in all stable versions. But since this was only
detected by valgrind and isn't known to cause issues in existing
deployments, it's probably better to wait a bit before backporting it
to avoid any breakage.. although the fix should be theoretically harmless.

diff --git a/src/pattern.c b/src/pattern.c
index 4b7540b40979b9221e54b4aec107bb3bf6c6a241..346fe59d553ebb937eba08dcea5fc91eecf8c3ab 100644 (file)
--- a/src/pattern.c
+++ b/src/pattern.c
@@ -432,6 +432,33 @@ struct pattern *pat_match_nothing(struct sample *smp, struct pattern_expr *expr,
                return NULL;
 }
 
+/* ensure the input sample can be read as a string without knowing its size,
+ * that is, ensure the terminating null byte is there
+ *
+ * The function may fail. Returns 1 on success and 0 on failure
+ */
+static inline int pat_match_ensure_str(struct sample *smp)
+{
+       if (smp->data.u.str.data < smp->data.u.str.size) {
+               /* we have to force a trailing zero on the test pattern and
+                * the buffer is large enough to accommodate it. If the flag
+                * CONST is set, duplicate the string
+                */
+               if (smp->flags & SMP_F_CONST) {
+                       if (!smp_dup(smp))
+                               return 0;
+               } else
+                       smp->data.u.str.area[smp->data.u.str.data] = '\0';
+       }
+       else {
+               /* Otherwise, the sample is duplicated. A trailing zero
+                * is automatically added to the string.
+                */
+               if (!smp_dup(smp))
+                       return 0;
+       }
+       return 1;
+}
 
 /* NB: For two strings to be identical, it is required that their length match */
 struct pattern *pat_match_str(struct sample *smp, struct pattern_expr *expr, int fill)
@@ -446,34 +473,10 @@ struct pattern *pat_match_str(struct sample *smp, struct pattern_expr *expr, int
 
        /* Lookup a string in the expression's pattern tree. */
        if (!eb_is_empty(&expr->pattern_tree)) {
-               char prev = 0;
-
-               if (smp->data.u.str.data < smp->data.u.str.size) {
-                       /* we may have to force a trailing zero on the test pattern and
-                        * the buffer is large enough to accommodate it. If the flag
-                        * CONST is set, duplicate the string
-                        */
-                       prev = smp->data.u.str.area[smp->data.u.str.data];
-                       if (prev) {
-                               if (smp->flags & SMP_F_CONST) {
-                                       if (!smp_dup(smp))
-                                               return NULL;
-                               } else {
-                                       smp->data.u.str.area[smp->data.u.str.data] = '\0';
-                               }
-                       }
-               }
-               else {
-                       /* Otherwise, the sample is duplicated. A trailing zero
-                        * is automatically added to the string.
-                        */
-                       if (!smp_dup(smp))
-                               return NULL;
-               }
+               if (!pat_match_ensure_str(smp))
+                       return NULL;
 
                node = ebst_lookup(&expr->pattern_tree, smp->data.u.str.area);
-               if (prev)
-                       smp->data.u.str.area[smp->data.u.str.data] = prev;
 
                while (node) {
                        elt = ebmb_entry(node, struct pattern_tree, node);
@@ -647,35 +650,11 @@ struct pattern *pat_match_beg(struct sample *smp, struct pattern_expr *expr, int
 
        /* Lookup a string in the expression's pattern tree. */
        if (!eb_is_empty(&expr->pattern_tree)) {
-               char prev = 0;
-
-               if (smp->data.u.str.data < smp->data.u.str.size) {
-                       /* we may have to force a trailing zero on the test pattern and
-                        * the buffer is large enough to accommodate it. If the flag
-                        * CONST is set, duplicate the string
-                        */
-                       prev = smp->data.u.str.area[smp->data.u.str.data];
-                       if (prev) {
-                               if (smp->flags & SMP_F_CONST) {
-                                       if (!smp_dup(smp))
-                                               return NULL;
-                               } else {
-                                       smp->data.u.str.area[smp->data.u.str.data] = '\0';
-                               }
-                       }
-               }
-               else {
-                       /* Otherwise, the sample is duplicated. A trailing zero
-                        * is automatically added to the string.
-                        */
-                       if (!smp_dup(smp))
-                               return NULL;
-               }
+               if (!pat_match_ensure_str(smp))
+                       return NULL;
 
                node = ebmb_lookup_longest(&expr->pattern_tree,
                                           smp->data.u.str.area);
-               if (prev)
-                       smp->data.u.str.area[smp->data.u.str.data] = prev;
 
                while (node) {
                        elt = ebmb_entry(node, struct pattern_tree, node);