Capsicumize traceroute6

[shubh]

Summary

The capsicum logic is pretty much the same that was used to capsicumize traceroute.

send_probe() changes the address in each iteration by incrementing the port number, which is not allowed in capability mode for UDP sockets.
Hence, the UDP socket was converted into a RAW socket, and its header was built in userspace. This way incrementing the port number didn't require any additional capability.

Also, these changes do not throw any extra warnings when WARNS?=3 is removed from the Makefile and hence this code can be built on D25603

Additional points:

• The unsandboxed code cannot run traceroute6 -T localhost, which is the same for the sandboxed code
• This code has only been tested for the localhost, since my service provider doesn't provide IPv6 connectivity

Test Plan

$ ktrace traceroute6 localhost
$ kdump | grep cap

[shubh]

Updated the diff with context

[oshogbo]

usr.sbin/traceroute6/Makefile

28

We are missing MK_CASPER check.

usr.sbin/traceroute6/traceroute6.c

250

man style(9)

First goyes "sys" then others inculudes.

Typo:
#include <stdio.h>

264

Why we need this check?

368

We don't need this extra line.

382

No extra lines.

392

Why we need this checks?

406

We are mixing defines, Why?

649

Style

if (capdns != NULL) {

650

If you would build this without HAVE_LIBCASPER the casper/cap_dns.h would not be included and this function prototype wouldn't exists.

970

When using cap_enter you have to check for errno as well, the system may be build without the capsicum support.

976

When using cap_enter you have to check for errno as well, the system may be build without the capsicum support.
There is no need to check 'cansandbox' here. If we are unable to enter capability mode we can still limit descriptors.

[shubh]

• Added MK_CASPER check to the Makefile
• Added WITH_CASPER defines instead of HAVE_LIBCASPER
• Checked for errno after cap_enter() call
• Removed cansandbox variable

[shubh]

Updated with context and removed a bug where it didn't work with the -I flag in the fist diff

[oshogbo]

usr.sbin/traceroute6/traceroute6.c

250

This new line was fine.

264

This wasn't fixed.

348

Do we need this ifdef?

959

If we don't use cansandbox anymore, then maybe better would be using caph_enter_casper?

965

We have to check errno or use caph_rights_limit

[shubh]

• Removed unnecessary ifdefs
• Added some capsicum helper functions

[oshogbo]

usr.sbin/traceroute6/traceroute6.c

382

Style(9):

VARIABLES
NEW_LINE
CODE.

648

What if casper doesn't exists? We still want to do getaddrinfo instead of cap_getaddrinfo right?
If casper is not build in all the cap_getaddrinfo will be replaced with getaddrinfo. So this check introduce a bug.

959

When using caph_* you don't need to check for errno:

The caph_enter, caph_rights_limit, caph_ioctls_limit and
caph_fcntls_limit are respectively equivalent to cap_enter(2),
cap_rights_limit(2), cap_ioctls_limit(2) and cap_fcntls_limit(2), it
returns success when the kernel is built without support of the
capability mode.

[shubh]

Minor fixes

[oshogbo]

usr.sbin/traceroute6/traceroute6.c

299

Will getipnodebyname work in capablility mode?

393

I would use nitems instead of hardcodign the size.

396

Same.

[markj]

This looks ok to me modulo the outstanding comments.

usr.sbin/traceroute6/Makefile

32

Please make sure these are consistently indented.

usr.sbin/traceroute6/traceroute6.c

299

It won't, but it is used before entering capability mode.

However, I don't think the substitution above makes sense. If a system doesn't have getipnodebyname(), it definitely won't have cap_gethostbyname2().

I would suggest deleting this block entirely, and add a comment above the getipnodebyname() saying that the call should be moved to after cap_enter(). e.g., /* XXX use after capability mode is entered */.

398

Since this block of code only sets a global variable, and the added local vars are unused afterward, let's move it into a separate function.

[shubh]

• Removed #ifndef HAVE_GETIPNODEBYNAME block completely and added a comment near the usage of getipnodebyname()
• Used nitems inside cap_dns_type_limit() and cap_dns_family_limit()
• Moved the logic for opening cap_dns to a new function capdns_open()

[markj]

I'll do some testing of this.

usr.sbin/traceroute6/traceroute6.c

1572

The parameter list should be void to match the declaration.

This function should also be static, like you did for the global variables, but I see that other functions in this file are not defined static, so we can fix it later all at once.

1574

Shouldn't it be "NAME2ADDR" and "ADDR2NAME"? The old limit names are supported for backward compatibility, but we should avoid adding new usages.