On Tue, Oct 29, 2013 at 03:24:18PM +0100, Lukas Slebodnik wrote:
On (29/10/13 10:05), Jakub Hrozek wrote:
>The code wrote into the middle of the packet to a space that was already
>reserved and allocated but then still advanced the pointer to the
>buffer.
>
>https://fedorahosted.org/sssd/ticket/2124
>From d533bb267ffb1447503f6af671a6d2d2db9ddd10 Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek(a)redhat.com>
>Date: Tue, 29 Oct 2013 07:10:30 +0100
>Subject: [PATCH] NSS: Fix service enumeration
>
>The code wrote into the middle of the packet to a space that was already
>reserved and allocated but then still advanced the pointer to the buffer.
>
>https://fedorahosted.org/sssd/ticket/2124
>---
> src/responder/nss/nsssrv_services.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/src/responder/nss/nsssrv_services.c
b/src/responder/nss/nsssrv_services.c
>index
79caa7d08cdfff25112eedab98a0419f1b1d154e..ff588592c814dc43e30f9e57ddab230efff15df2 100644
>--- a/src/responder/nss/nsssrv_services.c
>+++ b/src/responder/nss/nsssrv_services.c
>@@ -752,7 +752,7 @@ fill_service(struct sss_packet *packet,
> written_aliases++;
> talloc_zfree(tmpstr);
> }
>- SAFEALIGN_SET_UINT32(&body[aptr], written_aliases, &rsize);
>+ SAFEALIGN_SET_UINT32(&body[aptr], written_aliases, NULL);
>
> num++;
> }
>--
>1.8.3.1
>
Nice catch.
We have already reserved place for variable written_aliases.
Here is code:
-------------
ret = sss_packet_grow(packet, 2 * sizeof(uint16_t)
+ sizeof(uint32_t) // reserve place for variable written_aliases
+ cased_name.len
+ cased_proto.len);
sss_packet_get_body(packet, &body, &blen);
/* Store the port number */
SAFEALIGN_SET_UINT32(&body[rzero + rsize], (uint32_t)htons(port), &rsize);
... snip ...
/* We'll store the alias count here */
aptr = rzero+rsize; <<< remember offset for variable written_aliases
rsize += sizeof(uint32_t); <<< increment rsize
So it does not make sense to increment rsize twice and it was
a reason why we could access behind allocated array.
Please add comment before line, where variable is written to packet,
because it needn't be clear why rsize is not incremented.
I know that this patch will be backported to older branches.
In this case, do we want to use new macro SAFEALIGN_SETMEM_UINT32?
We can use it in master, but I would strongly prefer a separate patch so
that it's easily visible what changed.
I added the comment.