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

[Pieter Hintjens]

Hi Laurent,

I'm looking at this and wondering why valgrind does not catch the
error. Thanks for the help. I'll try to get a testable fix for you
asap.

-Pieter

On Mon, Aug 19, 2013 at 12:47 PM, Laurent Alebarde <l.alebarde at free.fr> wrote:
> Sorry Pieter, I made a mistake. My test (below) does not pass. I have to dig
> more.
>
>
> Le 19/08/2013 12:24, Laurent Alebarde a écrit :
>
> 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
>
>
>
>
> _______________________________________________
> 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
>