Page MenuHomeFreeBSD

Capsicumize traceroute6
ClosedPublic

Authored by shubh on Jul 9 2020, 8:20 PM.

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

Updated the diff with context

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

usr.sbin/traceroute6/traceroute6.c
264 ↗(On Diff #74255)

This wasn't fixed.

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

  • Removed unnecessary ifdefs
  • Added some capsicum helper functions
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.
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
shubh edited the summary of this revision. (Show Details)
  • 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.

This revision is now accepted and ready to land.Aug 5 2020, 3:22 PM
This revision was automatically updated to reflect the committed changes.