BUG/MEDIUM: pattern: prevent UAF on reused pattern expr
Since c5959fd ("MEDIUM: pattern: merge same pattern"), UAF (leading to
crash) can be experienced if the same pattern file (and match method) is
used in two default sections and the first one is not referenced later in
the config. In this case, the first default section will be cleaned up.
However, due to an unhandled case in the above optimization, the original
expr which the second default section relies on is mistakenly freed.
This issue was discovered while trying to reproduce GH #2708. The issue
was particularly tricky to reproduce given the config and sequence
required to make the UAF happen. Hopefully, Github user @asmnek not only
provided useful informations, but since he was able to consistently
trigger the crash in his environment he was able to nail down the crash to
the use of pattern file involved with 2 named default sections. Big thanks
to him.
To fix the issue, let's push the logic from c5959fd a bit further. Instead
of relying on "do_free" variable to know if the expression should be freed
or not (which proved to be insufficient in our case), let's switch to a
simple refcounting logic. This way, no matter who owns the expression, the
last one attempting to free it will be responsible for freeing it.
Refcount is implemented using a 32bit value which fills a previous 4 bytes
structure gap:
int mflags; /* 80 4 */
/* XXX 4 bytes hole, try to pack */
long unsigned int lock; /* 88 8 */
(output from pahole)
Even though it was not reproduced in 2.6 or below by @asmnek (the bug was
revealed thanks to another bugfix), this issue theorically affects all
stable versions (up to c5959fd), thus it should be backported to all
stable versions.
(cherry picked from commit 68cfb222b559b909415c9aa92114ca42c9a93459)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit fc0af6f2bb0bf99d9c84e888e3daaa5529da2a5c)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit fccf836bed93c0976012e940cfefeea64e352df5)
Signed-off-by: Willy Tarreau <w@1wt.eu>
diff --git a/include/haproxy/pattern-t.h b/include/haproxy/pattern-t.h
index 1d1de60..f907f7f 100644
--- a/include/haproxy/pattern-t.h
+++ b/include/haproxy/pattern-t.h
@@ -200,6 +200,7 @@
struct eb_root pattern_tree; /* may be used for lookup in large datasets */
struct eb_root pattern_tree_2; /* may be used for different types */
int mflags; /* flags relative to the parsing or matching method. */
+ uint32_t refcount; /* refcount used to know if the expr can be deleted or not */
__decl_thread(HA_RWLOCK_T lock); /* lock used to protect patterns */
};
@@ -209,7 +210,6 @@
*/
struct pattern_expr_list {
struct list list; /* Used for chaining pattern_expr in pattern_head. */
- int do_free;
struct pattern_expr *expr; /* The used expr. */
};
diff --git a/src/pattern.c b/src/pattern.c
index 116bf2b..5937f6a 100644
--- a/src/pattern.c
+++ b/src/pattern.c
@@ -2176,19 +2176,14 @@
expr->ref = ref;
HA_RWLOCK_INIT(&expr->lock);
-
- /* We must free this pattern if it is no more used. */
- list->do_free = 1;
}
else {
- /* If the pattern used already exists, it is already linked
- * with ref and we must not free it.
- */
- list->do_free = 0;
if (reuse)
*reuse = 1;
}
+ HA_ATOMIC_INC(&expr->refcount);
+
/* The new list element reference the pattern_expr. */
list->expr = expr;
@@ -2550,7 +2545,7 @@
list_for_each_entry_safe(list, safe, &head->head, list) {
LIST_DELETE(&list->list);
- if (list->do_free) {
+ if (HA_ATOMIC_SUB_FETCH(&list->expr->refcount, 1) == 0) {
LIST_DELETE(&list->expr->list);
HA_RWLOCK_WRLOCK(PATEXP_LOCK, &list->expr->lock);
head->prune(list->expr);