Re: kern/113398: [libc] initgroups fails rather than truncates if number of groups > NGROUPS_MAX meaning the user can no longer login

From: <ttw+bsd_at_cobbled.net>
Date: Fri, 08 Aug 2008 14:12:44 +0100
i'm using group accounts for restricting access to client's data and
have been paralysed by this limitation for some time.  as such i've been
looking to implement a more complete solution to this problem.  here are
my issues with the current patch

	1. doesn't actually solve the problem, only increases the
	ceiling

	2. increases kernel memory usage significantly as there are
	a number of buffers initialised from these variables.  i.e.
	if i want to increase it to 65534 then i'm using an additional
	128k in each ki_info (per thread?) and ucred (per process?)
	as well as other knock on effects

	3. the above increase occurs even if i don't actually need
	the space and i cannot therefore be conservative about needs
	without impacting usage

	4. mangles the current ki_info size and thus breaks binary
	compatability

	5. other unseen impacts e.g. will xucred still work as it's
	now too big (appears not as the IPC stuff defines a CMGROUPS_MAX
	for it's own purposes)

	6. still no explict handling of IPC/NFS limit within an
	application

so i've done the following and would appreciate some feedback before i
go completing these changes and submitting a complete patch.

	1. defined two group limits SYS_NGROUPS which is the system
	limit, currently this is 65534 (minus one for egid) as most
	'ngroup' are 'int' and IPC_NGROUPS which will be fixed at 16

	2. statically defined NGROUPS as 16 for compatibility.
	applications should use sysconf/syscall to get
	NGROUPS_MAX/KERN_NGROUPS instead

	3. removed NGROUPS_MAX to force the use of sysconf/syscall or
	sensible use of application dependant defaults

	4. defined KERN_NGROUPS as a changable sysctl variable and
	defined the initial value as IPC_NGROUPS

	5. renamed the 'cr_groups' array to be 'cr_ipc_groups' and
	defined it's length at IPC_NGROUPS.  then added a new pointer
	after 'endcopy' with the name 'cr_groups' such that re-compiled
	code will find the new group structure.  when the number of
	groups is less than IPC_GROUPS the 'cr_groups' simply points
	to the 'cr_ipc_groups'.

	6. altered the 'kinfo' initialisation to copy the 'ipc_groups'
	instead.  (may also use one of the 'kinfo' spare pointers to
	copy the extended group)

	7. removed KI_NGROUPS and defined ki_groups length as IPC_NGROUPS

	8. removed CMGROUPS_MAX, using IPC_NGROUPS instead

here are my current issues and need for feed back.

	1. will the common use of 'cr_ngroups' defeat any attempt to
	increase the use beyond 16?  it appears that "a lot"[tm] of
	the code will 'MAX(cr_ngroups,NGROUPS)' correctly but i'm not
	experienced enough to guess at the bigger picture

	2. does the above juggling actually do anything productive;
	i'm doing it to be conservative (i.e. keeping kinfo stuct
	static) but will it actually save anything; considering the
	alteration to the 'cred' stuct my guess is no.  i was thinking
	to add the extended groups using the spare pointers in kinfo
	but again, with this much change does it actually save anything?

	3. i'm thinking that keeping the current group list and morphing
	it into an ipc_group list so that we can manage the 'clipping'
	of the group list in a consistent manner. e.g. add another
	sysctl variable to clip the ipc_group list to just the egid
	or do "smart shuffling" of the visible ipc_groups but again,
	i'm less than confident this will positively impact the
	situation.  is the definition of an 'ipc_groups' list of any
	long term benefit?

	4. as far as i'm concerned the use of NGROUPS at compile time
	is wrong as it implies a limit to the number of groups below
	that of available memory constraints.  i see no actual reason
	to do that.  i do understand the argument of "restricting
	access via group usage" but unless we stick to 16 groups that
	is an issue we have to confront on NFS anyway.   am i failing
	to understand NGROUPS fully?

in summary, i'm pretty happy with the changes but i'm not confident
that they are actually what's required.  perhaps i'd be better just
redefining 'cr_groups' as a pointer and MALLOC'ing the space dynamically,
at least then the use of 'cr_ngroups' is consistent.

thanks for any feedback and apologies for the rambling considerations
of this message.
Received on Fri Aug 08 2008 - 13:18:38 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:39:33 UTC