PR #22528 follow-up: pool reaper health probe taints recent_conn_id, defeating the SSPI empty-creds reuse guard

[cybertron10]

I did this

Commit 1a17959 ("url: fix negotiate/ntlm connection reuse", PR #22528) added
url_allow_sspi_empty_creds() so a Negotiate/NTLM connection authenticated
with empty Windows SSPI credentials (ambient thread token) is not reused for
a different transfer. The one exception is when the connection is "the one
this transfer used before", tracked via data->state.recent_conn_id.

The exception can be satisfied by a connection the transfer never used.

Curl_cpool_prune_dead() runs as the first statement of url_attach_existing()
(lib/url.c:1166), on the same transfer that is about to look for a
connection to reuse. For each idle connection it checks, it briefly attaches
the transfer to the connection to run a liveness probe
(Curl_cpool_conn_seems_healthy, lib/conncache.c:906), then detaches it.
Curl_attach_connection() (lib/multi.c:954) sets
data->state.recent_conn_id = conn->connection_id on every attach, and
Curl_detach_connection() (lib/multi.c:926) never clears it. The health probe
thus leaves recent_conn_id pointing at the last probed idle connection.

By the time url_match_auth_ntlm()/url_match_auth_nego() consult the guard
(lib/url.c:946/1009), a fresh transfer that has never sent a byte over that
connection can pass the "same transfer used this connection" check. In the
common case - a pool with one idle authenticated connection, idle for more
than a second (both the pool cleanup interval and the connection's
lastchecked stamp need to be >= 1000ms) - this fires on the very next
request, not as a rare race.

Reproduction (clean checkout of 1a17959, Linux build; the pool mechanics are
platform independent, and the guard body itself was evaluated verbatim):

1. Transfer A completes against a keep-alive server, leaving connection #0
   idle in the pool ("Connection #0 ... left intact").

2. Wait 1200ms.

3. A brand new handle B (recent_conn_id == -1, curl's own "never used"
   sentinel set in Curl_open()) requests the same URL. The reaper's health
   probe transiently attaches B to conn #0, and B's recent_conn_id flips
   from -1 to 0 before B ever sends a byte:

   TOCTOU-VERIFY before: data=0x55819a6b6728 rcid=-1
   TOCTOU-VERIFY attach: data=0x55819a6b6728 id=0 <- health probe
   TOCTOU-VERIFY after: data=0x55819a6b6728 rcid=0 <- tainted
   TOCTOU-VERIFY attach: data=0x55819a6b6728 id=0 <- B's real attach

   • Reusing existing http: connection with host 127.0.0.1

The harness (public curl API only), the instrumentation patch and the
unedited transcript are in the attached zip.

This exact problem was flagged during the PR's own review, on
lib/multi.c:954, 26 minutes before the merge
(#22528 (comment)): "Updating
recent_conn_id on every attachment also records temporary internal probes
... An easy handle can thus acquire another handle's connection ID and
incorrectly pass the empty-SSPI-user exception ... defeating the TOCTOU
protection." The reply was "I find that analysis rather vague and
unconvincing", and the PR merged unchanged.

Impact: on Windows (USE_WINDOWS_SSPI), an application that shares one
connection pool across transfers running under different ambient security
tokens (per-request impersonation - the scenario the guard's own comment
describes: "This token can be switched at any time ... To avoid TOCTOU
attacks, do not reuse on empty credentials") can still have one identity's
request sent over another identity's already-authenticated connection.
Affected paths: direct Negotiate (url.c:1009), proxy NTLM (url.c:964),
proxy Negotiate (url.c:1027). Direct server NTLM is separately gated at
url.c:942 and is not affected. The same transient-attach taint also occurs
via conn_upkeep() (conncache.c:766, curl_easy_upkeep()).

Suggested fix: save and restore data->state.recent_conn_id around the
transient attach/detach in Curl_cpool_conn_seems_healthy()
(lib/conncache.c:906-935), so a liveness probe can never observably change
it. Alternatively, only record recent_conn_id when the transfer actually
selects and uses the connection, as it behaved before this commit when the
assignment lived in multi_done_locked().

.github-issue-evidence.zip

I expected the following

No response

curl/libcurl version

8.22.0-DEV

operating system

Linux

[icing]

Thanks, I agree with your observation. Working on a fix.

[martindukek]

@icing saw you self-assigned, had this mostly done before I noticed, so #22571 in case it's useful. Close it if you're going a different way.

Rather than save/restore around the probe, it records recent_conn_id only on genuine attachment: conn_upkeep and the six cshutdn paths make the same assumption. trc_print_ids needed its data->conn fallback back, since it relied on every attach recording the id.

Verified with @cybertron10's instrumentation: rcid stays -1 through the probe.