gamzinav/haproxy
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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.

(cherry picked from commit 8157c1caf26618d77b32be7906e4b608a8c0729b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
This commit is contained in:
Aurelien DARRAGON 2024-09-06 16:33:15 +02:00 committed by Christopher Faulet
parent c2c009086d
commit 690ee88577
1 changed files with 31 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

83
src/pattern.c
View File

@ -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);