BUG/MEDIUM: pattern: don't dereference static_pattern's data when not filling it

The match functions (pat_match_str() etc) may return a pointer to the
thread-local <static_pattern> without updating it when <fill> is not
set. However pattern_exec_match() then unconditionally duplicates the
returned pattern's sample data, dereferencing static_pattern.data, which
may still hold a stale pointer left there by an earlier lookup performed
on the same thread with <fill> set but without going through
pattern_exec_match(). This is what the CLI's "get map" does in
cli_io_handler_map_lookup(): it calls the match function directly, so
static_pattern.data points to the element's real sample data. If this
element is later freed (e.g. by "del map", "clear map", or the purge of
the old generation after "commit map"), the next ACL match on this thread
succeeding via the tree (ACLs use fill=0) reads freed memory in
pattern_exec_match().

This can be reproduced with an ACL and a map, both using virtual files,
with one thread:

    acl known url_param(q) -f virt@cliacl
    http-request set-var(txn.map) url_param(q),map(virt@climap)

then on the CLI: "add map virt@climap one first", "get map virt@climap
one", "prepare map virt@climap", "prepare acl virt@cliacl", "add map @1
virt@climap two third", "add acl @1 virt@cliacl two", "commit map @1
virt@climap", "commit acl @1 virt@cliacl", and finally a request to
"/?q=two" makes ASAN report a heap-use-after-free in pattern_exec_match().
Without ASAN, random garbage may be read, or the process may crash.

The fix consists in not touching the returned pattern at all when <fill>
is not set, since the caller only checks it against NULL. The problem was
introduced in 1.8 with commit b5997f740 ("MAJOR: threads/map: Make
acls/maps thread safe") so this must be backported to all stable versions.

diff --git a/src/pattern.c b/src/pattern.c
index f4ed2791d7ecc62a11f346b61e07b5034b0da406..ff6e4d54a2de2e2a8d7ac66a3b5b49af7b95a501 100644 (file)
--- a/src/pattern.c
+++ b/src/pattern.c
@@ -2729,16 +2729,17 @@ struct pattern *pattern_exec_match(struct pattern_head *head, struct sample *smp
                HA_RWLOCK_RDLOCK(PATEXP_LOCK, &list->expr->lock);
                pat = head->match(smp, list->expr, fill);
                if (pat) {
-                       /* We duplicate the pattern cause it could be modified
-                          by another thread */
-                       if (pat != &static_pattern) {
+                       /* If the caller wants a copy of the pattern (fill), we
+                        * duplicate it because it could be modified by another
+                        * thread.
+                        */
+                       if (fill && pat != &static_pattern) {
                                memcpy(&static_pattern, pat, sizeof(struct pattern));
                                pat = &static_pattern;
                        }
 
-                       /* We also duplicate the sample data for
-                          same reason */
-                       if (pat->data && (pat->data != &static_sample_data)) {
+                       /* We also duplicate the sample data for same reason */
+                       if (fill && pat->data && (pat->data != &static_sample_data)) {
                                switch(pat->data->type) {
                                        case SMP_T_STR:
                                                static_sample_data.type = SMP_T_STR;