Erik Cederstrand
2012-Oct-01 10:31 UTC
Opinion on checking return value of setuid(getuid())?
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
Konstantin Belousov
2012-Oct-01 11:01 UTC
Opinion on checking return value of setuid(getuid())?
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 ? -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 196 bytes Desc: not available Url : http://lists.freebsd.org/pipermail/freebsd-security/attachments/20121001/396fd6b4/attachment.pgp
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On 10/1/12 3:31 AM, 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.I didn't follow the idea -- In Linux's kernel/sys.c: SYSCALL_DEFINE1(setuid, uid_t, uid) { (...) kuid = make_kuid(ns, uid); (...) if (nsown_capable(CAP_SETUID)) { new->suid = new->uid = kuid; if (!uid_eq(kuid, old->uid)) { // <-- 1 retval = set_user(new); // <-- check done here if (retval < 0) goto error; } How can the check be even reached in setuid(getuid()) case? It's also conflict with intuition by the way -- we are not changing ownership of the process, thus the process number should not change... Cheers, -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0.17 (Darwin) iQEcBAEBCAAGBQJQaXl0AAoJEG80Jeu8UPuz22AIAIBhAdEscXjcsQR06qzFntn4 lVVLzlPH+KdgUezbE5uMWbtNj0Az7ny66QQ2ocgh5KK8bc5i1486T9+32k6X7Cft gxE7tpPGkrb6uT62TV4Z5TkJ3NLfqQ6pABiYFONUS72Zy2zPE9stq5X4XrySXlTh Oft6hpLK5qtxucD7RUKrj8Ofw6kugKm7+KDXqQUU2CuEkCZZUiY1KarJK1fyPHF7 9APaaWyWZt6yMj3qn/2btmR4GZoZMQdfUqe8EIhpxGdKseB81FIdHfDo2bzDGRcx jUUIbrFxLTypjXws2IPneHYaKpLfs5RWT6yKPRkdKIkfQYTeJMb0MjlD6q7acWo=hknO -----END PGP SIGNATURE-----