On Sun, Jun 03, 2012 at 08:42:04PM -0500, Bryan Drewery wrote: > Hi, > > I've written up a patch to add some privacy to last(1) while still > giving non-privileged users access to their own login history. > > This is still a work in progress. I am reaching out to make sure my > approach is proper and to get some input on code sharing. My goal is to > add this support to w(1) and who(1) as well. FWIW I have been running a > similar patch on my own shared-hosting systems (pre-utmpx) for a few years. > > Changes: > > * Added utmp group > * All utmpx files are 660 root:utmp > * last(1) runs setgid(utmp) and drops this as soon as the utmpx files > are opened. > * Users in the wheel or utmp group can see all entries > * IFF security.bsd.see_other_uids=0: users only see their own entries, > as well as shutdown/boot/init times. > * If security.bsd.see_other_uids=1, all entries are always shown, as it > does now. > > Justifications: > > Why the changes? This makes sense for shared hosting environments where > jails are not practical. A user should be able to see their own login > history, to see if someone has been accessing their account, but not to > see the IPs of other users. The intention is *not* to disallow them to > see that other users of the system. Obviously they can just cat > /etc/passwd. This is just about IP privacy. > > Why the setgid? Allow reading the entries, but disallow directly opening > the utx files. I've seen some shared hosts incorrectly chmod 0 > /usr/bin/last, but still leave the utmp files wide open for reading! > > Why the utmp group? It's consistent with other systems (OpenBSD, Linux), > and allows giving a user access to see all entries, without granting > them wheel or allowing a non-privileged user to run as setgid(wheel). It > also helps mitigates security concerns by using a specific group only > having extra privilege to utmpx files. > > I originally had not planned for security.bsd.see_other_uids, but > considering POLA and consistency, it makes sense.
I think the proposed change does make sense.
> 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.
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?
> --- 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.
> + 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.
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.
--
Pawel Jakub Dawidek http://www.wheelsystems.com
FreeBSD committer http://www.FreeBSD.org
Am I Evil? Yes, I Am! http://tupytaj.pl
pgp2ssyiX0rVx.pgp
Description: PGP signature
