Bug 31680 (CVE-2024-33602)

Summary: nscd: netgroup cache assumes NSS callback uses in-buffer strings
Product: glibc Reporter: Florian Weimer <fweimer>
Component: nscdAssignee: Florian Weimer <fweimer>
Status: RESOLVED FIXED    
Severity: normal CC: carlos, carnil, drepper.fsp, fweimer, sam
Priority: P2 Flags: fweimer: security+
Version: 2.40   
Target Milestone: 2.40   
Host: Target:
Build: Last reconfirmed:

Description Florian Weimer 2024-04-24 11:53:14 UTC
The buffer-resizing code in addgetnetgrentX assumes that all string pointers point into the supplied buffer:

                            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;
                                  }

I do not think this is implied by the NSS API contract. We should simplify this code to use two buffers that are resized separately.
Comment 1 Florian Weimer 2024-04-25 13:35:17 UTC
Fixed for glibc 2.40 via:

commit c04a21e050d64a1193a6daab872bca2528bda44b
Author: Florian Weimer <fweimer@redhat.com>
Date:   Thu Apr 25 15:01:07 2024 +0200

    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>