Opinion on checking return value of setuid(getuid())?

[Konstantin Belousov]

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