From fd3ce173aa054f7feaf49f49359c8f0566475d62 Mon Sep 17 00:00:00 2001 From: Amaury Denoyelle Date: Wed, 20 Mar 2024 09:25:03 +0100 Subject: [PATCH] BUG/MEDIUM: http_ana: ignore NTLM for reuse aggressive/always and no H1 Backend connections can be marked as private to prevent their sharing by multiple clients. Now, this has become an exception as only two reasons for data traffic can trigger this (checks are ignored here) : * http-reuse never * HTTP response with NTLM header The first case is easy to manage as the connection is flagged as private since its inception. However, the second case is dynamic as the connection can be flagged anytime during its lifetime. When using a backend protocol such as HTTP/2 with reuse mode aggressive or always, we face a design issue as the connection would be marked as private, despite potentially being shared by several clients at the same time. This is conceptually invalid, but worst it can trigger crashes on MUX stream detach callback depending on the order of release of the streams, by calling session_check_idle_conn() with a NULL session. It could also be possible to have several NTLM responses on a single connection for different sessions. In this case, connection owner is still being updated without attaching the connection to its correct session, which ultimately would cause a crash on session_check_idle_conn with an invalid session. Here are two backtrace examples from GDB for such cases : Thread 1 (Thread 0x7ff73e9fc700 (LWP 648859)): #0 session_check_idle_conn (conn=0x7ff72f597800, sess=0x0) at include/haproxy/session.h:209 #1 h2_detach (sd=) at src/mux_h2.c:4520 #2 0x000056151742be24 in sc_detach_endp (scp=scp@entry=0x7ff73e9f0f18) at src/stconn.c:376 #3 0x000056151742c208 in sc_destroy (sc=) at src/stconn.c:444 #4 0x0000561517370871 in stream_free (s=s@entry=0x7ff72a2dbd80) at src/stream.c:728 #5 0x000056151737541f in process_stream (t=t@entry=0x7ff72d5e2620, context=0x7ff72a2dbd80, state=) at src/stream.c:2645 #6 0x0000561517456cbb in run_tasks_from_lists (budgets=budgets@entry=0x7ff73e9f10d0) at src/task.c:632 #7 0x00005615174576b9 in process_runnable_tasks () at src/task.c:876 #8 0x000056151742275a in run_poll_loop () at src/haproxy.c:2996 #9 0x0000561517422db1 in run_thread_poll_loop (data=) at src/haproxy.c:3195 #10 0x00007ff789e081ca in start_thread () from /lib64/libpthread.so.0 #11 0x00007ff789a39e73 in clone () from /lib64/libc.so.6 (gdb) Thread 1 (Thread 0x7ff52e7fc700 (LWP 681458)): #0 0x0000556ebd6e7e69 in session_check_idle_conn (conn=0x7ff5787ff100, sess=0x7ff51d2539a0) at include/haproxy/session.h:209 #1 h2_detach (sd=) at src/mux_h2.c:4520 #2 0x0000556ebd7f3e24 in sc_detach_endp (scp=scp@entry=0x7ff52e7f0f18) at src/stconn.c:376 #3 0x0000556ebd7f4208 in sc_destroy (sc=) at src/stconn.c:444 #4 0x0000556ebd738871 in stream_free (s=s@entry=0x7ff520e28200) at src/stream.c:728 #5 0x0000556ebd73d41f in process_stream (t=t@entry=0x7ff565783700, context=0x7ff520e28200, state=) at src/stream.c:2645 #6 0x0000556ebd81ecbb in run_tasks_from_lists (budgets=budgets@entry=0x7ff52e7f10d0) at src/task.c:632 #7 0x0000556ebd81f6b9 in process_runnable_tasks () at src/task.c:876 #8 0x0000556ebd7ea75a in run_poll_loop () at src/haproxy.c:2996 #9 0x0000556ebd7eadb1 in run_thread_poll_loop (data=) at src/haproxy.c:3195 #10 0x00007ff5752081ca in start_thread () from /lib64/libpthread.so.0 #11 0x00007ff574e39e73 in clone () from /lib64/libc.so.6 (gdb) To solve this issue, simply ignore NTLM responses when using a multiplexer with streams support and the connection is not already attached to the session. The connection is not marked as private and will continue to be shared freely accross clients. This is considered conceptually valid as NTLM usage (rfc 4559) with HTTP is broken and was designed only with HTTP/1.1 in mind. A side-effect of the change is that SESS_FL_PREFER_LAST is also not set anymore on NTLM detection, which allows following requests to be load-balanced accross several server instances. The original behavior is kept for HTTP/1 or if the connection is already attached to the session. This last case happens when using HTTP/2 with default http-reuse safe mode since the following patch : 0d21deaded1a4bbd1e1700ab8386af1f1441ea73 MEDIUM: backend: add reused conn to sess if mux marked as HOL blocking This should be backported up to all stable releases. Up until 2.4, it can be taken as-is. For lesser versions, above patch is not present. In this case the condition should be restricted only to HTTP/1 usage : if (srv_conn && strcmp(srv_conn->mux->name, "H1") == 0) { --- doc/configuration.txt | 6 +++++- src/http_ana.c | 12 +++++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/doc/configuration.txt b/doc/configuration.txt index aac245e95..f92653975 100644 --- a/doc/configuration.txt +++ b/doc/configuration.txt @@ -7599,7 +7599,11 @@ http-reuse { never | safe | aggressive | always } with the request. Also note that connections with certain bogus authentication schemes (relying - on the connection) like NTLM are marked private and never shared. + on the connection) like NTLM are marked private if possible and never shared. + This won't be the case however when using a protocol with multiplexing + abilities and using reuse mode level value greater than the default "safe" + strategy as in this case nothing prevents the connection from being already + shared. A connection pool is involved and configurable with "pool-max-conn". diff --git a/src/http_ana.c b/src/http_ana.c index 25123265f..91aaeb451 100644 --- a/src/http_ana.c +++ b/src/http_ana.c @@ -1557,11 +1557,17 @@ int http_wait_for_response(struct stream *s, struct channel *rep, int an_bit) txn->flags |= TX_CON_WANT_TUN; } - /* check for NTML authentication headers in 401 (WWW-Authenticate) and - * 407 (Proxy-Authenticate) responses and set the connection to private + /* Check for NTML authentication headers in 401 (WWW-Authenticate) and + * 407 (Proxy-Authenticate) responses and set the connection to + * private. + * + * Note that this is not performed when using a true multiplexer unless + * connection is already attached to the session as nothing prevents it + * from being shared already by several sessions here. */ srv_conn = sc_conn(s->scb); - if (srv_conn) { + if (srv_conn && + (LIST_INLIST(&srv_conn->sess_el) || strcmp(srv_conn->mux->name, "H1") == 0)) { struct ist hdr; struct http_hdr_ctx ctx;