[DSE-Dev] [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs

Manoj Srivastava manoj.srivastava at stdc.com
Wed Jan 7 19:59:01 UTC 2009


On Wed, Jan 07 2009, Stephen Smalley wrote:

> On Tue, 2009-01-06 at 18:41 -0600, Manoj Srivastava wrote:
>> On Tue, Jan 06 2009, Daniel J Walsh wrote:
>> 
>> > Stephen Smalley wrote:
>> >> On Tue, 2009-01-06 at 08:51 -0600, Manoj Srivastava wrote:
>> >>> On Tue, Jan 06 2009, Daniel J Walsh wrote:
>> >>>
>> >>>> Manoj Srivastava wrote:
>> >>>>> On Mon, Jan 05 2009, Daniel J Walsh wrote:
>> >>>>>
>> >>>>>> Manoj Srivastava wrote:
>> >>>>>>> From: Manoj Srivastava <srivasta at debian.org>
>> >>>>>>>
>> >>>>>>> Some non-Debian packages (like qmail, shudder) create users not below
>> >>>>>>> MIN_UID, but above MAX_UID, in /etc/login.defs (non-system users are
>> >>>>>>> supposed to have uids between MIN_UID and MAX_UID.
>> >>>>>>>
>> >>>>>>> genhomedircon.c:gethomedirs() checks pwent.pw_uid against MIN_UID in
>> >>>>>>> /etc/login.defs to exclude system users from generating homedir
>> >>>>>>> contexts. But unfortunately it does not check it against MAX_UID
>> >>>>>>> setting from the same file.
>> >>>>>>>
>> >>>>>>> This gets us lines like the following in the
>> >>>>>>> contexts/files/file_contexts.homedirs file:
>> >>>>>>>
>> >>>>>>> ,----
>> >>>>>>> |  #
>> >>>>>>> |  # Home Context for user user_u
>> >>>>>>> |  #
>> >>>>>>> |  /var/qmail/[^/]*/.+     user_u:object_r:user_home_t:s0
>> >>>>>>> |  /var/qmail/[^/]*/\.ssh(/.*)?    user_u:object_r:user_home_ssh_t:s0
>> >>>>>>> |  /var/qmail/[^/]*/\.gnupg(/.+)?  user_u:object_r:user_gpg_secret_t:s0
>> >>>>>>> |  /var/qmail/[^/]*        -d      user_u:object_r:user_home_dir_t:s0
>> >>>>>>> |  /var/qmail/lost\+found/.*       <<none>>
>> >>>>>>> |  /var/qmail      -d      system_u:object_r:home_root_t:s0
>> >>>>>>> |  /var/qmail/\.journal    <<none>>
>> >>>>>>> |  /var/qmail/lost\+found  -d      system_u:object_r:lost_found_t:s0
>> >>>>>>> |  /tmp/gconfd-.*  -d      user_u:object_r:user_tmp_t:s0
>> >>>>>>> `----
>> >>>>>>>
>> >>>>>>> This commit adds checking uid value againt MAX_UID too.
>> >>>>>>>
>> >>>>>>> Signed-off-by: Manoj Srivastava <srivasta at debian.org>
>> >>>>>>> ---
>> >>>>>>>  src/genhomedircon.c |   22 ++++++++++++++++++----
>> >>>>>>>  1 files changed, 18 insertions(+), 4 deletions(-)
>> >>>>>>>
>> >>>>>>> diff --git a/src/genhomedircon.c b/src/genhomedircon.c
>> >>>>>>> index ce15807..a5306d7 100644
>> >>>>>>> --- a/src/genhomedircon.c
>> >>>>>>> +++ b/src/genhomedircon.c
>> >>>>>>> @@ -219,8 +219,8 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
>> >>>>>>>  	char *rbuf = NULL;
>> >>>>>>>  	char *path = NULL;
>> >>>>>>>  	long rbuflen;
>> >>>>>>> -	uid_t temp, minuid = 0;
>> >>>>>>> -	int minuid_set = 0;
>> >>>>>>> +	uid_t temp, minuid = 0, maxuid = 0;
>> >>>>>>> +	int minuid_set = 0, maxuid_set = 0;
>> >>>>>>>  	struct passwd pwstorage, *pwbuf;
>> >>>>>>>  	struct stat buf;
>> >>>>>>>  	int retval;
>> >>>>>>> @@ -270,6 +270,16 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
>> >>>>>>>  	}
>> >>>>>>>  	free(path);
>> >>>>>>>  	path = NULL;
>> >>>>>>> +	path = semanage_findval(PATH_ETC_LOGIN_DEFS, "UID_MAX", NULL);
>> >>>>>>> +	if (path && *path) {
>> >>>>>>> +		temp = atoi(path);
>> >>>>>>> +		if (!maxuid_set || temp > maxuid) {
>> >>>>>>> +			maxuid = temp;
>> >>>>>>> +			maxuid_set = 1;
>> >>>>>>> +		}
>> >>>>>>> +	}
>> >>>>>>> +	free(path);
>> >>>>>>> +	path = NULL;
>> >>>>>>>  
>> >>>>>>>  	path = semanage_findval(PATH_ETC_LIBUSER, "LU_UIDNUMBER", "=");
>> >>>>>>>  	if (path && *path) {
>> >>>>>>> @@ -286,6 +296,10 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
>> >>>>>>>  		minuid = 500;
>> >>>>>>>  		minuid_set = 1;
>> >>>>>>>  	}
>> >>>>>>> +	if (!maxuid_set) {
>> >>>>>>> +		maxuid = 60000;
>> >>>>>>> +		maxuid_set = 1;
>> >>>>>>> +	}
>> >>>>>>>  
>> >>>>>>>  	rbuflen = sysconf(_SC_GETPW_R_SIZE_MAX);
>> >>>>>>>  	if (rbuflen <= 0)
>> >>>>>>> @@ -295,7 +309,7 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
>> >>>>>>>  		goto fail;
>> >>>>>>>  	setpwent();
>> >>>>>>>  	while ((retval = getpwent_r(&pwstorage, rbuf, rbuflen, &pwbuf)) == 0) {
>> >>>>>>> -		if (pwbuf->pw_uid < minuid)
>> >>>>>>> +		if (pwbuf->pw_uid < minuid || pwbuf->pw_uid > maxuid)
>> >>>>>>>  			continue;
>> >>>>>>>  		if (!semanage_list_find(shells, pwbuf->pw_shell))
>> >>>>>>>  			continue;
>> >>>>>>> @@ -322,7 +336,7 @@ static semanage_list_t *get_home_dirs(genhomedircon_settings_t * s)
>> >>>>>>>  
>> >>>>>>>  			/* NOTE: old genhomedircon printed a warning on match */
>> >>>>>>>  			if (hand.matched) {
>> >>>>>>> -				WARN(s->h_semanage, "%s homedir %s or its parent directory conflicts with a file context already specified in the policy.  This usually indicates an incorrectly defined system account.  If it is a system account please make sure its uid is less than %u or its login shell is /sbin/nologin.", pwbuf->pw_name, pwbuf->pw_dir, minuid);
>> >>>>>>> +			  WARN(s->h_semanage, "%s homedir %s or its parent directory conflicts with a file context already specified in the policy.  This usually indicates an incorrectly defined system account.  If it is a system account please make sure its uid is less than %u or greater than %u or its login shell is /sbin/nologin.", pwbuf->pw_name, pwbuf->pw_dir, minuid, maxuid);
>> >>>>>>>  			} else {
>> >>>>>>>  				if (semanage_list_push(&homedir_list, path))
>> >>>>>>>  					goto fail;
>> >>>>>> I think the default MAX_UID is not big enough.
>> >>>>>>
>> >>>>>> Shouldn't it be MAXINT?
>> >>>>>         Not unless I am misunderstanding the logic here. The way I see
>> >>>>>  it, the code below:
>> >>>>> ,----
>> >>>>> | while ((retval = getpwent_r(&pwstorage, rbuf, rbuflen, &pwbuf)) == 0) {
>> >>>>> |         if (pwbuf->pw_uid < minuid || pwbuf->pw_uid > maxuid)
>> >>>>> |                 continue;
>> >>>>> |         if (!semanage_list_find(shells, pwbuf->pw_shell))
>> >>>>> |                 continue;
>> >>>>> |         if (strcmp(pwbuf->pw_dir, "/") == 0)
>> >>>>> |                 continue;
>> >>>>> |         if (semanage_str_count(pwbuf->pw_dir, '/') <= 1)
>> >>>>> |                 continue;
>> >>>>> |         if (!(path = strdup(pwbuf->pw_dir))) {
>> >>>>> |                 break;
>> >>>>> |         }
>> >>>>> |    /* DO STUFF HERE */
>> >>>>> | }
>> >>>>> `----
>> >>>>>
>> >>>>>         Does nothing iff: pw_uid < minuid || pw_uid > maxuid, so it
>> >>>>>  only does things if the UID is in the range legal for non-system users;
>> >>>>>  and the UID range for system users is UID_MIN < uid < UID_MAX.
>> >>>>>
>> >>>>>         If you change the upper bound to max int, then we will create
>> >>>>>  the entries for UIDs in the range above 60000 -- which is where the
>> >>>>>  bletcherous qmail install script places the qmail uid.
>> >>>>>
>> >>>>>         The current behaviour, but not checking the upper bound of the
>> >>>>>  acceptable user range would be equivalent to the raising the upper
>> >>>>>  bound to INT_MAX; and indeed, would make this patch redundant.
>> >>>>>
>> >>>>>         From login.defs(5):
>> >>>>> ,----
>> >>>>> |        UID_MAX (number), UID_MIN (number)
>> >>>>> |            Range of user IDs used for the creation of regular users by
>> >>>>> |            useradd or newusers.
>> >>>>> `----
>> >>>>>
>> >>>>>         Therefore I think that the right thing to do would be to check
>> >>>>>  for both the upper and the lower bound, not just the lower bound
>> >>>>>  check, which is all we do now.
>> >>>>>
>> >>>>>         manoj
>> >>>> my point being, if I have a legitimate UID > 60000 for a real user and
>> >>>> do not define UID_MAX, My account will not work.
>> >>>>
>> >>>> I do not have a problem with checking UID_MAX, just that the default
>> >>>> should be much higer then 60000.
>> >>>         I just went with the Debian policy default value; 60000 is the
>> >>>  upper limit of uid's on Debian (and probably most Debian derivative)
>> >>>  machines, and UID's lager than that are reserved by policy for Debian
>> >>>  packages to use (after discussion on the development mailing lists).
>> >> 
>> >> The same appears to be true in Fedora; UID_MAX is set to 60000 by
>> >> default in /etc/login.defs there as well.
>> >> 
>> >>>         Having said that, I have no objection to making the default max
>> >>>  uid a preprocessor constant, which can be tweaked by the
>> >>>  distributions. Should I send in an updated patch?
>> >> 
>> > Talking to Nalin, he thinks this number should be ignored,  Imagine a
>> > university with > 60000 students or a large government Department (Say
>> > DOD),  this will cause users with UID 600001 to not work correctly.
>> >
>> > UID_MAX is just there to tell useradd to give up when trying to find the
>> > next available UID to add, it means nothing when you have a network of
>> > Users.
>> 
>>         Is this not why we have /etc/login.defs in the first place?
>>  These installations should change UID_MAX to whatever makes
>>  sense for their site. Otherwise, we have a mismatch between stated
>>  policies (/etc/logindefs.conf) and practice, and I would much rather
>>  have our code follow the stated policy than not.
>
> As Dan pointed out, the UID_MAX value in login.defs is only used by
> useradd, and is not even strictly enforced (useradd -u 60002 john works
> just fine).  In an environment with a large number of users and a global
> user database, you can certainly exceed 60000 users or you may even
> happen to generate your uids in a manner that happens to put them all in
> the upper part of the uid space.  There are real systems with uids >
> 60000 for real users, yet the login.defs UID_MAX value has not been
> changed on such systems because it is irrelevant and it isn't enforced
> by anything.  So this patch would change behavior of libsemanage on such
> systems in an undesirable manner.  And it wouldn't help with cases like
> oracle where the pseudo user is added via useradd without any specified
> uid at all.
>
> I think Dan's earlier posting gets to more of the fundamental problems
> with genhomedircon's heuristics for finding home directory locations,
> and we need to address his points if we want it to work in general.

        Fair enough. In that case, I would like to point out that the
 current situation of only checking UID_MIN is causing actual problems
 right now on real user systems, and making genhomedircon to incorrectly
 guess where home directories exist.

        I'll treat this as an imperfect workaround until we fix
 semodule, and then I'll just revert the patch for Debian systems.

        manoj
-- 
Manoj Srivastava <manoj.srivastava at stdc.com> <srivasta at acm.org>        
1024D/BF24424C print 4966 F272 D093 B493 410B  924B 21BA DABB BF24 424C



More information about the SELinux-devel mailing list