On Tue, Oct 29, 2013 at 05:14:17PM +0100, Lukas Slebodnik wrote:
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
Thank you for the review, both patches were pushed to master and
patch #1 was pushed to sssd-1-11