[PATCH 0/3] MR8020: libs/ldap: Make GSS-SPNEGO SASL plugin work with LDAP server that uses Kerberos authentication.
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Cyrus SASL GSSAPI plugin treats it this way: https://github.com/cyrusimap/cyrus-sasl/blob/master/plugins/gssapi.c Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55304 Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- libs/ldap/libldap/sasl_w.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/libs/ldap/libldap/sasl_w.c b/libs/ldap/libldap/sasl_w.c index 43156720856..daa1a6e3f44 100644 --- a/libs/ldap/libldap/sasl_w.c +++ b/libs/ldap/libldap/sasl_w.c @@ -70,28 +70,35 @@ int sasl_decode( sasl_conn_t *handle, const char *input, unsigned int inputlen, unsigned int len; SecBuffer bufs[2] = { - { conn->trailer_size, SECBUFFER_TOKEN, NULL }, - { inputlen - conn->trailer_size - sizeof(len), SECBUFFER_DATA, NULL } + { 0, SECBUFFER_DATA, NULL }, + { conn->trailer_size, SECBUFFER_TOKEN, NULL } }; SecBufferDesc buf_desc = { SECBUFFER_VERSION, ARRAYSIZE(bufs), bufs }; SECURITY_STATUS status; int ret; + ldap_log_printf( NULL, LDAP_DEBUG_TRACE, "sasl_decode: %p,%u\n", input, inputlen ); + if (inputlen < sizeof(len) + conn->trailer_size) return SASL_FAIL; - if ((ret = grow_buffer( conn, inputlen - sizeof(len) )) < 0) return ret; - memcpy( conn->buf, input + sizeof(len), inputlen - sizeof(len) ); + len = ntohl( *(unsigned int *)input ); + if (inputlen < sizeof(len) + len) return SASL_FAIL; + + if ((ret = grow_buffer( conn, len )) < 0) return ret; + memcpy( conn->buf, input + sizeof(len), len ); bufs[0].pvBuffer = conn->buf; - bufs[1].pvBuffer = conn->buf + conn->trailer_size; + bufs[0].cbBuffer = len - conn->trailer_size; + bufs[1].pvBuffer = conn->buf + bufs[0].cbBuffer; status = DecryptMessage( &conn->ctxt_handle, &buf_desc, 0, NULL ); if (status == SEC_E_OK) { - *output = bufs[1].pvBuffer; - *outputlen = bufs[1].cbBuffer; + *output = bufs[0].pvBuffer; + *outputlen = bufs[0].cbBuffer; + return SASL_OK; } - return (status == SEC_E_OK) ? SASL_OK : SASL_FAIL; + return SASL_FAIL; } int sasl_encode( sasl_conn_t *handle, const char *input, unsigned int inputlen, const char **output, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8020
From: Dmitry Timoshkov <dmitry(a)baikal.ru> RFC4752 mentions these flags as required on the client side: https://www.rfc-editor.org/rfc/rfc4752#page-3 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55304 Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- libs/ldap/libldap/sasl_w.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/libs/ldap/libldap/sasl_w.c b/libs/ldap/libldap/sasl_w.c index 48028352ef4..1d514d0f047 100644 --- a/libs/ldap/libldap/sasl_w.c +++ b/libs/ldap/libldap/sasl_w.c @@ -1,5 +1,6 @@ /* * Copyright 2022 Hans Leidekker for CodeWeavers + * Copyright 2023 Dmitry Timoshkov * * SSPI based replacement for Cyrus SASL * @@ -18,6 +19,10 @@ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA */ +#include "portable.h" +#include "ldap-int.h" +#include "ldap_log.h" + #include <assert.h> #include <stdio.h> #include <stdlib.h> @@ -36,6 +41,7 @@ struct connection sasl_interact_t prompts[4]; unsigned int max_token; unsigned int trailer_size; + unsigned int qop; sasl_ssf_t ssf; char *buf; unsigned buf_size; @@ -147,6 +153,8 @@ int sasl_client_new( const char *service, const char *server, const char *localp SecPkgInfoA *info; int len; + ldap_log_printf( NULL, LDAP_DEBUG_TRACE, "sasl_client_new: service %s, server %s\n", service, server ); + if (!check_callback( prompt, SASL_CB_AUTHNAME ) || !check_callback( prompt, SASL_CB_GETREALM ) || !check_callback( prompt, SASL_CB_PASS )) return SASL_BADPARAM; @@ -261,10 +269,13 @@ int sasl_client_start( sasl_conn_t *handle, const char *mechlist, sasl_interact_ { 0, SECBUFFER_ALERT, NULL } }; SecBufferDesc out_buf_desc = { SECBUFFER_VERSION, ARRAYSIZE(out_bufs), out_bufs }; - ULONG attrs, flags = ISC_REQ_INTEGRITY | ISC_REQ_CONFIDENTIALITY; + /* FIXME: flags probably should depend on LDAP_OPT_SSPI_FLAGS */ + ULONG attrs, flags = ISC_REQ_INTEGRITY | ISC_REQ_CONFIDENTIALITY | ISC_REQ_MUTUAL_AUTH | ISC_REQ_SEQUENCE_DETECT; SECURITY_STATUS status; int ret; + ldap_log_printf( NULL, LDAP_DEBUG_TRACE, "sasl_client_start\n" ); + if (!*prompts) { *prompts = conn->prompts; @@ -311,9 +322,12 @@ int sasl_client_step( sasl_conn_t *handle, const char *serverin, unsigned int se }; SecBufferDesc in_buf_desc = { SECBUFFER_VERSION, ARRAYSIZE(in_bufs), in_bufs }; SecBufferDesc out_buf_desc = { SECBUFFER_VERSION, ARRAYSIZE(out_bufs), out_bufs }; - ULONG attrs, flags = ISC_REQ_INTEGRITY | ISC_REQ_CONFIDENTIALITY; + /* FIXME: flags probably should depend on LDAP_OPT_SSPI_FLAGS */ + ULONG attrs, flags = ISC_REQ_INTEGRITY | ISC_REQ_CONFIDENTIALITY | ISC_REQ_MUTUAL_AUTH | ISC_REQ_SEQUENCE_DETECT; SECURITY_STATUS status; + ldap_log_printf( NULL, LDAP_DEBUG_TRACE, "sasl_client_step: target %s\n", conn->target ); + status = InitializeSecurityContextA( NULL, &conn->ctxt_handle, conn->target, flags, 0, 0, &in_buf_desc, 0, &conn->ctxt_handle, &out_buf_desc, &attrs, NULL ); if (status == SEC_E_OK || status == SEC_I_CONTINUE_NEEDED) @@ -323,8 +337,13 @@ int sasl_client_step( sasl_conn_t *handle, const char *serverin, unsigned int se if (status == SEC_I_CONTINUE_NEEDED) return SASL_CONTINUE; else { + if (((flags & ISC_REQ_INTEGRITY) && !(attrs & ISC_RET_INTEGRITY)) || + ((flags & ISC_REQ_CONFIDENTIALITY) && !(attrs & ISC_RET_CONFIDENTIALITY))) + return SASL_BADSERV; /* refuse to continue if the server doesn't support requested security levels */ + conn->ssf = get_key_size( &conn->ctxt_handle ); conn->trailer_size = get_trailer_size( &conn->ctxt_handle ); + conn->qop = attrs; return SASL_OK; } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8020
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Cyrus SASL GSSAPI plugin does it this way: https://github.com/cyrusimap/cyrus-sasl/blob/master/plugins/gssapi.c Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55304 Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- libs/ldap/libldap/sasl_w.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/libs/ldap/libldap/sasl_w.c b/libs/ldap/libldap/sasl_w.c index 1d514d0f047..43156720856 100644 --- a/libs/ldap/libldap/sasl_w.c +++ b/libs/ldap/libldap/sasl_w.c @@ -108,21 +108,24 @@ int sasl_encode( sasl_conn_t *handle, const char *input, unsigned int inputlen, unsigned int len; int ret; + ldap_log_printf( NULL, LDAP_DEBUG_TRACE, "sasl_encode: %p,%u\n", input, inputlen ); + if ((ret = grow_buffer( conn, sizeof(len) + inputlen + conn->trailer_size )) < 0) return ret; - memcpy( conn->buf + sizeof(len) + conn->trailer_size, input, inputlen ); - bufs[0].pvBuffer = conn->buf + sizeof(len) + conn->trailer_size; - bufs[1].pvBuffer = conn->buf + sizeof(len); + memcpy( conn->buf + sizeof(len), input, inputlen ); + bufs[0].pvBuffer = conn->buf + sizeof(len); + bufs[1].pvBuffer = conn->buf + sizeof(len) + inputlen; - status = EncryptMessage( &conn->ctxt_handle, 0, &buf_desc, 0 ); + status = EncryptMessage( &conn->ctxt_handle, (conn->qop & ISC_RET_CONFIDENTIALITY) ? 0 : SECQOP_WRAP_NO_ENCRYPT, &buf_desc, 0 ); if (status == SEC_E_OK) { len = htonl( bufs[0].cbBuffer + bufs[1].cbBuffer ); memcpy( conn->buf, &len, sizeof(len) ); *output = conn->buf; *outputlen = sizeof(len) + bufs[0].cbBuffer + bufs[1].cbBuffer; + return SASL_OK; } - return (status == SEC_E_OK) ? SASL_OK : SASL_FAIL; + return SASL_FAIL; } const char *sasl_errstring( int saslerr, const char *langlist, const char **outlang ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8020
Hans Leidekker (@hans) commented about libs/ldap/libldap/sasl_w.c:
if (status == SEC_I_CONTINUE_NEEDED) return SASL_CONTINUE; else { + if (((flags & ISC_REQ_INTEGRITY) && !(attrs & ISC_RET_INTEGRITY)) || + ((flags & ISC_REQ_CONFIDENTIALITY) && !(attrs & ISC_RET_CONFIDENTIALITY))) + return SASL_BADSERV; /* refuse to continue if the server doesn't support requested security levels */
The RFC you mention is about Kerberos. Does NTLM have the same requirement? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103108
On Tue May 13 12:14:53 2025 +0000, Hans Leidekker wrote:
The RFC you mention is about Kerberos. Does NTLM have the same requirement? Frankly speaking I don't know. I don't have an appropriate infrustructure to test it either.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103111
On Tue May 13 12:20:54 2025 +0000, Dmitry Timoshkov wrote:
Frankly speaking I don't know. I don't have an appropriate infrustructure to test it either. Then I propose to omit this check or somehow make it depend on the protocol. The same goes for the other patches in this MR. We shouldn't change existing behavior without tests.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103118
On Tue May 13 14:26:30 2025 +0000, Dmitry Timoshkov wrote:
What is the reason to worry about NTLM? It won't work there, or there's other reason? What exactly won't work with NTLM? In general if the server was asked about intergrity or confidentiality and server reports that it doesn't support them that's a sign of forged response, and the communication should be aborted. What changes in other patches would need tests?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103131
On Tue May 13 12:43:13 2025 +0000, Hans Leidekker wrote:
Then I propose to omit this check or somehow make it depend on the protocol. The same goes for the other patches in this MR. We shouldn't change existing behavior without tests. What is the reason to worry about NTLM? It won't work there, or there's other reason? What exactly won't work with NTLM? In general if the server was asked about intergrity or confidentiality and server reports that it doesn't support them that's a sign of forged response, and the communication should be aborted.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103130
On Tue May 13 14:29:58 2025 +0000, Dmitry Timoshkov wrote:
What changes in other patches would need tests? Also it's probably worth to note that current code in libs/ldap/libldap/sasl_w.c doesn't have any tests, and is broken regarding Kerberos support. Besides NTLM tests don't work in Wine (and never worked before) because NTLM support in Wine is not functional, so it's not clear how LDAP+NTLM could be tested in Wine at all.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103138
On Tue May 13 14:52:54 2025 +0000, Dmitry Timoshkov wrote:
Also it's probably worth to note that current code in libs/ldap/libldap/sasl_w.c doesn't have any tests, and is broken regarding Kerberos support. Besides NTLM tests don't work in Wine (and never worked before) because NTLM support in Wine is not functional, so it's not clear how LDAP+NTLM could be tested in Wine at all. I understand that Kerberos/NTLM unit tests for LDAP connections are out of scope but we can at least do manual tests. I know NTLM worked when I wrote this code.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103140
On Tue May 13 15:07:32 2025 +0000, Hans Leidekker wrote:
I understand that Kerberos/NTLM unit tests for LDAP connections are out of scope but we can at least do manual tests. I know NTLM worked when I wrote this code. Probably you could have a look at the NTLM tests then to make them work in Wine? When I fixed them to run under recent Windows versions I also spent quite a bit of time looking for a version of Wine where NTLM tests could work, however NTLM tests didn't work in all versions of Wine I tested them with, including version of Wine where the tests were added.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103141
On Tue May 13 15:16:04 2025 +0000, Dmitry Timoshkov wrote:
Probably you could have a look at the NTLM tests then to make them work in Wine? When I fixed them to run under recent Windows versions I also spent quite a bit of time looking for a version of Wine where NTLM tests could work, however NTLM tests didn't work in all versions of Wine I tested them with, including version of Wine where the tests were added. For attributes returned by InitializeSecurityContext() probably I could add some tests to dlls/secur32/tests/ntlm.c to see how it supposed to behave under Windows when some certain attributes are requested for a connection.
I'm not sure what kind of tests should be added for encrypting/decrypting, Hans, do you have any particular suggestions? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103148
On Tue May 13 15:53:30 2025 +0000, Dmitry Timoshkov wrote:
For attributes returned by InitializeSecurityContext() probably I could add some tests to dlls/secur32/tests/ntlm.c to see how it supposed to behave under Windows when some certain attributes are requested for a connection. I'm not sure what kind of tests should be added for encrypting/decrypting, Hans, do you have any particular suggestions? I tested an NTLM authenticated LDAP connection and it still works. With your patch authentication fails due to that request flag check. If I remove the check I get a crash.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103164
On Tue May 13 17:38:01 2025 +0000, Hans Leidekker wrote:
I tested an NTLM authenticated LDAP connection and it still works. With your patch authentication fails due to that request flag check. If I remove the check I get a crash. Thanks for testing. So, what could be a way forward here?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103172
On Tue May 13 17:44:54 2025 +0000, Dmitry Timoshkov wrote:
Thanks for testing. So, what could be a way forward here? Also it would be helpful to get some more details about the crash. What kind of server of that? Which flag check leads to a failure? Where does it crash?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103182
On Tue May 13 18:30:15 2025 +0000, Dmitry Timoshkov wrote:
Also it would be helpful to get some more details about the crash. What kind of server of that? Which flag check leads to a failure? Where does it crash? If I can find more time I'll take a closer look. As for those RET flags I'm not sure we should be acting on them:
`The caller is responsible for determining whether the final context attributes are sufficient. If, for example, confidentiality was requested, but could not be established, some applications may choose to shut down the connection immediately.` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103183
On Tue May 13 18:30:30 2025 +0000, Hans Leidekker wrote:
If I can find more time I'll take a closer look. As for those RET flags I'm not sure we should be acting on them: `The caller is responsible for determining whether the final context attributes are sufficient. If, for example, confidentiality was requested, but could not be established, some applications may choose to shut down the connection immediately.` Yes, that's precisely the reason of the check. And the caller in this context is the SASL plugin.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103184
On Tue May 13 18:42:59 2025 +0000, Dmitry Timoshkov wrote:
Yes, that's precisely the reason of the check. And the caller in this context is the SASL plugin. I'm not sure about that. There's the LDAP_OPT_SSPI_FLAGS as you mention in your patch and it could be that we're supposed to build the REQ flags from it and return the RET flags for it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103186
On Tue May 13 18:54:00 2025 +0000, Hans Leidekker wrote:
I'm not sure about that. There's the LDAP_OPT_SSPI_FLAGS as you mention in your patch and it could be that we're supposed to build the REQ flags from it and return the RET flags for it. I've added a test
``` flags = 0; ret = ldap_get_option(ld, LDAP_OPT_SSPI_FLAGS, &flags); ok(!ret, "ldap_get_option error %#x\n", ret); ok(flags == (ISC_REQ_MUTUAL_AUTH | ISC_REQ_EXTENDED_ERROR), "got SSPI flags %08lx\n", flags); ``` I'll see if just requesting these flags work for LDAP over Kerberos. Hans, could you please test them with NTLM and report back? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103187
On Tue May 13 18:54:17 2025 +0000, Dmitry Timoshkov wrote:
I've added a test ``` flags = 0; ret = ldap_get_option(ld, LDAP_OPT_SSPI_FLAGS, &flags); ok(!ret, "ldap_get_option error %#x\n", ret); ok(flags == (ISC_REQ_MUTUAL_AUTH | ISC_REQ_EXTENDED_ERROR), "got SSPI flags %08lx\n", flags); ``` I'll see if just requesting these flags work for LDAP over Kerberos. Hans, could you please test them with NTLM and report back? Probably integrity and confidentiality should be added as well, not sure why they are not part of default SSPI flags.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103189
On Tue May 13 18:57:09 2025 +0000, Dmitry Timoshkov wrote:
Probably integrity and confidentiality should be added as well, not sure why they are not part of default SSPI flags. On the other hand if integrity or confidentiality are not reported as used after a successful creation of the context I'm not sure how such an NTLM context could be considered as a secure/valid one.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103190
On Tue May 13 19:00:57 2025 +0000, Dmitry Timoshkov wrote:
On the other hand if integrity or confidentiality are not reported as used after a successful creation of the context I'm not sure how such an NTLM context could be considered as a secure/valid one. It makes sense to let the caller decide what's sufficiently secure. If authentication is done over SSL for example then confidentiality doesn't matter much.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103192
On Tue May 13 19:11:05 2025 +0000, Hans Leidekker wrote:
It makes sense to let the caller decide what's sufficiently secure. If authentication is done over SSL for example then confidentiality doesn't matter much. I mean if the caller requested integrity and confidentiality (like your current code does) but server doesn't support them. I'd guess that NTLM doesn't support integrity (signing), and just using confidentiality (encryption) is sufficient in that case, and shouldn't be considered as an error.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103193
On Tue May 13 19:22:59 2025 +0000, Dmitry Timoshkov wrote:
I mean if the caller requested integrity and confidentiality (like your current code does) but server doesn't support them. I'd guess that NTLM doesn't support integrity (signing), and just using confidentiality (encryption) is sufficient in that case, and shouldn't be considered as an error. I did some experiments and ISC_REQ_MUTUAL_AUTH is the key flag that makes LDAP over Kerberos work for my configuration, without it authentication instead of 2 uses 1 step, and apparently this makes the security context unusable.
I don't insist that the server attributes check should be fatal, emitting a warning would suffice I guess. Hans, does adding only ISC_REQ_MUTUAL_AUTH work for your use case with NTLM? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103247
On Wed May 14 07:10:59 2025 +0000, Dmitry Timoshkov wrote:
I did some experiments and ISC_REQ_MUTUAL_AUTH is the key flag that makes LDAP over Kerberos work for my configuration, without it authentication instead of 2 uses 1 step, and apparently this makes the security context unusable. I don't insist that the server attributes check should be fatal, emitting a warning would suffice I guess. Hans, does adding only ISC_REQ_MUTUAL_AUTH work for your use case with NTLM? Retrieving SSPI flags before binding returns ISC_REQ_EXTENDED_ERROR | ISC_REQ_MUTUAL_AUTH, so that seems to be the default. Retrieving the flags after successful Negotiate bind (which picks NTLM) I get ISC_REQ_INTEGRITY | ISC_REQ_EXTENDED_ERROR | ISC_REQ_REPLAY_DETECT | ISC_REQ_SEQUENCE_DETECT.
I think we should use the same default and avoid any checks on returned flags. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103323
On Wed May 14 12:43:25 2025 +0000, Hans Leidekker wrote:
Retrieving SSPI flags before binding returns ISC_REQ_EXTENDED_ERROR | ISC_REQ_MUTUAL_AUTH, so that seems to be the default. Retrieving the flags after successful Negotiate bind (which picks NTLM) I get ISC_REQ_INTEGRITY | ISC_REQ_EXTENDED_ERROR | ISC_REQ_REPLAY_DETECT | ISC_REQ_SEQUENCE_DETECT. I think we should use the same default and avoid any checks on returned flags. Kerberos won't work without ISC_REQ_MUTUAL_AUTH, how should we decide when add it? Somehow detect NTLM/Kerberos?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103324
On Wed May 14 12:49:16 2025 +0000, Dmitry Timoshkov wrote:
Kerberos won't work without ISC_REQ_MUTUAL_AUTH, how should we decide when add it? Somehow detect NTLM/Kerberos? [ldap.diff](/uploads/32804cf1bcc30596071f1370884723c5/ldap.diff)
This change against current Wine works for NTLM. Does it also work for Kerberos? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103340
On Wed May 14 13:28:31 2025 +0000, Hans Leidekker wrote:
[ldap.diff](/uploads/32804cf1bcc30596071f1370884723c5/ldap.diff) This change against current Wine works for NTLM. Does it also work for Kerberos? After applying this diff 2 the steps authentication is performed with Kerberos, so yes, it helps. However I need to apply other 2 patches to make sasl_encode/sasl_decode work.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103359
On Wed May 14 14:43:10 2025 +0000, Dmitry Timoshkov wrote:
After applying this diff 2 the steps authentication is performed with Kerberos, so yes, it helps. However I need to apply other 2 patches to make sasl_encode/sasl_decode work. [ldap2.diff](/uploads/c48c87be4a65ff610c7c949598a8cb94/ldap2.diff)
Does this patch help by any chance? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103384
On Wed May 14 16:06:18 2025 +0000, Hans Leidekker wrote:
[ldap2.diff](/uploads/c48c87be4a65ff610c7c949598a8cb94/ldap2.diff) Does this patch help by any chance? No, it doesn't. Moreover adding ISC_REQ_USE_DCE_STYLE breaks Kerberos authentication with working sasl_encode/sasl/decode.
As explained in the patches sasl_encode() needs to properly construct output buffer placing trailer at the end of the buffer, and sasl_decode() should treat input buffer as data+trailer like https://github.com/cyrusimap/cyrus-sasl/blob/master/plugins/gssapi.c does. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103385
On Wed May 14 16:21:12 2025 +0000, Dmitry Timoshkov wrote:
No, it doesn't. Moreover adding ISC_REQ_USE_DCE_STYLE breaks Kerberos authentication with working sasl_encode/sasl/decode. As explained in the patches sasl_encode() needs to properly construct output buffer placing trailer at the end of the buffer, and sasl_decode() should treat input buffer as data+trailer like https://github.com/cyrusimap/cyrus-sasl/blob/master/plugins/gssapi.c does. ISC_REQ_USE_DCE_STYLE is what we use every else and it uses a different buffer layout, so I thought it was worth a shot. We want to avoid making assumptions about buffer layout here since that defeats the purpose of Negotiate (and SASL). I wonder if this should be fixed in the Negotiate provider instead. Currently we simply pass through to Kerberos/NTLM, perhaps that's not correct.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103403
On Wed May 14 17:47:33 2025 +0000, Hans Leidekker wrote:
ISC_REQ_USE_DCE_STYLE is what we use every else and it uses a different buffer layout, so I thought it was worth a shot. We want to avoid making assumptions about buffer layout here since that defeats the purpose of Negotiate (and SASL). I wonder if this should be fixed in the Negotiate provider instead. Currently we simply pass through to Kerberos/NTLM, perhaps that's not correct. USE_DCE_STYLE doesn't work for Kerberos, at least I had to remove it in winhttp and wininet to make authorization work.
Regarding SASL buffer layout I'd guess it's documented somewhere, but I didn't manage to find it. And it took quite a bit of effort to figure it out until I compared sasl_w.c code with the code from cyrusmap. P.S. Also we have to address the problem when EncryptMessage() returns token less in size than 64 (KERBEROS_SECURITY_TRAILER), for me it returns 60. The problem is that sasl_decode() assumes that input buffer trailer is 64 bytes long, that leads to wrong buffer sizes passed to DecryptMessage(), and it fails. I have a patch for that. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103408
There's no reason ISC_REQ_USE_DCE_STYLE can't work with Kerberos. It maps to GSS_C_DCE_STYLE which is supported by MIT Kerberos. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103409
On Wed May 14 18:26:57 2025 +0000, Hans Leidekker wrote:
There's no reason ISC_REQ_USE_DCE_STYLE can't work with Kerberos. It maps to GSS_C_DCE_STYLE which is supported by MIT Kerberos. Well, it doesn't work with the Samba domain server I'm connecting to, and the only solution was to remove this flag. A lot of time and effort was put to analyze Samba server logs and Kerberos logs on the client side, and there was anything helpful in there, so the decision was to stop wasting time on it. Feel free to investigate it on your own, and see how it goes.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103411
I'd like to clarify some things a bit. 1. These (and other similar) patches are supposed to fix https://bugs.winehq.org/show_bug.cgi?id=55304 which is a regression since wine-7.22. 2. Before the offending commit ldap_bind_s(ld, LDAP_AUTH_NEGOTIATE) worked perfectly for a host that uses Kerberos authorization. 3. The configuration where it worked used libldap from Linux host. libldap uses Cyrus SASL GSSAPI plugin:https://github.com/cyrusimap/cyrus-sasl/blob/master/plugins/gssapi.c. This plugin a) uses GSS_C_MUTUAL_FLAG | GSS_C_SEQUENCE_FLAG | GSS_C_CONF_FLAG | GSS_C_INTEG_FLAG flags b) doesn't use GSS_C_DCE_STYLE c) treats output buffer as data + token. So, the questions are 1. Did NTLM authorization work before switch to a bundled LDAP? How important is support for obsolete NTLM when preferred Kerberos is broken? Wouldn't it be better to have a working Kerberos support and later investigate if NTLM support could be added (if it worked at all before)? 2. What the point of discussing Kerberos specific RFC4752, DCE style, flags and output buffer format if the de-facto only SASL GSSAPI implementation used by libldap has the code that follows RFC4752 (https://github.com/cyrusimap/cyrus-sasl/blob/master/plugins/gssapi.c#L1597)? 3. Why invent something instead of using approach from a working GSSAPI plugin? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103473
We shouldn't trade Kerberos for NTLM. Kerberos is only available if the client is part of the server REALM. If that's not the case then a fallback to NTLM should happen. It still works like that on Windows. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103475
On Thu May 15 07:44:45 2025 +0000, Hans Leidekker wrote:
We shouldn't trade Kerberos for NTLM. Kerberos is only available if the client is part of the server REALM. If that's not the case then a fallback to NTLM should happen. It still works like that on Windows. I didn't ask about trading Kerberos for NTLM (it still would be helpful to know whether it worked before). The question was about priorities and using existing gssapi plugin as a reference.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103476
Can we please focus on the details? It's fine to use GSSAPI plugin as a reference (that was my suggestion originally) but it's not evident that changes are needed in libldap. We've gone from Unix libldap/sasl/gssapi to PE libldap/sspi/kerberos/Unix gssapi. We need to consider all parts that changed, not just libldap. Do you agree? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103477
This seems relevant for example: ``` #ifdef HAVE_GSS_KRB5_CRED_NO_CI_FLAGS_X /* The krb5 mechanism automatically adds INTEG and CONF flags even when * not specified, this has the effect of rendering explicit requests * of no confidentiality and integrity via setting maxssf 0 moot. * However to interoperate with Windows machines it needs to be * possible to unset these flags as Windows machines refuse to allow * two layers (say TLS and GSSAPI) to both provide these services. * So if we do not suppress these flags a SASL/GSS-SPNEGO negotiation * over, say, LDAPS will fail against Windows Servers */ ``` We should probably do the same in kerberos.dll. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103478
On Thu May 15 08:44:46 2025 +0000, Hans Leidekker wrote:
This seems relevant for example: ``` #ifdef HAVE_GSS_KRB5_CRED_NO_CI_FLAGS_X /* The krb5 mechanism automatically adds INTEG and CONF flags even when * not specified, this has the effect of rendering explicit requests * of no confidentiality and integrity via setting maxssf 0 moot. * However to interoperate with Windows machines it needs to be * possible to unset these flags as Windows machines refuse to allow * two layers (say TLS and GSSAPI) to both provide these services. * So if we do not suppress these flags a SASL/GSS-SPNEGO negotiation * over, say, LDAPS will fail against Windows Servers */ ``` We should probably do the same in kerberos.dll. What exactly do you propose to do in kerberos.dll? How would it help with output buffer format in sasl_encode/sasl_decode where they are supposed to treat encrypted buffer as data+token? Or flags passed to gss_init_sec_context() where gss plugin explicitly uses GSS_C_MUTUAL_FLAG | GSS_C_SEQUENCE_FLAG | GSS_C_CONF_FLAG | GSS_C_INTEG_FLAG?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103480
On Thu May 15 08:46:47 2025 +0000, Hans Leidekker wrote:
Can we please focus on the details? It's fine to use GSSAPI plugin as a reference (that was my suggestion originally) but it's not evident that changes are needed in libldap. We've gone from Unix libldap/sasl/gssapi to PE libldap/sspi/kerberos/Unix gssapi. We need to consider all parts that changed, not just libldap. Do you agree? Isn't discussing patches in this MR means focus on details? So far it's still unclear whether NTLM worked before, and why proposed changes don't work with NTLM.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103481
If I'd have to guess the encrypted buffer format in sasl_encode/sasl_decode should match what gss_wrap()/gss_unwrap() uses, and dlls/kerberos/unixlib.c,seal_message_no_vector()/ unseal_message_no_vector() already treats it like data+token. So, the other side of the wire sees a compatible encrypted buffer layout. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103582
I restored my test setup and compared packet captures with Wireshark. There's an obvious difference because we don't support SPNEGO in the Negotiate provider, instead we pass NTLM or Kerberos straight through which is also accepted by Windows. Even if NTLM is wrapped in SPNEGO the buffer layout is the same, i.e. the reverse of Kerberos, so there's no way around detecting the package in use. I'm attaching a patch that works here for Kerberos and NTLM. Does it work for you? [ldap.diff](/uploads/f17c839bd1ffed2b5f3d107e48437c8e/ldap.diff) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103809
On Mon May 19 10:31:52 2025 +0000, Hans Leidekker wrote:
I restored my test setup and compared packet captures with Wireshark. There's an obvious difference because we don't support SPNEGO in the Negotiate provider, instead we pass NTLM or Kerberos straight through which is also accepted by Windows. Even if NTLM is wrapped in SPNEGO the buffer layout is the same, i.e. the reverse of Kerberos, so there's no way around detecting the package in use. I'm attaching a patch that works here for Kerberos and NTLM. Does it work for you? [ldap.diff](/uploads/f17c839bd1ffed2b5f3d107e48437c8e/ldap.diff) Thanks for working on this. Yes, this patch works for me. Obviously it needs a a patch that adds GSS_C_MUTUAL_FLAG (I'm adding also GSS_C_SEQUENCE_FLAG, but that's a minor detail), otherwise your patch basically just adds additional logic if the security package is NTLM.
What are next steps with this? Should I update MR with your diff as a separate patch? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103818
On Mon May 19 10:31:52 2025 +0000, Dmitry Timoshkov wrote:
Thanks for working on this. Yes, this patch works for me. Obviously it needs a a patch that adds GSS_C_MUTUAL_FLAG (I'm adding also GSS_C_SEQUENCE_FLAG, but that's a minor detail), otherwise your patch basically just adds additional logic if the security package is NTLM. What are next steps with this? Should I update MR with your diff as a separate patch? I can't reproduce the issue with GSS_C_MUTUAL_FLAG. I can authenticate using Kerberos and NTLM with or without this flag. As discussed before I think it's fine to add GSS_C_MUTUAL_FLAG | GSS_C_EXTENDED_ERROR_FLAG as our tests indicate that these are default flags. I don't see any evidence that GSS_C_SEQUENCE_FLAG is needed.
You can include my patch in this MR. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103829
participants (3)
-
Dmitry Timoshkov -
Dmitry Timoshkov (＠dmitry)
-
Hans Leidekker (＠hans)