From: Aurelien DARRAGON <adarragon@haproxy.com>
Date: Mon, 9 Sep 2024 12:59:19 +0000 (+0200)
Subject: BUG/MEDIUM: pattern: prevent UAF on reused pattern expr
X-Git-Tag: v3.1-dev8~43
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=68cfb222b559b909415c9aa92114ca42c9a93459;p=thirdparty%2Fhaproxy.git

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

diff --git a/include/haproxy/pattern-t.h b/include/haproxy/pattern-t.h
index aa3a17878f..b84c7e71e4 100644
--- a/include/haproxy/pattern-t.h
+++ b/include/haproxy/pattern-t.h
@@ -207,6 +207,7 @@ struct pattern_expr {
 	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 */
 };
 
@@ -216,7 +217,6 @@ struct pattern_expr {
  */
 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 346fe59d55..7a2a15accf 100644
--- a/src/pattern.c
+++ b/src/pattern.c
@@ -2190,19 +2190,14 @@ struct pattern_expr *pattern_new_expr(struct pattern_head *head, struct pat_ref
 		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;
 
@@ -2583,7 +2578,7 @@ void pattern_prune(struct pattern_head *head)
 
 	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);