BUG/MEDIUM: proxy: fix UAF with {tcp,http}checks logformat expressions

When parsing a logformat expression using parse_logformat_string(), the
caller passes the proxy under which the expression is found as argument.

This information allows the logformat expression API to check if the
expression is compatible with the proxy settings.

Since 7a21c3a ("MAJOR: log: implement proper postparsing for logformat
expressions"), the proxy compatibilty checks are postponed after the proxy
is fully parsed to ensure proxy properties are fully resolved for checks
consistency.

The way it works, is that each time parse_logformat_string() is called for
a given expression and proxy, it schedules the expression for postchecking
by appending the expression to the list of pending expression checks on
the proxy (lf_checks struct). Then, when the proxy is called with the
REGISTER_POST_PROXY_CHECK() hook, it iterates over unchecked expressions
and performs the check, then it removes the expression from its list.

However, I overlooked a special case: if a logformat expression is used
on a proxy that is disabled or a default proxy:
REGISTER_POST_PROXY_CHECK() hook is never called. Because of that, lf
expressions may still point to the proxy after the proxy is freed.

For most logformat expressions, this isn't an issue because they are
stored within the proxy itself, but this isn't the case with
{tcp,http}checks logformat expressions: during deinit() sequence, all
proxies are first cleaned up, and only then shared checks are freed.

Because of that, the below config will trigger UAF since 7a21c3a:

uaf.conf:
  listen dummy
    bind localhost:2222

  backend testback
    disabled
    mode http
    option httpchk
    http-check send hdr test "test"
    http-check expect status 200

haproxy -f uaf.conf -c:

==152096== Invalid write of size 8
==152096==    at 0x21C317: lf_expr_deinit (log.c:3491)
==152096==    by 0x2334A3: free_tcpcheck_http_hdr (tcpcheck.c:84)
==152096==    by 0x2334A3: free_tcpcheck_http_hdr (tcpcheck.c:79)
==152096==    by 0x2334A3: free_tcpcheck_http_hdrs (tcpcheck.c:98)
==152096==    by 0x23365A: free_tcpcheck.part.0 (tcpcheck.c:130)
==152096==    by 0x2338B1: free_tcpcheck (tcpcheck.c:108)
==152096==    by 0x2338B1: deinit_tcpchecks (tcpcheck.c:3780)
==152096==    by 0x2CF9A4: deinit (haproxy.c:2949)
==152096==    by 0x2D0065: deinit_and_exit (haproxy.c:3052)
==152096==    by 0x169BC0: main (haproxy.c:3996)
==152096==  Address 0x52a8df8 is 6,968 bytes inside a block of size 7,168 free'd
==152096==    at 0x484B27F: free (vg_replace_malloc.c:872)
==152096==    by 0x2CF8AD: deinit (haproxy.c:2906)
==152096==    by 0x2D0065: deinit_and_exit (haproxy.c:3052)
==152096==    by 0x169BC0: main (haproxy.c:3996)

To fix the issue, let's ensure in proxy_free_common() that no unchecked
expressions may still point to the proxy after the proxy is freed by
purging the list (DEL_INIT is used to reset list items).

Special thanks to GH user @mhameed who filed a comprehensive issue with
all the relevant information required to reproduce the bug (see GH #2597),
after having first reported the issue on the alpine project bug tracker.

diff --git a/src/proxy.c b/src/proxy.c
index 4bfa31d3932fa431d3df8313e7f1131ff4e3b37f..eb39439b1522a9a9c773797a0d54f139e46414e7 100644 (file)
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -199,6 +199,7 @@ static inline void proxy_free_common(struct proxy *px)
 {
        struct acl *acl, *aclb;
        struct logger *log, *logb;
+       struct lf_expr *lf, *lfb;
 
        ha_free(&px->id);
        ha_free(&px->conf.file);
@@ -245,6 +246,13 @@ static inline void proxy_free_common(struct proxy *px)
                free_logger(log);
        }
 
+       /* ensure that remaining lf_expr that were not postchecked (ie: disabled
+        * or default proxy) don't keep a reference on the proxy which is about
+        * to be freed.
+        */
+       list_for_each_entry_safe(lf, lfb, &px->conf.lf_checks, list)
+               LIST_DEL_INIT(&lf->list);
+
        chunk_destroy(&px->log_tag);
 
        free_email_alert(px);