Re: [PATCH v2 2/2] packet_crypt: Make packet_{en,de}crypt fail consistently on len == 0

[Alan Dunn <amdunn@xxxxxxxxx>]

Doesn't assert only take action when NDEBUG is undefined?  That means
if this error occurs in a release version, where I believe cmake will
define NDEBUG, then without the check we might crash with the
potentially difficult to understand "Decrypt error" (which was
actually how this whole thing started for me).  Unless we want some
other version of assert.

On Thu, Feb 6, 2014 at 10:19 AM, Andreas Schneider <asn@xxxxxxxxxxxxxx> wrote:
> On Thursday 06 February 2014 08:12:47 Alan Dunn wrote:
>> Right now the behavior of packet_{en,de}crypt on len == 0 depends on
>> the behavior of malloc.  Instead, make these consistently fail based
>> on what I assume the desired behavior is due to the first error
>> message in each.
>>
>> Signed-off-by: Alan Dunn <amdunn@xxxxxxxxx>
>> ---
>>  src/packet_crypt.c |   13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/packet_crypt.c b/src/packet_crypt.c
>> index 50b8189..a12e3d9 100644
>> --- a/src/packet_crypt.c
>> +++ b/src/packet_crypt.c
>> @@ -22,6 +22,7 @@
>>   */
>>
>>  #include "config.h"
>> +#include <assert.h>
>>  #include <stdlib.h>
>>  #include <stdio.h>
>>  #include <string.h>
>> @@ -59,7 +60,11 @@ uint32_t packet_decrypt_len(ssh_session session, char
>> *crypted){ int packet_decrypt(ssh_session session, void *data,uint32_t len)
>> { struct ssh_cipher_struct *crypto = session->current_crypto->in_cipher;
>> char *out = NULL;
>> -  if(len % session->current_crypto->in_cipher->blocksize != 0){
>> +
>> +  assert(len);
>> +
>> +  if(len == 0 ||
>
> You can remove this check, assert() calls abort() in case it is 0 and that's
> what we really want :)
>
> We want to crash if len is 0. :)
>
>> +     len % session->current_crypto->in_cipher->blocksize != 0){
>>      ssh_set_error(session, SSH_FATAL, "Cryptographic functions must be set
>> on at least one blocksize (received %d)",len); return SSH_ERROR;
>>    }
>> @@ -89,10 +94,14 @@ unsigned char *packet_encrypt(ssh_session session, void
>> *data, uint32_t len) { unsigned int finallen;
>>    uint32_t seq;
>>
>> +  assert(len);
>> +
>>    if (!session->current_crypto) {
>>      return NULL; /* nothing to do here */
>>    }
>> -  if(len % session->current_crypto->in_cipher->blocksize != 0){
>> +
>> +  if(len == 0 ||
>> +     len % session->current_crypto->in_cipher->blocksize != 0){
>>        ssh_set_error(session, SSH_FATAL, "Cryptographic functions must be
>> set on at least one blocksize (received %d)",len); return NULL;
>>    }
>
> --
> Andreas Schneider                   GPG-ID: CC014E3D
> www.cryptomilk.org                asn@xxxxxxxxxxxxxx
>
>