Created attachment 17674 [details] vulnerability report See report for details.
Just to make sure: Yes, this is a problem, but exploiting it requires a bug in winbind or a malicious process acting as winbind. If you can do that, you've pretty much taken over the machine anyway.
Created attachment 17677 [details] Patch Björn, can you see if this builds on AIX? It *should* have SIZE_MAX.
(In reply to Volker Lendecke from comment #2) > Created attachment 17677 [details] > Patch Looks like the check should be ">=" to allow for the terminator. https://www.ibm.com/docs/en/aix/7.2?topic=s-sysconf-subroutine indicates that sysconf(_SC_NGROUPS_MAX) is possible on AIX, so we could also compare the server's response.data.num_entries against that. My preference is to just use a hardcoded limit for now though.
Created attachment 17678 [details] Patch
(In reply to Volker Lendecke from comment #4) > Created attachment 17678 [details] > Patch Looks okay, although the subsequent gid_list[] traversal should probably also be audited for safety... is the gid_list length checked against num_gids on the client side, or are both provided separately by the server? Same thing for the sid list before the ids2xids...
(In reply to David Disseldorp from comment #5) > Looks okay, although the subsequent gid_list[] traversal should probably > also be audited for safety... is the gid_list length checked against > num_gids on the client side, or are both provided separately by the server? > Same thing for the sid list before the ids2xids... Yep, true. We don't do that in winbind_nss_linux.c either. Now that we have to protect against a malicious winbind, this opens a big can of worms.
Hardening code is one thing (and generically good to prevent mistakes). Assuming a malicious winbindd is another thing altogether. If I may quote a famous movie, "Game over Man...!" :-). I don't think it's rational to treat winbindd as a malicious component and try and protect against it from the client side.
(In reply to Volker Lendecke from comment #2) looks good!
(In reply to Jeremy Allison from comment #7) > I don't think it's rational to treat winbindd as a malicious component and > try and protect against it from the client side. How do we deal with this bug report then? With a friendly winbind the int overflow initially reported will not happen. But for the reporter it is worth a security alert, so according to the security community we have to protect against a malicious winbind.
(In reply to Volker Lendecke from comment #9) I think we should treat it like a crash bug. We can harden the client code against errors in the data returned (just do regular sanity checks against array overflow etc). There's no reason we can't do defensive coding - but for a component that returns authorization data it's silly to "protect" against it. As for "the security community" - there is no security community. Just a bunch of individuals trying to make a name for themselves by considering every bug a vuln (IMHO of course). Let's do a bugfix and move on.
This bug was referenced in samba master: b3ed90a0541a271a7c6d4bee1201fa47adc3c0c1 f964c0c357214637f80d0089723b9b11d1b38f7e
(In reply to Samba QA Contact from comment #11) Sorry it seems b3ed90a0541a271a7c6d4bee1201fa47adc3c0c1 f964c0c357214637f80d0089723b9b11d1b38f7e got the bug url wrong 15260 instead of 15240.
I've marked some team-internal comments private and opened this bug as we will deal with this as with all other bugs, there isn't a security escalation here.