From owner-freebsd-hackers@FreeBSD.ORG Mon Jun 4 12:56:14 2012 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id E42751065673 for ; Mon, 4 Jun 2012 12:56:14 +0000 (UTC) (envelope-from bryan@shatow.net) Received: from secure.xzibition.com (secure.xzibition.com [173.160.118.92]) by mx1.freebsd.org (Postfix) with ESMTP id 77ED78FC14 for ; Mon, 4 Jun 2012 12:56:14 +0000 (UTC) DomainKey-Signature: a=rsa-sha1; c=nofws; d=shatow.net; h=message-id :date:from:mime-version:to:cc:subject:references:in-reply-to :content-type; q=dns; s=sweb; b=hANS7YbIdNmh+boOpw6LEh/CsEUDNmMs r6yNPlzdZMJ9a/1Im9RxIFPEb/iFre4kfthhmbiCgPOH+1k/3Vw6TbI6sDVqm2HY rLDxNw1nXxt2nDuJ7BdsjMsARWIhhJtJxHo7QGSVDK5dMrRoEJgj6L9FUgM0iU2q Ez5fFCz/G/8= DKIM-Signature: v=1; a=rsa-sha256; c=simple; d=shatow.net; h=message-id :date:from:mime-version:to:cc:subject:references:in-reply-to :content-type; s=sweb; bh=CzpfRhR9nRBYO3SvmkRLFyn5fTIByCyBZgnTw6 77vCM=; b=18PU+yQY4uhaE57N9ZqoMAfeZrCqSgr/9rTCr5KpyUnQyNLnj04+sz QSisgPUnyK3U+8bYLIMnmHlySdkeRKTFNO7aLj4wD9a/6WL4ZmyFAxrCP4f+6xKE N4cmIjAZ6rAloRwvmYpcOMkgqbmLrdf4oFE4Sdq79IXvs7u0rUEX4= Received: (qmail 72521 invoked from network); 4 Jun 2012 07:56:12 -0500 Received: from unknown (HELO ?10.10.1.87?) (bryan@shatow.net@10.10.1.87) by sweb.xzibition.com with ESMTPA; 4 Jun 2012 07:56:12 -0500 Message-ID: <4FCCB069.90808@shatow.net> Date: Mon, 04 Jun 2012 07:56:09 -0500 From: Bryan Drewery User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20120428 Thunderbird/12.0.1 MIME-Version: 1.0 To: Pawel Jakub Dawidek References: <4FCC126C.1020600@shatow.net> <20120604094214.GC1387@garage.freebsd.pl> In-Reply-To: <20120604094214.GC1387@garage.freebsd.pl> X-Enigmail-Version: 1.4.1 OpenPGP: id=3C9B0CF9; url=http://www.shatow.net/bryan/bryan.asc Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig83EF10B1304347AE9444FAB5" Cc: freebsd-hackers@freebsd.org Subject: Re: [RFC] last(1) with security.bsd.see_other_uids support X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Jun 2012 12:56:15 -0000 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--