Bug 223327
| Summary: | dhclient: close the pidfile before calling chroot(2) | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Oleg Ginzburg <olevole> | ||||
| Component: | bin | Assignee: | Mark Johnston <markj> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Some People | CC: | cem, emaste, jilles, kp, markj, meka, oshogbo, pi | ||||
| Priority: | --- | Keywords: | patch | ||||
| Version: | CURRENT | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 228911 | ||||||
| Attachments: |
|
||||||
|
Description
Oleg Ginzburg
2017-10-30 18:38:14 UTC
Hmm. This is quite interesting. I think the problem started with r322369 which changed libutil / pidfile_open() to keep the directory fd (rather than the pidfile fd) so it could unlink the pidfile in capability mode. That appears to conflict with chroot now, because you can't chroot if you've got a directory fd open to somewhere outside the new root. I don't think this patch is right, because it'll break the pidfile_remove() in routehandler(). I'm not at all sure how this should be fixed though. Although kern.chroot_allow_open_directories can be bypassed trivially via Unix domain socket file descriptor passing, it does serve a purpose in pointing out open chroots and jails like this one. The open directory allows full access to the / that dhclient was started from, defeating its chroot to /var/empty. In capability mode where ".." is disallowed, there is still full access to /var/run. It looks like the status quo is that the pidfile will not be removed when dhclient aborts after chrooting. The pidfile_remove() call will always fail. Before r322369 this was the case because of the chroot, and after r322369 this was the case because dhclient limits the pidfile descriptor to no rights (so that pidfile_verify() will fail). If this status quo is acceptable, the fix is to close the directory file descriptor using a newly added pidfile(3) function. If the status quo is not acceptable, it could be fixed by adding a not chrooted non-capmode intermediate process to do the remove or by moving the dhclient pidfile into its own directory and fixing the rights on the pidfile and directory descriptors. The latter also requires an addition to the pidfile(3) API. (In reply to Jilles Tjoelker from comment #2) > In capability mode where ".." is disallowed, there is still full access to > /var/run. Nitpicking a little bit: .. *is* allowed in capability mode, as long as it modifies a path below the reference descriptor. I realize that this doesn't address the general problem, but what's the reason for chrooting in the first place now that dhclient runs in capability mode? (In reply to Jilles Tjoelker from comment #2) I don't quite follow the comment about pidfile_verify() - we do retain the CAP_FSTAT capability on the pidfile descriptor (see r322370). (In reply to Mark Johnston from comment #4) Err, of course dhclient might run on a kernel compiled without capsicum support. (In reply to Mark Johnston from comment #4) Although the pidfile library retains the rights, dhclient itself does not: around line 2435 of sbin/dhclient/dhclient.c it removes all rights from the descriptor after writing the pidfile. With the 12 code clash being relatively close, is there a way to fix this? Given the comments before this one, I would expect at least one more capsicumed app had the same problem? (In reply to Goran Mekić from comment #7) > I would expect at least one more capsicumed app had the same problem? I think dhclient may be the only program that uses all of pidfile, capsicum, and chroot. (In reply to Mark Johnston from comment #4) > I realize that this doesn't address the general problem, but what's > the reason for chrooting in the first place now that dhclient runs > in capability mode? Perhaps we could enter capability mode, and iff that fails try chroot? (In reply to Ed Maste from comment #9) In addition, how about we also keep dhclient pidfiles under /var/run/dhclient so that dhclient doesn't have access to /var/run/* via the directory descriptor? I think that this represents the best compromise, and is relatively simple: https://reviews.freebsd.org/D16584 A commit references this bug: Author: markj Date: Mon Aug 6 16:22:02 UTC 2018 New revision: 337382 URL: https://svnweb.freebsd.org/changeset/base/337382 Log: dhclient: Don't chroot if we are in capability mode. The main dhclient process is Capsicumized but also chroots to restrict filesystem access. With r322369, pidfile(3) maintains a directory descriptor for the pidfile, which can cause the chroot to fail in certain cases. To minimize the problem, only chroot if we fail to enter capability mode, and store dhclient pidfiles in a subdirectory of /var/run, thus restricting access via pidfile(3)'s directory descriptor. PR: 223327 Reviewed by: cem, oshogbo Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D16584 Changes: head/etc/mtree/BSD.var.dist head/sbin/dhclient/dhclient.8 head/sbin/dhclient/dhclient.c head/sbin/init/rc.d/dhclient Committed the workaround in r337382. The general problem still exists, and I'm not sure what the best solution is. Anything that uses pidfile(3) and chroot(2) is susceptible to the problem. I think r337382 is a desirable change regardless: there's no point in chrooting if we're in capability mode, and there might be multiple dhclient pidfiles so it's nice to keep them in their own subdirectory. |
