BUG/MEDIUM: pattern: prevent uninitialized reads in pat_match_{str,beg}
authorAurelien DARRAGON <adarragon@haproxy.com>
Fri, 6 Sep 2024 14:33:15 +0000 (16:33 +0200)
committerChristopher Faulet <cfaulet@haproxy.com>
Fri, 8 Nov 2024 14:54:10 +0000 (15:54 +0100)
commit690ee88577ea592a10b22ecc6fd7abaa8b656989
treec274c2c6ab7ca0736fafa9417326277d5f4d063a
parentc2c009086d300d0f86b1274e0b9ba7cb798f3e1d
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.

(cherry picked from commit 8157c1caf26618d77b32be7906e4b608a8c0729b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
src/pattern.c