Re: [PATCH 1/2] packet: Do not decrypt zero length rest of buffer

[Andreas Schneider <asn@xxxxxxxxxxxxxx>]

On Wednesday 05 February 2014 20:14:06 Alan Dunn wrote:
> 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.

Thanks for your patch, it looks fine to me. I would like to request some 
cosmetic changes.
 
> 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) {

len = buffer_get_rest_len(session->in_buffer);
if (len != blocksize) {
}

So if you debug it with gdb you can do:

gdb> print len


:)

> +                    rc = packet_decrypt(session,
> +                                       
> ((uint8_t*)buffer_get_rest(session->in_buffer) + blocksize), +             
>                           buffer_get_rest_len(session->in_buffer) -
> blocksize); +                    if (rc < 0) {

Then use len here too, avoid a function call...

> +                        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 &&

Here too:

len = ssh_buffer_get_len(session->in_buffer);

> +            packet_decrypt(session,
>                ssh_buffer_get_begin(session->in_buffer),
>                ssh_buffer_get_len(session->in_buffer)) < 0) {

rc = packet_decrypt()
if (rc < 0) {
}


Simply easier to debug ...


>            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));


Thanks for your contribution.




	-- andreas

-- 
Andreas Schneider                   GPG-ID: CC014E3D
www.cryptomilk.org                asn@xxxxxxxxxxxxxx