vauth: clear SSPI credentials on AcquireCredentialsHandle failure#21642
vauth: clear SSPI credentials on AcquireCredentialsHandle failure#21642pen-pal wants to merge 4 commits into
Conversation
Per the Microsoft SSPI contract, when AcquireCredentialsHandle() fails
the output handle contents are undefined and must not be passed to any
other security function. The current code leaves krb5->credentials as
a non-NULL calloc'd buffer, which means:
- Curl_auth_cleanup_gssapi() later calls FreeCredentialsHandle() on
an invalid handle. SSPI providers tend to handle this gracefully
(returning SEC_E_INVALID_HANDLE) but it is technically API misuse.
- The outer "if(!krb5->credentials)" guard in this function skips
re-initialization on retry, so a transient acquire failure leaves
the connection's auth permanently broken until teardown.
Free the buffer and NULL the pointer on failure so retries can
re-initialize and the cleanup path skips the invalid handle.
Build-tested on Windows target via mingw-w64 (x86_64-w64-mingw32-gcc
15.2.0) cross-compile with --enable-sspi; runtime behavior against
live Kerberos KDCs not exercised.
…lure
Per the Microsoft SSPI contract, when AcquireCredentialsHandle() fails
the output handle contents are undefined and must not be passed to any
other security function. The current code leaves nego->credentials as
a non-NULL calloc'd buffer, which means:
- Curl_auth_cleanup_spnego() later calls FreeCredentialsHandle() on
an invalid handle. SSPI providers tend to handle this gracefully
(returning SEC_E_INVALID_HANDLE) but it is technically API misuse.
- The outer "if(!nego->credentials)" guard in this function skips
re-initialization on retry, so a transient acquire failure leaves
the connection's auth permanently broken until teardown.
Free the buffer and NULL the pointer on failure so retries can
re-initialize and the cleanup path skips the invalid handle.
Build-tested on Windows target via mingw-w64 (x86_64-w64-mingw32-gcc
15.2.0) cross-compile with --enable-sspi; runtime behavior against
live Negotiate providers not exercised.
Per the Microsoft SSPI contract, when AcquireCredentialsHandle() fails the output handle contents are undefined and must not be passed to any other security function. Currently, on failure ntlm->credentials stays set to the calloc'd buffer, and the next call's Curl_auth_cleanup_ntlm pass (run at the top of Curl_auth_create_ntlm_type1_message) calls FreeCredentialsHandle() on the invalid handle. In practice Windows SSPI providers return SEC_E_INVALID_HANDLE without crashing, but this is API misuse. Free the buffer and NULL the pointer on failure so cleanup skips the invalid handle. Build-tested on Windows target via mingw-w64 (x86_64-w64-mingw32-gcc 15.2.0) cross-compile with --enable-sspi; runtime behavior against live NTLM providers not exercised.
|
the comments aren't needed and you can use curlx_safefree to null it for example if(status != SEC_E_OK) {
curlx_safefree(krb5->credentials);
return CURLE_LOGIN_DENIED;
}That said I'm not sure this is needed. FreeCredentialsHandle does not crash on invalid handle and for the 2nd problem I don't think we can end up after a login denied where the credentials is used but better safe than sorry I guess so I'm not opposed. |
|
I'm just a bit concerned that this is done blindly (with an AI) without even testing it manually once. |
Considering what it does I think it's fine. He's clearing the credentials for something that may happen and I don't see a need to find a code path where it does happen because it's good practice to do that. Generally, FreeCredentialsHandle on zeroed credentials or credentials from a failed AcquireCredentialsHandle is ok, we do it elsewhere, however in the case of SSPI he's right that we're evaluating credentials as a condition of initialization. So, hallucination or not it seems like a good idea for SSPI. |
|
Thanks |
- Clear credentials on AcquireCredentialsHandle failure so it is not used on a subsequent call. SSPI initialization may evaluate the credentials pointer to determine whether or not a prior call to AcquireCredentialsHandle was successful, therefore we must clear it on a failed call. Closes curl#21642
Summary
Three small SSPI cleanup patches across
lib/vauth/:krb5_sspi.c— clearkrb5->credentialsafter a failedAcquireCredentialsHandle()spnego_sspi.c— clearnego->credentialsafter a failedAcquireCredentialsHandle()ntlm_sspi.c— clearntlm->credentialsafter a failedAcquireCredentialsHandle()The problem
Per the Microsoft SSPI contract for
AcquireCredentialsHandle, on failure the output handle is invalid and must not be passed to any other security function.The current code in all three sites leaves the heap-allocated
CredHandlebuffer referenced from the auth struct, which produces two real-world consequences:API misuse on cleanup. Each protocol's cleanup function (
Curl_auth_cleanup_gssapi,Curl_auth_cleanup_spnego,Curl_auth_cleanup_ntlm) later callsFreeCredentialsHandle()on the calloc'd-but-invalid handle. In practice all Windows SSPIproviders I'm aware of return
SEC_E_INVALID_HANDLEand do not crash, but this is API misuse and not contract-safe across SSPI provider implementations.State-machine wart (krb5, spnego only). The outer
if(!field->credentials)guard at the top of each function skips re-initialization on subsequent calls. After a transientAcquireCredentialsHandlefailure, the field stays non-NULL but invalid, so the next attempt at authentication on the same connection re-enters with a dead handle andInitializeSecurityContextfails — the connection's auth is wedged until the connection itself is torn down. NTLM is not affected by this aspect since it callsCurl_auth_cleanup_ntlmat function entry.The fix
On the
AcquireCredentialsHandlefailure path,curlx_freethe buffer and set the field toNULL. After the change:if(field->credentials)guard correctly skips the invalid handle.Testing
x86_64-w64-mingw32(gcc 15.2.0) with--enable-sspi. Zero warnings or errors across the full libcurl build. The three patched objects emit as PE/COFF.make checksrcpasses.