On (29/10/13 16:44), Jakub Hrozek wrote:
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.
From ab86dc24e9d5b5594e790cf4def23121b27d7f0e 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 1/2] NSS: Fix service enumeration
I tested patch with master an sssd-1-9 branch.
ACK
From e137eaf703c1e5ca89d6523d3aa51e5e7a3448a1 Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Tue, 29 Oct 2013 16:41:13 +0100
Subject: [PATCH 2/2] NSS: Use new safealign macros in NSS responder
ACK
LS