On 6/4/2012 4:42 AM, Pawel Jakub Dawidek wrote:
> On Sun, Jun 03, 2012 at 08:42:04PM -0500, Bryan Drewery wrote:
>> Questions:
>>
>> To add this support to w(1) and who(1), I want to share the
>> is_user_restricted() function among all 3 binaries. I don't think this
>> really belongs in libc/libutil, but maybe it does. I could just add a
>> shared file into usr.bin/last/ and link it in with all 3, but I don't
>> really like this approach as then usr.bin/{w,who} would depend on
>> usr.bin/last.
>
> A library is definiately a better place, although then I wouldn't pass
> see_other_uids as an argument, but obtain it within the function itself.
Pawel,
Thank you for the input.
I will cleanup your findings and experiment with moving it into the library.
More comments below.
>
> Some comments to the code below.
>
>> -/var/log/utx.log 644 3 * @01T05 B
>> +/var/log/utx.log root:utmp 660 3 * @01T05 B
>
> Why does the utmp group has to have write access to this file.
> If I understand correctly it just reads from it, no?
I suppose I did this from experience and other custom changes. It does
make sense to limit it to root writing, but then apps like screen can
not write to it if not setuid root.
That's outside the scope of this change though and potentially wrong, so
I will make this 640 for now.
>
>> --- a/lib/libc/gen/pututxline.c
>> +++ b/lib/libc/gen/pututxline.c
>> @@ -179,10 +179,13 @@
>> int fd;
>>
>> /* Initialize utx.active with a single BOOT_TIME record. */
>> - fd = _open(_PATH_UTX_ACTIVE, O_CREAT|O_RDWR|O_TRUNC, 0644);
>> + fd = _open(_PATH_UTX_ACTIVE, O_CREAT|O_RDWR|O_TRUNC, 0660);
>> if (fd < 0)
>> return;
>> - _write(fd, fu, sizeof(*fu));
>> + if (fchown(fd, 0, _UTMP_GID) < 0)
>> + warnx("Unable to set root:utmp on " _PATH_UTX_ACTIVE);
>> + else
>> + _write(fd, fu, sizeof(*fu));
>> _close(fd);
>> }
>
> fchown() doesn't hurt here, I guess, but I would use UID_ROOT instead of 0.
> Doing explicit fchmod(2) might make sense too, as umask might cut some
> of the requested permissions.
>
>> @@ -269,13 +272,18 @@
>> vec[1].iov_len = l;
>> l = htobe16(l);
>>
>> - fd = _open(_PATH_UTX_LOG, O_CREAT|O_WRONLY|O_APPEND, 0644);
>> + fd = _open(_PATH_UTX_LOG, O_CREAT|O_WRONLY|O_APPEND, 0660);
>> if (fd < 0)
>> return (-1);
>> - if (_writev(fd, vec, 2) == -1)
>> + if (fchown(fd, 0, _UTMP_GID) < 0) {
>> + warnx("Unable to set root:utmp on " _PATH_UTX_LOG);
>> error = errno;
>> - else
>> - error = 0;
>> + } else {
>> + if (_writev(fd, vec, 2) == -1)
>> + error = errno;
>> + else
>> + error = 0;
>> + }
>> _close(fd);
>> errno = error;
>> return (error == 0 ? 0 : 1);
>
> This looks very familiar to the above. Maybe we can get rid of this code
> duplication?
>
>> /*
>> + * Return whether or not the given user can see all entries or not
>> + */
>> +static int
>> +is_user_restricted(struct passwd *pw, int see_other_uids)
>> +{
>> + int restricted = 1; /* Default to restricted access */
>> + gid_t *groups;
>> + int ngroups, gid, cnt;
>> + long ngroups_max;
>> + struct group *group;
>> +
>> + if (geteuid() == 0 || see_other_uids)
>> + restricted = 0;
>> + else {
>> + /* Check if the user is in a privileged group */
>> + ngroups_max = sysconf(_SC_NGROUPS_MAX) + 1;
>
> sysconf(3) can fail, at least in theory, so maybe:
>
> ngroups_max = sysconf(_SC_NGROUPS_MAX);
> if (ngroups_max == -1)
> ngroups_max = NGROUPS_MAX;
> ngroups_max++;
>
>> + if ((groups = malloc(sizeof(gid_t) * (ngroups_max))) == NULL)
>> + err(1, "malloc");
>
> When this goes into library you has to return an error here.
>
>> + ngroups = ngroups_max;
>> + (void) getgrouplist(pw->pw_name, pw->pw_gid, groups, &ngroups);
>
> You know that getgrouplist(3) returns groups from the system files and
> not actuall process groups? Was that intended? IMHO you should use
> getgroups(2) here. And again you ignore return value.
Most of this section was copied from usr.bin/id/id.c
I will review both and take your findings into consideration.
>
>> + for (cnt = 0; cnt < ngroups; ++cnt) {
>> + gid = groups[cnt];
>> + group = getgrgid(gid);
>> + /* User is in utmp or wheel group, they can see all */
>> + if (strncmp("utmp", group->gr_name, 4) == 0 ||
>> strncmp("wheel",
>> group->gr_name, 5) == 0) {
>
> strncmp(3) is bad idea here. If the user is a member of utmpfoo group or
> wheelx group you turn off restrictions.
Woops!
>
> I'd really use getgroups(2) and look for GID_WHEEL or _UTMP_GID.
>
>> @@ -212,7 +255,30 @@ struct idtab {
>> /* Load the last entries from the file. */
>> if (setutxdb(UTXDB_LOG, file) != 0)
>> err(1, "%s", file);
>> +
>> + /* drop setgid now that the db is open */
>
> Style: Sentence should start with capital letter and end with a period.
>
>> + setgid(getgid());
>
> And if setgid(2) fails?
>
>> + /* Lookup current user information */
>
> Style: Sentence should end with a period.
>
>> + pw = getpwuid(getuid());
>
> And if getpwuid(3) fails?
>
>> + len = sizeof(see_other_uids);
>> + if (sysctlbyname("security.bsd.see_other_uids", &see_other_uids, &len,
>> NULL, 0))
>
> sysctlbyname(3) doesn't return bool.
>
>> + see_other_uids = 0;
>> + restricted = is_user_restricted(pw, see_other_uids);
>> +
>> while ((ut = getutxent()) != NULL) {
>> + /* Skip this entry if the invoking user is not permitted
>> + * to see it */
>> + if (restricted &&
>> + !(ut->ut_type == BOOT_TIME ||
>> + ut->ut_type == SHUTDOWN_TIME ||
>> + ut->ut_type == OLD_TIME ||
>> + ut->ut_type == NEW_TIME ||
>> + ut->ut_type == INIT_PROCESS) &&
>> + strncmp(ut->ut_user, pw->pw_name, sizeof(ut->ut_user)))
>
> That's one complex if. And again strncmp(3) used instead of strcmp(3).
> Also strncmp(3) doesn't return bool. If getpwuid(3) failed earlier you
> have NULL pointer dereference here.
>
As for the "bool" checks, I was checking for non-zero, but I will make
it explicit.
Regards,
Bryan Drewery
signature.asc
Description: OpenPGP digital signature
