negotiate/ntlm connection reuse

[icing]

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.

[Copilot]

Pull request overview

Updates connection reuse matching for connection-bound NTLM and Negotiate authentication.

Changes:

• Checks existing authentication state before reuse.
• Requires matching credentials and origin parameters.
• Applies equivalent logic to proxy authentication.

Suppressed comments (1)

lib/url.c:930

• A connection can simultaneously carry proxy NTLM and origin Negotiate state. For a forward proxy, different origin hosts share the same pool destination; this branch accepts the matching proxy NTLM state, and the later force_reuse return skips url_match_auth_nego(), so its new creds_origin check never runs. That can reuse an origin-authenticated connection for a different host. Validate all active connection-bound auth states before honoring force_reuse.

  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))

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

lib/url.c:1002

• The proxy Negotiate path omits the empty-user rejection used for origin Negotiate. Equal empty credentials therefore allow reuse of a connection authenticated with an ambient security token, preserving the TOCTOU risk this change is intended to prevent. Require a non-empty user before accepting the proxy connection.

       !Curl_creds_same(m->needle->http_proxy.creds, conn->http_proxy.creds))

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Suppressed comments (3)

lib/url.c:949

• Authenticated proxy NTLM connections with empty SSPI credentials still pass this equality check because both credential objects contain an empty user. A different easy handle using a changed ambient token can therefore reuse the old authenticated proxy connection. Add the same USE_WINDOWS_SSPI empty-user/recent-connection restriction as origin authentication (after making recent-ID tracking reliable).

    if(!m->want_proxy_ntlm_http ||
       !Curl_creds_same(m->needle->http_proxy.creds, conn->http_proxy.creds))

lib/url.c:919

• This unconditional empty-user rejection runs before the USE_WINDOWS_SSPI block below, making its “unless this connection is the one used by this transfer” exception unreachable. A valid ambient-credential SSPI NTLM flow can therefore reject its own connection and restart the connection-bound handshake. Restrict this rejection to non-SSPI builds, or let the guarded recent-ID check decide empty-user reuse.

       !Curl_creds_has_user(conn->creds) ||

lib/curl_trc.c:95

• Using only recent_conn_id loses the connection ID for file:// traces: that path attaches the connection while its ID is still -1, then assigns the ID through Curl_cpool_add() (lib/url.c:2297-2299), so the remembered value is never updated. Prefer the currently attached connection ID and fall back to the remembered one.

  curl_off_t cid = data->state.recent_conn_id;

[jay]

Deny connection reuse for empty usernames for NTLM/Negotiate using connections.

Is this for security reasons? It seems to me if the user is doing -u : and there is some outside username and password that was obtained in the environment then it's reasonable to continue using the connection rather than tear it. Like in Windows how you could do something like --negotiate -u : https://foo/bar https://foo/baz.

[bagder]

Is this for security reasons?

Yes. We can't know if the user that isn't known, might be a different one in the second transfer. So to play it safe, we have to deny reuse.

[icing]

Deny connection reuse for empty usernames for NTLM/Negotiate using connections.

Is this for security reasons? It seems to me if the user is doing -u : and there is some outside username and password that was obtained in the environment then it's reasonable to continue using the connection rather than tear it. Like in Windows how you could do something like --negotiate -u : https://foo/bar https://foo/baz.

Windows, as I learned, takes the Thread/Process SecurityToken to authenticate. That token can be switched by the application at any time. Since we are async and have callbacks, no way to be sure what the actual token is that was/will be used. Looks to me like a broken design by the Windows Gods.

[jay]

Ok. IMO though some change may be possible outside of curl it's reasonable to continue reusing an existing connection. As I understand it in these cases the connection is what is authenticated here not the transfer.