391 lines
13 KiB
Diff
391 lines
13 KiB
Diff
|
From a9a8d3eebb145779a18d90e3966009a1daa63cd8 Mon Sep 17 00:00:00 2001
|
||
|
From: Florian Weimer <fweimer@redhat.com>
|
||
|
Date: Thu, 25 Apr 2024 15:01:07 +0200
|
||
|
Subject: [PATCH] CVE-2024-33601, CVE-2024-33602: nscd: netgroup: Use two
|
||
|
buffers in addgetnetgrentX (bug 31680)
|
||
|
|
||
|
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.
|
||
|
|
||
|
Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
|
||
|
(cherry picked from commit c04a21e050d64a1193a6daab872bca2528bda44b)
|
||
|
---
|
||
|
nscd/netgroupcache.c | 219 ++++++++++++++++++++++++-------------------
|
||
|
1 file changed, 121 insertions(+), 98 deletions(-)
|
||
|
|
||
|
diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
|
||
|
index e22ffa5884..e8fe041846 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, ¬found, 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, ¬found, 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
|
||
|
@@ -603,7 +621,7 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
|
||
|
}
|
||
|
|
||
|
out:
|
||
|
- free (tofree);
|
||
|
+ addgetnetgrentX_scratch_free (&scratch);
|
||
|
return timeout;
|
||
|
}
|
||
|
|
||
|
@@ -613,11 +631,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;
|
||
|
}
|
||
|
|
||
|
@@ -661,5 +680,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.45.0
|
||
|
|