inetd: a light introduction to capsicum

[kevans]

Summary

This isn't a complete solution, but perhaps roughly the first half of one. This should effectively limit all fds opened by inetd, even those we hook up to child stdin/out/err, and enters capability mode for some of the builtin handlers.

I think the other half of the solution is to devise a syntax for inetd.conf to indicate that we should go ahead and enter capability mode for non-builtins.

[oshogbo]

Thank you for working on this!
This doesn't seems like simple peace of code.

usr.sbin/inetd/builtins.c

212

Is any of those time can be run in the capability program?
If so does we need caph_cache_tzdata ?

usr.sbin/inetd/inetd.c

347

Up to you: but this can be writen shorten:
caph_rights_limit(signalpipe[1], cap_rights_init(&sigtxpipe_rights, CAP_WRITE));

377

This is my private preference, so it's up to you what you will do with it:
I just prefer to have a fixed list of capability rights for the descriptors other ways it's getting compacted to say from the code what capabilities they are.

422

Why don't lmit it in the first if?

908

We don't need a caph_cache_catpages ?

912

Should we use casper service here for the syslog?

[kevans]

In D23022#504488, @oshogbo wrote:

Thank you for working on this!
This doesn't seems like simple peace of code.

Thanks for the review! Unfortunately it is a bit windy, and I'm not all that experienced with capsicum'izing things, but we'll get there. =-)

usr.sbin/inetd/builtins.c

212

Nope- those ones are confined to builtins that don't fork

usr.sbin/inetd/inetd.c

377

So, the problem is that I don't want to be duplicating the caph_stream_rights() because then that would need to be maintained, but I need them since the invoked program may attempt to caph_limit_stdio().

422

Will do

912

This branch doesn't get hit in the caph_enter() path, but both are things we'll need to consider going forward (that I hadn't previously) when we allow inetd.conf specifying that a service should be capsicumized.

[markj]

This looks like a good start. If you are not already doing so, please try testing with kern.trap_enotcap set to 1.

usr.sbin/inetd/builtins.c

80

I'd consider combining bi_fork and bi_capenter into a bi_flags variable. That'd make it easier to read the table.

85

Why do we not support cap mode for any of the UDP services? Seems like discard_dg should be trivial.

630

It reads a bit strangely to check fakeid_fd == -1 after calling caph_limit_stream(). You might consider extending the existing if-statement instead, i.e.

if (fakeid_fd == -1 ||
    caph_limit_stream(...) == -1 ||
    fstat(...) == -1 ||
    ...

usr.sbin/inetd/inetd.c

903

This could be collapsed into the following if-statement (for now). You are duplicating the sep->se_bi != NULL test.

[kevans]

Add a BIF_CAPENTER flag and address some other review commentary. The new
BIF_CAPENTER flag implies BIF_FORK.

[markj]

usr.sbin/inetd/builtins.c

632

Why caph_limit_stream()? fakeid_fd isn't wrapped with a FILE. I'm not sure why it's useful to limit rights here at all (fakeid_fd doesn't get inherited by children?), but cap_rights_limit(fakeid_fd, CAP_READ) would make more sense.

usr.sbin/inetd/inetd.c

300

ctrl_ioctls seems like a slightly clearer name.

388

Can this just be calloc()?

402

I don't quite understand this reasoning.

457

771

Are we limiting rights on the same socket each time the service is invoked?

[kevans]

usr.sbin/inetd/builtins.c

85

It's less about it being a UDP service and more about not forking for it -- I generally declined to touch any that we currently don't fork off in the first iteration.

632

Fair point; dropped entirely

usr.sbin/inetd/inetd.c

402

It might have made more sense to chop off the first sentence entirely, maybe something more like:

Service sockets are created via accept(2), thus inheriting the rights of the control
socket.  Therefore, control caps must be a superset of those needed by the services.
Apply rights here for both service sockets and control socket to make it easier to
audit that this is happening.

771

Reading it again, yeah... this would seem to boil down to setup_ctrl_caps(sep->se_fd, sep);

[markj]

usr.sbin/inetd/inetd.c

402

Ok, that seems fine to me.

771

Not the end of the world I guess, but worth an XXX comment or so if you prefer to keep it as is for now.