From 2d2627deab733d9803e5b52b5f185b195471328a Mon Sep 17 00:00:00 2001 From: akallabeth Date: Tue, 15 Feb 2022 08:04:17 +0000 Subject: [PATCH] Fixed SSPI fallback to NTLM (#7642) * Fixed SSPI fallback to NTLM * Fixed wide/ansi mixup * WITH_GSS fixes * Move to WinPR as this is not related to FreeRDP * Add option WITH_GSS_NO_NTLM_FALLBACK to disable NTLM fallback * Abort NLA if status is SEC_E_NO_CREDENTIALS * Properly invalidate sspi::SubContext --- CMakeLists.txt | 17 ---------- cmake/ConfigOptions.cmake | 1 - libfreerdp/core/CMakeLists.txt | 4 --- libfreerdp/core/nla.c | 3 +- winpr/CMakeLists.txt | 23 +++++++++++++ winpr/libwinpr/sspi/Negotiate/negotiate.c | 39 +++++++++++++++-------- 6 files changed, 51 insertions(+), 36 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index c8fb8fbe6..4136ee7cb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -811,23 +811,6 @@ if (WITH_OPENH264 AND NOT OPENH264_FOUND) endif() set(WITH_OPENH264 ${OPENH264_FOUND}) -if ( (WITH_GSSAPI) AND (NOT GSS_FOUND)) - message(WARNING "-DWITH_GSSAPI=ON is set, but not GSSAPI implementation was found, disabling") -elseif(WITH_GSSAPI) - if(GSS_FLAVOUR STREQUAL "MIT") - add_definitions("-DWITH_GSSAPI -DWITH_GSSAPI_MIT") - if(GSS_VERSION_1_13) - add_definitions("-DHAVE_AT_LEAST_KRB_V1_13") - endif() - include_directories(${_GSS_INCLUDE_DIR}) - elseif(GSS_FLAVOUR STREQUAL "Heimdal") - add_definitions("-DWITH_GSSAPI -DWITH_GSSAPI_HEIMDAL") - include_directories(${_GSS_INCLUDE_DIR}) - else() - message(WARNING "Kerberos version not detected") - endif() -endif() - if(TARGET_ARCH MATCHES "x86|x64") if (NOT APPLE) # Intel Performance Primitives diff --git a/cmake/ConfigOptions.cmake b/cmake/ConfigOptions.cmake index a9288cabf..a16931039 100644 --- a/cmake/ConfigOptions.cmake +++ b/cmake/ConfigOptions.cmake @@ -149,7 +149,6 @@ option(WITH_DEBUG_RINGBUFFER "Enable Ringbuffer debug messages" ${DEFAULT_DEBUG_ option(WITH_DEBUG_SYMBOLS "Pack debug symbols to installer" OFF) option(WITH_CCACHE "Use ccache support if available" ON) option(WITH_CLANG_FORMAT "Detect clang-format. run 'cmake --build . --target clangformat' to format." ON) -option(WITH_GSSAPI "Compile support for kerberos authentication. (EXPERIMENTAL)" OFF) option(WITH_DSP_EXPERIMENTAL "Enable experimental sound encoder/decoder formats" OFF) if (WITH_FFMPEG) diff --git a/libfreerdp/core/CMakeLists.txt b/libfreerdp/core/CMakeLists.txt index 654257e3b..66e77a104 100644 --- a/libfreerdp/core/CMakeLists.txt +++ b/libfreerdp/core/CMakeLists.txt @@ -147,10 +147,6 @@ endif() freerdp_library_add(${OPENSSL_LIBRARIES}) -if(WITH_GSSAPI) - freerdp_library_add(${GSS_LIBRARIES}) -endif() - if(BUILD_TESTING) add_subdirectory(test) endif() diff --git a/libfreerdp/core/nla.c b/libfreerdp/core/nla.c index 5b5ec6249..de9adba4d 100644 --- a/libfreerdp/core/nla.c +++ b/libfreerdp/core/nla.c @@ -786,10 +786,11 @@ int nla_client_begin(rdpNla* nla) goto fail; nla_set_state(nla, NLA_STATE_NEGO_TOKEN); break; + case SEC_E_NO_CREDENTIALS: case SEC_I_INCOMPLETE_CREDENTIALS: case SEC_E_INCOMPLETE_MESSAGE: default: - break; + goto fail; } rc = 1; diff --git a/winpr/CMakeLists.txt b/winpr/CMakeLists.txt index 5ad3b3bd1..6d49c2423 100644 --- a/winpr/CMakeLists.txt +++ b/winpr/CMakeLists.txt @@ -48,6 +48,29 @@ option(WITH_NATIVE_SSPI "Use native SSPI modules" ${NATIVE_SSPI}) option(WITH_SMARTCARD_INSPECT "Enable SmartCard API Inspector" OFF) option(WITH_DEBUG_MUTEX "Print mutex debug messages" ${DEFAULT_DEBUG_OPTION}) option(WITH_ICU "Use ICU for unicode conversion" OFF) +option(WITH_GSSAPI "Compile support for kerberos authentication. (EXPERIMENTAL)" OFF) +if ( (WITH_GSSAPI) AND (NOT GSS_FOUND)) + message(WARNING "-DWITH_GSSAPI=ON is set, but not GSSAPI implementation was found, disabling") +elseif(WITH_GSSAPI) + if(GSS_FLAVOUR STREQUAL "MIT") + add_definitions("-DWITH_GSSAPI -DWITH_GSSAPI_MIT") + if(GSS_VERSION_1_13) + add_definitions("-DHAVE_AT_LEAST_KRB_V1_13") + endif() + include_directories(${_GSS_INCLUDE_DIR}) + elseif(GSS_FLAVOUR STREQUAL "Heimdal") + add_definitions("-DWITH_GSSAPI -DWITH_GSSAPI_HEIMDAL") + include_directories(${_GSS_INCLUDE_DIR}) + else() + message(WARNING "Kerberos version not detected") + endif() +endif() + +include(CMakeDependentOption) +CMAKE_DEPENDENT_OPTION(WITH_GSS_NO_NTLM_FALLBACK "Do not fall back to NTLM if no kerberos ticket available" OFF "WITH_GSSAPI" OFF) +if (WITH_GSS_NO_NTLM_FALLBACK) + add_definitions("-DWITH_GSS_NO_NTLM_FALLBACK") +endif() option(WITH_DEBUG_NTLM "Print NTLM debug messages" ${DEFAULT_DEBUG_OPTION}) if(WITH_DEBUG_NTLM) diff --git a/winpr/libwinpr/sspi/Negotiate/negotiate.c b/winpr/libwinpr/sspi/Negotiate/negotiate.c index 94b73301c..41a6d8065 100644 --- a/winpr/libwinpr/sspi/Negotiate/negotiate.c +++ b/winpr/libwinpr/sspi/Negotiate/negotiate.c @@ -25,6 +25,7 @@ #include #include #include +#include #include "negotiate.h" @@ -70,16 +71,19 @@ const SecPkgInfoW NEGOTIATE_SecPkgInfoW = { static void negotiate_SetSubPackage(NEGOTIATE_CONTEXT* context, const TCHAR* name) { + WINPR_ASSERT(context); + WINPR_ASSERT(name); + if (_tcsnccmp(name, KERBEROS_SSP_NAME, ARRAYSIZE(KERBEROS_SSP_NAME)) == 0) { - context->sspiA = (const SecurityFunctionTableA*)&KERBEROS_SecurityFunctionTableA; - context->sspiW = (const SecurityFunctionTableW*)&KERBEROS_SecurityFunctionTableW; + context->sspiA = &KERBEROS_SecurityFunctionTableA; + context->sspiW = &KERBEROS_SecurityFunctionTableW; context->kerberos = TRUE; } else { - context->sspiA = (const SecurityFunctionTableA*)&NTLM_SecurityFunctionTableA; - context->sspiW = (const SecurityFunctionTableW*)&NTLM_SecurityFunctionTableW; + context->sspiA = &NTLM_SecurityFunctionTableA; + context->sspiW = &NTLM_SecurityFunctionTableW; context->kerberos = FALSE; } } @@ -129,6 +133,8 @@ static SECURITY_STATUS SEC_ENTRY negotiate_InitializeSecurityContextW( { if (!pInput) { + context->sspiW->DeleteSecurityContext(&(context->SubContext)); + SecInvalidateHandle(&context->SubContext); negotiate_SetSubPackage(context, KERBEROS_SSP_NAME); } @@ -137,20 +143,23 @@ static SECURITY_STATUS SEC_ENTRY negotiate_InitializeSecurityContextW( TargetDataRep, pInput, Reserved2, &(context->SubContext), pOutput, pfContextAttr, ptsExpiry); +#if !defined(WITH_GSS_NO_NTLM_FALLBACK) if (status == SEC_E_NO_CREDENTIALS) { WLog_WARN(TAG, "No Kerberos credentials. Retry with NTLM"); ErrorInitContextKerberos = TRUE; - context->sspiA->DeleteSecurityContext(&(context->SubContext)); - negotiate_ContextFree(context); - return status; + context->sspiW->DeleteSecurityContext(&(context->SubContext)); + SecInvalidateHandle(&context->SubContext); } +#endif } - else + + if (ErrorInitContextKerberos) { if (!pInput) { - context->sspiA->DeleteSecurityContext(&(context->SubContext)); + context->sspiW->DeleteSecurityContext(&(context->SubContext)); + SecInvalidateHandle(&context->SubContext); negotiate_SetSubPackage(context, NTLM_SSP_NAME); } @@ -188,6 +197,8 @@ static SECURITY_STATUS SEC_ENTRY negotiate_InitializeSecurityContextA( { if (!pInput) { + context->sspiA->DeleteSecurityContext(&(context->SubContext)); + SecInvalidateHandle(&context->SubContext); negotiate_SetSubPackage(context, KERBEROS_SSP_NAME); } @@ -196,21 +207,23 @@ static SECURITY_STATUS SEC_ENTRY negotiate_InitializeSecurityContextA( TargetDataRep, pInput, Reserved2, &(context->SubContext), pOutput, pfContextAttr, ptsExpiry); +#if !defined(WITH_GSS_NO_NTLM_FALLBACK) if (status == SEC_E_NO_CREDENTIALS) { WLog_WARN(TAG, "No Kerberos credentials. Retry with NTLM"); ErrorInitContextKerberos = TRUE; context->sspiA->DeleteSecurityContext(&(context->SubContext)); - negotiate_ContextFree(context); - sspi_SecureHandleSetLowerPointer(phNewContext, NULL); - return status; + SecInvalidateHandle(&context->SubContext); } +#endif } - else + + if (ErrorInitContextKerberos) { if (!pInput) { context->sspiA->DeleteSecurityContext(&(context->SubContext)); + SecInvalidateHandle(&context->SubContext); negotiate_SetSubPackage(context, NTLM_SSP_NAME); }