* [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs @ 2009-01-05 22:45 Manoj Srivastava 2009-01-05 22:56 ` Daniel J Walsh 0 siblings, 1 reply; 17+ messages in thread From: Manoj Srivastava @ 2009-01-05 22:45 UTC (permalink / raw) To: selinux; +Cc: selinux-devel, Manoj Srivastava From: Manoj Srivastava <srivasta@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@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; -- 1.5.6.5 -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs 2009-01-05 22:45 [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs Manoj Srivastava @ 2009-01-05 22:56 ` Daniel J Walsh 2009-01-05 23:22 ` Manoj Srivastava 0 siblings, 1 reply; 17+ messages in thread From: Daniel J Walsh @ 2009-01-05 22:56 UTC (permalink / raw) To: Manoj Srivastava; +Cc: selinux, selinux-devel, Manoj Srivastava -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Manoj Srivastava wrote: > From: Manoj Srivastava <srivasta@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@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? -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iEYEARECAAYFAklikDcACgkQrlYvE4MpobOO5QCg4zOQCJ2I3ajjf0lAOKbZpq27 F6sAoIa1MaJDR+albJlApB5N+NwpxabD =OT2M -----END PGP SIGNATURE----- -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs 2009-01-05 22:56 ` Daniel J Walsh @ 2009-01-05 23:22 ` Manoj Srivastava 2009-01-06 12:52 ` Daniel J Walsh 0 siblings, 1 reply; 17+ messages in thread From: Manoj Srivastava @ 2009-01-05 23:22 UTC (permalink / raw) To: Daniel J Walsh; +Cc: selinux, selinux-devel On Mon, Jan 05 2009, Daniel J Walsh wrote: > Manoj Srivastava wrote: >> From: Manoj Srivastava <srivasta@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@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 -- Manoj Srivastava <manoj.srivastava@stdc.com> <srivasta@acm.org> 1024D/BF24424C print 4966 F272 D093 B493 410B 924B 21BA DABB BF24 424C -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs 2009-01-05 23:22 ` Manoj Srivastava @ 2009-01-06 12:52 ` Daniel J Walsh 2009-01-06 14:51 ` Manoj Srivastava 0 siblings, 1 reply; 17+ messages in thread From: Daniel J Walsh @ 2009-01-06 12:52 UTC (permalink / raw) To: Daniel J Walsh, selinux, selinux-devel -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Manoj Srivastava wrote: > On Mon, Jan 05 2009, Daniel J Walsh wrote: > >> Manoj Srivastava wrote: >>> From: Manoj Srivastava <srivasta@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@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. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iEYEARECAAYFAkljU/oACgkQrlYvE4MpobO3HQCeJiGm+FOc4E8Bjf9mV8bBANYV jzIAoMDWczZmf2sfKpln1E9CrQeXFFpM =omnl -----END PGP SIGNATURE----- -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs 2009-01-06 12:52 ` Daniel J Walsh @ 2009-01-06 14:51 ` Manoj Srivastava 2009-01-06 15:09 ` Stephen Smalley 2009-01-06 16:50 ` Daniel J Walsh 0 siblings, 2 replies; 17+ messages in thread From: Manoj Srivastava @ 2009-01-06 14:51 UTC (permalink / raw) To: Daniel J Walsh; +Cc: selinux, selinux-devel 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@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@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). 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? manoj -- Manoj Srivastava <manoj.srivastava@stdc.com> <srivasta@acm.org> 1024D/BF24424C print 4966 F272 D093 B493 410B 924B 21BA DABB BF24 424C -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs 2009-01-06 14:51 ` Manoj Srivastava @ 2009-01-06 15:09 ` Stephen Smalley 2009-01-06 16:30 ` Daniel J Walsh 2009-01-06 16:50 ` Daniel J Walsh 1 sibling, 1 reply; 17+ messages in thread From: Stephen Smalley @ 2009-01-06 15:09 UTC (permalink / raw) To: Manoj Srivastava; +Cc: Daniel J Walsh, selinux, selinux-devel 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@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@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 -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs 2009-01-06 15:09 ` Stephen Smalley @ 2009-01-06 16:30 ` Daniel J Walsh 2009-01-06 17:39 ` Stephen Smalley 2009-01-07 0:41 ` Manoj Srivastava 0 siblings, 2 replies; 17+ messages in thread From: Daniel J Walsh @ 2009-01-06 16:30 UTC (permalink / raw) To: Stephen Smalley; +Cc: Manoj Srivastava, selinux, selinux-devel -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 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@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@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. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iEYEARECAAYFAkljhzoACgkQrlYvE4MpobPuSACfQBlP2aRK2MVWwtGy48+nXzCJ o4gAoJqRBIyWl4XiRGvMZc6BmsQEFNAN =UGGE -----END PGP SIGNATURE----- -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs 2009-01-06 16:30 ` Daniel J Walsh @ 2009-01-06 17:39 ` Stephen Smalley 2009-01-06 19:13 ` Daniel J Walsh 2009-01-07 0:41 ` Manoj Srivastava 1 sibling, 1 reply; 17+ messages in thread From: Stephen Smalley @ 2009-01-06 17:39 UTC (permalink / raw) To: Daniel J Walsh; +Cc: Manoj Srivastava, selinux, selinux-devel On Tue, 2009-01-06 at 11:30 -0500, Daniel J Walsh wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > 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@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@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. This seems similar to the discussion you raised in: http://marc.info/?l=selinux&m=122286879230076&w=2 Unfortunately there wasn't any real follow-up at the time. -- Stephen Smalley National Security Agency -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs 2009-01-06 17:39 ` Stephen Smalley @ 2009-01-06 19:13 ` Daniel J Walsh 0 siblings, 0 replies; 17+ messages in thread From: Daniel J Walsh @ 2009-01-06 19:13 UTC (permalink / raw) To: Stephen Smalley; +Cc: Manoj Srivastava, selinux, selinux-devel -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Stephen Smalley wrote: > On Tue, 2009-01-06 at 11:30 -0500, Daniel J Walsh wrote: >> -----BEGIN PGP SIGNED MESSAGE----- >> Hash: SHA1 >> >> 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@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@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. > > This seems similar to the discussion you raised in: > http://marc.info/?l=selinux&m=122286879230076&w=2 > > Unfortunately there wasn't any real follow-up at the time. > I would like to make a suggestion we add a couple of flags to /etc/selinux/semanage.conf # semanage searches the passwd database for parents of home directories of "real users". semanage uses these values to setup the default file # context, so that files in the "real users" home directory are labeled # correctly. # semanage tries to identify "real users" by looking for # users with UID greater then UID_MIN as specified in /etc/login.defs. # UID_MIN defaults to 500. The user must also have a shell specified in # /etc/shells, excluding /bin/false and /sbin/nologin. On machines with # thousands of entries in the passwd database, semanage can take a very # long time to run. # # Alternatively semanage will not search the passwd database when you # specify HOMEPARENT value. # # HOMEPARENT takes as its value a colon-separated list of directories # # HOMEPARENT=/home:/export/home:/usr/local/home # # Specify HOMEEXCLUDE to tell semanage to ignore these directories even # if they seem to be parents of "real user" accounts # # HOMEEXCLUDE=/var/lib -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iEYEARECAAYFAkljrUcACgkQrlYvE4MpobP+jgCbBrnnUxD1i/5rY2WiJfD04ABl JMoAoMVGxiq6RcMj70qnXjxvK+7fRapg =dDaC -----END PGP SIGNATURE----- -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs 2009-01-06 16:30 ` Daniel J Walsh 2009-01-06 17:39 ` Stephen Smalley @ 2009-01-07 0:41 ` Manoj Srivastava 2009-01-07 12:57 ` Stephen Smalley 1 sibling, 1 reply; 17+ messages in thread From: Manoj Srivastava @ 2009-01-07 0:41 UTC (permalink / raw) To: Daniel J Walsh; +Cc: Stephen Smalley, selinux, selinux-devel 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@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@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. 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. manoj -- Manoj Srivastava <manoj.srivastava@stdc.com> <srivasta@acm.org> 1024D/BF24424C print 4966 F272 D093 B493 410B 924B 21BA DABB BF24 424C -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs 2009-01-07 0:41 ` Manoj Srivastava @ 2009-01-07 12:57 ` Stephen Smalley 2009-01-07 19:59 ` Manoj Srivastava 0 siblings, 1 reply; 17+ messages in thread From: Stephen Smalley @ 2009-01-07 12:57 UTC (permalink / raw) To: Manoj Srivastava; +Cc: Daniel J Walsh, selinux, selinux-devel 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@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@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 -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs 2009-01-07 12:57 ` Stephen Smalley @ 2009-01-07 19:59 ` Manoj Srivastava 2009-01-07 20:36 ` Daniel J Walsh 0 siblings, 1 reply; 17+ messages in thread From: Manoj Srivastava @ 2009-01-07 19:59 UTC (permalink / raw) To: Stephen Smalley; +Cc: Daniel J Walsh, selinux, selinux-devel 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@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@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@stdc.com> <srivasta@acm.org> 1024D/BF24424C print 4966 F272 D093 B493 410B 924B 21BA DABB BF24 424C -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs 2009-01-07 19:59 ` Manoj Srivastava @ 2009-01-07 20:36 ` Daniel J Walsh 2009-01-08 14:33 ` [DSE-Dev] " Manoj Srivastava 0 siblings, 1 reply; 17+ messages in thread From: Daniel J Walsh @ 2009-01-07 20:36 UTC (permalink / raw) To: Stephen Smalley, Daniel J Walsh, selinux, selinux-devel -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Manoj Srivastava wrote: > 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@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@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 What does the passwd entry that it is getting fooled by look like? Does the account really need a real shell? IE Do people actually login to the home directory? -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iEYEARECAAYFAkllElgACgkQrlYvE4MpobMD7wCg6fsXreti1IPpOW5rGab6IPl7 +KoAn3NUZUWxtyMnrUgXInLTsjiptglo =lsLC -----END PGP SIGNATURE----- -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [DSE-Dev] [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs 2009-01-07 20:36 ` Daniel J Walsh @ 2009-01-08 14:33 ` Manoj Srivastava 2009-01-08 15:44 ` Daniel J Walsh 2009-01-28 11:02 ` selinux 0 siblings, 2 replies; 17+ messages in thread From: Manoj Srivastava @ 2009-01-08 14:33 UTC (permalink / raw) To: Daniel J Walsh; +Cc: Stephen Smalley, selinux, selinux-devel Hi, [Trimming the patch and early discussion] On Wed, Jan 07 2009, Daniel J Walsh wrote: > Manoj Srivastava wrote: >> On Wed, Jan 07 2009, Stephen Smalley wrote: >>> 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. > What does the passwd entry that it is getting fooled by look like? Does > the account really need a real shell? IE Do people actually login to > the home directory? I do not have passwd data from the machine in question, though I can ask. What I do have are the results of fixfiles relabel / : ,----[ file contexts in /var ] | drwxr-xr-x 15 root root system_u:object_r:home_root_t:s0 4096 Dec 29 13:35 . | drwxr-xr-x 21 root root system_u:object_r:root_t:s0 4096 Dec 29 14:21 .. | drwxr-xr-x 2 root root user_u:object_r:user_home_dir_t:s0 4096 May 7 2008 backups | drwxr-xr-x 7 root root user_u:object_r:user_home_dir_t:s0 4096 Dec 29 14:17 cache | drwxr-xr-x 25 root root user_u:object_r:user_home_dir_t:s0 4096 Dec 29 14:17 lib | drwxrwsr-x 2 root staff user_u:object_r:user_home_dir_t:s0 4096 Mar 11 2008 local | drwxrwxrwt 2 root root user_u:object_r:user_home_dir_t:s0 4096 Dec 29 18:14 lock | drwxr-xr-x 6 root root system_u:object_r:var_log_t:s0 4096 Dec 29 18:19 log | drwx------ 2 root root system_u:object_r:lost_found_t:s0 16384 May 5 2008 lost+found | drwxrwsr-x 2 root mail user_u:object_r:user_home_dir_t:s0 4096 May 5 2008 mail | drwxr-xr-x 2 root root user_u:object_r:user_home_dir_t:s0 4096 May 5 2008 opt | drwxr-xr-x 2 root qmail system_u:object_r:home_root_t:s0 4096 Dec 29 13:38 qmail | drwxr-xr-x 7 root root system_u:object_r:var_run_t:s0 4096 Dec 29 18:14 run | drwxr-xr-x 5 root root user_u:object_r:user_home_dir_t:s0 4096 Dec 29 14:17 spool | drwxrwxrwt 3 root root system_u:object_r:tmp_t:s0 4096 Dec 29 18:06 tmp `---- Every time "semanage user" is run, we get: ,----[ contexts/files/file_contexts.homedirs ] | # | # | # User-specific file contexts, generated via libsemanage | # use semanage command to manage system users to change the file_context | # | # | | # | # Home Context for user user_u | # | | /home/[^/]*/.+ user_u:object_r:user_home_t:s0 | /home/[^/]*/\.ssh(/.*)? user_u:object_r:user_home_ssh_t:s0 | /home/[^/]*/\.gnupg(/.+)? user_u:object_r:user_gpg_secret_t:s0 | /home/[^/]* -d user_u:object_r:user_home_dir_t:s0 | /home/lost\+found/.* <<none>> | /home -d system_u:object_r:home_root_t:s0 | /home/\.journal <<none>> | /home/lost\+found -d system_u:object_r:lost_found_t:s0 | | | # | # Home Context for user user_u | # | | /var/[^/]*/.+ user_u:object_r:user_home_t:s0 | /var/[^/]*/\.ssh(/.*)? user_u:object_r:user_home_ssh_t:s0 | /var/[^/]*/\.gnupg(/.+)? user_u:object_r:user_gpg_secret_t:s0 | /var/[^/]* -d user_u:object_r:user_home_dir_t:s0 | /var/lost\+found/.* <<none>> | /var -d system_u:object_r:home_root_t:s0 | /var/\.journal <<none>> | /var/lost\+found -d system_u:object_r:lost_found_t:s0 | | | # | # 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 | | | # | # Home Context for user root | # | | /root/.+ root:object_r:sysadm_home_t:s0 | /root/\.ssh(/.*)? root:object_r:sysadm_home_ssh_t:s0 | /root/\.gnupg(/.+)? root:object_r:sysadm_gpg_secret_t:s0 | /root -d root:object_r:sysadm_home_dir_t:s0 | /tmp/gconfd-root -d root:object_r:sysadm_tmp_t:s0 `---- This makes the machine unusable when in enforcing mode. Additionally, when there was unconfined se-module loaded there were unconfined_u instead of user_u as the second and third "users" in this file (that is, qmail and whatever added /var/spool). You need to hand edit $POLICY/contexts/files/file_contexts.homedirs and $POLICY/modules/active/file_contexts.homedirs by removing invalid entries (mentioning /var). ,----[ semanage user -l ] | root sysadm s0 s0-s0:c0.c1023 staff_r sysadm_r system_r | staff_u staff s0 s0-s0:c0.c1023 staff_r sysadm_r | sysadm_u sysadm s0 s0-s0:c0.c1023 sysadm_r | system_u user s0 s0-s0:c0.c1023 system_r | unconfined_u unconfined s0 s0-s0:c0.c1023 system_r unconfined_r | user_u user s0 s0 user_r `---- ,----[ semanage login -l ] | __default__ user_u s0 | root root s0-s0:c0.c1023 | system_u system_u s0-s0:c0.c1023 `---- ,----[ semodule -l ] | dhcp 1.6.0 | dmidecode 1.3.0 | gpg 1.6.0 | mysql 1.8.0 | netutils 1.6.0 | ssh 1.10.1 | sudo 1.3.0 | tcpd 1.3.0 | tzdata 1.2.0 `---- manoj -- Manoj Srivastava <manoj.srivastava@stdc.com> <srivasta@acm.org> 1024D/BF24424C print 4966 F272 D093 B493 410B 924B 21BA DABB BF24 424C -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [DSE-Dev] [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs 2009-01-08 14:33 ` [DSE-Dev] " Manoj Srivastava @ 2009-01-08 15:44 ` Daniel J Walsh 2009-01-28 11:02 ` selinux 1 sibling, 0 replies; 17+ messages in thread From: Daniel J Walsh @ 2009-01-08 15:44 UTC (permalink / raw) To: Daniel J Walsh, Stephen Smalley, selinux, selinux-devel -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Manoj Srivastava wrote: > Hi, > > [Trimming the patch and early discussion] > > On Wed, Jan 07 2009, Daniel J Walsh wrote: >> Manoj Srivastava wrote: >>> On Wed, Jan 07 2009, Stephen Smalley wrote: >>>> 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. > >> What does the passwd entry that it is getting fooled by look like? Does >> the account really need a real shell? IE Do people actually login to >> the home directory? > > I do not have passwd data from the machine in question, though I > can ask. What I do have are the results of fixfiles relabel / : > > ,----[ file contexts in /var ] > | drwxr-xr-x 15 root root system_u:object_r:home_root_t:s0 4096 Dec 29 13:35 . > | drwxr-xr-x 21 root root system_u:object_r:root_t:s0 4096 Dec 29 14:21 .. > | drwxr-xr-x 2 root root user_u:object_r:user_home_dir_t:s0 4096 May 7 2008 backups > | drwxr-xr-x 7 root root user_u:object_r:user_home_dir_t:s0 4096 Dec 29 14:17 cache > | drwxr-xr-x 25 root root user_u:object_r:user_home_dir_t:s0 4096 Dec 29 14:17 lib > | drwxrwsr-x 2 root staff user_u:object_r:user_home_dir_t:s0 4096 Mar 11 2008 local > | drwxrwxrwt 2 root root user_u:object_r:user_home_dir_t:s0 4096 Dec 29 18:14 lock > | drwxr-xr-x 6 root root system_u:object_r:var_log_t:s0 4096 Dec 29 18:19 log > | drwx------ 2 root root system_u:object_r:lost_found_t:s0 16384 May 5 2008 lost+found > | drwxrwsr-x 2 root mail user_u:object_r:user_home_dir_t:s0 4096 May 5 2008 mail > | drwxr-xr-x 2 root root user_u:object_r:user_home_dir_t:s0 4096 May 5 2008 opt > | drwxr-xr-x 2 root qmail system_u:object_r:home_root_t:s0 4096 Dec 29 13:38 qmail > | drwxr-xr-x 7 root root system_u:object_r:var_run_t:s0 4096 Dec 29 18:14 run > | drwxr-xr-x 5 root root user_u:object_r:user_home_dir_t:s0 4096 Dec 29 14:17 spool > | drwxrwxrwt 3 root root system_u:object_r:tmp_t:s0 4096 Dec 29 18:06 tmp > `---- > > Every time "semanage user" is run, we get: > ,----[ contexts/files/file_contexts.homedirs ] > | # > | # > | # User-specific file contexts, generated via libsemanage > | # use semanage command to manage system users to change the file_context > | # > | # > | > | # > | # Home Context for user user_u > | # > | > | /home/[^/]*/.+ user_u:object_r:user_home_t:s0 > | /home/[^/]*/\.ssh(/.*)? user_u:object_r:user_home_ssh_t:s0 > | /home/[^/]*/\.gnupg(/.+)? user_u:object_r:user_gpg_secret_t:s0 > | /home/[^/]* -d user_u:object_r:user_home_dir_t:s0 > | /home/lost\+found/.* <<none>> > | /home -d system_u:object_r:home_root_t:s0 > | /home/\.journal <<none>> > | /home/lost\+found -d system_u:object_r:lost_found_t:s0 > | > | > | # > | # Home Context for user user_u > | # > | > | /var/[^/]*/.+ user_u:object_r:user_home_t:s0 > | /var/[^/]*/\.ssh(/.*)? user_u:object_r:user_home_ssh_t:s0 > | /var/[^/]*/\.gnupg(/.+)? user_u:object_r:user_gpg_secret_t:s0 > | /var/[^/]* -d user_u:object_r:user_home_dir_t:s0 > | /var/lost\+found/.* <<none>> > | /var -d system_u:object_r:home_root_t:s0 > | /var/\.journal <<none>> > | /var/lost\+found -d system_u:object_r:lost_found_t:s0 > | > | > | # > | # 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 > | > | > | # > | # Home Context for user root > | # > | > | /root/.+ root:object_r:sysadm_home_t:s0 > | /root/\.ssh(/.*)? root:object_r:sysadm_home_ssh_t:s0 > | /root/\.gnupg(/.+)? root:object_r:sysadm_gpg_secret_t:s0 > | /root -d root:object_r:sysadm_home_dir_t:s0 > | /tmp/gconfd-root -d root:object_r:sysadm_tmp_t:s0 > `---- > > This makes the machine unusable when in enforcing mode. > Additionally, when there was unconfined se-module loaded there were > unconfined_u instead of user_u as the second and third "users" in this > file (that is, qmail and whatever added /var/spool). > > You need to hand edit > $POLICY/contexts/files/file_contexts.homedirs and > $POLICY/modules/active/file_contexts.homedirs by removing invalid > entries (mentioning /var). > > ,----[ semanage user -l ] > | root sysadm s0 s0-s0:c0.c1023 staff_r sysadm_r system_r > | staff_u staff s0 s0-s0:c0.c1023 staff_r sysadm_r > | sysadm_u sysadm s0 s0-s0:c0.c1023 sysadm_r > | system_u user s0 s0-s0:c0.c1023 system_r > | unconfined_u unconfined s0 s0-s0:c0.c1023 system_r unconfined_r > | user_u user s0 s0 user_r > `---- > > ,----[ semanage login -l ] > | __default__ user_u s0 > | root root s0-s0:c0.c1023 > | system_u system_u s0-s0:c0.c1023 > `---- > > ,----[ semodule -l ] > | dhcp 1.6.0 > | dmidecode 1.3.0 > | gpg 1.6.0 > | mysql 1.8.0 > | netutils 1.6.0 > | ssh 1.10.1 > | sudo 1.3.0 > | tcpd 1.3.0 > | tzdata 1.2.0 > `---- > > manoj > -- > Manoj Srivastava <manoj.srivastava@stdc.com> <srivasta@acm.org> > 1024D/BF24424C print 4966 F272 D093 B493 410B 924B 21BA DABB BF24 424C > > -- > This message was distributed to subscribers of the selinux mailing list. > If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with > the words "unsubscribe selinux" without quotes as the message. Well semanage is supposed to prevent this from happening. I think the latest version does this. I looks for a conflicting regex for the home root directory and then refuses to add the home dir. But the one you seem to be using is broken. I think fixing the passwd/homedir entry to use a shell of /sbin/nologin or /bin/false will fix your problem -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iEYEARECAAYFAklmH08ACgkQrlYvE4MpobPGxQCgi5FfF6HalhDacq9nCh5PHANU zUwAn2eGztoFQAJ47Sxs8KTMKc5M4bWb =nEn7 -----END PGP SIGNATURE----- -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [DSE-Dev] [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs 2009-01-08 14:33 ` [DSE-Dev] " Manoj Srivastava 2009-01-08 15:44 ` Daniel J Walsh @ 2009-01-28 11:02 ` selinux 1 sibling, 0 replies; 17+ messages in thread From: selinux @ 2009-01-28 11:02 UTC (permalink / raw) To: Daniel J Walsh, Stephen Smalley, selinux, selinux-devel Sorry for breaking into your conversation, but it was me who posted the bugreport. (My name is Alexey G. Shpagin, I am system administrator and I am posting here for a first time.) On Thu, Jan 08, 2009 at 08:33:54AM -0600, Manoj Srivastava wrote: > Hi, > > [Trimming the patch and early discussion] > > On Wed, Jan 07 2009, Daniel J Walsh wrote: > > Manoj Srivastava wrote: > >> On Wed, Jan 07 2009, Stephen Smalley wrote: > >>> 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 should note, that useradd -u 52 john works not even worse. What about the systems, that have legal user uids below UID_MIN? UID_MIN is also not enforced anywhere except the automatic uid generation in useradd or maybe some custom pam modules (don't know of any). > > >>> 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. > > > What does the passwd entry that it is getting fooled by look like? Does > > the account really need a real shell? IE Do people actually login to > > the home directory? > > I do not have passwd data from the machine in question, though I > can ask. What I do have are the results of fixfiles relabel / : The accounts were created not by hand, but by (pre|post)install script of qmail package. That script is not perfect (and already have some minor issues), but still I'm not sure if adding system users with /bin/sh was done by mistake or for some reason, yet to be checked. In the case you are still curious, here they are: alias:x:64010:65534:qmail alias,,,:/var/qmail/alias:/bin/sh qmaild:x:64011:65534:qmail daemon,,,:/var/qmail:/bin/sh qmails:x:64012:64010:qmail send,,,:/var/qmail:/bin/sh qmailr:x:64013:64010:qmail remote,,,:/var/qmail:/bin/sh qmailq:x:64014:64010:qmail queue,,,:/var/qmail:/bin/sh qmaill:x:64015:65534:qmail log,,,:/var/qmail:/bin/sh qmailp:x:64016:65534:qmail pw,,,:/var/qmail:/bin/sh What I can't keep in mind anymore is the way I want it to be: 1. Every nontrivial behavior (you call it heuristics) of libsemanage must be documented prior deployment (and documentation be referenced from at least semanage(8) man page). It looks like libsemanage already have enough heuristics and deserves it's own manpage in (8). I think, that the way of dependency of something perceived as component of selinux (that stated to have no relation to UNIX uids and accounts) on records from /etc/passwd is pretty nontrivial. Something tells me, that it should somehow map UNIX accounts to selinux labels, but I can only guess about what is the exact process. Even more, hidden dependency on some_but_not_others config parameters from logins.defs is just the example of nontrivial and counterintuitional behavior. (It will be easier for me (without need for sources) to track down the issue if UID_MIN were not used too, or UID_(MIN|MAX) were used both). 2. It is hard for me to determine, what are the login.defs' UID_MIN and UID_MAX for, and especially what they are not. The (only|best) way they can be used is during the initial configuration of libsemanage by postinstallation scripts that create SEPARATE configuration file for libsemanage and copy there all the needed settings from whatever you want. Maybe asking the administrator some questions. Also allowing him to reconfigure that again at his request. Only in separate configs should exists that UID_MIN, optional UID_MAX (and maybe also useful UIDS_EXCLUDE, USERS_EXCLUDE, and even better - GIDS_EXCLUDE, GROUPS_EXCLUDE) that will affect applications deploying libsemanage. They should be documented also, but if not, it will already be easier to guess their effect if they will live inside something like /etc/selinux/libsemanage/automatic_accounts_mapping.conf (well, at least easier for me). And it will be natural for [new] system administrator, who changed /etc/login.defs in some way, not to hope that it will affect selinux components in some other way. It will be natural for him to make an attempt to find corresponding selinux configuration parameters to correct them accordingly (if that seem appropriate for him). Still I wonder whether some other selinux components (in other libraries) depend on some old (more traditional than SELinux) UNIX configuration files in some way... Consider this as a subjective opinion of a mediocre system administrator, that have to deal with your creatures. Thanks for your time and many thanks for that creatures. -- Alexey G. Shpagin System administrator Volga Telecom -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs 2009-01-06 14:51 ` Manoj Srivastava 2009-01-06 15:09 ` Stephen Smalley @ 2009-01-06 16:50 ` Daniel J Walsh 1 sibling, 0 replies; 17+ messages in thread From: Daniel J Walsh @ 2009-01-06 16:50 UTC (permalink / raw) To: Daniel J Walsh, selinux, selinux-devel -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 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@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@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). > > 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? > > manoj Well that does not really solve my problem since UID_MAX is defined in the file as 60000, it will still stop looking there. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iEYEARECAAYFAklji7gACgkQrlYvE4MpobNx3QCgpt8y0aCWg1tDY70o1J+Xa1v3 vIwAoI4QyCgIm0DiwXs5mdnba9Wv7Op3 =y479 -----END PGP SIGNATURE----- -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without quotes as the message. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2009-01-28 11:02 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-01-05 22:45 [libsemanage] Also check for the uppoer bound on user ids in /etc/login.defs Manoj Srivastava 2009-01-05 22:56 ` Daniel J Walsh 2009-01-05 23:22 ` Manoj Srivastava 2009-01-06 12:52 ` Daniel J Walsh 2009-01-06 14:51 ` Manoj Srivastava 2009-01-06 15:09 ` Stephen Smalley 2009-01-06 16:30 ` Daniel J Walsh 2009-01-06 17:39 ` Stephen Smalley 2009-01-06 19:13 ` Daniel J Walsh 2009-01-07 0:41 ` Manoj Srivastava 2009-01-07 12:57 ` Stephen Smalley 2009-01-07 19:59 ` Manoj Srivastava 2009-01-07 20:36 ` Daniel J Walsh 2009-01-08 14:33 ` [DSE-Dev] " Manoj Srivastava 2009-01-08 15:44 ` Daniel J Walsh 2009-01-28 11:02 ` selinux 2009-01-06 16:50 ` Daniel J Walsh
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.