From 1a17959fc7a80864c9c9b37097856bdb4a465cf8 Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Mon, 10 Aug 2026 10:17:06 +0200 Subject: [PATCH] url: fix negotiate/ntlm connection reuse Reorder logic when looking for matching connections. Check candidate Negotiate/NTLM state first. Require same "input" parameters when connection is already authenticated. Same for proxy. Deny connection reuse for empty usernames for NTLM/Negotiate using connections. Reported-by: Martin Dukek Closes #22528 --- lib/conncache.c | 2 +- lib/curl_trc.c | 3 +- lib/easy.c | 3 +- lib/multi.c | 2 +- lib/url.c | 124 +++++++++++++++++++++++++++++++----------------- 5 files changed, 86 insertions(+), 48 deletions(-) diff --git a/lib/conncache.c b/lib/conncache.c index 11ef033435..cec444731c 100644 --- a/lib/conncache.c +++ b/lib/conncache.c @@ -270,7 +270,6 @@ void Curl_cpool_xfer_init(struct Curl_easy *data) { struct cpool *cpool = cpool_get_instance(data); - DEBUGASSERT(cpool); if(cpool) { CPOOL_LOCK(cpool, data); /* the identifier inside the connection cache */ @@ -283,6 +282,7 @@ void Curl_cpool_xfer_init(struct Curl_easy *data) } else { /* We should not get here, but in a non-debug build, do something */ + DEBUGASSERT(0); data->id = 0; data->state.lastconnect_id = -1; } diff --git a/lib/curl_trc.c b/lib/curl_trc.c index 4e4204a07b..b52193270e 100644 --- a/lib/curl_trc.c +++ b/lib/curl_trc.c @@ -92,8 +92,7 @@ static struct curl_trc_feat Curl_trc_feat_ids = { static size_t trc_print_ids(struct Curl_easy *data, char *buf, size_t maxlen) { - curl_off_t cid = data->conn ? - data->conn->connection_id : data->state.recent_conn_id; + curl_off_t cid = data->state.recent_conn_id; if(data->id >= 0) { if(cid >= 0) return curl_msnprintf(buf, maxlen, CURL_TRC_FMT_IDSDC, data->id, cid); diff --git a/lib/easy.c b/lib/easy.c index eeb2791706..aa2cad22c9 100644 --- a/lib/easy.c +++ b/lib/easy.c @@ -1233,7 +1233,8 @@ static CURLcode easy_connection(struct Curl_easy *data, sfd = Curl_getconnectinfo(data, connp); if(sfd == CURL_SOCKET_BAD) { - failf(data, "Failed to get recent socket"); + failf(data, "Failed to get last socket used for connection #%" FMT_OFF_T, + data->state.lastconnect_id); return CURLE_UNSUPPORTED_PROTOCOL; } diff --git a/lib/multi.c b/lib/multi.c index 4a6f431f6d..dd4b9bfe8a 100644 --- a/lib/multi.c +++ b/lib/multi.c @@ -642,7 +642,6 @@ static void multi_done_locked(struct connectdata *conn, } data->state.done = TRUE; /* called now! */ - data->state.recent_conn_id = conn->connection_id; Curl_dnscache_prune(data); @@ -952,6 +951,7 @@ void Curl_attach_connection(struct Curl_easy *data, DEBUGASSERT(conn); DEBUGASSERT(conn->attached_xfers < UINT32_MAX); data->conn = conn; + data->state.recent_conn_id = conn->connection_id; conn->attached_xfers++; /* all attached transfers must be from the same multi */ if(!conn->attached_multi) diff --git a/lib/url.c b/lib/url.c index 5998cd69bc..5234a53443 100644 --- a/lib/url.c +++ b/lib/url.c @@ -900,43 +900,75 @@ static bool url_match_ssl_config(struct connectdata *conn, return TRUE; } +#if defined(USE_SPNEGO) || defined(USE_NTLM) +static bool url_allow_sspi_empty_creds(struct Curl_creds *conn_creds, + struct Curl_easy *data, + struct connectdata *conn) +{ +#ifdef USE_WINDOWS_SSPI + /* Empty user: SSPI on Windows can make use of an "ambient" + * user from a "SecurityToken" associated with the current thread or + * process. This token can be switched at any time. We are therefore + * not able to find out reliably what token the connection really + * used, nor what token in the next connect attempt will use. + * To avoid TOCTOU attacks, do not reuse on empty credentials + * UNLESS this connection is the one used by this transfer before. */ + if(!Curl_creds_has_user(conn_creds) && + (data->state.recent_conn_id != conn->connection_id)) + return FALSE; +#else + (void)conn_creds; + (void)data; + (void)conn; +#endif + return TRUE; +} +#endif /* USE_SPNEGO || USE_NTLM */ + #ifdef USE_NTLM static bool url_match_auth_ntlm(struct connectdata *conn, struct url_conn_match *m) { - /* If we are looking for an HTTP+NTLM connection, check if this is - already authenticating with the right credentials. If not, keep - looking so that we can reuse NTLM connections if - possible. (Especially we must not reuse the same connection if - partway through a handshake!) */ - if(m->want_ntlm_http) { + if(conn->http_ntlm_state != NTLMSTATE_NONE) { + /* Connection is using NTLM. We cannot reuse if transfer + * has different Auth input parameters. + * Empty user: Negotiate on Windows can make use of an "ambient" + * user from a "SecurityToken" associated with the current thread or + * process. This token can be switched at any time. We are therefore + * not able to find out reliably what token the connection really + * used, nor what token in the next connect attempt will use. + * To avoid TOCTOU attacks, do not reuse on empty credentials. */ + if(!m->want_ntlm_http || + !Curl_creds_has_user(conn->creds) || + !Curl_creds_same(conn->creds, m->data->state.creds) || + !Curl_peer_equal(conn->creds_origin, m->data->state.origin)) + return FALSE; + if(!url_allow_sspi_empty_creds(conn->creds, m->data, conn)) + return FALSE; + } + else if(m->want_ntlm_http) { + /* Transfer wants NTLM, connection is not using it. + * Do not reuse when connection has credentials and they differ. */ if(conn->creds && (!Curl_creds_same(conn->creds, m->data->state.creds) || - !Curl_peer_equal(conn->creds_origin, m->data->state.origin))) { - /* connection credentials in play and not the same or not for the - * same origin. */ + !Curl_peer_equal(conn->creds_origin, m->data->state.origin))) return FALSE; - } - } - else if(conn->http_ntlm_state != NTLMSTATE_NONE) { - /* Connection is using NTLM auth but we do not want NTLM */ - return FALSE; } #ifndef CURL_DISABLE_PROXY /* Same for Proxy NTLM authentication */ - if(m->want_proxy_ntlm_http) { - /* Both conn->http_proxy.user and conn->http_proxy.passwd can be - * NULL */ - if(!conn->http_proxy.creds) + if(conn->proxy_ntlm_state != NTLMSTATE_NONE) { + if(!m->want_proxy_ntlm_http || + !Curl_creds_same(m->needle->http_proxy.creds, conn->http_proxy.creds)) return FALSE; - - if(!Curl_creds_same(m->needle->http_proxy.creds, conn->http_proxy.creds)) + if(!url_allow_sspi_empty_creds(m->needle->http_proxy.creds, + m->data, conn)) return FALSE; } - else if(conn->proxy_ntlm_state != NTLMSTATE_NONE) { - /* Proxy connection is using NTLM auth but we do not want NTLM */ - return FALSE; + else if(m->want_proxy_ntlm_http) { + if(conn->http_proxy.creds && + !Curl_creds_same(m->needle->http_proxy.creds, conn->http_proxy.creds)) + return FALSE; } #endif if(m->want_ntlm_http || m->want_proxy_ntlm_http) { @@ -967,34 +999,39 @@ static bool url_match_auth_ntlm(struct connectdata *conn, static bool url_match_auth_nego(struct connectdata *conn, struct url_conn_match *m) { - /* If we are looking for an HTTP+Negotiate connection, check if this is - already authenticating with the right credentials. If not, keep looking - so that we can reuse Negotiate connections if possible. */ - if(m->want_nego_http) { + if(conn->http_negotiate_state != GSS_AUTHNONE) { + /* Connection is using Negotiate. We cannot reuse if transfer + * has different Auth input parameters. */ + if(!m->want_nego_http || + !Curl_creds_same(conn->creds, m->data->state.creds) || + !Curl_peer_equal(conn->creds_origin, m->data->state.origin)) + return FALSE; + if(!url_allow_sspi_empty_creds(conn->creds, m->data, conn)) + return FALSE; + } + else if(m->want_nego_http) { + /* Transfer wants Negotiate, connection is not using it. + * Do not reuse when connection has credentials and they differ. */ if(conn->creds && (!Curl_creds_same(conn->creds, m->data->state.creds) || !Curl_peer_equal(conn->creds_origin, m->data->state.origin))) return FALSE; } - else if(conn->http_negotiate_state != GSS_AUTHNONE) { - /* Connection is using Negotiate auth but we do not want Negotiate */ - return FALSE; - } #ifndef CURL_DISABLE_PROXY /* Same for Proxy Negotiate authentication */ - if(m->want_proxy_nego_http) { - /* Both conn->http_proxy.user and conn->http_proxy.passwd can be - * NULL */ - if(!conn->http_proxy.creds) + if(conn->proxy_negotiate_state != GSS_AUTHNONE) { + if(!m->want_proxy_nego_http || + !Curl_creds_same(m->needle->http_proxy.creds, conn->http_proxy.creds)) return FALSE; - - if(!Curl_creds_same(m->needle->http_proxy.creds, conn->http_proxy.creds)) + if(!url_allow_sspi_empty_creds(m->needle->http_proxy.creds, + m->data, conn)) return FALSE; } - else if(conn->proxy_negotiate_state != GSS_AUTHNONE) { - /* Proxy connection is using Negotiate auth but we do not want Negotiate */ - return FALSE; + else if(m->want_proxy_nego_http) { + if(conn->http_proxy.creds && + !Curl_creds_same(m->needle->http_proxy.creds, conn->http_proxy.creds)) + return FALSE; } #endif if(m->want_nego_http || m->want_proxy_nego_http) { @@ -2370,11 +2407,12 @@ static CURLcode url_find_or_create_conn(struct Curl_easy *data) DEBUGF(curl_mfprintf(stderr, "Error: init connection SSL config\n")); goto out; } - /* attach it and no longer own it */ + + /* Add needle to conn pool, which assigns the connection id. + * Attach regardless of result, for correct handling. */ + result = Curl_cpool_add(data, needle); Curl_attach_connection(data, needle); needle = NULL; - - result = Curl_cpool_add(data, data->conn); if(result) goto out;