From ef66c4a4a0270102e001e907da86d7deee5580c2 Mon Sep 17 00:00:00 2001 From: Richard Markiewicz Date: Thu, 6 Aug 2026 22:00:17 -0400 Subject: [PATCH] fix(sspi): remove stale rewrite docs and harden credential diagnostics Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 18 +++--------- dll/Sspi.cpp | 77 ++++++++++++++++++++-------------------------------- 2 files changed, 34 insertions(+), 61 deletions(-) diff --git a/README.md b/README.md index 2b3cdc3..f2c8a2b 100644 --- a/README.md +++ b/README.md @@ -102,20 +102,10 @@ The workaround is strictly opt-in and stays inert unless a PIN is available: wit credential is left untouched so the normal Windows prompt path is used. It only activates for a marshaled certificate credential; any other credential is passed through unchanged. -When the certificate records the legacy **Microsoft Base Smart Card Crypto Provider**, the credential is -repointed at the **Microsoft Smart Card Key Storage Provider** instead. A card minidriver is reachable -both through the Base CSP (CAPI) and through the KSP (CNG) under the same container name, but -[KB5066793](https://support.microsoft.com/help/5066793) stopped honouring the CAPI route for RSA smart -card keys. A certificate still associated with the legacy CSP therefore yields a credential LSASS cannot -service — the Kerberos certificate logon is declined locally, SPNEGO falls back to NTLM, and where NTLM -is disabled the failure surfaces as *"Authentication failed because NTLM authentication has been -disabled"* rather than as a smart card error. Naming the KSP reaches the same key over the path that -remains supported. - -The provider name is rewritten in place, so the credential keeps the exact byte layout -`CredPackAuthenticationBufferW` produced; the two provider names are the same length, so nothing moves. -Certificates recording any other provider — including third-party CSPs, which do not imply a minidriver -— are left exactly as they were. +The credential is passed on exactly as `CredPackAuthenticationBufferW` produced it. The provider and +container names it records come from the certificate, and the server resolves them through the redirected +reader — so the same smart card middleware has to be installed on the remote host as on the client. A +mismatch surfaces as *"the key container does not exist on the smart card"*. `KerbCertificateLogon` is independent of `PasswordContainsSCardPin`. The latter is the stock RDP setting that tells the client the password field holds a smart card PIN, so that a smart card diff --git a/dll/Sspi.cpp b/dll/Sspi.cpp index bd4a915..42a78b6 100644 --- a/dll/Sspi.cpp +++ b/dll/Sspi.cpp @@ -15,7 +15,7 @@ #include -static bool g_PcapInitialized = false; +static INIT_ONCE g_PcapEnvInitOnce = INIT_ONCE_STATIC_INIT; static MsRdpEx_PcapFile* g_PcapFile = NULL; static bool g_PcapEnabled = false; @@ -34,13 +34,12 @@ void MsRdpEx_SetPcapFilePath(const char* pcapFilePath) strcpy_s(g_PcapFilePath, MSRDPEX_MAX_PATH, pcapFilePath); } -void MsRdpEx_PcapEnvInit() +// Reached from AcquireCredentialsHandleW and from the message paths, both of which run concurrently on +// several connection worker threads, so first use has to be serialized rather than guarded by a plain flag. +static BOOL CALLBACK MsRdpEx_PcapEnvInitOnce(PINIT_ONCE, PVOID, PVOID*) { char* envvar; - if (g_PcapInitialized) - return; - bool pcapDump = MsRdpEx_GetEnvBool("MSRDPEX_PCAP_DUMP", false); MsRdpEx_SetPcapEnabled(pcapDump); @@ -58,7 +57,12 @@ void MsRdpEx_PcapEnvInit() free(envvar); - g_PcapInitialized = true; + return TRUE; +} + +void MsRdpEx_PcapEnvInit() +{ + InitOnceExecuteOnce(&g_PcapEnvInitOnce, MsRdpEx_PcapEnvInitOnce, NULL, NULL); } static MsRdpEx_PcapFile* MsRdpEx_GetPcapFile() @@ -989,33 +993,6 @@ static void sspi_FormatCspInfoName(const MsRdpEx_KerbSmartCardCspInfo* cspInfo, } } -// Locate the KERB_SMARTCARD_CSP_INFO inside a packed credential, rejecting anything that does not match the -// layout this module assumes. KERB_SMARTCARD_CSP_INFO is not in the SDK, so its offsets are asserted at -// compile time and re-checked here against real bytes: the blob repeats its own length, so a disagreement -// with CspDataLength means this is not the structure we think it is. -static MsRdpEx_KerbSmartCardCspInfo* sspi_TryGetCspInfo(void* pLogon, DWORD cbLogon) -{ - KERB_CERTIFICATE_LOGON* logon = (KERB_CERTIFICATE_LOGON*)pLogon; - MsRdpEx_KerbSmartCardCspInfo* cspInfo = NULL; - - if (!pLogon || !MsRdpEx_CanReadUnsafePtr(pLogon, sizeof(KERB_CERTIFICATE_LOGON))) - return NULL; - - if (logon->MessageType != KerbCertificateLogon) - return NULL; - - if ((logon->CspDataLength < MSRDPEX_CSP_INFO_HEADER_SIZE) || !logon->CspData) - return NULL; - - cspInfo = (MsRdpEx_KerbSmartCardCspInfo*) sspi_ResolvePackedField(pLogon, cbLogon, - logon->CspData, logon->CspDataLength); - - if (!cspInfo || (cspInfo->dwCspInfoLen != logon->CspDataLength)) - return NULL; - - return cspInfo; -} - // Dump a KERB_CERTIFICATE_LOGON without revealing secrets: the PIN is reported as a length only, and the // certificate itself never appears (the credential carries a container name, not the certificate). static void sspi_LogKerbCertificateLogon(const char* label, void* pLogon, DWORD cbLogon) @@ -1037,6 +1014,16 @@ static void sspi_LogKerbCertificateLogon(const char* label, void* pLogon, DWORD return; } + // CanReadUnsafePtr proves the pages are committed, not that the allocation extends this far, so a known + // size too small to hold the fixed header has to be rejected before the first dereference. Zero means the + // size is simply unknown (a buffer that is not a LocalAlloc block) and is handled further down. + if (cbLogon && (cbLogon < sizeof(KERB_CERTIFICATE_LOGON))) + { + MsRdpEx_LogPrint(DEBUG, "KerbCertificateLogon(%s): truncated, size=%u is below the header size", + label, cbLogon); + return; + } + logon = (KERB_CERTIFICATE_LOGON*)pLogon; if (logon->MessageType != KerbCertificateLogon) @@ -1057,6 +1044,16 @@ static void sspi_LogKerbCertificateLogon(const char* label, void* pLogon, DWORD return; } + // Without a known allocation size the extent check in sspi_ResolvePackedField cannot run, and walking the + // blob would then trust CspDataLength alone and could read past the credential. The fields above are + // already bounded by CanReadUnsafePtr, so report them and stop. A credential arriving in-band often has + // no LocalAlloc size (reconnect passes such a buffer), so this is a normal outcome, not a failure. + if (!cbLogon) + { + MsRdpEx_LogPrint(DEBUG, "KerbCertificateLogon(%s): CspData not parsed, allocation size unknown", label); + return; + } + cspInfo = (MsRdpEx_KerbSmartCardCspInfo*) sspi_ResolvePackedField(pLogon, cbLogon, logon->CspData, logon->CspDataLength); @@ -1283,20 +1280,6 @@ static bool sspi_IsCertificateMarshaledUserName(WCHAR* userName) return result; } -// Repoint a packed credential at the smart card KSP by overwriting, in place, the provider name that -// CredPackAuthenticationBufferW copied from the certificate. -// -// The Base Smart Card Crypto Provider is the CAPI front end over a card minidriver, so a certificate that -// records it is by definition on a minidriver card -- and every minidriver card is also reachable through the -// Smart Card KSP, under the same container name. KB5066793 stopped honouring the CAPI route for RSA smart card -// keys, so naming the KSP reaches the same key over the path that remains supported. No probe of the card is -// needed to know the route exists, which matters: touching the card from this code path (NCrypt -> KSP -> -// minidriver -> WinSCard) faults inside the smart card stack while the RDP client is mid-connect. -// -// Patched in place rather than rebuilt, so the buffer keeps the byte layout Windows produced. A hand-built -// equivalent carrying identical field values but a different internal layout is rejected by LSASS, which -// evidently derives the CspData extent from the buffer rather than trusting the offsets alone. The two -// provider names are both 41 characters, so the strings after this one do not move. static bool sspi_CreatePackedCertificateLogon(WCHAR* marshaledCertificateUserName, WCHAR* pin, HLOCAL* phPackedCredentials, DWORD* pcbPackedCredentials) {