Details

Reviewers

markj
oshogbo

Commits

rS365378: traceroute6: capsicumize it

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

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

shubh created this revision.Jul 9 2020, 8:20 PM

Herald added a subscriber: jonathan. · View Herald TranscriptJul 9 2020, 8:20 PM

shubh requested review of this revision.Jul 9 2020, 8:20 PM

Updated the diff with context

oshogbo added inline comments.Jul 10 2020, 7:49 AM

usr.sbin/traceroute6/Makefile
28 ↗	(On Diff #74255)	We are missing MK_CASPER check.
usr.sbin/traceroute6/traceroute6.c
250 ↗	(On Diff #74255)	man style(9) First goyes "sys" then others inculudes. Typo: #include <stdio.h>
264 ↗	(On Diff #74255)	Why we need this check?
368 ↗	(On Diff #74255)	We don't need this extra line.
382 ↗	(On Diff #74255)	No extra lines.
392 ↗	(On Diff #74255)	Why we need this checks?
406 ↗	(On Diff #74255)	We are mixing defines, Why?
649 ↗	(On Diff #74255)	Style if (capdns != NULL) {
650 ↗	(On Diff #74255)	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 ↗	(On Diff #74255)	When using cap_enter you have to check for errno as well, the system may be build without the capsicum support.
976 ↗	(On Diff #74255)	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.

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

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

oshogbo added inline comments.Jul 20 2020, 7:38 AM

usr.sbin/traceroute6/traceroute6.c
250 ↗	(On Diff #74600)	This new line was fine.
347 ↗	(On Diff #74600)	Do we need this ifdef?
954 ↗	(On Diff #74600)	If we don't use cansandbox anymore, then maybe better would be using caph_enter_casper?
960 ↗	(On Diff #74600)	We have to check errno or use caph_rights_limit
264 ↗	(On Diff #74255)	This wasn't fixed.

Removed unnecessary ifdefs
Added some capsicum helper functions

oshogbo added inline comments.Jul 27 2020, 12:09 PM

usr.sbin/traceroute6/traceroute6.c
378 ↗	(On Diff #74718)	Style(9): VARIABLES NEW_LINE CODE.
634 ↗	(On Diff #74718)	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.
944 ↗	(On Diff #74718)	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.

Minor fixes

oshogbo added inline comments.Aug 3 2020, 8:59 AM

usr.sbin/traceroute6/traceroute6.c
298 ↗	(On Diff #74999)	Will getipnodebyname work in capablility mode?
389 ↗	(On Diff #74999)	I would use nitems instead of hardcodign the size.
392 ↗	(On Diff #74999)	Same.

This looks ok to me modulo the outstanding comments.

usr.sbin/traceroute6/Makefile
34 ↗	(On Diff #74999)	Please make sure these are consistently indented.
usr.sbin/traceroute6/traceroute6.c
298 ↗	(On Diff #74999)	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 */.
394 ↗	(On Diff #74999)	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.

This revision is now accepted and ready to land.Aug 4 2020, 2:23 PM

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()

This revision now requires review to proceed.Aug 5 2020, 2:21 AM

I'll do some testing of this.

usr.sbin/traceroute6/traceroute6.c
1527 ↗	(On Diff #75406)	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.
1529 ↗	(On Diff #75406)	Shouldn't it be "NAME2ADDR" and "ADDR2NAME"? The old limit names are supported for backward compatibility, but we should avoid adding new usages.