[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