Page MenuHomeFreeBSD

Capsicumize traceroute6
ClosedPublic

Authored by shubh on Jul 9 2020, 8:20 PM.
Referenced Files
Unknown Object (File)
Tue, Mar 12, 3:42 AM
Unknown Object (File)
Tue, Mar 12, 3:42 AM
Unknown Object (File)
Tue, Mar 12, 3:42 AM
Unknown Object (File)
Tue, Mar 12, 3:38 AM
Unknown Object (File)
Tue, Mar 12, 3:38 AM
Unknown Object (File)
Tue, Mar 12, 3:38 AM
Unknown Object (File)
Tue, Mar 12, 3:37 AM
Unknown Object (File)
Tue, Mar 12, 3:37 AM

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

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

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

Updated the diff with context

usr.sbin/traceroute6/Makefile
30–36

We are missing MK_CASPER check.

usr.sbin/traceroute6/traceroute6.c
249–250

man style(9)

First goyes "sys" then others inculudes.

Typo:
#include <stdio.h>

263

Why we need this check?

367

We don't need this extra line.

405

No extra lines.

415

Why we need this checks?

429

We are mixing defines, Why?

645

Style

if (capdns != NULL) {

646

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

965

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

971

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
250

This new line was fine.

263

This wasn't fixed.

347

Do we need this ifdef?

954

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

960

We have to check errno or use caph_rights_limit

  • Removed unnecessary ifdefs
  • Added some capsicum helper functions
usr.sbin/traceroute6/traceroute6.c
380

Style(9):

VARIABLES
NEW_LINE
CODE.
644

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.

954

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

Will getipnodebyname work in capablility mode?

391

I would use nitems instead of hardcodign the size.

394

Same.

This looks ok to me modulo the outstanding comments.

usr.sbin/traceroute6/Makefile
34

Please make sure these are consistently indented.

usr.sbin/traceroute6/traceroute6.c
298

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

396

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
1556

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.

1558

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.