capsicum and ping(8)

[Mikhail]

Hello, Pawel!

On 00:02 10-Jan 2014 Pawel Jakub Dawidek wrote:
> 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.

I've moved casper initialization before our first gethostbyname2() and
added statements to use those, if they're available. I didn't move
cap_enter() for now, since our hands are still tied with bind() and
connect() system calls which use the results of those DNS lookups.

Also, code, limiting DNS resolution only to 'ADDR' after those
gethostbyname2()'s has been added.

> [...]
> > +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.

Fixed.

> > +#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.

Fixed.

> >  	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.

Comment has been added.

I've tried dup(), but seem it duplicates just a reference to an object,
but not the object itself, and we end up with one socket (as in old
ping) and two references to it, and this socket is failing to receive
multicast/broadcast. Maybe I misunderstood your proposition.

> > +	if (options & F_NUMERIC)
> > +		cansandbox = 1;
> 
> Can you make 'cansandbox' to be of type 'bool'?
 
Fixed.

> > +	/*
> > +	 * 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.

Fixed.

> > +	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.
 
Fixed.

Also, I've fixed few other things which were found:

 - Makefile is changed to not to define 'HAVE_LIBCAPSICUM', if 'RESCUE'
   is defined, it allows us to pass 'rescue' build since for now we
   can't link it with libcapsicum (changing Makefile looks more neat
   than .c file);

 - clearing CAP_SETSOCKOPT for 'srecv' was moved lower;

 - closing comments for short #ifdef/#endifs were removed;

 - libnv references were removed from Makefile, since we don't use it
   directly.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: ping_20140113.patch
Type: text/x-diff
Size: 13305 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-security/attachments/20140113/b4f6831f/attachment.patch>