[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0vndpU6dR92dZsTLIVmYBOjR13rK0041oorCG__cMRT5a8fhJn0lElW6LDnYrjZ3osd-36Et2kKBM4mR2ggPywKqahN9PfFmnd3JeLk3FP0=@proton.me>
Date: Sun, 15 Mar 2026 22:20:19 +0000
From: 0rbitingZer0@...ton.me
To: "dalias@...c.org" <dalias@...c.org>
Cc: "musl@...ts.openwall.com" <musl@...ts.openwall.com>
Subject: Re: NSCD client getgr_a.c heap overflow + integer overflow
Rich,
I was going through the patch focusing on the loop bound. With GRMEMCNT=1 and a single valid member, i+1<1 is false from the start so the loop never runs, and the post-check catches 0!=1 as EIO.
Might be worth double checking? I think i<groupbuf[GRMEMCNT] could be the right bound there but let me know if I'm reading it wrong.
Cheers,
On Sunday, 15 March 2026 at 8:39 PM, dalias@...c.org <dalias@...c.org> wrote:
> On Sun, Mar 15, 2026 at 06:21:52PM +0000, 0rbitingZer0@...ton.me wrote:
> > Hi,
> >
> > During an independent source audit of musl 1.2.5 (current git master, v1.2.5-102-g1b76ff07) I found a heap overflow write in the NSCD group response parsing in src/passwd/getgr_a.c. I reported this to Rich privately, and he spotted an additional integer overflow in the same code path. He suggested moving the discussion here since no embargo is needed.
> >
> > Rich's assessment of the security relevance:
> >
> > > the only means of passing input to this interface is to be the
> > > owner of /var/run/nscd/socket, a privileged path that's already
> > > expected to have authority over user/group identities. However,
> > > if you're running the service there with root privileges dropped
> > > and perhaps some sandboxing to mitigate bugs in the nscd code
> > > that make it vulnerable to attacks from remote backends, bugs in
> > > musl's nscd client do enable an attacker to move from there to
> > > here. So I think it's security-relevant. Not as a direct vuln
> > > but as an unintended lateral movement vector.
> >
> > And on the fix:
> >
> > > The level of validation needed here was probably deemed low
> > > because the source of the data is privileged. But this should
> > > be fixed.
> >
> > Here are the details.
> >
> > 1. Heap overflow write (lines 127-136):
> >
> > The member pointer array is allocated based on GRMEMCNT (line 118):
> >
> > char **tmp = realloc(*mem, (groupbuf[GRMEMCNT]+1)*sizeof(char*));
> >
> > The loop at line 129 scans the member data for null bytes and writes a pointer for each one:
> >
> > for (ptr = mem[0][0], i = 0; ptr != mem[0][0]+grlist_len; ptr++)
> > if (!*ptr) mem[0][++i] = ptr+1;
> >
> > The count check at line 133 runs after the loop completes:
> >
> > if (i != groupbuf[GRMEMCNT]) {
> > rv = EIO;
> > goto cleanup_f;
> > }
> >
> > If the data contains more null bytes than GRMEMCNT declares, the loop writes past the allocated array before the check fires. The data shape is deliverable through the real NSCD protocol: GRMEMCNT controls how many name_len fields are read (line 80), grlist_len is their sum, and the actual data blob is fread'd at line 108. Null byte placement within the blob is fully attacker-controlled.
> >
> > ASan confirms: heap-buffer-overflow WRITE of size 8, 0 bytes after a 16-byte region (GRMEMCNT=1, 2-slot array, 5 null-terminated strings in data).
> >
> > 2. Integer overflow (lines 113-114):
> >
> > Rich noted:
> >
> > > This and lines 113 and 114 above it already have a bug:
> > > integer overflow adding 1 without testing for overflow.
> > > Just adding 1U isn't a solution either because then a value
> > > of -1 would wrongly pass. There is just a need for more
> > > validation here.
> >
> > Happy to post the PoC if useful.
> >
> > 0rbitingZer0
>
> Does the attached look ok for a conservative fix? I think the
> validation should be made more consistent in this file in general,
> using a consistent interpretation of negative int32 values as invalid
> vs unsigned, etc., but for now I wanted to avoid having to make
> further changes like adjusting the type/signedness of the index var i,
> and the attached looks like the most straightforward fix for the issue
> at hand.
>
> Also: I think assigning the count+1 to *mmem is probably wrong. It's
> supposed to be the number of members not the number of slots counting
> the null termination, or at least that seems to be how the callers are
> using it. But if so the mistake is harmless, and I'd rather not poke
> with it during "release window". Something to consider cleaning up
> later though, if my analysis here is correct.
>
> Rich
>
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
