Date: Mon, 04 Jun 2012 07:56:09 -0500 From: Bryan Drewery <bryan@shatow.net> To: Pawel Jakub Dawidek <pjd@FreeBSD.org> Cc: freebsd-hackers@freebsd.org Subject: Re: [RFC] last(1) with security.bsd.see_other_uids support Message-ID: <4FCCB069.90808@shatow.net> In-Reply-To: <20120604094214.GC1387@garage.freebsd.pl> References: <4FCC126C.1020600@shatow.net> <20120604094214.GC1387@garage.freebsd.pl>
next in thread | previous in thread | raw e-mail | index | archive | help
This is an OpenPGP/MIME signed message (RFC 2440 and 3156)
--------------enig83EF10B1304347AE9444FAB5
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
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.
>=20
> 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=
=2E
Pawel,
Thank you for the input.
I will cleanup your findings and experiment with moving it into the libra=
ry.
More comments below.
>=20
> Some comments to the code below.
>=20
>> -/var/log/utx.log 644 3 * @01T05 B
>> +/var/log/utx.log root:utmp 660 3 * @01T05 B
>=20
> 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.
>=20
>> --- 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 =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);
>> }
>=20
> fchown() doesn't hurt here, I guess, but I would use UID_ROOT instead o=
f 0.
> Doing explicit fchmod(2) might make sense too, as umask might cut some
> of the requested permissions.
>=20
>> @@ -269,13 +272,18 @@
>> vec[1].iov_len =3D l;
>> l =3D htobe16(l);
>>
>> - 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);
>=20
> This looks very familiar to the above. Maybe we can get rid of this cod=
e
> duplication?
>=20
>> /*
>> + * 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;
>=20
> sysconf(3) can fail, at least in theory, so maybe:
>=20
> ngroups_max =3D sysconf(_SC_NGROUPS_MAX);
> if (ngroups_max =3D=3D -1)
> ngroups_max =3D NGROUPS_MAX;
> ngroups_max++;
>=20
>> + if ((groups =3D malloc(sizeof(gid_t) * (ngroups_max))) =3D=3D NULL)=
>> + err(1, "malloc");
>=20
> When this goes into library you has to return an error here.
>=20
>> + ngroups =3D ngroups_max;
>> + (void) getgrouplist(pw->pw_name, pw->pw_gid, groups, &ngroups);
>=20
> 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.
>=20
>> + 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) {
>=20
> strncmp(3) is bad idea here. If the user is a member of utmpfoo group o=
r
> wheelx group you turn off restrictions.
Woops!
>=20
> I'd really use getgroups(2) and look for GID_WHEEL or _UTMP_GID.
>=20
>> @@ -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 */
>=20
> Style: Sentence should start with capital letter and end with a period.=
>=20
>> + setgid(getgid());
>=20
> And if setgid(2) fails?
>=20
>> + /* Lookup current user information */
>=20
> Style: Sentence should end with a period.
>=20
>> + pw =3D getpwuid(getuid());
>=20
> And if getpwuid(3) fails?
>=20
>> + len =3D sizeof(see_other_uids);
>> + if (sysctlbyname("security.bsd.see_other_uids", &see_other_uids, &le=
n,
>> NULL, 0))
>=20
> sysctlbyname(3) doesn't return bool.
>=20
>> + 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)))
>=20
> 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
As for the "bool" checks, I was checking for non-zero, but I will make
it explicit.
Regards,
Bryan Drewery
--------------enig83EF10B1304347AE9444FAB5
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQIcBAEBAgAGBQJPzLBpAAoJEG54KsA8mwz5bXIP/1v/vj5QhDntm5otqv/rhzO6
Ea0CECOvFS8rM6suyNzgdo3J1XPVSJE1l9DSAFBxFmN1CTReOJkHes0kx9OkPREh
2V9xCtkxxvNWSVrSvXJu1Utxst8DS1KiZFH055f4Kf4DElP11EfBdrAyAEDIzkyF
Ns9J8gvr6J17b0yqfxYVl5zbwHO/QOm5xy4XisnbKsgsJNtUPck6IYflEwYLJi+7
UKyZIvc77xS9tnJVv81OIoc33kLdJrJQesARkxHBTd4/USmKuhhoR3kyzTDhA18l
ewzIpYVrlu6/t2hr9LyORmgekbJxLS0ZLNt3kU7sndv+p1rEdUlBNXtX9QtB89fw
sMj3aL8SfVuNUmqT8e9DtkrCQvQ+RIsJnu2XQiLjmhpOLUiPYRG/VKt7qOencK/Y
1YI89MJruE5t7zpWmAvHpWiZUeJgsE/kWOIhm/+UAlceo0EXSDUCGsn3bZeBIubo
23VXlzIupPl/NGmhFX/bkIwBHTGyY45XvbuwZxzJQr73exYj6ijvrOro15Hh2yxX
4d8X/S7wzQ5JFUj6NS5YQbdGTZoUjW1y6X6u7x+uYDCJT5Fd/NHUlrZ6qhbK8ewm
bifjO3/lNACMZfDgvQMK4cEve3soNVaDYEoJOpu6zQLgKuYhpXR/FhL0CXcZcb1x
Jdm1oU4Cg793420UhBTq
=3EyX
-----END PGP SIGNATURE-----
--------------enig83EF10B1304347AE9444FAB5--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4FCCB069.90808>