| From 7a5864cac60e06000394128a5a2817b03542f5a3 Mon Sep 17 00:00:00 2001 |
| From: Florian Weimer <fweimer@redhat.com> |
| Date: Thu, 25 Apr 2024 15:01:07 +0200 |
| Subject: [PATCH 15/15] 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 787e44d8514a..aaabbbb003e4 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 "../inet/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 = (char *) rawmemchr (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.44.0.769.g3c40516874-goog |
| |