Date: Mon, 1 Oct 2012 13:49:01 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Erik Cederstrand <erik@cederstrand.dk> Cc: "freebsd-security@freebsd.org" <freebsd-security@freebsd.org> Subject: Re: Opinion on checking return value of setuid(getuid())? Message-ID: <20121001104901.GJ35915@deviant.kiev.zoral.com.ua> In-Reply-To: <9DD86238-51C8-4F38-B7EB-BD773039888B@cederstrand.dk> References: <9DD86238-51C8-4F38-B7EB-BD773039888B@cederstrand.dk>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --] On Mon, Oct 01, 2012 at 12:31:21PM +0200, Erik Cederstrand wrote: > I'm looking through the clang analyzer reports and found this one: http://scan.freebsd.your.org/freebsd-head/sbin.ping/2012-09-30-amd64/report-R9ZgC6.html#EndPath > > It's complaining that, if setuid() fails for some reason, the process will continue with root privileges because the process is suid root. > > At first glance, it seems unnecessary to check the return value of "setuid(getuid())" since the user should always be able to drop privileges to itself. So I filed this bug with LLVM: http://llvm.org/bugs/show_bug.cgi?id=13979 > > It turns out that setuid() *may* fail if the user hits its process limit. Apparently FreeBSD doesn't check the limit in the specific setuid(getuid()) case (I can't find the code anywhere right now) so this is not an issue, but Linux does. However, if FreeBSD decides to change the setuid() implementation at some point, the issue may surface again. > > A simple fix would be something like: > > Index: /freebsd/repos/head_scratch/src/sbin/ping/ping.c > =================================================================== > --- /freebsd/repos/head_scratch/src/sbin/ping/ping.c (revision 240960) > +++ /freebsd/repos/head_scratch/src/sbin/ping/ping.c (working copy) > @@ -255,7 +255,8 @@ > s = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP); > sockerrno = errno; > > - setuid(getuid()); > + if (setuid(getuid()) != 0) > + err(EX_NOPERM, "setuid() failed"); > uid = getuid(); > > alarmtimeout = df = preload = tos = 0; > > > There's an alternative approach for NetBSD with a patch to kern_exec.c here: http://mail-index.netbsd.org/tech-security/2008/01/12/msg000026.html but I have no idea if this applies to FreeBSD. > > I'd like an opinion on which way to go before filing PRs because we have around 200 of these warnings in the FreeBSD repo. > > Thanks, > Erik_______________________________________________ setuid() might also fail for other reasons, e.g. due to custom MAC module. In case of ping, does the failure of dropping the suid bit is important ? [-- Attachment #2 --] -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (FreeBSD) iEYEARECAAYFAlBpdR0ACgkQC3+MBN1Mb4ggdgCgsSvcMGGhjl+hLr2f4R7jfQNs jnwAn2E+gAplg2dhGGUcWqMIpmQf+/l7 =68KI -----END PGP SIGNATURE-----help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121001104901.GJ35915>