[DSE-Dev] [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs
Stephen Smalley
sds at tycho.nsa.gov
Tue Jan 6 15:09:10 UTC 2009
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?
--
Stephen Smalley
National Security Agency
More information about the SELinux-devel
mailing list