From 4b3b2707769d97794f46a4418e4f8582c44dd242 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 6 May 2026 13:59:46 +0000 Subject: [PATCH 1/2] fix(security): prevent session hijack on cleanup OOM and stale credential reuse Finding 1 (Critical): ngx_http_upstream_free_ntlm_peer - when ngx_pool_cleanup_add returns NULL, undo the cache insertion and goto invalid instead of silently continuing. Escalate log level to NGX_LOG_ERR so the failure is visible in production logs. Finding 2 (High): ngx_http_upstream_get_ntlm_peer - when a cached upstream connection exists for the client but the current request carries new NTLM/Negotiate credentials (ntlm_init==1) and the upstream connection has already completed the handshake (c->requests >= 2), evict and close the old authenticated session rather than silently reusing it for a potentially different identity. The c->requests >= 2 threshold lets the mid-handshake Type-3 message still reuse the same upstream TCP connection (requests==1 at that point) while catching any subsequent re-authentication attempt. Add TEST 5 to t/001-sanity.t covering the re-auth eviction path. Agent-Logs-Url: https://github.com/Securepoint/nginx-ntlm-module/sessions/97e86965-9ec8-4edf-8181-22e3722b03fd Co-authored-by: dsabotta <262123149+dsabotta@users.noreply.github.com> --- ngx_http_upstream_ntlm_module.c | 53 +++++++++++++++++++++++++++++---- t/001-sanity.t | 32 ++++++++++++++++++++ 2 files changed, 80 insertions(+), 5 deletions(-) diff --git a/ngx_http_upstream_ntlm_module.c b/ngx_http_upstream_ntlm_module.c index 2a106ff..e703304 100644 --- a/ngx_http_upstream_ntlm_module.c +++ b/ngx_http_upstream_ntlm_module.c @@ -258,6 +258,33 @@ 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 +435,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; diff --git a/t/001-sanity.t b/t/001-sanity.t index 25c3801..87c82e3 100644 --- a/t/001-sanity.t +++ b/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 = $string_gen->randpattern("CcCcCcCCcC"); +$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] From 4b77397c4785c3873c6cd53751b3e91625725972 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 6 May 2026 14:00:52 +0000 Subject: [PATCH 2/2] style: address code review feedback (log string, do-while token gen) Agent-Logs-Url: https://github.com/Securepoint/nginx-ntlm-module/sessions/97e86965-9ec8-4edf-8181-22e3722b03fd Co-authored-by: dsabotta <262123149+dsabotta@users.noreply.github.com> --- ngx_http_upstream_ntlm_module.c | 4 +--- t/001-sanity.t | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/ngx_http_upstream_ntlm_module.c b/ngx_http_upstream_ntlm_module.c index e703304..64c262f 100644 --- a/ngx_http_upstream_ntlm_module.c +++ b/ngx_http_upstream_ntlm_module.c @@ -273,9 +273,7 @@ static ngx_int_t ngx_http_upstream_get_ntlm_peer(ngx_peer_connection_t *pc, */ 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)", + "ntlm: new credentials on established session, evicting upstream connection %p (requests=%ui)", c, c->requests); item->in_cache = 0; item->peer_connection = NULL; diff --git a/t/001-sanity.t b/t/001-sanity.t index 87c82e3..991f33e 100644 --- a/t/001-sanity.t +++ b/t/001-sanity.t @@ -6,8 +6,8 @@ repeat_each(2); my $string_gen = String::Random->new; our $random_token = $string_gen->randpattern("CcCcCcCCcC"); -our $random_token2 = $string_gen->randpattern("CcCcCcCCcC"); -$random_token2 = $string_gen->randpattern("CcCcCcCCcC") while $random_token2 eq $random_token; +our $random_token2; +do { $random_token2 = $string_gen->randpattern("CcCcCcCCcC"); } while $random_token2 eq $random_token; # Start the nodejs backend my $pid = fork();