[Nagiosplug-devel] check_ping and check_icmp confusion

[Andreas Ericsson]

Jason Crawford wrote:
> On 2/22/06, Andreas Ericsson <ae at op5.se> wrote:
> 
>>
>>On a side note, both ping and check_icmp drop their root-privileges
>>(unless run as root, anyways) immediately after obtaining the raw socket
>>necessary to send layer 2 packets.
>>
>>However, if this is a very large concern for lots of people I could make
>>check_icmp do all its work inside a chroot(2) jail. Then you'd be safer
>>running check_icmp than /bin/ping.
> 
> 
> Well I was more referring to the fact that the system's ping binary
> has gone through very extensive and thorough security audits (it's
> been around for how many years?)


Since 1968, I believe. I don't think very many lines of the original 
code remain anywhere now though.

> so there are less likely to be issues
> in the privileged code.


I'm assuming you mean the lines of code that runs with root privileges 
here. These are the interesting parts of check_icmp.c:

int main(int argc, char **argv)
{
	int sock_recv_buf = IP_MAX_PACKET + 128;

	icmp_sock = socket(PF_INET, SOCK_RAW, IPPROTO_ICMP);
	setsockopt(icmp_sock, SOL_SOCKET, SO_RCVBUF,
		(void *)&sock_recv_buf, sizeof(sock_recv_buf));
	setsockopt(icmp_sock, SOL_SOCKET, SO_SNDBUF,
		(void *)&sock_recv_buf, sizeof(sock_recv_buf));

	/* drop privileges (no effect if not setsuid or !geteuid() */
	setuid(getuid());

	....
}

There are more variables, and naturally I check the result of the 
syscalls. My point remains though. Only three actions take place, in any 
path of execution, prior to dropping the privileges. Each and every one 
with hard-coded values. This is done prior to parsing the arguments, so 
userland or network input never go into the program while running as 
root, ever.


> However, a chroot(2) jail would make it even
> better.
> 

Not necessarily. Unless it's hardcoded one has to slurp it from the 
command line arguments or someplace else, which means parsing userland 
input, or reading an environment variable, as root (ordinary users 
aren't allowed to chroot(2)).

What I'm saying here is that it might make it more secure (although I 
doubt it's needed). However that goes though, people will wonder why 
their plugins start saying

check_icmp: Failed to chroot() to '/var/empty': No such file or directory.

I'm not particularly interested in answering that stream of questions, 
but I could provide a non-official patch that you and other as 
security-minded as you could use though.

-- 
Andreas Ericsson                   andreas.ericsson at op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231