Date: Mon, 4 Jun 2012 11:42:15 +0200 From: Pawel Jakub Dawidek <pjd@FreeBSD.org> To: Bryan Drewery <bryan@shatow.net> Cc: freebsd-hackers@freebsd.org Subject: Re: [RFC] last(1) with security.bsd.see_other_uids support Message-ID: <20120604094214.GC1387@garage.freebsd.pl> In-Reply-To: <4FCC126C.1020600@shatow.net> References: <4FCC126C.1020600@shatow.net>
next in thread | previous in thread | raw e-mail | index | archive | help
--vtzGhvizbBRQ85DL
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Sun, Jun 03, 2012 at 08:42:04PM -0500, Bryan Drewery wrote:
> Hi,
>=20
> 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.
>=20
> 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 year=
s.
>=20
> Changes:
>=20
> * 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=3D0: users only see their own entries,
> as well as shutdown/boot/init times.
> * If security.bsd.see_other_uids=3D1, all entries are always shown, as it
> does now.
>=20
> Justifications:
>=20
> 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.
>=20
> 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!
>=20
> 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.
>=20
> 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:
>=20
> 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;
>=20
> /* Initialize utx.active with a single BOOT_TIME record. */
> - fd =3D _open(_PATH_UTX_ACTIVE, O_CREAT|O_RDWR|O_TRUNC, 0644);
> + fd =3D _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 =3D l;
> l =3D htobe16(l);
>=20
> - fd =3D _open(_PATH_UTX_LOG, O_CREAT|O_WRONLY|O_APPEND, 0644);
> + fd =3D _open(_PATH_UTX_LOG, O_CREAT|O_WRONLY|O_APPEND, 0660);
> if (fd < 0)
> return (-1);
> - if (_writev(fd, vec, 2) =3D=3D -1)
> + if (fchown(fd, 0, _UTMP_GID) < 0) {
> + warnx("Unable to set root:utmp on " _PATH_UTX_LOG);
> error =3D errno;
> - else
> - error =3D 0;
> + } else {
> + if (_writev(fd, vec, 2) =3D=3D -1)
> + error =3D errno;
> + else
> + error =3D 0;
> + }
> _close(fd);
> errno =3D error;
> return (error =3D=3D 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 =3D 1; /* Default to restricted access */
> + gid_t *groups;
> + int ngroups, gid, cnt;
> + long ngroups_max;
> + struct group *group;
> +
> + if (geteuid() =3D=3D 0 || see_other_uids)
> + restricted =3D 0;
> + else {
> + /* Check if the user is in a privileged group */
> + ngroups_max =3D sysconf(_SC_NGROUPS_MAX) + 1;
sysconf(3) can fail, at least in theory, so maybe:
ngroups_max =3D sysconf(_SC_NGROUPS_MAX);
if (ngroups_max =3D=3D -1)
ngroups_max =3D NGROUPS_MAX;
ngroups_max++;
> + if ((groups =3D malloc(sizeof(gid_t) * (ngroups_max))) =3D=3D NULL)
> + err(1, "malloc");
When this goes into library you has to return an error here.
> + ngroups =3D 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 =3D 0; cnt < ngroups; ++cnt) {
> + gid =3D groups[cnt];
> + group =3D getgrgid(gid);
> + /* User is in utmp or wheel group, they can see all */
> + if (strncmp("utmp", group->gr_name, 4) =3D=3D 0 || strncmp("wheel",
> group->gr_name, 5) =3D=3D 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) !=3D 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 =3D getpwuid(getuid());
And if getpwuid(3) fails?
> + len =3D 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 =3D 0;
> + restricted =3D is_user_restricted(pw, see_other_uids);
> +
> while ((ut =3D getutxent()) !=3D NULL) {
> + /* Skip this entry if the invoking user is not permitted
> + * to see it */
> + if (restricted &&
> + !(ut->ut_type =3D=3D BOOT_TIME ||
> + ut->ut_type =3D=3D SHUTDOWN_TIME ||
> + ut->ut_type =3D=3D OLD_TIME ||
> + ut->ut_type =3D=3D NEW_TIME ||
> + ut->ut_type =3D=3D 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.
--=20
Pawel Jakub Dawidek http://www.wheelsystems.com
FreeBSD committer http://www.FreeBSD.org
Am I Evil? Yes, I Am! http://tupytaj.pl
--vtzGhvizbBRQ85DL
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)
iEYEARECAAYFAk/MgvYACgkQForvXbEpPzSTawCgolAOwuol9MCBa0tiZS9ztvUE
VOYAoKpuzfWT8KizhPJGRbtHVAo+ybgd
=OBCt
-----END PGP SIGNATURE-----
--vtzGhvizbBRQ85DL--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120604094214.GC1387>