[zeromq-dev] curvezmq code review : possible memory violation in s_produce_hello - s_encrypt - memcpy

[Laurent Alebarde]

Pieter,

I am far from having analysed all the CurveZMQ library, but the 
following change passes the tests :

src/cusrc/curvezmq_codec.c - s_encrypt :

Before :
     size_t box_size = crypto_box_ZEROBYTES + size;
     byte *plain = malloc (box_size);
     byte *box = malloc (box_size);
.....
     memcpy (target, box + crypto_box_BOXZEROBYTES, size + 
crypto_box_BOXZEROBYTES);

After :
     size_t plain_size = crypto_box_ZEROBYTES + size;
     byte *plain = malloc (plain_size);
     size_t box_size = crypto_box_BOXZEROBYTES + size;
     byte *box = malloc (box_size);
.....
     memcpy (target, box + crypto_box_BOXZEROBYTES, size);



Le 18/08/2013 10:07, Pieter Hintjens a écrit :
> Thanks for this analysis. I'll look at it when I have an hour or two;
> it's too tricky to fix in passing.
>
> On Thu, Aug 15, 2013 at 12:30 PM, Laurent Alebarde <l.alebarde at free.fr> wrote:
>> Hi all,
>>
>> I am reviewing the curvezmq code with a debugger :
>>
>> Fact :
>> in curvezmq_codec.c (s_encrypt from s_produce_hello, line 288) :
>> memcpy (target, box + crypto_box_BOXZEROBYTES, size +
>> crypto_box_BOXZEROBYTES);
>> copies from (box + crypto_box_BOXZEROBYTES) (size + crypto_box_BOXZEROBYTES)
>> bytes
>> Debugger aborts (while the executable itself in a shell works well).
>>
>> Consequences :
>>      1) an amount of (crypto_box_BOXZEROBYTES) undefined bytes after the box
>> are copied to target, while the box is malloc-ed with a size of
>> (crypto_box_ZEROBYTES + size)
>>      2) the spec "target must be nonce + box" is not fullfilled since the
>> first crypto_box_BOXZEROBYTES bytes of the box are not copied to target (may
>> be ok since they are zeros)
>>
>> Possible solutions :
>>      1) change to : memcpy (target, box + crypto_box_BOXZEROBYTES, size);
>>          and set appropriatly the remaining (crypto_box_BOXZEROBYTES) bytes
>> of target
>>      2) change to : memcpy (target, box, size + crypto_box_BOXZEROBYTES);
>>          and adapt the size of target : hello_t.box becomes : bytes box[96]
>>      3) change to : memcpy (target, box + crypto_box_BOXZEROBYTES, size);
>>          and adapt the size of target : hello_t.box becomes : bytes box[64]
>>
>> In addition : box should be malloc-ed with a size of
>> (crypto_box_BOXZEROBYTES + size) instead of (crypto_box_ZEROBYTES + size)
>>
>> Cheers,
>>
>>
>> Laurent.
>>
>> _______________________________________________
>> zeromq-dev mailing list
>> zeromq-dev at lists.zeromq.org
>> http://lists.zeromq.org/mailman/listinfo/zeromq-dev
>>
> _______________________________________________
> zeromq-dev mailing list
> zeromq-dev at lists.zeromq.org
> http://lists.zeromq.org/mailman/listinfo/zeromq-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.zeromq.org/pipermail/zeromq-dev/attachments/20130819/b353f564/attachment.htm>