fix: null client_connection in Finding 2 eviction path + OOM cleanup fault-injection

[Copilot]

Two follow-up issues identified in PR #6's security fixes for NTLM session hijack prevention.

Changes

• ngx_http_upstream_ntlm_module.c — Finding 2 eviction path (ngx_http_upstream_get_ntlm_peer)
  The stale-credential eviction block was missing item->client_connection = NULL;, violating the ownership invariant (documented in the struct comment) that every path moving an item out of conf->cache must null both connection pointers. Finding 1's fix in ngx_http_upstream_free_ntlm_peer already did this correctly.

  // Before — inconsistent with Finding 1 fix
  item->in_cache = 0;
  item->peer_connection = NULL;
  // client_connection left pointing at evicted client
  ngx_queue_remove(q);
  
  // After
  item->in_cache = 0;
  item->peer_connection = NULL;
  item->client_connection = NULL;  // ← added
  ngx_queue_remove(q);

• ngx_http_upstream_ntlm_module.c — Fault-injection macro
  Added a compile-time shim to force ngx_pool_cleanup_add to return NULL, enabling test coverage of the OOM cleanup handler path (Finding 1) that otherwise has zero automated regression coverage:

  #ifdef NGX_NTLM_TEST_CLEANUP_NULL
  #  undef  ngx_pool_cleanup_add
  #  define ngx_pool_cleanup_add(pool, size) NULL
  #endif

• t/001-sanity.t — TEST 6
  Documents expected behavior when ngx_pool_cleanup_add returns NULL: the upstream session must not be cached and a follow-up request must not receive the old authenticated session. Marked --- SKIP in normal CI; requires a build with -DNGX_NTLM_TEST_CLEANUP_NULL. A comment block above the test describes the build and run procedure.

Original prompt

Context

PR #6 (fix(security): prevent session hijack via cleanup OOM and stale credential reuse) introduced two security fixes. A code review identified two remaining issues that need to be addressed in a follow-up:

---

Issue 1 — Missing client_connection = NULL in Finding 2 eviction path

In ngx_http_upstream_get_ntlm_peer (Finding 2 fix), when evicting a stale authenticated session due to new credentials, item->client_connection is not cleared before the item is returned to the free list:

// Current code (lines ~278-284 in ngx_http_upstream_ntlm_module.c)
item->in_cache = 0;
item->peer_connection = NULL;
// ← item->client_connection = NULL; is MISSING
ngx_queue_remove(q);
ngx_queue_insert_head(&hndp->conf->free, q);
ngx_http_upstream_ntlm_close(c);
break;

This is inconsistent with the Finding 1 fix in ngx_http_upstream_free_ntlm_peer which correctly nulls all three fields:

item->in_cache = 0;
item->peer_connection = NULL;
item->client_connection = NULL;  // ← correctly present here

The struct comment (the in_cache ownership invariant block) documents that every code path moving an item out of conf->cache MUST null out both connection pointers. This is violated in the Finding 2 path.

Fix: Add item->client_connection = NULL; immediately after item->peer_connection = NULL; in the Finding 2 eviction block inside ngx_http_upstream_get_ntlm_peer.

---

Issue 2 — No test coverage for Finding 1 (OOM cleanup handler path)

The Finding 1 fix (OOM in ngx_pool_cleanup_add) has zero automated test coverage. Add a compile-time fault-injection mechanism and a corresponding test.

Fault injection macro

In ngx_http_upstream_ntlm_module.c, add a compile-time shim so that test builds can force ngx_pool_cleanup_add to return NULL:

#ifdef NGX_NTLM_TEST_CLEANUP_NULL
#  undef  ngx_pool_cleanup_add
#  define ngx_pool_cleanup_add(pool, size) NULL
#endif

Place this immediately before the ngx_http_upstream_free_ntlm_peer function definition (or near the top of the file after the includes), so it only affects that translation unit.

New test in t/001-sanity.t

Add TEST 6 to t/001-sanity.t that documents the expected behavior when the cleanup handler cannot be allocated. The test should be marked skip unless the fault-injection build is used, but should clearly document what the test verifies:

=== TEST 6: OOM in cleanup handler must not cache the upstream connection
When ngx_pool_cleanup_add returns NULL, the upstream session MUST NOT be
inserted into the cache.  A subsequent request on the same client connection
must therefore NOT receive the old authenticated upstream session.
This test requires a build with -DNGX_NTLM_TEST_CLEANUP_NULL.
--- SKIP
--- http_config
    upstream backend {
        server 127.0.0.1:19841;
        ntlm;
    }
--- config
    location /t {
        proxy_pass http://backend;
        proxy_http_version 1.1;
        proxy_set_header Connection "";
    }
--- pipelined_requests eval
["GET /t", "GET /t"]
--- more_headers eval
["Authorization: NTLM " . $::random_token, "Authorization: NTLM " . $::random_token]
--- response_body eval
["OK", "OK"]
--- error_log
ntlm: failed to allocate cleanup handler
--- response_headers eval
["X-NGX-NTLM-AUTH: " . $::random_token, "X-NGX-NTLM-AUTH: " . $::random_token]

Also add a brief comment block above TEST 6 explaining how to build nginx with the fault-injection flag for local testing.

---

Summary of changes required

1. ngx_http_upstream_ntlm_module.c: Add item->client_connection = NULL; in the Finding 2 eviction block in ngx_http_upstream_get_ntlm_peer, and add the NGX_NTLM_TEST_CLEANUP_NULL fault-injection macro block.
2. t/001-sanity.t: Add TEST 6 with the OOM scenario documentation and skip guard.

The base branch for this PR is copilot/research-session-handling-vulnerabilities (the branch from PR #6).

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: what do you think about this pr. do you see any problems?

Assistant: Here's my full review of PR #6:

---

📋 PR Summary

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

State	🟡 Open (Draft)
Author	Copilot (bot)
Files Changed	2 (ngx_http_upstream_ntlm_module.c, t/001-sanity.t)
Changes	+78 / -5 lines
Reviews	None yet
CI	Not reported
Mergeable	✅ Clean

This PR addresses two distinct security vulnerabilities in the NTLM upstream session cache:

1. Finding 1 (Critical): Session hijack via OOM in cleanup handler registration
2. Finding 2 (High): Stale credential reuse on an established keep-alive connection

---

🔧 Core Changes

Finding 1 — Cleanup Handler OOM (ngx_http_upstream_free_ntlm_peer)

Previously, if `ngx_pool_cle...

This pull request was created from Copilot chat.