fix(security): prevent session hijack via cleanup OOM and stale credential reuse

ngx_http_upstream_ntlm_module.c

@@ -258,6 +258,31 @@ static ngx_int_t ngx_http_upstream_get_ntlm_peer(ngx_peer_connection_t *pc,
         if (item->client_connection == hndp->client_connection) {
             c = item->peer_connection;
 
+            /*
+             * If the current request carries new NTLM/Negotiate credentials
+             * and the cached upstream connection has already served at least
+             * two requests (i.e. the NTLM three-way handshake is complete),
+             * this is a re-authentication attempt for a potentially different
+             * identity.  Evict and close the old authenticated session rather
+             * than silently reusing it.
+             *
+             * When c->requests == 1 the handshake is still in progress
+             * (Type-1 sent, Type-3 is the next expected request); in that
+             * case reuse must be allowed so the exchange can complete on the
+             * same upstream TCP connection.
+             */
+            if (hndp->ntlm_init && c->requests >= 2) {
+                ngx_log_debug2(NGX_LOG_DEBUG_HTTP, pc->log, 0,
+                               "ntlm: new credentials on established session, evicting upstream connection %p (requests=%ui)",
+                               c, c->requests);
+                item->in_cache = 0;
+                item->peer_connection = NULL;
+                ngx_queue_remove(q);
+                ngx_queue_insert_head(&hndp->conf->free, q);
+                ngx_http_upstream_ntlm_close(c);
+                break;
+            }
+
             /*
              * Clear ownership fields BEFORE touching the queues so that any
              * handler that checks in_cache (close_handler, cleanup) sees the
@@ -408,12 +433,28 @@ static void ngx_http_upstream_free_ntlm_peer(ngx_peer_connection_t *pc,
     if (cleanup_item == NULL) {
         cln = ngx_pool_cleanup_add(item->client_connection->pool, 0);
         if (cln == NULL) {
-            ngx_log_debug0(NGX_LOG_DEBUG_HTTP, pc->log, 0,
-                           "ntlm free peer ngx_pool_cleanup_add returned null");
-        } else {
-            cln->handler = ngx_http_upstream_client_conn_cleanup;
-            cln->data = item;
+            ngx_log_error(NGX_LOG_ERR, pc->log, 0,
+                          "ntlm: failed to allocate cleanup handler,"
+                          " not caching upstream connection");
+            /*
+             * Without a cleanup handler we cannot close the upstream
+             * connection when the client disconnects.  The cached item
+             * would hold a stale client_connection pointer that nginx may
+             * later recycle for a completely different client, allowing
+             * get_ntlm_peer to hand an already-authenticated upstream
+             * session to the wrong client (session hijack via pointer
+             * reuse).  Undo the cache insertion and treat this connection
+             * as uncacheable.
+             */
+            item->in_cache = 0;
+            item->peer_connection = NULL;
+            item->client_connection = NULL;
+            ngx_queue_remove(q);
+            ngx_queue_insert_head(&hndp->conf->free, q);
+            goto invalid;
         }
+        cln->handler = ngx_http_upstream_client_conn_cleanup;
+        cln->data = item;
     }
 
     pc->connection = NULL;

t/001-sanity.t

@@ -6,6 +6,8 @@ repeat_each(2);
 
 my $string_gen = String::Random->new;
 our $random_token = $string_gen->randpattern("CcCcCcCCcC");
+our $random_token2;
+do { $random_token2 = $string_gen->randpattern("CcCcCcCCcC"); } while $random_token2 eq $random_token;
 
 # Start the nodejs backend
 my $pid = fork();
@@ -137,3 +139,33 @@ also
 ["========","X-NGX-NTLM-AUTH: ","X-NGX-NTLM-AUTH: "]
 --- no_error_log
 [error]
+
+
+=== TEST 5: New NTLM credentials on an established connection must evict the old session
+After the initial NTLM handshake completes (upstream connection has served at
+least 2 requests), presenting new credentials on the same keep-alive client
+connection must not reuse the old authenticated upstream session.  The module
+must evict the old session and obtain a fresh upstream connection so the new
+credentials are negotiated independently.
+--- http_config
+    upstream backend {
+        server localhost:19841;
+        server localhost:19842;
+        ntlm;
+    }
+--- config
+    location /t {
+        proxy_pass http://backend;
+        proxy_http_version 1.1;
+        proxy_set_header Connection "";
+    }
+--- pipelined_requests eval
+["GET /t", "GET /t", "GET /t"]
+--- more_headers eval
+["Authorization: NTLM " . $::random_token, "", "Authorization: NTLM " . $::random_token2]
+--- response_body eval
+["OK", "OK", "OK"]
+--- response_headers eval
+["X-NGX-NTLM-AUTH: " . $::random_token, "X-NGX-NTLM-AUTH: " . $::random_token, "X-NGX-NTLM-AUTH: " . $::random_token2]
+--- no_error_log
+[error]