[<prev] [<thread-prev] [day] [month] [year] [list]
Message-ID: <zTsgO2LDxTFSqarQVAtK43k2AzZiawiimade9iIxzodWH07RrMk2KCCg9hsI9NehVolvOZBFpdTiC-i6Jy68pdOzV-3GX1jZsxRPN3t8laI=@proton.me>
Date: Mon, 16 Mar 2026 22:02:03 +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,
Went through v2, looks right to me. i<cnt lets the single-member case through correctly now and the else bails before any OOB write on extra nulls. INT32_MAX-1 guard on the alloc looks good too.
Cheers,
On Sunday, 15 March 2026 at 10:39 PM, dalias@...c.org <dalias@...c.org> wrote:
> 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
> > >
>
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.
