bug in master src/lib/libc/hash/siphash.c

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

bug in master src/lib/libc/hash/siphash.c

Mark Karpilovskij
Dear OpenBSD developers,

I am writing to briefly inform you of a small bug that we have
encountered in your implementation of SipHash as we utilized your code
in our nameserver Knot DNS (thank you for that!).

Specifically the issue is that the line src/lib/libc/hash/siphash.c:107
reads

memcpy(&ctx->buf[used], ptr, len);

while it should read

memcpy(&ctx->buf, ptr, len);

The reasoning behind this change in the function SipHash_Update is as
follows: the data passed to the SipHash_Update function in ptr are
partitioned to chunks of size sizeof(ctx->buf) and processed. If there
are less than sizeof(ctx->buf) bytes left at the end, this remainder is
just copied to the buffer and not processed, which is what the mentioned
memcpy does. However, after processing all the previous blocks, the
buffer is empty and so the write should start the beginning of ctx->buf.
Instead the write starts at ctx->buf + used, where used is the number of
leftover bytes from the previous update, which were however already
processed on lines 83-95 and after that the content of the variable used
is no longer relevant and should not be used.

If only a single call to SipHash_Update is performed or if the size of
processed data is a multiple of sizeof(ctx->buf), this bug does nothing.
However when we performed several updates of various lengths, the data
written to ctx->buf were too long and the call to memcpy overwrote other
data, which led to various unexpected behavior.

Thank you for reading this report, hopefully you will find it useful.


Best regards,

Mark Karpilovskij, developer at CZ.NIC labs

https://www.knot-dns.cz/



Reply | Threaded
Open this post in threaded view
|

Re: bug in master src/lib/libc/hash/siphash.c

Ted Unangst-6
Mark Karpilovskij wrote:
> If only a single call to SipHash_Update is performed or if the size of
> processed data is a multiple of sizeof(ctx->buf), this bug does nothing.
> However when we performed several updates of various lengths, the data
> written to ctx->buf were too long and the call to memcpy overwrote other
> data, which led to various unexpected behavior.

I concur. Here's a diff for review.

Index: siphash.c
===================================================================
RCS file: /cvs/src/lib/libc/hash/siphash.c,v
retrieving revision 1.6
diff -u -p -r1.6 siphash.c
--- siphash.c 12 Apr 2017 17:41:49 -0000 1.6
+++ siphash.c 22 Dec 2017 01:13:59 -0000
@@ -104,7 +104,7 @@ SipHash_Update(SIPHASH_CTX *ctx, int rc,
  }
 
  if (len > 0)
- memcpy(&ctx->buf[used], ptr, len);
+ memcpy(ctx->buf, ptr, len);
 }
 DEF_WEAK(SipHash_Update);
 

Reply | Threaded
Open this post in threaded view
|

Re: bug in master src/lib/libc/hash/siphash.c

Todd C. Miller-2
On Thu, 21 Dec 2017 20:15:00 -0500, "Ted Unangst" wrote:

> Mark Karpilovskij wrote:
> > If only a single call to SipHash_Update is performed or if the size of
> > processed data is a multiple of sizeof(ctx->buf), this bug does nothing.
> > However when we performed several updates of various lengths, the data
> > written to ctx->buf were too long and the call to memcpy overwrote other
> > data, which led to various unexpected behavior.
>
> I concur. Here's a diff for review.

Looks correct since len is the remainder.  OK millert@

 - todd