[PATCH v3 0/4] MR8020: libs/ldap: Make GSS-SPNEGO SASL plugin work with LDAP server that uses Kerberos authentication.
-- v3: libs/ldap: Encrypted buffer layout is different for NTLM and Kerberos. libs/ldap: sasl_decode() should treat intput buffer as data + token. libs/ldap: sasl_encode() should construct output buffer as data + token. libs/ldap: When initializing security context ask for mutual authentication. 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 | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/libs/ldap/libldap/sasl_w.c b/libs/ldap/libldap/sasl_w.c index 48028352ef4..8dda8420613 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,8 @@ struct connection sasl_interact_t prompts[4]; unsigned int max_token; unsigned int trailer_size; + unsigned int flags; + unsigned int qop; sasl_ssf_t ssf; char *buf; unsigned buf_size; @@ -147,6 +154,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 +270,12 @@ 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; + ULONG attrs; SECURITY_STATUS status; int ret; + ldap_log_printf( NULL, LDAP_DEBUG_TRACE, "sasl_client_start\n" ); + if (!*prompts) { *prompts = conn->prompts; @@ -276,7 +287,9 @@ int sasl_client_start( sasl_conn_t *handle, const char *mechlist, sasl_interact_ (SEC_WINNT_AUTH_IDENTITY_A *)&id, NULL, NULL, &conn->cred_handle, NULL ); if (status != SEC_E_OK) return SASL_FAIL; - status = InitializeSecurityContextA( &conn->cred_handle, NULL, conn->target, flags, + /* FIXME: flags probably should depend on LDAP_OPT_SSPI_FLAGS */ + conn->flags = ISC_REQ_INTEGRITY | ISC_REQ_CONFIDENTIALITY | ISC_REQ_MUTUAL_AUTH | ISC_REQ_EXTENDED_ERROR; + status = InitializeSecurityContextA( &conn->cred_handle, NULL, conn->target, conn->flags, 0, 0, NULL, 0, &conn->ctxt_handle, &out_buf_desc, &attrs, NULL ); if (status == SEC_E_OK || status == SEC_I_CONTINUE_NEEDED) { @@ -311,10 +324,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; + ULONG attrs; SECURITY_STATUS status; - status = InitializeSecurityContextA( NULL, &conn->ctxt_handle, conn->target, flags, 0, 0, + ldap_log_printf( NULL, LDAP_DEBUG_TRACE, "sasl_client_step: target %s\n", conn->target ); + + status = InitializeSecurityContextA( NULL, &conn->ctxt_handle, conn->target, conn->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 +338,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 (((conn->flags & ISC_REQ_INTEGRITY) && !(attrs & ISC_RET_INTEGRITY)) || + ((conn->flags & ISC_REQ_CONFIDENTIALITY) && !(attrs & ISC_RET_CONFIDENTIALITY))) + ldap_log_printf( NULL, LDAP_DEBUG_TRACE, "sasl_client_step: server doesn't support some of the requested security levels\n" ); + 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 8dda8420613..61b68de7e26 100644 --- a/libs/ldap/libldap/sasl_w.c +++ b/libs/ldap/libldap/sasl_w.c @@ -109,21 +109,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
From: Dmitry Timoshkov <dmitry(a)baikal.ru> Cyrus SALS 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 61b68de7e26..2fa09bfd652 100644 --- a/libs/ldap/libldap/sasl_w.c +++ b/libs/ldap/libldap/sasl_w.c @@ -71,28 +71,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; + 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 ); - if ((ret = grow_buffer( conn, inputlen - sizeof(len) )) < 0) return ret; - memcpy( conn->buf, input + sizeof(len), inputlen - sizeof(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: Hans Leidekker <hans(a)codeweavers.com> Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- libs/ldap/libldap/sasl_w.c | 42 +++++++++++++++++++++++++++++++++----- 1 file changed, 37 insertions(+), 5 deletions(-) diff --git a/libs/ldap/libldap/sasl_w.c b/libs/ldap/libldap/sasl_w.c index 2fa09bfd652..88753330fd4 100644 --- a/libs/ldap/libldap/sasl_w.c +++ b/libs/ldap/libldap/sasl_w.c @@ -43,6 +43,7 @@ struct connection unsigned int trailer_size; unsigned int flags; unsigned int qop; + unsigned short package_id; sasl_ssf_t ssf; char *buf; unsigned buf_size; @@ -87,9 +88,17 @@ int sasl_decode( sasl_conn_t *handle, const char *input, unsigned int inputlen, if ((ret = grow_buffer( conn, len )) < 0) return ret; memcpy( conn->buf, input + sizeof(len), len ); - bufs[0].pvBuffer = conn->buf; bufs[0].cbBuffer = len - conn->trailer_size; - bufs[1].pvBuffer = conn->buf + bufs[0].cbBuffer; + if (conn->package_id == RPC_C_AUTHN_GSS_KERBEROS) + { + bufs[0].pvBuffer = conn->buf; + bufs[1].pvBuffer = conn->buf + bufs[0].cbBuffer; + } + else + { + bufs[0].pvBuffer = conn->buf + conn->trailer_size; + bufs[1].pvBuffer = conn->buf; + } status = DecryptMessage( &conn->ctxt_handle, &buf_desc, 0, NULL ); if (status == SEC_E_OK) @@ -119,9 +128,18 @@ int sasl_encode( sasl_conn_t *handle, const char *input, unsigned int inputlen, 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), input, inputlen ); - bufs[0].pvBuffer = conn->buf + sizeof(len); - bufs[1].pvBuffer = conn->buf + sizeof(len) + inputlen; + if (conn->package_id == RPC_C_AUTHN_GSS_KERBEROS) + { + memcpy( conn->buf + sizeof(len), input, inputlen ); + bufs[0].pvBuffer = conn->buf + sizeof(len); + bufs[1].pvBuffer = conn->buf + sizeof(len) + inputlen; + } + else + { + 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); + } status = EncryptMessage( &conn->ctxt_handle, (conn->qop & ISC_RET_CONFIDENTIALITY) ? 0 : SECQOP_WRAP_NO_ENCRYPT, &buf_desc, 0 ); if (status == SEC_E_OK) @@ -269,6 +287,18 @@ static ULONG get_trailer_size( CtxtHandle *ctx ) return sizes.cbSecurityTrailer; } +static unsigned short get_package_id( CtxtHandle *ctx ) +{ + SecPkgContext_NegotiationInfoW info; + unsigned short id; + + memset( &info, 0, sizeof(info) ); + if (QueryContextAttributesW( ctx, SECPKG_ATTR_NEGOTIATION_INFO, &info )) return 0; + id = info.PackageInfo->wRPCID; + FreeContextBuffer( info.PackageInfo ); + return id; +} + int sasl_client_start( sasl_conn_t *handle, const char *mechlist, sasl_interact_t **prompts, const char **clientout, unsigned int *clientoutlen, const char **mech ) { @@ -311,6 +341,7 @@ int sasl_client_start( sasl_conn_t *handle, const char *mechlist, sasl_interact_ { conn->ssf = get_key_size( &conn->ctxt_handle ); conn->trailer_size = get_trailer_size( &conn->ctxt_handle ); + conn->package_id = get_package_id( &conn->ctxt_handle ); return SASL_OK; } } @@ -354,6 +385,7 @@ int sasl_client_step( sasl_conn_t *handle, const char *serverin, unsigned int se conn->ssf = get_key_size( &conn->ctxt_handle ); conn->trailer_size = get_trailer_size( &conn->ctxt_handle ); + conn->package_id = get_package_id( &conn->ctxt_handle ); conn->qop = attrs; return SASL_OK; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8020
This merge request was approved by Dmitry Timoshkov. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020
My patch actually replaces your encode/decode patches. It makes no sense to split the buffer handling like this without updating the buffer descriptor. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103841
Hans Leidekker (@hans) commented about libs/ldap/libldap/sasl_w.c:
SecPkgInfoA *info; int len;
+ ldap_log_printf( NULL, LDAP_DEBUG_TRACE, "sasl_client_new: service %s, server %s\n", service, server ); +
These log lines interleave with Wine's when multiple threads are involved. I think it would be better to leave them out Note that +secur32 adds the same information. Or we should find a way to log through Wine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103842
Hans Leidekker (@hans) commented about libs/ldap/libldap/sasl_w.c:
if (status == SEC_I_CONTINUE_NEEDED) return SASL_CONTINUE; else { + if (((conn->flags & ISC_REQ_INTEGRITY) && !(attrs & ISC_RET_INTEGRITY)) || + ((conn->flags & ISC_REQ_CONFIDENTIALITY) && !(attrs & ISC_RET_CONFIDENTIALITY))) + ldap_log_printf( NULL, LDAP_DEBUG_TRACE, "sasl_client_step: server doesn't support some of the requested security levels\n" );
I don't see any evidence that this is needed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8020#note_103843
participants (4)
-
Dmitry Timoshkov -
Dmitry Timoshkov (＠dmitry)
-
Hans Leidekker
-
Hans Leidekker (＠hans)