Remove chroot(2) call from dhclient.
ClosedPublic
Actions

Authored by markj on Aug 3 2018, 8:34 PM.

Details

Reviewers

emaste
cem
jilles
oshogbo

Group Reviewers

manpages

Commits

rS337382: dhclient: Don't chroot if we are in capability mode.

Summary

dhclient runs in capability mode, so its filesystem access is already
restricted. Moreover, libutil keeps open a directory descriptor for the
pidfile (/var/run by default). If kern.chroot_allow_open_directories is
not set, this descriptor will cause the chroot to fail. To fix this,
stop using chroot(), and store pidfiles under /var/run/dhclient/ instead
to restrict the scope of libutil's descriptor. This is not perfect
since different dhclient instances can still mess with each other's
pidfiles despite the use of capability mode, but I don't think that this
is a serious problem.

Also stop removing rights from the pidfile descriptor after writing the
PID. This interferes with pidfile_verify(), called from
pidfile_close(). We already restrict rights to the pidfile somewhat in
libutil; one possible follow-up enhancement would be drop the
CAP_PWRITE and CAP_TRUNCATE rights in pidfile_write(). Of course, this
would mean that pidfile_write() could only be called once, but that's
most likely fine.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 18583
Build 18275: arc lint + arc unit

Event Timeline

markj created this revision.Aug 3 2018, 8:34 PM

Harbormaster completed remote builds in B18565: Diff 46267.Aug 3 2018, 8:34 PM

markj edited the summary of this revision. (Show Details)Aug 3 2018, 8:35 PM

markj added reviewers: emaste, cem, jilles, oshogbo.

cem accepted this revision.Aug 3 2018, 9:05 PM

cem added inline comments.

sbin/dhclient/dhclient.c
423	For whatever reason, _PATH_VARRUN has already got a trailing slash. Not that it matters too much for lookup, and I agree this is more clear. I think spelling it without the %s might also be more clear? `asprintf(..., _PATH_VARRUN "/...", *argv)` The uppercase name implies it is a macro, rather than some extern variable. I'm not sure if that makes it reasonable to expect it to be a string literal or not.
531–534	I might have missed earlier discussion, but why not just wrap this in `if (cap_getmode(&dummy) != 0) {`? (I.e., kernel does not support CAPABILITY_MODE.)

This revision is now accepted and ready to land.Aug 3 2018, 9:05 PM

olevole_olevole.ru added a subscriber: olevole_olevole.ru.Aug 3 2018, 11:04 PM

markj added inline comments.Aug 3 2018, 11:36 PM

sbin/dhclient/dhclient.c
531–534	Hmm, we still have to handle the original problem then. ... is there any chance capsicum support will become non-optional at some point? :)

(I guess I already marked "approved" so Phab won't let me again. But I just want to be clear that in spite of my line comments, I approve this change as-is.)

sbin/dhclient/dhclient.c
531–534	Given we're already creating /var/run/dhclient for capsicum, could we also use that for the chroot in place of /var/empty? The pid file doesn't seem especially sensitive for an attacker. But I'm confused. How does chrooted (or capsicum'd) dhclient access its leases file in /var/db today? $ sudo procstat -f $(cat /var/run/dhclient.igb0.pid) PID COMM FD T V FLAGS REF OFFSET PRO NAME 565 dhclient text v r r------- - - - /sbin/dhclient 565 dhclient cwd v d r------- - - - /var/empty 565 dhclient root v d r------- - - - /var/empty 565 dhclient jail v d r------- - - - /var/empty 565 dhclient 0 v c rw------ 6 0 - /dev/null 565 dhclient 1 v c rw------ 6 0 - /dev/null 565 dhclient 2 v c rw------ 6 0 - /dev/null 565 dhclient 3 v d r----n-- 2 0 - /var/run 565 dhclient 4 s - rw---n-- 2 0 UDS 0 0 - 565 dhclient 5 v r -w---n-l 2 0 - /var/run/dhclient.igb0.pid 565 dhclient 7 v c r------- 1 12880 - /dev/bpf 565 dhclient 8 s - rw------ 1 0 ? 565 dhclient 9 v r -w------ 1 5495 - /var/db/dhclient.leases.igb0 565 dhclient 11 p - rw------ 1 0 - - ... is there any chance capsicum support will become non-optional at some point? :) Yeah, that's the question. I expect laptop-class systems and better to have capsicum non-optionally. (I thought that was planned for 12, actually.) But I am imagining small embedded systems (MIPS/riscv/arm32, e.g., routers) might not want to pay any penalty for capsicum, and still have some protection for dhclient. That said, I don't have any quantified sense of how costly capsicum support is. I'm willing to assume we'll just require capsicum on everywhere and therefore the chroot can be discarded. Do you have a good idea of how the capsicum sandboxing interacts with both the pidfile, /dev/bpf, and the lease file today?

I thought that was planned for 12, actually.

Capsicum is generally enabled in GENERIC config files now (for some time); there is an open question about removing the #ifdefs and kernel config goo.

In D16584#352320, @emaste wrote:

I thought that was planned for 12, actually.

Capsicum is generally enabled in GENERIC config files now (for some time); there is an open question about removing the #ifdefs and kernel config goo.

What would it take to also enable in the myriad non-GENERIC embedded board configs, in a way that doesn't make them useless or annoy their users/developers? I think that's probably a necessary step towards removing the config(8) option and ifdefs entirely? If not adding the option to configs, at least researching what the pain level will be.

In D16584#352320, @emaste wrote:

I thought that was planned for 12, actually.

Capsicum is generally enabled in GENERIC config files now (for some time); there is an open question about removing the #ifdefs and kernel config goo.

FWIW, I've hit this issue before when modifying an application to use Capsicum, and ended up making lack of support a fatal error. The lack of mandatory support makes Capsicum harder to use.

sbin/dhclient/dhclient.c
531–534	Given we're already creating /var/run/dhclient for capsicum, could we also use that for the chroot in place of /var/empty? The pid file doesn't seem especially sensitive for an attacker. The issue is that we'd have to modify pidfile(3) to close the directory descriptor. Do you have a good idea of how the capsicum sandboxing interacts with both the pidfile, /dev/bpf, and the lease file today? For pidfiles, the only requirement today is that you call pidfile_open() before entering capability mode. pidfile_open() keeps rights to both the pidfile and the directory containing the pidfile. The latter is so that we can unlink the pidfile in capability mode when exiting. dhclient uses some privilege separation and has a non-sandboxed process which can send raw packets. I'm not sure how it manages lease files.

Fall back to a chroot if we are not sandboxed after calling
caph_enter_casper().

Document the problem with chroot_allow_open_directories in
the code and man page. Because capsicum is enabled in most
kernel configurations and chroot_allow_open_directories defaults
to 1, the scope of the problem reported in PR 223327 is
reduced significantly. If Capsicum is made non-optional,
the problem goes away.

This revision now requires review to proceed.Aug 4 2018, 4:53 PM

Herald added a subscriber: jonathan. · View Herald TranscriptAug 4 2018, 4:53 PM

Harbormaster completed remote builds in B18583: Diff 46289.Aug 4 2018, 4:53 PM

cem accepted this revision.Aug 4 2018, 5:37 PM

This revision is now accepted and ready to land.Aug 4 2018, 5:37 PM

oshogbo accepted this revision.Aug 5 2018, 10:22 PM

oshogbo added inline comments.