[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20260315223945.GR1827@brightrain.aerifal.cx>
Date: Sun, 15 Mar 2026 18:39:46 -0400
From: "dalias@...c.org" <dalias@...c.org>
To: 0rbitingZer0@...ton.me
Cc: "musl@...ts.openwall.com" <musl@...ts.openwall.com>
Subject: Re: NSCD client getgr_a.c heap overflow + integer overflow
On Sun, Mar 15, 2026 at 10:20:19PM +0000, 0rbitingZer0@...ton.me wrote:
> 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.
That's why I posted it for checking. The i+1<cnt check is necessary
because [++i] slot is assigned, but the loop needs to continue to the
end of the last string to validate that it ends in the right place. So
I think we just need to put the check inside the loop.
How does the new version here look? I've switched to the test i<cnt
rather than i+1<cnt, since there is a slot available one past the end
which will subsequently be zeroed, and I think this is necessary in
order not to error out on the final null terminator.
Rich
> 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
> >
View attachment "getgr_a-v2.diff" of type "text/plain" (958 bytes)
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.
