fix(pageant): include NUL terminator in WM_COPYDATA payload - #747
Closed
litcc wants to merge 1 commit into
Closed
Conversation
Pageant expects the mapping name sent through WM_COPYDATA to be an ASCIZ string and validates the final byte of cbData. The previous code used CString::as_bytes(), so cbData omitted the terminating NUL even though the pointer referenced a NUL-terminated buffer. Pageant therefore rejected the request and the client observed an early EOF. Use as_bytes_with_nul() for both the payload length and pointer.
Author
Owner
|
Thank you! I've decided to go with #624 |
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 does this PR do?
Fixes the legacy WM_COPYDATA Pageant transport by including the
terminating NUL byte in
COPYDATASTRUCT.cbData.Background
This issue was discovered while developing and testing the SSH-agent
forwarding feature in the NyaTerm project.
On Windows, NyaTerm exhibited the following symptoms when authenticating
through Pageant:
Pageant was running with a valid software key loaded. The failure
occurred on a local, non-domain Windows account where
GetUserNameExA(NameUserPrincipal)could not resolve the user principalname. The russh Pageant client then fell back from the named-pipe
transport to the legacy WM_COPYDATA transport.
The same failure was reproduced by NyaTerm's Windows Pageant integration
test.
Root cause
The Pageant mapping name is created as a
CString, so the allocatedbuffer is NUL-terminated. However, the previous implementation used
CString::as_bytes()when populatingCOPYDATASTRUCT:as_bytes()excludes the terminating NUL from the reported length.Therefore, although
lpDatapoints to a NUL-terminated buffer,cbDatadoes not include that byte.The PuTTY Pageant implementation explicitly validates that the data
described by
COPYDATASTRUCTis an ASCIZ string and rejects the requestwhen the final byte is not NUL-terminated:
PuTTY Pageant implementation
The rejected request causes the client-side stream to observe an early
EOF.
Fix
Use
CString::as_bytes_with_nul()for both the payload length andpointer:
This ensures that
cbDataincludes the terminating NUL required by thePageant WM_COPYDATA protocol.
The transport selection, memory mapping, and request flow remain
unchanged.
Validation
The fix was validated on Windows with Pageant running and a software
Ed25519 key loaded.
The NyaTerm Pageant integration test was run with:
The test passed after changing only the WM_COPYDATA payload handling:
The named-pipe implementation was left unchanged. This confirms that
the failure was isolated to the WM_COPYDATA path.
Additional checks:
Scope
This is a minimal WM_COPYDATA protocol-correctness fix.
It is independent of
#726, which addresses
username fallback for the Windows named-pipe transport.
No named-pipe behavior is changed by this PR.