[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH 1/2] packet: Do not decrypt zero length rest of buffer
[Thread Prev] | [Thread Next]
- Subject: [PATCH 1/2] packet: Do not decrypt zero length rest of buffer
- From: Alan Dunn <amdunn@xxxxxxxxx>
- Reply-to: libssh@xxxxxxxxxx
- Date: Wed, 5 Feb 2014 20:14:06 -0600
- To: libssh@xxxxxxxxxx
- Cc: Alan Dunn <amdunn@xxxxxxxxx>
If we receive a packet of length exactly blocksize, then packet_decrypt gets called on a buffer of size 0. The check at the beginning of packet_decrypt indicates that the function should be called on buffers of at least one blocksize, though the check allows through zero length. As is packet_decrypt can return -1 when len is 0 because malloc can return NULL in this case: according to the ISO C standard, malloc is free to return NULL or a pointer that can be freed when size == 0, and uclibc by default will return NULL here (in "non-glibc-compatible" mode). The net result is that when using uclibc connections with libssh can anomalously fail. Alternatively, packet_decrypt (and probably packet_encrypt for consistency) could be made to always succeed on len == 0 without depending on the behavior of malloc. Thanks to Josh Berlin for bringing conneciton failures with uclibc to my attention. Signed-off-by: Alan Dunn <amdunn@xxxxxxxxx> --- src/packet.c | 16 ++++++++++------ src/packet1.c | 5 ++++- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/packet.c b/src/packet.c index 87b9100..7f75b44 100644 --- a/src/packet.c +++ b/src/packet.c @@ -251,12 +251,16 @@ int ssh_packet_socket_callback(const void *data, size_t receivedlen, void *user) * Decrypt the rest of the packet (blocksize bytes already * have been decrypted) */ - rc = packet_decrypt(session, - ((uint8_t*)buffer_get_rest(session->in_buffer) + blocksize), - buffer_get_rest_len(session->in_buffer) - blocksize); - if (rc < 0) { - ssh_set_error(session, SSH_FATAL, "Decrypt error"); - goto error; + + /* The following check avoids decrypting zero bytes */ + if (buffer_get_rest_len(session->in_buffer) != blocksize) { + rc = packet_decrypt(session, + ((uint8_t*)buffer_get_rest(session->in_buffer) + blocksize), + buffer_get_rest_len(session->in_buffer) - blocksize); + if (rc < 0) { + ssh_set_error(session, SSH_FATAL, "Decrypt error"); + goto error; + } } /* copy the last part from the incoming buffer */ diff --git a/src/packet1.c b/src/packet1.c index ec72f16..4728b4b 100644 --- a/src/packet1.c +++ b/src/packet1.c @@ -168,7 +168,8 @@ int ssh_packet_socket_callback1(const void *data, size_t receivedlen, void *user * We decrypt everything, missing the lenght part (which was * previously read, unencrypted, and is not part of the buffer */ - if (packet_decrypt(session, + if (ssh_buffer_get_len(session->in_buffer) > 0 && + packet_decrypt(session, ssh_buffer_get_begin(session->in_buffer), ssh_buffer_get_len(session->in_buffer)) < 0) { ssh_set_error(session, SSH_FATAL, "Packet decrypt error"); @@ -300,6 +301,8 @@ int packet_send1(ssh_session session) { ssh_buffer_get_len(session->out_buffer)); #endif + /* session->out_buffer should have more than sizeof(uint32_t) bytes + in it as required for packet_encrypt */ packet_encrypt(session, (unsigned char *)ssh_buffer_get_begin(session->out_buffer) + sizeof(uint32_t), ssh_buffer_get_len(session->out_buffer) - sizeof(uint32_t)); -- 1.7.9.5
Re: [PATCH 1/2] packet: Do not decrypt zero length rest of buffer | Andreas Schneider <asn@xxxxxxxxxxxxxx> |
[PATCH 0/2] Fix connection success dependency on malloc behavior | Alan Dunn <amdunn@xxxxxxxxx> |