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

Recv and send all bytes via network #510

Merged
merged 1 commit into from
Sep 23, 2020
Merged

Conversation

cerevra
Copy link
Contributor

@cerevra cerevra commented Aug 29, 2020

Describe the bug
libssh2 functions return error LIBSSH2_ERROR_AGENT_PROTOCOL for valid client-server protocol.
Reason: posix functions recv()/send() accept buffer_start_pointer and buffer_len. Theese functions return succeed len which could be less then buffer_len. libssh2 doesn't cover this case ( 1, 2 ): so only part of message could be finished.

It leads to operations with garbage at the end of buffer on recv().
For example in agent_list_identities() in my project:

=== agent_list_identities got len: 689
=== agent_list_identities got *s: 12
=== agent_list_identities got num_identities: 2
=== agent_list_identities === next identity
=== agent_list_identities got blob_len: 277
=== agent_list_identities got comment_len: 35
=== agent_list_identities === next identity
=== agent_list_identities got blob_len: 279
=== agent_list_identities got comment_len: 3486502863
=== agent_list_identities goto error #7: -42

To Reproduce
I can't provide real steps in client: it depends on network quality and server side.

Expected behavior
Functions should return success code if client-server protocol is correct.

Version (please complete the following information):

  • OS: linux
  • libssh2 version 1.8.0

Additional context
Duplicate: #511

@cerevra
Copy link
Contributor Author

cerevra commented Aug 29, 2020

Sorry for several issues: I'm not very experienced in github

@cerevra cerevra mentioned this pull request Sep 1, 2020
@cerevra
Copy link
Contributor Author

cerevra commented Sep 1, 2020

Test in broken in master: #513

@cerevra
Copy link
Contributor Author

cerevra commented Sep 1, 2020

@bagder
@mback2k
Could you please take a look at this issue? How can I move it forward?

@mback2k
Copy link
Member

mback2k commented Sep 1, 2020

I like the proposed solution, but I am wondering why this issue is coming up now. Isn't it the callers responsibility to call the API until all data is available? @bagder @willco007

@willco007
Copy link
Member

@mback2k I was just thinking the same thing. The solution seems good, but it seems odd this would be coming up now.

@pkittenis
Copy link
Contributor

Indeed clients are currently handling this, and this code is also in examples.

@willco007
Copy link
Member

Looking at the code further, I think @pkittenis is right. All the places in sftp.c I looked at all take into account the possibility of a partial reply.

@cerevra
Copy link
Contributor Author

cerevra commented Sep 3, 2020

OK, let's fix only definitely broken function =). agent_transact_unix() encupsulates operation with network and doesn't check length - i.e. result of the first recv/send call. agent_transact_unix() is used only in internal functions agent_list_identities() and agent_sign().
The only solution I see is to handle case with incomplete buffer inside of agent_transact_unix().

As far as I see length is checked for other usages of LIBSSH2_SEND().

Could I ask you to review next iteration of this patch?
@bagder @mback2k @willco007 @pkittenis

@cerevra
Copy link
Contributor Author

cerevra commented Sep 8, 2020

Are there any comments for patch? Could I provide any help here?

@cerevra
Copy link
Contributor Author

cerevra commented Sep 14, 2020

Could you please take a look at this issue?
@bagder @mback2k @willco007 @pkittenis

@cerevra
Copy link
Contributor Author

cerevra commented Sep 22, 2020

Could I provide any help to move forward this issue?

@torkve
Copy link

torkve commented Sep 23, 2020

Hi all!

I have client based on libssh2, that suffers from this very issue since my ssh-agent yields chunked data.
This patch appears to fix it. Could you merge it or mark if any problems exist, please?

@willco007 willco007 merged commit 9ae9ff3 into libssh2:master Sep 23, 2020
@cerevra cerevra deleted the fix branch September 24, 2020 07:32
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.

None yet

5 participants