[SSSD] [PATCH] NSS: Fix service enumeration
Jakub Hrozek
jhrozek at redhat.com
Tue Oct 29 15:44:43 UTC 2013
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 at 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.
-------------- next part --------------
>From ab86dc24e9d5b5594e790cf4def23121b27d7f0e Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Tue, 29 Oct 2013 07:10:30 +0100
Subject: [PATCH 1/2] 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 | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/responder/nss/nsssrv_services.c b/src/responder/nss/nsssrv_services.c
index 79caa7d08cdfff25112eedab98a0419f1b1d154e..471cf31daba15258b9e0cc230cd2558c53864763 100644
--- a/src/responder/nss/nsssrv_services.c
+++ b/src/responder/nss/nsssrv_services.c
@@ -752,7 +752,11 @@ fill_service(struct sss_packet *packet,
written_aliases++;
talloc_zfree(tmpstr);
}
- SAFEALIGN_SET_UINT32(&body[aptr], written_aliases, &rsize);
+
+ /* We must not advance rsize here, the data has already been
+ * allocated and skipped earlier when aptr was assigned to.
+ */
+ SAFEALIGN_SET_UINT32(&body[aptr], written_aliases, NULL);
num++;
}
--
1.8.3.1
-------------- next part --------------
>From e137eaf703c1e5ca89d6523d3aa51e5e7a3448a1 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Tue, 29 Oct 2013 16:41:13 +0100
Subject: [PATCH 2/2] NSS: Use new safealign macros in NSS responder
---
src/responder/nss/nsssrv_services.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/responder/nss/nsssrv_services.c b/src/responder/nss/nsssrv_services.c
index 471cf31daba15258b9e0cc230cd2558c53864763..52a2f440d4c6d7de4403de0ebf6987e17ffd2c3e 100644
--- a/src/responder/nss/nsssrv_services.c
+++ b/src/responder/nss/nsssrv_services.c
@@ -700,7 +700,7 @@ fill_service(struct sss_packet *packet,
sss_packet_get_body(packet, &body, &blen);
/* Store the port number */
- SAFEALIGN_SET_UINT32(&body[rzero + rsize], (uint32_t)htons(port), &rsize);
+ SAFEALIGN_SETMEM_UINT32(&body[rzero + rsize], (uint32_t)htons(port), &rsize);
/* Get the aliases */
el = ldb_msg_find_element(msg, SYSDB_NAME_ALIAS);
@@ -756,7 +756,7 @@ fill_service(struct sss_packet *packet,
/* We must not advance rsize here, the data has already been
* allocated and skipped earlier when aptr was assigned to.
*/
- SAFEALIGN_SET_UINT32(&body[aptr], written_aliases, NULL);
+ SAFEALIGN_SETMEM_UINT32(&body[aptr], written_aliases, NULL);
num++;
}
--
1.8.3.1
More information about the sssd-devel
mailing list