spnego: block NTLM fallback in SPNEGO negotiation - #21315
Conversation
2a32002 to
81812e3
Compare
81812e3 to
0267a63
Compare
|
Failing CI checks seem to be unrelated to these changes - flakes or also seen on other PRs and in commits on master:
|
|
For the record (mainly for your benefit, @bagder 😉): Git for Windows already integrated the Windows-specific part of this, in preparation for moving all Git for Windows users off of NTLM until September. |
|
Analysis of PR #21315 at 0267a63d: Test ../../tests/http/test_40_socks.py::TestSocks::test_40_03_dl_serial[http/1.1-socks4] failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them). Test ../../tests/http/test_05_errors.py::TestErrors::test_05_09_handshake_eof failed, but it has been 9.5% flaky lately, so it's probably NOT a fault of the PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them). Test 1675 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them). Test 1501 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 10 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (2, to be exact) so it may be that this failure is rather a systemic issue with this job and not with this specific PR. Generated by Testclutch |
|
May I ask, is this an LLM-generated patch and/or PR message? |
I did make use of LLMs to help me explore the codebase (I'm a first time curl contributor) and find the correct places to modify the code, or insert flags for the compile-time defs - why I missed the I also used Copilot to create a first draft of the PR description from the commits which I subsequently added and reworked myself. I checked any code/API completions from LLM-driven editor autocomplete. |
|
Isn't it more the job of GSS and SSPI to refrain from using NTLM under the hood? Is there really value in curl detecting this and pull out the hammer? |
This is a fair question. The issue is that today there is no way for applications using curl to actually use systems like SSPI to restrict the SSPs in use (e.g. not allowing NTLM). SSPI provides in its API a way to filter the packages ( From the user's point of view, the only control they would have is to modify their entire system to do something like disable NTLM everywhere (Group Policy docs) - these things require administrator permissions and are a system-wide hammer, so this isn't viable for most users. Aside: if there's an issue with the idea of curl treating NTLM specifically as a bad package, I'd also be open to changing this to a more generic |
0267a63 to
7fc6197
Compare
|
Addressed feedback from @vszakats. Fixups squashed; here's the range-diff: Range-diff
|
7fc6197 to
a8c82c8
Compare
dscho
left a comment
There was a problem hiding this comment.
I finished reviewing the patches, and I think they are good. (I can't approve, of course, as I have no explicit access (read or otherwise) to this repository, otherwise I would approve).
Regarding the SPNEGO-no-NTLM feature: I sense that there is great reluctance to introduce this knob. I can see merits in both positions: A feature comes with a maintenance cost. On the other hand, users who currently rely on the functionality would most likely appreciate a smoother gliding path than upgrading and discovering that their previously-working setup broke without them feeling they had proper warning beforehand.
General point about this PR: In the interest of security, I would like to argue that NTLM via SPNEGO should be turned off by default, it is a big footgun for adminstrators who thought that they disabled NTLM only to realize that their Kerberos installation allowed downgrading to NTLM. Or worse: users who disabled NTLM, only to get bitten by that SPNEGO "backdoor".
If the consensus is not to introduce this SPNEGO-no-NTLM feature, I would like to register my desire on behalf of Git for Windows' users that the functionality of this feature be accepted and merged and "turned on by default", i.e. to disallow NTLM via SPNEGO always. That is, if this PR is not accepted in the current form, I would like to ask that at least the changes to lib/vauth/ would be accepted (and then no longer guarded behind a flag).
|
I think we are ready to accept this PR without build option and without feature flag and just completely disable NTLM in SPNEGO. We just need to wait for the feature window to open again, which it should do on May 9 if nothing bad happens. Then we can ship this in 8.21.0 in June and then drop the rest of the NTLM as planned in September for the release coming in October (probably 8.23.0). |
There was a problem hiding this comment.
Pull request overview
This PR hardens SPNEGO (Negotiate) authentication by preventing NTLM from being selected as a negotiated sub-mechanism, with a transitional compile-time switch (--disable-negotiate-ntlm / CURL_DISABLE_NEGOTIATE_NTLM) and accompanying feature/test plumbing.
Changes:
- Add compile-time support to block NTLM within SPNEGO for both SSPI (Windows) and GSS-API (Unix/macOS), plus runtime feature reporting (
SPNEGO-no-NTLM). - Extend internal GSS-API wrappers to support passing explicit credentials and to query the negotiated mechanism (including stub support for debug tests).
- Add new integration tests (test2092/test2093) and wire them to a new test feature flag (
negotiate-ntlm-disabled).
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/runtests.pl | Detect new SPNEGO-no-NTLM feature and expose it as negotiate-ntlm-disabled for test gating. |
| tests/data/test2092 | New test: NTLM-only stub creds + NTLM blocked ⇒ request proceeds without Authorization header. |
| tests/data/test2093 | New test: Kerberos stub creds + NTLM blocked ⇒ Negotiate still succeeds and sends Authorization header. |
| tests/data/Makefile.am | Register the new test cases in the test suite manifest. |
| lib/version.c | Add SPNEGO-no-NTLM to the feature name list when CURL_DISABLE_NEGOTIATE_NTLM is enabled. |
| lib/vauth/vauth.h | Add SPNEGO GSS-API credential handle storage; switch SSPI identity types to _EX. |
| lib/vauth/spnego_sspi.c | For SSPI, exclude NTLM via SEC_WINNT_AUTH_IDENTITY_EX.PackageList="!ntlm" under the compile-time gate. |
| lib/vauth/spnego_gssapi.c | For GSS-API, optionally restrict negotiated mechs via gss_set_neg_mechs() and always reject negotiated NTLM via context inquiry. |
| lib/vauth/krb5_gssapi.c | Update Curl_gss_init_sec_context() call sites for the new credential-handle parameter. |
| lib/vauth/digest_sspi.c | Switch SSPI identity usage to SEC_WINNT_AUTH_IDENTITY_EX. |
| lib/socks_gssapi.c | Update Curl_gss_init_sec_context() call site for the new credential-handle parameter. |
| lib/ldap.c | Switch LDAP SSPI identity struct to SEC_WINNT_AUTH_IDENTITY_EX. |
| lib/curl_sspi.h | Update SSPI helper function signatures to use SEC_WINNT_AUTH_IDENTITY_EX. |
| lib/curl_sspi.c | Initialize SEC_WINNT_AUTH_IDENTITY_EX with Version/Length; update free helper signature. |
| lib/curl_gssapi.h | Extend Curl_gss_init_sec_context() signature; add Curl_gss_inquire_context() wrapper declaration. |
| lib/curl_gssapi.c | Implement new Curl_gss_init_sec_context() signature and add a stub-capable Curl_gss_inquire_context() wrapper. |
| lib/curl_config-cmake.h.in | Add CURL_DISABLE_NEGOTIATE_NTLM CMake-configurable define (plus related review note). |
| docs/libcurl/curl_version_info.md | Document the new SPNEGO-no-NTLM feature string. |
| docs/INSTALL-CMAKE.md | Document HAVE_GSS_SET_NEG_MECHS as a CMake-detected capability. |
| docs/CURL-DISABLE.md | Document CURL_DISABLE_NEGOTIATE_NTLM. |
| configure.ac | Add gss_set_neg_mechs detection and --disable-negotiate-ntlm configuration option. |
| CMakeLists.txt | Add CURL_DISABLE_NEGOTIATE_NTLM option and detect gss_set_neg_mechs() availability. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@bagder, sounds good! Just pushed the next update with more squashed fixups:
Range-diff vs last push
|
a8c82c8 to
a9f56a7
Compare
|
Build failures because |
a9f56a7 to
856142f
Compare
|
Adding guards for stubs when the Kerberos impl does not have Range-diff
|
7a28caf to
4eaccd9
Compare
|
I think the remaining CI failure ( The test itself seems to pass with exit 0 + no Should I add a valgrind suppression for The other two failing checks seem unrelated:
|
Replace SEC_WINNT_AUTH_IDENTITY with SEC_WINNT_AUTH_IDENTITY_EX across all SSPI authentication code. The extended structure adds Version, Length, and PackageList fields while remaining backwards compatible with all SSPI functions. Available since Windows XP. Curl_create_sspi_identity now sets the Version and Length fields when initializing the structure. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Use the SEC_WINNT_AUTH_IDENTITY_EX PackageList field to pass '!ntlm' to the Negotiate SSP, preventing NTLM from being selected during SPNEGO negotiation on Windows. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Block NTLM within GSS-API SPNEGO negotiation. Acquire explicit credentials, enumerate available mechanisms, filter out the NTLMSSP OID, and restrict via gss_set_neg_mechs(). As a belt-and-braces check, also verify the negotiated mechanism after context establishment and reject NTLM if it was selected. Add a cred_handle parameter to Curl_gss_init_sec_context so SPNEGO can pass the restricted credentials. Probe for gss_set_neg_mechs() availability (HAVE_GSS_SET_NEG_MECHS) in configure and CMake. Not all GSS-API implementations provide gss_set_neg_mechs(); when it is absent the proactive filtering is skipped and the post-handshake mechanism check is the sole guard. Update test2057: with NTLM now unconditionally blocked in SPNEGO, the previous expectation of a successful multi-round NTLM handshake no longer holds. The test now verifies that when only NTLM stub credentials are available, negotiate auth is silently skipped and the request is sent without an Authorization header. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Extend the GSS-API debug stub layer to support the NTLM blocking logic without a real Kerberos environment. Add stub_gss_acquire_cred (no-op success), stub_gss_indicate_mechs (returns KRB5 and/or NTLMSSP OIDs based on CURL_STUB_GSS_CREDS), and stub_gss_inquire_context (returns the negotiated mechanism OID). Wrap these behind Curl_gss_acquire_cred, Curl_gss_indicate_mechs, and Curl_gss_inquire_context so the stubs are transparently selected when CURL_STUB_GSS_CREDS is set. Update spnego_gssapi.c to use the wrappers. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Add test2093 which verifies that Kerberos credentials still succeed when NTLM is blocked within SPNEGO. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
4eaccd9 to
729b0c4
Compare
|
Rebased on latest master.. Range-diff
But looks like GitHub is having issues with Git clones. Several CI checks are failing with failure to checkout actions; even 500 errors from |
MIT Kerberos' libgssapi_krb5 has an internal allocation in
gss_display_status() that is never freed, even when callers correctly
release the returned buffer via gss_release_buffer(). Valgrind's
leak-check therefore flags a 'definitely lost' block whenever curl
formats a GSS-API error message via Curl_gss_log_error().
The leak is in the system library, not in curl.
Add a Memcheck:Leak suppression matching the stack:
realloc -> ... -> gss_display_status -> display_gss_error
-> Curl_gss_log_error
The wildcard '...' bridges over an anonymous frame inside
libgssapi_krb5.so that valgrind reports as '???'.
This unblocks the CM libressl krb5 valgrind 2 CI job, which uses MIT
Kerberos as the system GSS-API and previously failed test 2057 on the
valgrind leak rather than on any test logic mismatch.
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
|
Added a new commit to suppress the MIT krb5 From the An internal allocation in This should unblock the |
@bagder it looks like this was missed? the author gave two pings. is it now intended to go straight to drop in sep? |
|
Yes, unfortunately this and a few more PRs didn't make it during this feature window primarily because I was too occupied handling vulnerability reports. I mentioned it in my May 28 email. |
|
@bagder can you clarify, should this PR be closed and then drop NTLM in September? |
I think we can merge this already in this window, as a first small step. We just need to fix the merge conflicts... |
|
@mjcheetham if you can fix the merge conflicts and force-push here, I think we might be ready to merge! |
|
Thanks @mjcheetham. @dscho rebased this as #22410 so we're going with that and no more change is necessary on your part. |
- Switch the Windows SSPI identity struct to SEC_WINNT_AUTH_IDENTITY_EX to use !ntlm in PackageList to prevent NTLM from being offered. - For GSS filter out NTLMSSP OID, and restrict via gss_set_neg_mechs() to prevent NTLM from being offered. - Extend the GSS-API debug stub layer to support the NTLM blocking logic without a real Kerberos environment. - Update test 2057 to check that negotiate auth is silently skipped with no Authorization header when only NTLM stub credentials are available. - Add SPNEGO NTLM blocking test 2093 which verifies that Kerberos credentials still succeed when NTLM is blocked within SPNEGO. - Suppress tests valgrind leak for MIT krb5 gss_display_status, since the leak is in the library and not in curl. To suppress the tests valgrind leak, the wildcard '...' bridges over an anonymous frame inside libgssapi_krb5.so that valgrind reports as '???'. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com> Aided-by: Johannes Schindelin Closes curl#21315 Closes curl#22410
|
Thank you all, and thanks @dscho for rebasing for me whilst I was out yesterday! |
- update `GSS_C_DELEG_POLICY_FLAG` comment to include Apple GSS, add date, and amend MIT Kerberos version to 1.7+ (was: 1.8+) Ref: krb5/krb5@45875a4 Ref: apple-oss-distributions/Heimdal@1635de3 - document `HAVE_GSS_SET_NEG_MECHS`/`gss_set_neg_mechs()`. Ref: krb5/krb5@079eed2 It's also committed to Heimdal, but not present in a release as of 7.8.0 (current latest). Ref: heimdal/heimdal@735039d Follow-up to a8881e5 #21315 #22410 Follow-up to d169ad6 #22052 Closes #22419
Best reviewed commit-by-commit.
Motivation
Even if an application omits
CURLAUTH_NTLMfromCURLOPT_HTTPAUTHto prevent bare NTLM authentication, NTLM can still be used under the hood whenCURLAUTH_NEGOTIATEis enabled — SPNEGO may silently select NTLM as its negotiated sub-mechanism.On Windows, SSPI's Negotiate SSP includes NTLM with no easy way to prevent it. On other platforms, plugins such as gss-ntlmssp can add NTLM support to MIT Kerberos's GSS-API, enabling the same NTLM-over-SPNEGO fallback.
This is a follow-up to #21076. Rather than introducing a runtime option, this PR unconditionally blocks NTLM within SPNEGO negotiation.
Approach
Commits 1–5 implement unconditional NTLM blocking — SPNEGO will never select NTLM, regardless of build configuration.
SSPI (Windows)
Uses the
SEC_WINNT_AUTH_IDENTITY_EXPackageListfield to pass!ntlmto the Negotiate SSP, preventing NTLM from being offered during negotiation.GSS-API (Unix/macOS)
Acquires explicit credentials, enumerates available mechanisms, filters out the NTLMSSP OID (
1.3.6.1.4.1.311.2.2.10), and restricts the credential viagss_set_neg_mechs(). As a belt-and-braces check (and also for GSS-API implementations that lackgss_set_neg_mechs()) also inspects the negotiated mechanism after context establishment viagss_inquire_context()and rejects NTLM if it was selected.Commits
auth: upgrade SSPI identity to SEC_WINNT_AUTH_IDENTITY_EX— replaceSEC_WINNT_AUTH_IDENTITYwith the extended structure across all SSPI auth code, adding thePackageListfield needed by commit 2.spnego/sspi: block NTLM via PackageList exclusion— use the!ntlmpackage exclusion on Windows.spnego/gss-api: block NTLM via gss_set_neg_mechs— filter NTLM from GSS-API SPNEGO and verify the negotiated mechanism post-handshake, update test2057 to expect no more NTLM auth.gss-api: stub gss_inquire_context for debug builds— extend the existing GSS-API stub layer so the NTLM detection logic can be tested without a real Kerberos environment.tests: add SPNEGO NTLM blocking tests— test2092 (Kerberos creds → still works when NTLM is blocked).