[DSE-Dev] [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs
Stephen Smalley
sds at tycho.nsa.gov
Wed Jan 7 12:57:32 UTC 2009
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.
>
> I think I no not see the value with ignoring the upper bound to
> user IDs, it serves as a sanity check, for example, on some machines
> against the bugs that happened due to qmail creating a user with no
> regards to policies.
--
Stephen Smalley
National Security Agency
More information about the SELinux-devel
mailing list