[PATCH v2 0/1] MR2533: wldap32: Implement setting LDAP_OPT_SERVER_CERTIFICATE and add tests.
ctxt_handle is moved to the top of struct tlsw_session so that it can be easily retrieved via LDAP_OPT_X_TLS_SSL_CTX. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54727 -- v2: wldap32: Implement setting LDAP_OPT_SERVER_CERTIFICATE and add tests. https://gitlab.winehq.org/wine/wine/-/merge_requests/2533
From: Alex Henrie <alexhenrie24(a)gmail.com> ctxt_handle is moved to the top of struct tlsw_session so that it can be easily retrieved via LDAP_OPT_X_TLS_SSL_CTX. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54727 --- dlls/wldap32/Makefile.in | 2 +- dlls/wldap32/bind.c | 7 +++++- dlls/wldap32/init.c | 33 ++++++++++++++++++++++++++++ dlls/wldap32/option.c | 7 ++++-- dlls/wldap32/tests/Makefile.in | 2 +- dlls/wldap32/tests/parse.c | 39 ++++++++++++++++++++++++++++++++++ dlls/wldap32/winldap_private.h | 16 ++++++++++++-- include/winldap.h | 2 ++ libs/ldap/libldap/tls_w.c | 2 +- 9 files changed, 102 insertions(+), 8 deletions(-) diff --git a/dlls/wldap32/Makefile.in b/dlls/wldap32/Makefile.in index ed8630e47af..903f6f67666 100644 --- a/dlls/wldap32/Makefile.in +++ b/dlls/wldap32/Makefile.in @@ -1,6 +1,6 @@ MODULE = wldap32.dll IMPORTLIB = wldap32 -IMPORTS = $(LDAP_PE_LIBS) secur32 ws2_32 user32 +IMPORTS = $(LDAP_PE_LIBS) crypt32 secur32 ws2_32 user32 EXTRAINCL = $(LDAP_PE_CFLAGS) C_SRCS = \ diff --git a/dlls/wldap32/bind.c b/dlls/wldap32/bind.c index 5441aeba6c7..9a1c366c76d 100644 --- a/dlls/wldap32/bind.c +++ b/dlls/wldap32/bind.c @@ -67,6 +67,7 @@ ULONG CDECL ldap_bindW( LDAP *ld, WCHAR *dn, WCHAR *cred, ULONG method ) if (!ld) return ~0u; if (method != WLDAP32_LDAP_AUTH_SIMPLE) return WLDAP32_LDAP_PARAM_ERROR; + if (!check_certificate( ld )) return WLDAP32_LDAP_SERVER_DOWN; if (dn && !(dnU = strWtoU( dn ))) goto exit; if (cred) @@ -178,6 +179,7 @@ ULONG CDECL ldap_bind_sW( LDAP *ld, WCHAR *dn, WCHAR *cred, ULONG method ) TRACE( "(%p, %s, %p, %#lx)\n", ld, debugstr_w(dn), cred, method ); if (!ld) return WLDAP32_LDAP_PARAM_ERROR; + if (!check_certificate( ld )) return WLDAP32_LDAP_SERVER_DOWN; if (method == WLDAP32_LDAP_AUTH_SIMPLE) { @@ -269,6 +271,7 @@ ULONG CDECL ldap_sasl_bindW( LDAP *ld, const PWCHAR dn, const PWCHAR mechanism, debugstr_w(mechanism), cred, serverctrls, clientctrls, message ); if (!ld || !dn || !mechanism || !cred || !message) return WLDAP32_LDAP_PARAM_ERROR; + if (!check_certificate( ld )) return WLDAP32_LDAP_SERVER_DOWN; if (!(dnU = strWtoU( dn ))) goto exit; if (!(mechanismU = strWtoU( mechanism ))) goto exit; @@ -334,6 +337,7 @@ ULONG CDECL ldap_sasl_bind_sW( LDAP *ld, const PWCHAR dn, const PWCHAR mechanism debugstr_w(mechanism), cred, serverctrls, clientctrls, serverdata ); if (!ld || !dn || !mechanism || !cred || !serverdata) return WLDAP32_LDAP_PARAM_ERROR; + if (!check_certificate( ld )) return WLDAP32_LDAP_SERVER_DOWN; if (!(dnU = strWtoU( dn ))) goto exit; if (!(mechanismU = strWtoU( mechanism ))) goto exit; @@ -395,7 +399,7 @@ ULONG CDECL ldap_simple_bindW( LDAP *ld, WCHAR *dn, WCHAR *passwd ) TRACE( "(%p, %s, %p)\n", ld, debugstr_w(dn), passwd ); - if (!ld) return ~0u; + if (!ld || !check_certificate( ld )) return ~0u; if (dn && !(dnU = strWtoU( dn ))) goto exit; if (passwd) @@ -452,6 +456,7 @@ ULONG CDECL ldap_simple_bind_sW( LDAP *ld, WCHAR *dn, WCHAR *passwd ) TRACE( "(%p, %s, %p)\n", ld, debugstr_w(dn), passwd ); if (!ld) return WLDAP32_LDAP_PARAM_ERROR; + if (!check_certificate( ld )) return WLDAP32_LDAP_SERVER_DOWN; if (dn && !(dnU = strWtoU( dn ))) goto exit; if (passwd) diff --git a/dlls/wldap32/init.c b/dlls/wldap32/init.c index b8010a2da12..3496f3a2bbe 100644 --- a/dlls/wldap32/init.c +++ b/dlls/wldap32/init.c @@ -24,12 +24,43 @@ #include "winbase.h" #include "winnls.h" #include "winternl.h" +#include "schannel.h" +#include "sspi.h" #include "wine/debug.h" #include "winldap_private.h" WINE_DEFAULT_DEBUG_CHANNEL(wldap32); +BOOLEAN check_certificate( LDAP *ld ) +{ + VERIFYSERVERCERT *callback = CERT_CALLBACK(ld); + CtxtHandle *tls_context; + const CERT_CONTEXT *cert; + ULONG ret; + + if (!callback) + return TRUE; + + if (ldap_connect( CTX(ld) ) != LDAP_SUCCESS) + return FALSE; + + if (ldap_get_option( CTX(ld), LDAP_OPT_X_TLS_SSL_CTX, &tls_context ) != LDAP_SUCCESS) + return FALSE; + + if (QueryContextAttributesW( tls_context, SECPKG_ATTR_REMOTE_CERT_CONTEXT, &cert ) != SEC_E_OK) + return FALSE; + + ret = callback( ld, &cert ); + + if (ret) + TRACE( "accepted\n" ); + else + WARN( "rejected\n" ); + + return ret; +} + /* Split a space separated string of hostnames into a string array */ static char **split_hostnames( const char *hostnames ) { @@ -423,6 +454,8 @@ ULONG CDECL ldap_start_tls_sW( LDAP *ld, ULONG *retval, LDAPMessage **result, LD else { ret = map_error( ldap_start_tls_s( CTX(ld), serverctrlsU, clientctrlsU ) ); + if (!ret && !check_certificate( ld )) + ret = WLDAP32_LDAP_LOCAL_ERROR; } exit: diff --git a/dlls/wldap32/option.c b/dlls/wldap32/option.c index 2a2b7e9a517..38398c6d46c 100644 --- a/dlls/wldap32/option.c +++ b/dlls/wldap32/option.c @@ -291,6 +291,7 @@ ULONG CDECL ldap_set_optionA( LDAP *ld, int option, void *value ) case WLDAP32_LDAP_OPT_PROTOCOL_VERSION: case WLDAP32_LDAP_OPT_REFERRALS: case WLDAP32_LDAP_OPT_REFERRAL_HOP_LIMIT: + case WLDAP32_LDAP_OPT_SERVER_CERTIFICATE: case WLDAP32_LDAP_OPT_SIZELIMIT: case WLDAP32_LDAP_OPT_SSL: case WLDAP32_LDAP_OPT_TIMELIMIT: @@ -330,7 +331,6 @@ ULONG CDECL ldap_set_optionA( LDAP *ld, int option, void *value ) case WLDAP32_LDAP_OPT_SASL_METHOD: case WLDAP32_LDAP_OPT_SECURITY_CONTEXT: case WLDAP32_LDAP_OPT_SEND_TIMEOUT: - case WLDAP32_LDAP_OPT_SERVER_CERTIFICATE: case WLDAP32_LDAP_OPT_SERVER_ERROR: case WLDAP32_LDAP_OPT_SERVER_EXT_ERROR: case WLDAP32_LDAP_OPT_SIGN: @@ -443,6 +443,10 @@ ULONG CDECL ldap_set_optionW( LDAP *ld, int option, void *value ) FIXME( "ignoring referral hop limit\n" ); return WLDAP32_LDAP_SUCCESS; + case WLDAP32_LDAP_OPT_SERVER_CERTIFICATE: + CERT_CALLBACK(ld) = value; + return WLDAP32_LDAP_SUCCESS; + case WLDAP32_LDAP_OPT_DEREF: case WLDAP32_LDAP_OPT_DESC: case WLDAP32_LDAP_OPT_ERROR_NUMBER: @@ -515,7 +519,6 @@ ULONG CDECL ldap_set_optionW( LDAP *ld, int option, void *value ) case WLDAP32_LDAP_OPT_SASL_METHOD: case WLDAP32_LDAP_OPT_SECURITY_CONTEXT: case WLDAP32_LDAP_OPT_SEND_TIMEOUT: - case WLDAP32_LDAP_OPT_SERVER_CERTIFICATE: case WLDAP32_LDAP_OPT_SERVER_ERROR: case WLDAP32_LDAP_OPT_SERVER_EXT_ERROR: case WLDAP32_LDAP_OPT_SIGN: diff --git a/dlls/wldap32/tests/Makefile.in b/dlls/wldap32/tests/Makefile.in index 05fe5b9af6d..0911147a784 100644 --- a/dlls/wldap32/tests/Makefile.in +++ b/dlls/wldap32/tests/Makefile.in @@ -1,5 +1,5 @@ TESTDLL = wldap32.dll -IMPORTS = wldap32 +IMPORTS = crypt32 wldap32 C_SRCS = \ ber.c \ diff --git a/dlls/wldap32/tests/parse.c b/dlls/wldap32/tests/parse.c index 8cd313cd6ca..dc329a09785 100644 --- a/dlls/wldap32/tests/parse.c +++ b/dlls/wldap32/tests/parse.c @@ -544,6 +544,44 @@ static void test_opt_ssl(void) ldap_unbind( ld ); } +static BOOLEAN CDECL verify_certificate( LDAP *ld, const CERT_CONTEXT **cert ) +{ + CertFreeCertificateContext(*cert); + return FALSE; +} + +static void test_opt_server_certificate(void) +{ + LDAP *ld; + ULONG ret, version = LDAP_VERSION3; + + ld = ldap_initA( (char *)"db.debian.org", 636 ); + ok( ld != NULL, "ldap_init failed\n" ); + ret = ldap_set_optionA( ld, LDAP_OPT_PROTOCOL_VERSION, &version ); + ok( !ret, "ldap_set_optionA should succeed, got %#lx\n", ret ); + ret = ldap_set_optionA( ld, LDAP_OPT_SSL, LDAP_OPT_ON ); + ok( !ret, "ldap_set_optionA should succeed, got %#lx\n", ret ); + ret = ldap_set_optionA( ld, LDAP_OPT_SERVER_CERTIFICATE, &verify_certificate ); + ok( !ret, "ldap_set_optionA should succeed, got %#lx\n", ret ); + ret = ldap_connect( ld, NULL ); + todo_wine ok( ret == LDAP_SERVER_DOWN, "ldap_connect should fail, got %#lx\n", ret ); + ret = ldap_simple_bindA( ld, NULL, NULL ); + ok( ret == (ULONG)-1, "ldap_simple_bindA should fail, got %#lx\n", ret ); + ret = ldap_simple_bind_sA( ld, NULL, NULL ); + ok( ret == LDAP_SERVER_DOWN, "ldap_simple_bind_sA should fail, got %#lx\n", ret ); + ldap_unbind( ld ); + + ld = ldap_initA( (char *)"db.debian.org", 389 ); + ok( ld != NULL, "ldap_init failed\n" ); + ret = ldap_set_optionA( ld, LDAP_OPT_PROTOCOL_VERSION, &version ); + ok( !ret, "ldap_set_optionA should succeed, got %#lx\n", ret ); + ret = ldap_set_optionA( ld, LDAP_OPT_SERVER_CERTIFICATE, &verify_certificate ); + ok( !ret, "ldap_set_optionA should succeed, got %#lx\n", ret ); + ret = ldap_start_tls_sA( ld, NULL, NULL, NULL, NULL ); + ok( ret == LDAP_LOCAL_ERROR, "ldap_start_tls_sA should fail, got %#lx\n", ret ); + ldap_unbind( ld ); +} + START_TEST (parse) { LDAP *ld; @@ -552,6 +590,7 @@ START_TEST (parse) test_ldap_server_control(); test_ldap_bind_sA(); test_opt_ssl(); + test_opt_server_certificate(); ld = ldap_initA( (char *)"db.debian.org", 389 ); ok( ld != NULL, "ldap_init failed\n" ); diff --git a/dlls/wldap32/winldap_private.h b/dlls/wldap32/winldap_private.h index 7adf77f151e..c145eafefb8 100644 --- a/dlls/wldap32/winldap_private.h +++ b/dlls/wldap32/winldap_private.h @@ -21,6 +21,7 @@ #include <assert.h> #include <stdlib.h> #include "winternl.h" +#include "wincrypt.h" #include "winnls.h" #define LDAP_NEEDS_PROTOTYPES @@ -362,8 +363,18 @@ typedef struct ldapsearch struct WLDAP32_berval *cookie; } LDAPSearch; -#define CTX(ld) (*(LDAP **)ld->Reserved3) -#define SERVER_CTRLS(ld) (*(struct berval ***)(ld->Reserved3 + sizeof(LDAP *))) +typedef BOOLEAN (CDECL VERIFYSERVERCERT)(LDAP*,const CERT_CONTEXT**); + +struct reserved +{ + LDAP *ctx; + struct berval **server_ctrls; + VERIFYSERVERCERT *cert_callback; +}; + +#define CTX(ld) (((struct reserved *)ld->Reserved3)->ctx) +#define SERVER_CTRLS(ld) (((struct reserved *)ld->Reserved3)->server_ctrls) +#define CERT_CALLBACK(ld) (((struct reserved *)ld->Reserved3)->cert_callback) #define MSG(entry) (entry->Request) #define BER(ber) ((BerElement *)((ber)->opaque)) @@ -540,6 +551,7 @@ struct WLDAP32_berval ** CDECL ldap_get_values_lenA( LDAP *, LDAPMessage *, struct WLDAP32_berval ** CDECL ldap_get_values_lenW( LDAP *, LDAPMessage *, WCHAR * ) __WINE_DEALLOC(WLDAP32_ldap_value_free_len); +BOOLEAN check_certificate( LDAP *ld ) DECLSPEC_HIDDEN; ULONG map_error( int ) DECLSPEC_HIDDEN; static inline char *strWtoU( const WCHAR *str ) diff --git a/include/winldap.h b/include/winldap.h index 825d3f63b1e..9e91d0e653a 100644 --- a/include/winldap.h +++ b/include/winldap.h @@ -391,6 +391,8 @@ typedef struct ldap_apifeature_infoW DECL_WINELIB_TYPE_AW(LDAPAPIFeatureInfo) +typedef BOOLEAN (CDECL VERIFYSERVERCERT)(LDAP*,const CERT_CONTEXT**); + #ifdef __cplusplus extern "C" { diff --git a/libs/ldap/libldap/tls_w.c b/libs/ldap/libldap/tls_w.c index bc64b9f62e5..9519a435fc6 100644 --- a/libs/ldap/libldap/tls_w.c +++ b/libs/ldap/libldap/tls_w.c @@ -46,8 +46,8 @@ typedef struct tlsw_ctx { } tlsw_ctx; typedef struct tlsw_session { - CredHandle cred_handle; CtxtHandle ctxt_handle; + CredHandle cred_handle; Sockbuf_IO_Desc *sbiod; struct berval peer_der_dn; } tlsw_session; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2533
Hans Leidekker (@hans) commented about dlls/wldap32/bind.c:
TRACE( "(%p, %s, %p)\n", ld, debugstr_w(dn), passwd );
if (!ld) return WLDAP32_LDAP_PARAM_ERROR; + if (!check_certificate( ld )) return WLDAP32_LDAP_SERVER_DOWN;
Users don't necessarily need to call bind so this check would be needed in any call that requires a server connection. I think it would be better to implement WLDAP32_ldap_connect() and add the check there. Then you'd call WLDAP32_ldap_connect() in all functions that need a connection. OpenLDAP ldap_connect() doesn't take a timeout but I think we can ignore that for now and just print a fixme. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2533#note_28349
Hans Leidekker (@hans) commented about dlls/wldap32/init.c:
#include "winbase.h" #include "winnls.h" #include "winternl.h" +#include "schannel.h" +#include "sspi.h"
#include "wine/debug.h" #include "winldap_private.h"
WINE_DEFAULT_DEBUG_CHANNEL(wldap32);
+BOOLEAN check_certificate( LDAP *ld ) +{ Maybe call it check_server_certificate() to avoid confusion with client certificates?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2533#note_28350
Hans Leidekker (@hans) commented about dlls/wldap32/init.c:
else { ret = map_error( ldap_start_tls_s( CTX(ld), serverctrlsU, clientctrlsU ) ); + if (!ret && !check_certificate( ld )) + ret = WLDAP32_LDAP_LOCAL_ERROR;
If you add a check to all functions that need a connection then it shouldn't be needed here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2533#note_28351
Hans Leidekker (@hans) commented about dlls/wldap32/winldap_private.h:
struct WLDAP32_berval *cookie; } LDAPSearch;
-#define CTX(ld) (*(LDAP **)ld->Reserved3) -#define SERVER_CTRLS(ld) (*(struct berval ***)(ld->Reserved3 + sizeof(LDAP *))) +typedef BOOLEAN (CDECL VERIFYSERVERCERT)(LDAP*,const CERT_CONTEXT**); + +struct reserved +{ + LDAP *ctx; + struct berval **server_ctrls; + VERIFYSERVERCERT *cert_callback; +};
'struct private_data' would be clearer than 'struct reserved'. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2533#note_28352
Hans Leidekker (@hans) commented about libs/ldap/libldap/tls_w.c:
} tlsw_ctx;
typedef struct tlsw_session { - CredHandle cred_handle; CtxtHandle ctxt_handle; + CredHandle cred_handle; Please add a comment here such as /* ctxt_handle must be the first field */
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2533#note_28353
participants (3)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Hans Leidekker (＠hans)