capsicum and ping(8)

[Pawel Jakub Dawidek]

On Thu, Jan 09, 2014 at 08:19:04PM +0400, mp39590 at gmail.com wrote:
> Hello.
> 
> I would like to propose a patch for ping(8), which adds support for
> capability mode sandbox.
> 
> The goals for this little project were:
> 
> a) to see what problems/burdens could be faced with compartmentalization
>    of a network utility;
> 
> b) increase security level of the day-to-day application with minimal
>    intrusion to the code.
> 
> To summarize on 'a)', following changes were made in original ping to
> meet capsicum requirements:
> 
> 1) sendto() was replaced with connect()+send() pair, since we're not
>    allowed to issue sendto() with non-NULL destination;
> 
> 2) one socket 's' was replaced with two sockets 's' for sending and 's1'
>    for receiving. It was done for special use case, when user ping
>    multicast or broadcast address. As connect() man page states, socket
>    is allowed to receive messages only from address to which it was
>    connect()'ed and this is nonsense for multicast/broadcast;
> 
> 3) pr_addr() function has been slightly rewritten to support casper
>    daemon and its cap_gethostbyaddr() function;
> 
> 4) some setsockopts() were adjusted, since we use two sockets instead of
>    one.
> 
> Place for cap_enter() call was chosen to balance simplicity of the logic
> for entering capability mode, code changes and protection from
> potentially dangerous place (receiving/"parsing" packets from the
> network).

Now that you added casper to the game, I'd move gethostbyname2() until
after we  enter the sandbox and open system.dns service, but before we
limit the service to only reverse lookups. It does process network
packets after all.

In practise this doesn't change much currently, as casper process
responsible for doing DNS lookups is not sandboxed, but in the future it
might be sandboxed using different techniques and would be nice to get
this extra protection for free.

> Finally, this compartmentalization logic will apply:
> 
>  - If '-n' (numeric output) flag is given - enter capability mode;
> 
>  - Else, if build WITH_CASPER: try to communicate with it, on fail issue
>    warning and proceed without capsicum, if cap_init() is successful all
>    other casper errors (e. g. not being able to initialize DNS services)
>    treated as fatal and ping aborts;
> 
>  - Else, if build WITHOUT_CASPER: proceed without capsicum.
> 
> Also, please note, that ping has '-d' flag, which turn on SO_DEBUG
> setsockopt() and its behavior depends on external code (which also
> doesn't exist not, but could be written in future). If we enter capsicum
> with this option (although, I'm sure it's not widely used) this (future)
> external code may not work completely, since capsicum impose a lot of
> restrictions.
> 
> I would like to ask your comments/reviews on this patch and approach.
> 
> Thanks to Gleb Smirnoff, Pawel Jakub Dawidek and Robert Watson for
> helping me with some tricky capsicum things, which I tried to summarize
> here.
> 
> Be well.

Great! The patch overall looks very nice and complete, good work.
Few minor nits inline.

> --- sbin/ping/ping.c	(revision 260485)
> +++ sbin/ping/ping.c	(working copy)
[...]
> +int s;				/* send socket file descriptor */
> +int s1;				/* receive socket file descriptor */

I'd much prefer to use some more meaningful variable names. Like 'ssend'
and 'srecv' or something similar.

> +#ifdef HAVE_LIBCAPSICUM
> +cap_channel_t *capdns;
> +#endif /* HAVE_LIBCAPSICUM */

Not sure why other globals variables aren't static, but it should be
static. I don't think it is used anywhere else outside this source file.

>  	s = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP);
>  	sockerrno = errno;
> +	s1 = socket(AF_INET, SOCK_RAW, IPPROTO_ICMP);
> +	sock1errno = errno;

I wonder if dup(2) would be more intuitive here. Looking at the code I
expect those two sockets to be different, but they aren't. dup(2) would
make that 100% clear. I'll leave it to you to decide, I don't have a
strong opinion on this, really.

I'd still prefer to see the explanation you gave in the e-mail why do we
need those two sockets in a comment.

> +	if (options & F_NUMERIC)
> +		cansandbox = 1;

Can you make 'cansandbox' to be of type 'bool'?

> +	/*
> +	 * Here we enter capapility mode (see capsicum(4)). Further down
> +	 * creation of new file descriptors is forbidden.

This is not entirely true. You can create file descriptors with dup(2),
dup2(2), fcntl(F_DUP2FD), socket(2), etc. What you can't do is to
address global namespaces. I'd clarify that comment.

> +	cap_rights_init(&rights_s1, CAP_RECV, CAP_EVENT, CAP_SETSOCKOPT);
> +	if (cap_rights_limit(s1, &rights_s1) < 0 && errno != ENOSYS)
> +		err(1, "cap_rights_limit socket1");
> +
> +	cap_rights_init(&rights_s, CAP_SEND, CAP_SETSOCKOPT);
> +	if (cap_rights_limit(s, &rights_s) < 0 && errno != ENOSYS)
> +		err(1, "cap_rights_limit socket");
[...]
> +	cap_rights_clear(&rights_s1, CAP_SETSOCKOPT);
> +	if (cap_rights_limit(s1, &rights_s1) < 0 && errno != ENOSYS)
> +		err(1, "cap_rights_limit socket1 setsockopt");
[...]
> +	/*
> +	 * We don't call setsockopt() anywhere further for 's', we don't need
> +	 * corresponding capability, drop it.
> +	 */
> +	cap_rights_clear(&rights_s, CAP_SETSOCKOPT);
> +	if (cap_rights_limit(s, &rights_s) < 0 && errno != ENOSYS)
> +		err(1, "cap_rights_limit socket setsockopt");

This made me wonder. We have two choices here: either use
cap_rights_init() first and then drop capability rights we don't need
anymore with cap_rights_clear() as you did or to use cap_rights_init()
twice and provide list of capability rights explicitly. I think I'm more
in favour of providing capability rights explicitly. If we add some
additional rights in the future to the first cap_rights_init() we may
forget add them to cap_rights_clear() below. That's why I'd change the
code not to use cap_rights_clear(), but add a comment above second
cap_rights_init() round saying which right we are removing. As a nice
side-effect this would allow us to use only one 'rights' variable.

I know this is nitpicking, but the Capsicum-related changes we are doing
today will serve as examples tomorrow, so I'd like them to be just right.

-- 
Pawel Jakub Dawidek                       http://www.wheelsystems.com
FreeBSD committer                         http://www.FreeBSD.org
Am I Evil? Yes, I Am!                     http://mobter.com
-------------- 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/20140109/3ce702b9/attachment.sig>