fix(sspi): preserve the caller's CREDSSP_CRED when synthesizing a certificate logon - #177
Merged
Marc-André Moreau (mamoreau-devolutions) merged 1 commit intoAug 7, 2026
Conversation
…tificate logon Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot started reviewing on behalf of
Richard Markiewicz (thenextman)
August 6, 2026 22:59
View session
There was a problem hiding this comment.
Pull request overview
Fixes CredSSP certificate logon credential construction and improves multi-session correlation and diagnostics.
Changes:
- Preserves caller-provided CredSSP and Schannel credentials.
- Adds worker-thread session binding and stacked scope tracking.
- Expands smart-card and disconnect diagnostics.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
README.md |
Documents smart-card provider behavior. |
include/MsRdpEx/Sspi.h |
Exposes thread-session binding. |
include/MsRdpEx/RdpSettings.h |
Adds GUID session access. |
dll/Sspi.cpp |
Implements credential, correlation, and diagnostic changes. |
dll/RdpSettings.cpp |
Binds property-reading workers to sessions. |
dll/MsRdpClient.cpp |
Logs disconnect information. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return false; | ||
| } | ||
|
|
||
| sspi_LogKerbCertificateLogon("credpack", hPackedCredentials, cbPackedCredentials); |
Comment on lines
+1144
to
+1145
| sspi_LogKerbCertificateLogon("incoming", pCred->pSpnegoCred, | ||
| sspi_GetLocalAllocSizeAsDword(pCred->pSpnegoCred)); |
|
|
||
| if (pAuthData) | ||
| sspi_LogCredSspAuthMetadata(pszPackageA, pAuthData, returnAddress); | ||
| sspi_LogCredSspAuthMetadata(pszPackageA, pAuthData, returnAddress); |
Comment on lines
+1603
to
+1606
| // The environment-backed switches below are read here rather than left to MsRdpEx_GetPcapFile, which only | ||
| // runs once messages start flowing — that is after this call, so the first connection in a process would | ||
| // otherwise see them unset. | ||
| MsRdpEx_PcapEnvInit(); |
Marc-André Moreau (mamoreau-devolutions)
merged commit Aug 7, 2026
b1bd670
into
master
1 check passed
Richard Markiewicz (thenextman)
added a commit
that referenced
this pull request
Aug 7, 2026
#178) Follow-up to #177, addressing review comments. ## Stale documentation for a rewrite that no longer exists #177 removed the Base CSP → KSP provider rewrite, but left behind a README section and a doc comment above `sspi_CreatePackedCertificateLogon` that still describe it. Both are removed here. To be explicit, since the review suggested implementing the rewrite instead: that would regress the fix. The credential that works names `Microsoft Base Smart Card Crypto Provider` with `KeySpec = 1` — the legacy CAPI provider — and the KSP retarget was in the arm that failed during testing. The claim was wrong, not the code. The README text is replaced with something accurate and more useful: the credential is passed through as `CredPackAuthenticationBufferW` produced it, and the provider and container names it records are resolved by the server through the redirected reader — so the same smart card middleware must be installed on both ends. A mismatch surfaces as *"the key container does not exist on the smart card"*, which is a real failure mode and easy to misattribute to the client. ## Dead code `sspi_TryGetCspInfo` lost its last caller when the rewrite was removed and is deleted. ## Bounded credential dump `sspi_GetLocalAllocSizeAsDword` returns 0 when the incoming `pSpnegoCred` is not a `LocalAlloc` block, which is common — a reconnect passes such a buffer. Zero disables the extent check in `sspi_ResolvePackedField`, so walking the blob would then trust `CspDataLength` alone and could read past the credential. `MsRdpEx_CanReadUnsafePtr` still guarded every access, so this could not fault or touch unmapped memory; the exposure was logging bytes beyond the allocation. Now the fixed-size header is still reported and only the `CspData` walk is skipped when the size is unknown, which keeps the diagnostic useful in exactly the reconnect case that produces it. ## Thread-safe environment initialisation `MsRdpEx_PcapEnvInit` guarded initialisation with a plain `bool` while writing shared globals, including a `strcpy_s` into `g_PcapFilePath`. It is reached from `AcquireCredentialsHandleW` and from the message paths, both of which run concurrently on several connection worker threads. This race predates #177 — the message-path caller is older and hotter — and the written values are idempotent, so impact was minimal. It is now `InitOnceExecuteOnce`, matching the two existing uses in the same file. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
sspi_TryBuildCertificateLogonFromSettingsbuilt aCREDSSP_CREDfrom zeroed memory:Type = CredsspCertificateCreds,pSchannelCred = NULL. The CredSSP handshake then completes withSEC_E_OK, but the server never attempts a logon: the DC logs nothing at all; and the connection is refused as "the user is notauthorized for remote logon". That message is misleading: it isn't a permissions problem, there is simply no authenticated user to authorize.
The fix is to keep the caller's
CREDSSP_CRED(notablypSchannelCred, and theCredExwrapper) and swap onlypSpnegoCred, withType = CredsspSubmitBufferBoth. That is the shape mstscax produces for its own certificate logon via CredUI.sspi_TryBuildCertificateLogonRewritecarried the same defect and is fixed the same way. It is currently unreachable: the settings builder is tried first; but would have failed identically.What we know vs. what we assume
Verified by A/B on a real smart card, one variable at a time:
TypepSchannelCredCredsspCertificateCredsCredsspCertificateCredsSEC_E_INVALID_TOKENCredsspSubmitBufferBothBoth halves are required; neither alone is sufficient. Confirmed end to end by a successful PKINIT on the DC (event 4768, pre-auth type 16, matching certificate thumbprint) where previously the DC saw no events whatsoever.
Assumed, not proven: that this also resolves the customer reports that prompted the work. Their symptom was "NTLM authentication has been disabled" rather than "not authorized". A credential yielding no usable Kerberos identity plausibly falls back to NTLM, which fails loudly where NTLM is disabled and quietly where it isn't — but that bridge is unproven, and the reporting environment's middleware was never reproduced here. The defect being fixed is unconditional and middleware-independent, so anyone who enabled the feature hit it; whether it was their only problem is open.
Also included
Connectscope is established on the UI thread, so resolution always fell through to a "single active kerb session" guess that ignored how many other sessions were active. With several tabs open, an unrelated connection could be handed the smart card credential. The worker thread now binds itself in-band when it reads its ownUserName/ServerNameUsedForAuthentication, and the guess additionally requires that it be the only active session before answering.MsRdpEx_GetPcapFile, which runs after credential acquisition, so the first connection in a process could see them unset.Diagnostics
Deliberately enabled whenever
KerbCertificateLogonis on, with no second opt-in — a customer log should be useful on first capture. All of it is secret-free: the PIN appears as a length only, and the credential carries a container name rather than the certificate.Covers the credential shape the host supplied, the full
CspData(container / CSP / card / reader /KeySpec) for both the incoming and synthesized credentials, why synthesis was skipped when it was, which session-resolution strategy was used, andServer authentication FAILEDatWARN.KERB_SMARTCARD_CSP_INFOis absent from the SDK, so its layout is asserted at compile time and re-checked at runtime againstCspDataLength.Known limitations
CspDatais minidriver-specific, and the server resolves it through the redirected reader. A client with vendor middleware against a server without it fails with "the key container does not exist on the smart card". Nothing here changes that; it is worth checking before blaming the client.BeginSspiSessionScopereturns early unlessMsRdpEx_UsePrivateAxLayout(), so no session is egistered and thread binding cannot resolve. Pre-existing, unchanged here.pAuthData == NULLpath is untested. It falls back to the previous certificate-only shape and now logs aWARN; every observed caller supplies one.CspDatawere tried and made no difference; that code was dropped rather than kept on speculation.