Fix use-after-free and double-free in upstream NTLM connection teardown

[Copilot]

Two high-severity memory safety bugs in the idle-connection close path that can lead to recv() on a recycled file descriptor and potential double-free of the connection pool.

Bug 1: Use-after-free in ngx_http_upstream_ntlm_close_handler

Between the item->in_cache check and the recv() syscall, a concurrent client disconnect could trigger ngx_http_upstream_client_conn_cleanup, which calls ngx_close_connection(c) — setting c->fd = -1 — before recv() executes on the now-closed/recycled descriptor.

Added an explicit c->fd guard before the syscall:

if (c->fd == (ngx_socket_t) -1) {
    goto close;
}

n = recv(c->fd, buf, 1, MSG_PEEK);

Bug 2: Double-free of connection pool in ngx_http_upstream_ntlm_close

The previous order destroyed the pool then called ngx_close_connection(c), which may also access c->pool depending on nginx version — risking a double-free. Fix follows the safe pattern: null out c->pool first so ngx_close_connection sees it as NULL, then destroy the pool after:

// Before
if (c->pool) {
    ngx_destroy_pool(c->pool);
    c->pool = NULL;
}
ngx_close_connection(c);

// After
ngx_pool_t *pool = c->pool;
c->pool = NULL;
ngx_close_connection(c);
if (pool) {
    ngx_destroy_pool(pool);
}

This is safe across all nginx versions regardless of whether ngx_close_connection internally touches c->pool.

Original prompt

Two high-severity memory safety bugs in ngx_http_upstream_ntlm_module.c

Bug 1: Use-after-free in ngx_http_upstream_ntlm_close_handler

In ngx_http_upstream_ntlm_close_handler(), the code checks item->in_cache and retrieves c = item->peer_connection, then later calls recv(c->fd, buf, 1, MSG_PEEK). Between the initial check and the recv() call, ngx_http_upstream_client_conn_cleanup could fire (e.g. client disconnects), which calls ngx_http_upstream_ntlm_item_release(item) (setting item->peer_connection = NULL) and then ngx_http_upstream_ntlm_close(c) (destroying c->pool and calling ngx_close_connection(c)). The close_handler then does recv(c->fd, ...) on a closed/recycled file descriptor.

Fix: Add a guard checking c->fd != (ngx_socket_t) -1 before calling recv(). Also re-verify item->in_cache && item->peer_connection == c immediately before the recv() syscall to narrow the race window. The function currently looks like:

static void
ngx_http_upstream_ntlm_close_handler(ngx_event_t *ev)
{
    // ...
    item = ev->data;

    if (item == NULL || !item->in_cache) {
        return;
    }

    c = item->peer_connection;

    if (c == NULL) {
        return;
    }

    if (c->close || c->read->timedout) {
        goto close;
    }

    n = recv(c->fd, buf, 1, MSG_PEEK);   // <-- PROBLEM: c may have been freed
    // ...

It should be changed to add a c->fd check before recv():

    if (c->close || c->read->timedout) {
        goto close;
    }

    if (c->fd == (ngx_socket_t) -1) {
        goto close;
    }

    n = recv(c->fd, buf, 1, MSG_PEEK);

Bug 2: Double pool destroy in ngx_http_upstream_ntlm_close

ngx_http_upstream_ntlm_close() calls ngx_destroy_pool(c->pool) and sets c->pool = NULL, then calls ngx_close_connection(c). In nginx 1.28.x, ngx_close_connection() may also access c->pool. The standard nginx pattern (as used in ngx_http_upstream_keepalive_module) is to destroy the pool and then call ngx_close_connection, but the order matters and the pool should only be explicitly destroyed if ngx_close_connection in the target nginx version does NOT destroy it. For safety with nginx 1.28.3, follow the exact pattern used by nginx's own keepalive module: call ngx_destroy_pool first, set c->pool = NULL, then ngx_close_connection. Actually in nginx 1.28.x ngx_close_connection does NOT destroy the pool (it was removed), so the current order is safe but the pool NULL-check should be strengthened. However, ngx_close_connection in some builds checks c->pool and if non-NULL frees it — leading to double-free if the pool was already destroyed but c->pool not yet set to NULL due to compiler reordering.

Fix: Use the pattern that is safe regardless of nginx version — set c->pool = NULL atomically by saving the pool pointer first:

static void
ngx_http_upstream_ntlm_close(ngx_connection_t *c)
{
#if (NGX_HTTP_SSL)
    if (c->ssl) {
        c->ssl->no_wait_shutdown = 1;
        c->ssl->no_send_shutdown = 1;

        if (ngx_ssl_shutdown(c) == NGX_AGAIN) {
            c->ssl->handler = ngx_http_upstream_ntlm_close;
            return;
        }
    }
#endif

    ngx_pool_t *pool = c->pool;
    c->pool = NULL;
    ngx_close_connection(c);
    if (pool) {
        ngx_destroy_pool(pool);
    }
}

This ensures ngx_close_connection sees c->pool == NULL (so it won't try to destroy it), and we destroy the pool ourselves afterwards. This matches the safe pattern and avoids any double-free regardless of nginx version.

Files to modify

• ngx_http_upstream_ntlm_module.c — apply both fixes as described above.

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

User: This nginx mdule will be used with nginx 1.28.3
Analyze the code for security issues, specially for errors with session handling.
And Check for potential memory access errors and memory leaks.

Assistant: Here is a thorough security and memory-safety audit of the module. Overall the code is well-structured and demonstrates awareness of the key pitfalls (the documented invariants I1–I4 are solid). However, several issues remain:

---

🔴 High Severity

1. Use-After-Free in ngx_http_upstream_ntlm_close_handler (line 728)

    item = ev->data;

    if (item == NULL || !item->in_cache) {
        return;
    }

    c = item->peer_connection;
    // ...
    n = recv(c->fd, buf, 1, MSG_PEEK);       // <-- uses c

    // ... later ...
close:
    if (!item->in_cache || item->peer_connection != c) {  // <-- re-checks
        return;
    }

Between the first item->in_cache check (line 712) and the recv() call (line 728), ngx_http_upstream_client_conn_cleanup could fire (e.g. if the client disconne...

This pull request was created from Copilot chat.