Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Extension encoded length #64

Closed
wants to merge 1 commit into from

Conversation

overhacked
Copy link
Collaborator

Extension is odd compared to other messages because its Unparsed payload does not include a length field. The implementation of Encode::encode for Extension accounts for this by writing the bytes directly from the Vec<u8> member of Unparsed, but the implementation of Encode::encoded_len for Extension did not account for the 4 extra bytes (a u32) that <Vec<u8> as Encode>::encode normally prepends.

Therefore, an Extension message on the wire claims it has 4 more bytes than its payload actually contains, which hangs most SSH agent server implementations waiting for the 4 additional bytes to arrive.

Extension is odd compared to other messages because its `Unparsed`
payload does not include a length field. The implementation of
`Encode::encode` for `Extension` accounts for this by writing the bytes
directly from the `Vec<u8>` member of `Unparsed`, but the implementation
of `Encode::encoded_len` for `Extension` did not account for the 4 extra
bytes (a `u32`) that `<Vec<u8> as Encode>::encode` normally prepends.

Therefore, an Extension message on the wire claims it has 4 more bytes
than its payload actually contains, which hangs most SSH agent server
implementations waiting for the 4 additional bytes to arrive.
@wiktor-k
Copy link
Owner

wiktor-k commented May 7, 2024

Thanks for the fix! 🎉

Would you be able to fix the sign-off? (it should be as easy as git commit --amend --signoff && git push --force-with-lease)

Now it occurred to me that we didn't have a test vector for an extension in our test cases so I need to add one really fast 😅

wiktor-k added a commit that referenced this pull request May 7, 2024
`Unparsed` contains raw bytes and as such the length is already stored
inside and doesn't require any additional wrapping.

Additionally fix the `Extension::encoded_len` implementation to use
`Unparsed::encoded_len` instead of directly accessing the `Vec`.

Co-authored-by: Ross Williams <[email protected]>
Fixes: #64
Signed-off-by: Wiktor Kwapisiewicz <[email protected]>
@wiktor-k
Copy link
Owner

wiktor-k commented May 7, 2024

@overhacked I've filed a different PR solving the same bug: https://github.com/wiktor-k/ssh-agent-lib/pull/66/files

The difference is that I've adjusted the encoded_len of the Unparsed (which is where I think the fix should be since it's Unparsed that has the special logic) and adjusted Extension::encoded_len to use that implementation (it should be doing that from the beginning).

Additionally I've added an extension binary message for tests and fixed the tests to test for correct values of encoded_len.

I hope you don't mind me taking the liberty of fixing it in a different way and appending your name to the commit (with Co-authored-by). If there are any concerns I'm all 👂s

👋

@overhacked
Copy link
Collaborator Author

Closing in favor of #66.

@overhacked overhacked closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants