[PATCH 0/4] Various nscd security fixes

[PATCH 0/4] Various nscd security fixes

[Florian Weimer]

Carlos filed bug 31677, and it it turns out that this is a reachable
stack-based buffer overflow.  The data looks quite attacker-controlled
to me and probably can contain NUL bytes with a custom client, so this
looks quite exploitable to my untrained eye.

Unfortunately, the reproducer kept crashing after the initial patch,
hence the second and third commit.  The two issues fixed in the last
commit were discovered by reading through the code.

By my count, this needs four different CVE identifiers:

  Bug 31677: the stack-based buffer overflow (commit 1)
  Bug 31678: two distinct null pointer dereferences (commit 2, commit 3)
    (same flaw type, presumably same version range, so MERGE from a
    CVE perspective)
  Bug 31679: process termination on malloc failure (commit 4)
  Byg 31680: memory corruption due to incorrect callback API assumption
    (commit 4)

Florian Weimer (4):
  nscd: Stack-based buffer overflow in netgroup cache (bug 31677)
  nscd: Do not send missing not-found response in addgetnetgrentX (bug
    31678)
  nscd: Avoid null pointer crashes after notfound response (bug 31678)
  nscd: netgroup: Use two buffers in addgetnetgrentX (bug 31680)

 nscd/netgroupcache.c | 247 +++++++++++++++++++++++--------------------
 1 file changed, 135 insertions(+), 112 deletions(-)


base-commit: f4724843ada64a51d66f65d3199fe431f9d4c254
-- 
2.44.0

[PATCH 1/4] nscd: Stack-based buffer overflow in netgroup cache (bug 31677)

[Florian Weimer]

Using alloca matches what other caches do.  The request length is
bounded by MAXKEYLEN.
---
 nscd/netgroupcache.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
index 0c6e46f15c..24fbac7668 100644
--- a/nscd/netgroupcache.c
+++ b/nscd/netgroupcache.c
@@ -502,12 +502,11 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
       = (struct indataset *) mempool_alloc (db,
 					    sizeof (*dataset) + req->key_len,
 					    1);
-  struct indataset dataset_mem;
   bool cacheable = true;
   if (__glibc_unlikely (dataset == NULL))
     {
       cacheable = false;
-      dataset = &dataset_mem;
+      dataset = alloca (sizeof (*dataset) + req->key_len);
     }
 
   datahead_init_pos (&dataset->head, sizeof (*dataset) + req->key_len,
-- 
2.44.0

[PATCH 2/4] nscd: Do not send missing not-found response in addgetnetgrentX (bug 31678)

[Florian Weimer]

If we failed to add a not-found response to the cache, the dataset
point can be null, resulting in a null pointer dereference.
---
 nscd/netgroupcache.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
index 24fbac7668..8709fb77b6 100644
--- a/nscd/netgroupcache.c
+++ b/nscd/netgroupcache.c
@@ -147,7 +147,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
       /* No such service.  */
       cacheable = do_notfound (db, fd, req, key, &dataset, &total, &timeout,
 			       &key_copy);
-      goto writeout;
+      goto maybe_cache_add;
     }
 
   memset (&data, '\0', sizeof (data));
@@ -348,7 +348,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
     {
       cacheable = do_notfound (db, fd, req, key, &dataset, &total, &timeout,
 			       &key_copy);
-      goto writeout;
+      goto maybe_cache_add;
     }
 
   total = buffilled;
@@ -410,14 +410,12 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
   }
 
   if (he == NULL && fd != -1)
-    {
-      /* We write the dataset before inserting it to the database
-	 since while inserting this thread might block and so would
-	 unnecessarily let the receiver wait.  */
-    writeout:
+    /* We write the dataset before inserting it to the database since
+       while inserting this thread might block and so would
+       unnecessarily let the receiver wait.  */
       writeall (fd, &dataset->resp, dataset->head.recsize);
-    }
 
+ maybe_cache_add:
   if (cacheable)
     {
       /* If necessary, we also propagate the data to disk.  */
-- 
2.44.0

[PATCH 3/4] nscd: Avoid null pointer crashes after notfound response (bug 31678)

[Florian Weimer]

The addgetnetgrentX call in addinnetgrX may have failed to produce
a result, so the result variable in addinnetgrX can be NULL.
Use db->negtimeout as the fallback value if there is no result data;
the timeout is also overwritten below.

Also avoid sending a second not-found response.  (The client
disconnects after receiving the first response, so the data stream did
not go out of sync even without this fix.)  It is still beneficial to
add the negative response to the mapping, so that the client can get
it from there in the future, instead of going through the socket.
---
 nscd/netgroupcache.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
index 8709fb77b6..8f9eb84e39 100644
--- a/nscd/netgroupcache.c
+++ b/nscd/netgroupcache.c
@@ -509,14 +509,15 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
 
   datahead_init_pos (&dataset->head, sizeof (*dataset) + req->key_len,
 		     sizeof (innetgroup_response_header),
-		     he == NULL ? 0 : dh->nreloads + 1, result->head.ttl);
+		     he == NULL ? 0 : dh->nreloads + 1,
+		     result == NULL ? db->negtimeout : result->head.ttl);
   /* Set the notfound status and timeout based on the result from
      getnetgrent.  */
-  dataset->head.notfound = result->head.notfound;
+  dataset->head.notfound = result == NULL || result->head.notfound;
   dataset->head.timeout = timeout;
 
   dataset->resp.version = NSCD_VERSION;
-  dataset->resp.found = result->resp.found;
+  dataset->resp.found = result != NULL && result->resp.found;
   /* Until we find a matching entry the result is 0.  */
   dataset->resp.result = 0;
 
@@ -564,7 +565,9 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
       goto out;
     }
 
-  if (he == NULL)
+  /* addgetnetgrentX may have already sent a notfound response.  Do
+     not send another one.  */
+  if (he == NULL && dataset->resp.found)
     {
       /* We write the dataset before inserting it to the database
 	 since while inserting this thread might block and so would
-- 
2.44.0

[PATCH 4/4] nscd: netgroup: Use two buffers in addgetnetgrentX (bug 31680)

[Florian Weimer]

This avoids potential memory corruption when the underlying NSS
callback function does not use the buffer space to store all strings
(e.g., for constant strings).

Instead of custom buffer management, two scratch buffers are used.
This increases stack usage somewhat.

Scratch buffer allocation failure is handled by return -1
(an invalid timeout value) instead of terminating the process.
This fixes bug 31679.
---
 nscd/netgroupcache.c | 219 ++++++++++++++++++++++++-------------------
 1 file changed, 121 insertions(+), 98 deletions(-)

diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
index 8f9eb84e39..8b5ebfaf31 100644
--- a/nscd/netgroupcache.c
+++ b/nscd/netgroupcache.c
@@ -23,6 +23,7 @@
 #include <stdlib.h>
 #include <unistd.h>
 #include <sys/mman.h>
+#include <scratch_buffer.h>
 
 #include "../nss/netgroup.h"
 #include "nscd.h"
@@ -65,6 +66,16 @@ struct dataset
   char strdata[0];
 };
 
+/* Send a notfound response to FD.  Always returns -1 to indicate an
+   ephemeral error.  */
+static time_t
+send_notfound (int fd)
+{
+  if (fd != -1)
+    TEMP_FAILURE_RETRY (send (fd, &notfound, sizeof (notfound), MSG_NOSIGNAL));
+  return -1;
+}
+
 /* Sends a notfound message and prepares a notfound dataset to write to the
    cache.  Returns true if there was enough memory to allocate the dataset and
    returns the dataset in DATASETP, total bytes to write in TOTALP and the
@@ -83,8 +94,7 @@ do_notfound (struct database_dyn *db, int fd, request_header *req,
   total = sizeof (notfound);
   timeout = time (NULL) + db->negtimeout;
 
-  if (fd != -1)
-    TEMP_FAILURE_RETRY (send (fd, &notfound, total, MSG_NOSIGNAL));
+  send_notfound (fd);
 
   dataset = mempool_alloc (db, sizeof (struct dataset) + req->key_len, 1);
   /* If we cannot permanently store the result, so be it.  */
@@ -109,11 +119,78 @@ do_notfound (struct database_dyn *db, int fd, request_header *req,
   return cacheable;
 }
 
+struct addgetnetgrentX_scratch
+{
+  /* This is the result that the caller should use.  It can be NULL,
+     point into buffer, or it can be in the cache.  */
+  struct dataset *dataset;
+
+  struct scratch_buffer buffer;
+
+  /* Used internally in addgetnetgrentX as a staging area.  */
+  struct scratch_buffer tmp;
+
+  /* Number of bytes in buffer that are actually used.  */
+  size_t buffer_used;
+};
+
+static void
+addgetnetgrentX_scratch_init (struct addgetnetgrentX_scratch *scratch)
+{
+  scratch->dataset = NULL;
+  scratch_buffer_init (&scratch->buffer);
+  scratch_buffer_init (&scratch->tmp);
+
+  /* Reserve space for the header.  */
+  scratch->buffer_used = sizeof (struct dataset);
+  static_assert (sizeof (struct dataset) < sizeof (scratch->tmp.__space),
+		 "initial buffer space");
+  memset (scratch->tmp.data, 0, sizeof (struct dataset));
+}
+
+static void
+addgetnetgrentX_scratch_free (struct addgetnetgrentX_scratch *scratch)
+{
+  scratch_buffer_free (&scratch->buffer);
+  scratch_buffer_free (&scratch->tmp);
+}
+
+/* Copy LENGTH bytes from S into SCRATCH.  Returns NULL if SCRATCH
+   could not be resized, otherwise a pointer to the copy.  */
+static char *
+addgetnetgrentX_append_n (struct addgetnetgrentX_scratch *scratch,
+			  const char *s, size_t length)
+{
+  while (true)
+    {
+      size_t remaining = scratch->buffer.length - scratch->buffer_used;
+      if (remaining >= length)
+	break;
+      if (!scratch_buffer_grow_preserve (&scratch->buffer))
+	return NULL;
+    }
+  char *copy = scratch->buffer.data + scratch->buffer_used;
+  memcpy (copy, s, length);
+  scratch->buffer_used += length;
+  return copy;
+}
+
+/* Copy S into SCRATCH, including its null terminator.  Returns false
+   if SCRATCH could not be resized.  */
+static bool
+addgetnetgrentX_append (struct addgetnetgrentX_scratch *scratch, const char *s)
+{
+  if (s == NULL)
+    s = "";
+  return addgetnetgrentX_append_n (scratch, s, strlen (s) + 1) != NULL;
+}
+
+/* Caller must initialize and free *SCRATCH.  If the return value is
+   negative, this function has sent a notfound response.  */
 static time_t
 addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
 		 const char *key, uid_t uid, struct hashentry *he,
-		 struct datahead *dh, struct dataset **resultp,
-		 void **tofreep)
+		 struct datahead *dh, struct addgetnetgrentX_scratch *scratch)
 {
   if (__glibc_unlikely (debug_level > 0))
     {
@@ -132,14 +209,10 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
 
   char *key_copy = NULL;
   struct __netgrent data;
-  size_t buflen = MAX (1024, sizeof (*dataset) + req->key_len);
-  size_t buffilled = sizeof (*dataset);
-  char *buffer = NULL;
   size_t nentries = 0;
   size_t group_len = strlen (key) + 1;
   struct name_list *first_needed
     = alloca (sizeof (struct name_list) + group_len);
-  *tofreep = NULL;
 
   if (netgroup_database == NULL
       && !__nss_database_get (nss_database_netgroup, &netgroup_database))
@@ -151,8 +224,6 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
     }
 
   memset (&data, '\0', sizeof (data));
-  buffer = xmalloc (buflen);
-  *tofreep = buffer;
   first_needed->next = first_needed;
   memcpy (first_needed->name, key, group_len);
   data.needed_groups = first_needed;
@@ -195,8 +266,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
 		while (1)
 		  {
 		    int e;
-		    status = getfct.f (&data, buffer + buffilled,
-				       buflen - buffilled - req->key_len, &e);
+		    status = getfct.f (&data, scratch->tmp.data,
+				       scratch->tmp.length, &e);
 		    if (status == NSS_STATUS_SUCCESS)
 		      {
 			if (data.type == triple_val)
@@ -204,68 +275,10 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
 			    const char *nhost = data.val.triple.host;
 			    const char *nuser = data.val.triple.user;
 			    const char *ndomain = data.val.triple.domain;
-
-			    size_t hostlen = strlen (nhost ?: "") + 1;
-			    size_t userlen = strlen (nuser ?: "") + 1;
-			    size_t domainlen = strlen (ndomain ?: "") + 1;
-
-			    if (nhost == NULL || nuser == NULL || ndomain == NULL
-				|| nhost > nuser || nuser > ndomain)
-			      {
-				const char *last = nhost;
-				if (last == NULL
-				    || (nuser != NULL && nuser > last))
-				  last = nuser;
-				if (last == NULL
-				    || (ndomain != NULL && ndomain > last))
-				  last = ndomain;
-
-				size_t bufused
-				  = (last == NULL
-				     ? buffilled
-				     : last + strlen (last) + 1 - buffer);
-
-				/* We have to make temporary copies.  */
-				size_t needed = hostlen + userlen + domainlen;
-
-				if (buflen - req->key_len - bufused < needed)
-				  {
-				    buflen += MAX (buflen, 2 * needed);
-				    /* Save offset in the old buffer.  We don't
-				       bother with the NULL check here since
-				       we'll do that later anyway.  */
-				    size_t nhostdiff = nhost - buffer;
-				    size_t nuserdiff = nuser - buffer;
-				    size_t ndomaindiff = ndomain - buffer;
-
-				    char *newbuf = xrealloc (buffer, buflen);
-				    /* Fix up the triplet pointers into the new
-				       buffer.  */
-				    nhost = (nhost ? newbuf + nhostdiff
-					     : NULL);
-				    nuser = (nuser ? newbuf + nuserdiff
-					     : NULL);
-				    ndomain = (ndomain ? newbuf + ndomaindiff
-					       : NULL);
-				    *tofreep = buffer = newbuf;
-				  }
-
-				nhost = memcpy (buffer + bufused,
-						nhost ?: "", hostlen);
-				nuser = memcpy ((char *) nhost + hostlen,
-						nuser ?: "", userlen);
-				ndomain = memcpy ((char *) nuser + userlen,
-						  ndomain ?: "", domainlen);
-			      }
-
-			    char *wp = buffer + buffilled;
-			    wp = memmove (wp, nhost ?: "", hostlen);
-			    wp += hostlen;
-			    wp = memmove (wp, nuser ?: "", userlen);
-			    wp += userlen;
-			    wp = memmove (wp, ndomain ?: "", domainlen);
-			    wp += domainlen;
-			    buffilled = wp - buffer;
+			    if (!(addgetnetgrentX_append (scratch, nhost)
+				  && addgetnetgrentX_append (scratch, nuser)
+				  && addgetnetgrentX_append (scratch, ndomain)))
+			      return send_notfound (fd);
 			    ++nentries;
 			  }
 			else
@@ -317,8 +330,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
 		      }
 		    else if (status == NSS_STATUS_TRYAGAIN && e == ERANGE)
 		      {
-			buflen *= 2;
-			*tofreep = buffer = xrealloc (buffer, buflen);
+			if (!scratch_buffer_grow (&scratch->tmp))
+			  return send_notfound (fd);
 		      }
 		    else if (status == NSS_STATUS_RETURN
 			     || status == NSS_STATUS_NOTFOUND
@@ -351,10 +364,17 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
       goto maybe_cache_add;
     }
 
-  total = buffilled;
+  /* Capture the result size without the key appended.   */
+  total = scratch->buffer_used;
+
+  /* Make a copy of the key.  The scratch buffer must not move after
+     this point.  */
+  key_copy = addgetnetgrentX_append_n (scratch, key, req->key_len);
+  if (key_copy == NULL)
+    return send_notfound (fd);
 
   /* Fill in the dataset.  */
-  dataset = (struct dataset *) buffer;
+  dataset = scratch->buffer.data;
   timeout = datahead_init_pos (&dataset->head, total + req->key_len,
 			       total - offsetof (struct dataset, resp),
 			       he == NULL ? 0 : dh->nreloads + 1,
@@ -363,11 +383,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
   dataset->resp.version = NSCD_VERSION;
   dataset->resp.found = 1;
   dataset->resp.nresults = nentries;
-  dataset->resp.result_len = buffilled - sizeof (*dataset);
-
-  assert (buflen - buffilled >= req->key_len);
-  key_copy = memcpy (buffer + buffilled, key, req->key_len);
-  buffilled += req->key_len;
+  dataset->resp.result_len = total - sizeof (*dataset);
 
   /* Now we can determine whether on refill we have to create a new
      record or not.  */
@@ -398,7 +414,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
     if (__glibc_likely (newp != NULL))
       {
 	/* Adjust pointer into the memory block.  */
-	key_copy = (char *) newp + (key_copy - buffer);
+	key_copy = (char *) newp + (key_copy - (char *) dataset);
 
 	dataset = memcpy (newp, dataset, total + req->key_len);
 	cacheable = true;
@@ -439,7 +455,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
     }
 
  out:
-  *resultp = dataset;
+  scratch->dataset = dataset;
 
   return timeout;
 }
@@ -460,6 +476,9 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
   if (user != NULL)
     key = strchr (key, '\0') + 1;
   const char *domain = *key++ ? key : NULL;
+  struct addgetnetgrentX_scratch scratch;
+
+  addgetnetgrentX_scratch_init (&scratch);
 
   if (__glibc_unlikely (debug_level > 0))
     {
@@ -475,12 +494,8 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
 							    group, group_len,
 							    db, uid);
   time_t timeout;
-  void *tofree;
   if (result != NULL)
-    {
-      timeout = result->head.timeout;
-      tofree = NULL;
-    }
+    timeout = result->head.timeout;
   else
     {
       request_header req_get =
@@ -489,7 +504,10 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
 	  .key_len = group_len
 	};
       timeout = addgetnetgrentX (db, -1, &req_get, group, uid, NULL, NULL,
-				 &result, &tofree);
+				 &scratch);
+      result = scratch.dataset;
+      if (timeout < 0)
+	goto out;
     }
 
   struct indataset
@@ -601,7 +619,7 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
     }
 
  out:
-  free (tofree);
+  addgetnetgrentX_scratch_free (&scratch);
   return timeout;
 }
 
@@ -611,11 +629,12 @@ addgetnetgrentX_ignore (struct database_dyn *db, int fd, request_header *req,
 			const char *key, uid_t uid, struct hashentry *he,
 			struct datahead *dh)
 {
-  struct dataset *ignore;
-  void *tofree;
-  time_t timeout = addgetnetgrentX (db, fd, req, key, uid, he, dh,
-				    &ignore, &tofree);
-  free (tofree);
+  struct addgetnetgrentX_scratch scratch;
+  addgetnetgrentX_scratch_init (&scratch);
+  time_t timeout = addgetnetgrentX (db, fd, req, key, uid, he, dh, &scratch);
+  addgetnetgrentX_scratch_free (&scratch);
+  if (timeout < 0)
+    timeout = 0;
   return timeout;
 }
 
@@ -659,5 +678,9 @@ readdinnetgr (struct database_dyn *db, struct hashentry *he,
       .key_len = he->len
     };
 
-  return addinnetgrX (db, -1, &req, db->data + he->key, he->owner, he, dh);
+  int timeout = addinnetgrX (db, -1, &req, db->data + he->key, he->owner,
+			     he, dh);
+  if (timeout < 0)
+    timeout = 0;
+  return timeout;
 }
-- 
2.44.0

Re: [PATCH 1/4] nscd: Stack-based buffer overflow in netgroup cache (bug 31677)

[Siddhesh Poyarekar]

On 2024-04-24 12:08, Florian Weimer wrote:
> Using alloca matches what other caches do.  The request length is
> bounded by MAXKEYLEN.
> ---
>   nscd/netgroupcache.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
> index 0c6e46f15c..24fbac7668 100644
> --- a/nscd/netgroupcache.c
> +++ b/nscd/netgroupcache.c
> @@ -502,12 +502,11 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
>         = (struct indataset *) mempool_alloc (db,
>   					    sizeof (*dataset) + req->key_len,
>   					    1);
> -  struct indataset dataset_mem;
>     bool cacheable = true;
>     if (__glibc_unlikely (dataset == NULL))
>       {
>         cacheable = false;

Can you please add a comment here stating that KEY_LEN is bounded by 
MAXKEYLEN?  Looks OK otherwise.

> -      dataset = &dataset_mem;
> +      dataset = alloca (sizeof (*dataset) + req->key_len);
>       }
>   
>     datahead_init_pos (&dataset->head, sizeof (*dataset) + req->key_len,

Re: [PATCH 2/4] nscd: Do not send missing not-found response in addgetnetgrentX (bug 31678)

[Siddhesh Poyarekar]

On 2024-04-24 12:08, Florian Weimer wrote:
> If we failed to add a not-found response to the cache, the dataset
> point can be null, resulting in a null pointer dereference.
> ---

LGTM.

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

>   nscd/netgroupcache.c | 14 ++++++--------
>   1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
> index 24fbac7668..8709fb77b6 100644
> --- a/nscd/netgroupcache.c
> +++ b/nscd/netgroupcache.c
> @@ -147,7 +147,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>         /* No such service.  */
>         cacheable = do_notfound (db, fd, req, key, &dataset, &total, &timeout,
>   			       &key_copy);
> -      goto writeout;
> +      goto maybe_cache_add;
>       }
>   
>     memset (&data, '\0', sizeof (data));
> @@ -348,7 +348,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>       {
>         cacheable = do_notfound (db, fd, req, key, &dataset, &total, &timeout,
>   			       &key_copy);
> -      goto writeout;
> +      goto maybe_cache_add;
>       }
>   
>     total = buffilled;
> @@ -410,14 +410,12 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>     }
>   
>     if (he == NULL && fd != -1)
> -    {
> -      /* We write the dataset before inserting it to the database
> -	 since while inserting this thread might block and so would
> -	 unnecessarily let the receiver wait.  */
> -    writeout:
> +    /* We write the dataset before inserting it to the database since
> +       while inserting this thread might block and so would
> +       unnecessarily let the receiver wait.  */
>         writeall (fd, &dataset->resp, dataset->head.recsize);
> -    }
>   
> + maybe_cache_add:
>     if (cacheable)
>       {
>         /* If necessary, we also propagate the data to disk.  */

Re: [PATCH 3/4] nscd: Avoid null pointer crashes after notfound response (bug 31678)

[Siddhesh Poyarekar]

On 2024-04-24 12:08, Florian Weimer wrote:
> The addgetnetgrentX call in addinnetgrX may have failed to produce
> a result, so the result variable in addinnetgrX can be NULL.
> Use db->negtimeout as the fallback value if there is no result data;
> the timeout is also overwritten below.
> 
> Also avoid sending a second not-found response.  (The client
> disconnects after receiving the first response, so the data stream did
> not go out of sync even without this fix.)  It is still beneficial to
> add the negative response to the mapping, so that the client can get
> it from there in the future, instead of going through the socket.
> ---

LGTM.

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

>   nscd/netgroupcache.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
> index 8709fb77b6..8f9eb84e39 100644
> --- a/nscd/netgroupcache.c
> +++ b/nscd/netgroupcache.c
> @@ -509,14 +509,15 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
>   
>     datahead_init_pos (&dataset->head, sizeof (*dataset) + req->key_len,
>   		     sizeof (innetgroup_response_header),
> -		     he == NULL ? 0 : dh->nreloads + 1, result->head.ttl);
> +		     he == NULL ? 0 : dh->nreloads + 1,
> +		     result == NULL ? db->negtimeout : result->head.ttl);
>     /* Set the notfound status and timeout based on the result from
>        getnetgrent.  */
> -  dataset->head.notfound = result->head.notfound;
> +  dataset->head.notfound = result == NULL || result->head.notfound;
>     dataset->head.timeout = timeout;
>   
>     dataset->resp.version = NSCD_VERSION;
> -  dataset->resp.found = result->resp.found;
> +  dataset->resp.found = result != NULL && result->resp.found;
>     /* Until we find a matching entry the result is 0.  */
>     dataset->resp.result = 0;
>   
> @@ -564,7 +565,9 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
>         goto out;
>       }
>   
> -  if (he == NULL)
> +  /* addgetnetgrentX may have already sent a notfound response.  Do
> +     not send another one.  */
> +  if (he == NULL && dataset->resp.found)
>       {
>         /* We write the dataset before inserting it to the database
>   	 since while inserting this thread might block and so would

Re: [PATCH 4/4] nscd: netgroup: Use two buffers in addgetnetgrentX (bug 31680)

[Siddhesh Poyarekar]

On 2024-04-24 12:08, Florian Weimer wrote:
> This avoids potential memory corruption when the underlying NSS
> callback function does not use the buffer space to store all strings
> (e.g., for constant strings).
> 
> Instead of custom buffer management, two scratch buffers are used.
> This increases stack usage somewhat.
> 
> Scratch buffer allocation failure is handled by return -1
> (an invalid timeout value) instead of terminating the process.
> This fixes bug 31679.
> ---

LGTM.

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

>   nscd/netgroupcache.c | 219 ++++++++++++++++++++++++-------------------
>   1 file changed, 121 insertions(+), 98 deletions(-)
> 
> diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
> index 8f9eb84e39..8b5ebfaf31 100644
> --- a/nscd/netgroupcache.c
> +++ b/nscd/netgroupcache.c
> @@ -23,6 +23,7 @@
>   #include <stdlib.h>
>   #include <unistd.h>
>   #include <sys/mman.h>
> +#include <scratch_buffer.h>
>   
>   #include "../nss/netgroup.h"
>   #include "nscd.h"
> @@ -65,6 +66,16 @@ struct dataset
>     char strdata[0];
>   };
>   
> +/* Send a notfound response to FD.  Always returns -1 to indicate an
> +   ephemeral error.  */
> +static time_t
> +send_notfound (int fd)
> +{
> +  if (fd != -1)
> +    TEMP_FAILURE_RETRY (send (fd, &notfound, sizeof (notfound), MSG_NOSIGNAL));
> +  return -1;
> +}
> +
>   /* Sends a notfound message and prepares a notfound dataset to write to the
>      cache.  Returns true if there was enough memory to allocate the dataset and
>      returns the dataset in DATASETP, total bytes to write in TOTALP and the
> @@ -83,8 +94,7 @@ do_notfound (struct database_dyn *db, int fd, request_header *req,
>     total = sizeof (notfound);
>     timeout = time (NULL) + db->negtimeout;
>   
> -  if (fd != -1)
> -    TEMP_FAILURE_RETRY (send (fd, &notfound, total, MSG_NOSIGNAL));
> +  send_notfound (fd);
>   
>     dataset = mempool_alloc (db, sizeof (struct dataset) + req->key_len, 1);
>     /* If we cannot permanently store the result, so be it.  */
> @@ -109,11 +119,78 @@ do_notfound (struct database_dyn *db, int fd, request_header *req,
>     return cacheable;
>   }
>   
> +struct addgetnetgrentX_scratch
> +{
> +  /* This is the result that the caller should use.  It can be NULL,
> +     point into buffer, or it can be in the cache.  */
> +  struct dataset *dataset;
> +
> +  struct scratch_buffer buffer;
> +
> +  /* Used internally in addgetnetgrentX as a staging area.  */
> +  struct scratch_buffer tmp;
> +
> +  /* Number of bytes in buffer that are actually used.  */
> +  size_t buffer_used;
> +};
> +
> +static void
> +addgetnetgrentX_scratch_init (struct addgetnetgrentX_scratch *scratch)
> +{
> +  scratch->dataset = NULL;
> +  scratch_buffer_init (&scratch->buffer);
> +  scratch_buffer_init (&scratch->tmp);
> +
> +  /* Reserve space for the header.  */
> +  scratch->buffer_used = sizeof (struct dataset);
> +  static_assert (sizeof (struct dataset) < sizeof (scratch->tmp.__space),
> +		 "initial buffer space");
> +  memset (scratch->tmp.data, 0, sizeof (struct dataset));
> +}
> +
> +static void
> +addgetnetgrentX_scratch_free (struct addgetnetgrentX_scratch *scratch)
> +{
> +  scratch_buffer_free (&scratch->buffer);
> +  scratch_buffer_free (&scratch->tmp);
> +}
> +
> +/* Copy LENGTH bytes from S into SCRATCH.  Returns NULL if SCRATCH
> +   could not be resized, otherwise a pointer to the copy.  */
> +static char *
> +addgetnetgrentX_append_n (struct addgetnetgrentX_scratch *scratch,
> +			  const char *s, size_t length)
> +{
> +  while (true)
> +    {
> +      size_t remaining = scratch->buffer.length - scratch->buffer_used;
> +      if (remaining >= length)
> +	break;
> +      if (!scratch_buffer_grow_preserve (&scratch->buffer))
> +	return NULL;
> +    }
> +  char *copy = scratch->buffer.data + scratch->buffer_used;
> +  memcpy (copy, s, length);
> +  scratch->buffer_used += length;
> +  return copy;
> +}
> +
> +/* Copy S into SCRATCH, including its null terminator.  Returns false
> +   if SCRATCH could not be resized.  */
> +static bool
> +addgetnetgrentX_append (struct addgetnetgrentX_scratch *scratch, const char *s)
> +{
> +  if (s == NULL)
> +    s = "";
> +  return addgetnetgrentX_append_n (scratch, s, strlen (s) + 1) != NULL;
> +}
> +
> +/* Caller must initialize and free *SCRATCH.  If the return value is
> +   negative, this function has sent a notfound response.  */
>   static time_t
>   addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>   		 const char *key, uid_t uid, struct hashentry *he,
> -		 struct datahead *dh, struct dataset **resultp,
> -		 void **tofreep)
> +		 struct datahead *dh, struct addgetnetgrentX_scratch *scratch)
>   {
>     if (__glibc_unlikely (debug_level > 0))
>       {
> @@ -132,14 +209,10 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>   
>     char *key_copy = NULL;
>     struct __netgrent data;
> -  size_t buflen = MAX (1024, sizeof (*dataset) + req->key_len);
> -  size_t buffilled = sizeof (*dataset);
> -  char *buffer = NULL;
>     size_t nentries = 0;
>     size_t group_len = strlen (key) + 1;
>     struct name_list *first_needed
>       = alloca (sizeof (struct name_list) + group_len);
> -  *tofreep = NULL;
>   
>     if (netgroup_database == NULL
>         && !__nss_database_get (nss_database_netgroup, &netgroup_database))
> @@ -151,8 +224,6 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>       }
>   
>     memset (&data, '\0', sizeof (data));
> -  buffer = xmalloc (buflen);
> -  *tofreep = buffer;
>     first_needed->next = first_needed;
>     memcpy (first_needed->name, key, group_len);
>     data.needed_groups = first_needed;
> @@ -195,8 +266,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>   		while (1)
>   		  {
>   		    int e;
> -		    status = getfct.f (&data, buffer + buffilled,
> -				       buflen - buffilled - req->key_len, &e);
> +		    status = getfct.f (&data, scratch->tmp.data,
> +				       scratch->tmp.length, &e);
>   		    if (status == NSS_STATUS_SUCCESS)
>   		      {
>   			if (data.type == triple_val)
> @@ -204,68 +275,10 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>   			    const char *nhost = data.val.triple.host;
>   			    const char *nuser = data.val.triple.user;
>   			    const char *ndomain = data.val.triple.domain;
> -
> -			    size_t hostlen = strlen (nhost ?: "") + 1;
> -			    size_t userlen = strlen (nuser ?: "") + 1;
> -			    size_t domainlen = strlen (ndomain ?: "") + 1;
> -
> -			    if (nhost == NULL || nuser == NULL || ndomain == NULL
> -				|| nhost > nuser || nuser > ndomain)
> -			      {
> -				const char *last = nhost;
> -				if (last == NULL
> -				    || (nuser != NULL && nuser > last))
> -				  last = nuser;
> -				if (last == NULL
> -				    || (ndomain != NULL && ndomain > last))
> -				  last = ndomain;
> -
> -				size_t bufused
> -				  = (last == NULL
> -				     ? buffilled
> -				     : last + strlen (last) + 1 - buffer);
> -
> -				/* We have to make temporary copies.  */
> -				size_t needed = hostlen + userlen + domainlen;
> -
> -				if (buflen - req->key_len - bufused < needed)
> -				  {
> -				    buflen += MAX (buflen, 2 * needed);
> -				    /* Save offset in the old buffer.  We don't
> -				       bother with the NULL check here since
> -				       we'll do that later anyway.  */
> -				    size_t nhostdiff = nhost - buffer;
> -				    size_t nuserdiff = nuser - buffer;
> -				    size_t ndomaindiff = ndomain - buffer;
> -
> -				    char *newbuf = xrealloc (buffer, buflen);
> -				    /* Fix up the triplet pointers into the new
> -				       buffer.  */
> -				    nhost = (nhost ? newbuf + nhostdiff
> -					     : NULL);
> -				    nuser = (nuser ? newbuf + nuserdiff
> -					     : NULL);
> -				    ndomain = (ndomain ? newbuf + ndomaindiff
> -					       : NULL);
> -				    *tofreep = buffer = newbuf;
> -				  }
> -
> -				nhost = memcpy (buffer + bufused,
> -						nhost ?: "", hostlen);
> -				nuser = memcpy ((char *) nhost + hostlen,
> -						nuser ?: "", userlen);
> -				ndomain = memcpy ((char *) nuser + userlen,
> -						  ndomain ?: "", domainlen);
> -			      }
> -
> -			    char *wp = buffer + buffilled;
> -			    wp = memmove (wp, nhost ?: "", hostlen);
> -			    wp += hostlen;
> -			    wp = memmove (wp, nuser ?: "", userlen);
> -			    wp += userlen;
> -			    wp = memmove (wp, ndomain ?: "", domainlen);
> -			    wp += domainlen;
> -			    buffilled = wp - buffer;
> +			    if (!(addgetnetgrentX_append (scratch, nhost)
> +				  && addgetnetgrentX_append (scratch, nuser)
> +				  && addgetnetgrentX_append (scratch, ndomain)))
> +			      return send_notfound (fd);
>   			    ++nentries;
>   			  }
>   			else
> @@ -317,8 +330,8 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>   		      }
>   		    else if (status == NSS_STATUS_TRYAGAIN && e == ERANGE)
>   		      {
> -			buflen *= 2;
> -			*tofreep = buffer = xrealloc (buffer, buflen);
> +			if (!scratch_buffer_grow (&scratch->tmp))
> +			  return send_notfound (fd);
>   		      }
>   		    else if (status == NSS_STATUS_RETURN
>   			     || status == NSS_STATUS_NOTFOUND
> @@ -351,10 +364,17 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>         goto maybe_cache_add;
>       }
>   
> -  total = buffilled;
> +  /* Capture the result size without the key appended.   */
> +  total = scratch->buffer_used;
> +
> +  /* Make a copy of the key.  The scratch buffer must not move after
> +     this point.  */
> +  key_copy = addgetnetgrentX_append_n (scratch, key, req->key_len);
> +  if (key_copy == NULL)
> +    return send_notfound (fd);
>   
>     /* Fill in the dataset.  */
> -  dataset = (struct dataset *) buffer;
> +  dataset = scratch->buffer.data;
>     timeout = datahead_init_pos (&dataset->head, total + req->key_len,
>   			       total - offsetof (struct dataset, resp),
>   			       he == NULL ? 0 : dh->nreloads + 1,
> @@ -363,11 +383,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>     dataset->resp.version = NSCD_VERSION;
>     dataset->resp.found = 1;
>     dataset->resp.nresults = nentries;
> -  dataset->resp.result_len = buffilled - sizeof (*dataset);
> -
> -  assert (buflen - buffilled >= req->key_len);
> -  key_copy = memcpy (buffer + buffilled, key, req->key_len);
> -  buffilled += req->key_len;
> +  dataset->resp.result_len = total - sizeof (*dataset);
>   
>     /* Now we can determine whether on refill we have to create a new
>        record or not.  */
> @@ -398,7 +414,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>       if (__glibc_likely (newp != NULL))
>         {
>   	/* Adjust pointer into the memory block.  */
> -	key_copy = (char *) newp + (key_copy - buffer);
> +	key_copy = (char *) newp + (key_copy - (char *) dataset);
>   
>   	dataset = memcpy (newp, dataset, total + req->key_len);
>   	cacheable = true;
> @@ -439,7 +455,7 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req,
>       }
>   
>    out:
> -  *resultp = dataset;
> +  scratch->dataset = dataset;
>   
>     return timeout;
>   }
> @@ -460,6 +476,9 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
>     if (user != NULL)
>       key = strchr (key, '\0') + 1;
>     const char *domain = *key++ ? key : NULL;
> +  struct addgetnetgrentX_scratch scratch;
> +
> +  addgetnetgrentX_scratch_init (&scratch);
>   
>     if (__glibc_unlikely (debug_level > 0))
>       {
> @@ -475,12 +494,8 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
>   							    group, group_len,
>   							    db, uid);
>     time_t timeout;
> -  void *tofree;
>     if (result != NULL)
> -    {
> -      timeout = result->head.timeout;
> -      tofree = NULL;
> -    }
> +    timeout = result->head.timeout;
>     else
>       {
>         request_header req_get =
> @@ -489,7 +504,10 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
>   	  .key_len = group_len
>   	};
>         timeout = addgetnetgrentX (db, -1, &req_get, group, uid, NULL, NULL,
> -				 &result, &tofree);
> +				 &scratch);
> +      result = scratch.dataset;
> +      if (timeout < 0)
> +	goto out;
>       }
>   
>     struct indataset
> @@ -601,7 +619,7 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
>       }
>   
>    out:
> -  free (tofree);
> +  addgetnetgrentX_scratch_free (&scratch);
>     return timeout;
>   }
>   
> @@ -611,11 +629,12 @@ addgetnetgrentX_ignore (struct database_dyn *db, int fd, request_header *req,
>   			const char *key, uid_t uid, struct hashentry *he,
>   			struct datahead *dh)
>   {
> -  struct dataset *ignore;
> -  void *tofree;
> -  time_t timeout = addgetnetgrentX (db, fd, req, key, uid, he, dh,
> -				    &ignore, &tofree);
> -  free (tofree);
> +  struct addgetnetgrentX_scratch scratch;
> +  addgetnetgrentX_scratch_init (&scratch);
> +  time_t timeout = addgetnetgrentX (db, fd, req, key, uid, he, dh, &scratch);
> +  addgetnetgrentX_scratch_free (&scratch);
> +  if (timeout < 0)
> +    timeout = 0;
>     return timeout;
>   }
>   
> @@ -659,5 +678,9 @@ readdinnetgr (struct database_dyn *db, struct hashentry *he,
>         .key_len = he->len
>       };
>   
> -  return addinnetgrX (db, -1, &req, db->data + he->key, he->owner, he, dh);
> +  int timeout = addinnetgrX (db, -1, &req, db->data + he->key, he->owner,
> +			     he, dh);
> +  if (timeout < 0)
> +    timeout = 0;
> +  return timeout;
>   }

Re: [PATCH 0/4] Various nscd security fixes

[Carlos O'Donell]

On 4/24/24 12:08, Florian Weimer wrote:
> Carlos filed bug 31677, and it it turns out that this is a reachable
> stack-based buffer overflow.  The data looks quite attacker-controlled
> to me and probably can contain NUL bytes with a custom client, so this
> looks quite exploitable to my untrained eye.
> 
> Unfortunately, the reproducer kept crashing after the initial patch,
> hence the second and third commit.  The two issues fixed in the last
> commit were discovered by reading through the code.
> 
> By my count, this needs four different CVE identifiers:

As part of the glibc security team I've reserved and updated bugzilla with 4 CVE IDs.

>   Bug 31677: the stack-based buffer overflow (commit 1)
>   Bug 31678: two distinct null pointer dereferences (commit 2, commit 3)
>     (same flaw type, presumably same version range, so MERGE from a
>     CVE perspective)
>   Bug 31679: process termination on malloc failure (commit 4)
>   Byg 31680: memory corruption due to incorrect callback API assumption
>     (commit 4)
> 
> Florian Weimer (4):
>   nscd: Stack-based buffer overflow in netgroup cache (bug 31677)
>   nscd: Do not send missing not-found response in addgetnetgrentX (bug
>     31678)
>   nscd: Avoid null pointer crashes after notfound response (bug 31678)
>   nscd: netgroup: Use two buffers in addgetnetgrentX (bug 31680)
> 
>  nscd/netgroupcache.c | 247 +++++++++++++++++++++++--------------------
>  1 file changed, 135 insertions(+), 112 deletions(-)
> 
> 
> base-commit: f4724843ada64a51d66f65d3199fe431f9d4c254

-- 
Cheers,
Carlos.

Re: [PATCH 0/4] Various nscd security fixes

[Cristian Rodríguez]

On Wed, Apr 24, 2024 at 12:08 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> Carlos filed bug 31677, and it it turns out that this is a reachable
> stack-based buffer overflow.  The data looks quite attacker-controlled
> to me and probably can contain NUL bytes with a custom client, so this
> looks quite exploitable to my untrained eye.
>
> Unfortunately, the reproducer kept crashing after the initial patch,
> hence the second and third commit.  The two issues fixed in the last
> commit were discovered by reading through the code.


I 'm probably missing something but isn't NSCD de-facto EOL ?
It hasn't seen much progress for several years and Im sure it probably
has more of these kinds of bugs..

Re: [PATCH 0/4] Various nscd security fixes

[Florian Weimer]

* Cristian Rodríguez:

> On Wed, Apr 24, 2024 at 12:08 PM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> Carlos filed bug 31677, and it it turns out that this is a reachable
>> stack-based buffer overflow.  The data looks quite attacker-controlled
>> to me and probably can contain NUL bytes with a custom client, so this
>> looks quite exploitable to my untrained eye.
>>
>> Unfortunately, the reproducer kept crashing after the initial patch,
>> hence the second and third commit.  The two issues fixed in the last
>> commit were discovered by reading through the code.
>
>
> I 'm probably missing something but isn't NSCD de-facto EOL ?

It's not EOL for some downstreams.  We have to fix issues as we
encounter them.  We might as well share the patches with upstream.

This shouldn't prevent removal of nscd from the upstream sources.
If that happens, we can still contribute fixes to stable branches.

Thanks,
Florian